git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 1/4] Introduce a function to lock/unlock file descriptors when appending

From
Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Date
Aug 9, 2018, 17:35 UTC
Message-ID
<e449ed75fe3705692175017f98438815aeccf0fb.1533836122.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.17.git.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>

This function will be used to make write accesses in trace_write() a bit safer.

Note: this patch takes a very different approach for cross-platform
support than Git is historically taking: the original approach is to
first implement everything on Linux, using the functions available on
Linux, and then trying to emulate these functions on platforms that do
not have those functions such as Windows.

This leads to all kinds of undesirable quirks in Git's source code (and performance characteristics) because of the lack of a proper abstraction layer: instead of declaring functions for the functionality we *actually* need, we abuse POSIX functions to say what we need, even if those functions serve much broader purposes (and do not make at all clear what the caller wants of them).

For example, when Git wants to determine whether a path refers to a symbolic link, it calls lstat(). That POSIX function has to be emulated on Windows, painstakingly filling all the information lstat() would, only for the caller to throw away everything except that one bit of information, and all of the time figuring out the mtime/atime/ctime and file size and whatnot was spent in vain.

If we were to follow that approach, we would extend the fcntl() emulation in compat/mingw.c after this commit, to support the function added in this commit.

But fcntl() allows a lot more versatile file region locking that we actually need, to by necessity the fcntl() emulation would be quite complex: To support the `l_whence = SEEK_CUR` (which we would use, if it did not require so much book-keeping due to our writing between locking and unlocking the file), we would have to call `SetFilePointerEx()` (after obtaining the `HANDLE` on which all Win32 API functions work instead of the integer file descriptors used by all POSIX functions). Multiplying the number of Win32 API round-trips. Of course, we could implement an incomplete emulation of fcntl()'s F_WRLCK, but that would be unsatisfying.

An alternative approach would be to use the `flock()` function, whose semantics are much closer to existing Win32 API. But `flock()` is not even a POSIX standard, so we would have to implement emulation of `flock()` for *other* platforms. And it would again be the wrong thing to do, as we would once again fail to have an abstraction that clearly says what *exact*functionality we want to use.

To set a precedent for a better approach, let's introduce a proper abstraction: a function that says in its name precisely what Git wants it to do (as opposed to *how* it does it on Linux): lock_or_unlock_fd_for_appending().

The next commit will provide a Windows-specific implementation of this function/functionality.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
squash! Introduce a function to lock/unlock file descriptors when appending
---
 git-compat-util.h |  2 ++
 wrapper.c         | 14 ++++++++++++++
 2 files changed, 16 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 9a64998b2..13b83bade 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1202,6 +1202,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 #define getc_unlocked(fh) getc(fh)
 #endif
 
+extern int lock_or_unlock_fd_for_appending(int fd, int lock_it);
+
 /*
  * Our code often opens a path to an optional file, to work on its
  * contents when we can successfully open it.  We can ignore a failure
diff --git a/wrapper.c b/wrapper.c
index e4fa9d84c..6c2116272 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -690,3 +690,17 @@ int xgethostname(char *buf, size_t len)
 		buf[len - 1] = 0;
 	return ret;
 }
+
+#ifndef GIT_WINDOWS_NATIVE
+int lock_or_unlock_fd_for_appending(int fd, int lock_it)
+{
+	struct flock flock;
+
+	flock.l_type = lock_it ? F_WRLCK : F_UNLCK;
+	flock.l_whence = SEEK_SET;
+	flock.l_start = 0;
+	flock.l_len = 0xffffffff; /* arbitrary number of bytes */
+
+	return fcntl(fd, F_SETLKW, &flock);
+}
+#endif
-- 
gitgitgadget
Previous: Johannes Schindelin via GitGitGadgetNext: Junio C Hamano
Message 2 of 43 in “t5552: fix flakiness by introducing proper locking for GIT_TRACE”
  1. 0/4 t5552: fix flakiness by introducing proper locking for GIT_TRACEJohannes Schindelin via GitGitGadget, Aug 9, 2018
  2. 1/4 Introduce a function to lock/unlock file descriptors when appendingJohannes Schindelin via GitGitGadget, Aug 9, 2018
  3. Junio C HamanoAug 9, 2018
  4. Johannes SchindelinAug 10, 2018
  5. 2/4 mingw: implement lock_or_unlock_fd_for_appending()Johannes Schindelin via GitGitGadget, Aug 9, 2018
  6. 3/4 trace: lock the trace file to avoid racy trace_write() callsJohannes Schindelin via GitGitGadget, Aug 9, 2018
  7. 4/4 trace: verify that locking worksJohannes Schindelin via GitGitGadget, Aug 9, 2018
  8. Jeff KingAug 9, 2018
  9. Junio C HamanoAug 9, 2018
  10. Junio C HamanoAug 9, 2018
  11. Jeff KingAug 9, 2018
  12. Jeff KingAug 10, 2018
  13. Junio C HamanoAug 10, 2018
  14. Junio C HamanoAug 10, 2018
  15. Johannes SchindelinAug 10, 2018
  16. Jeff KingAug 10, 2018
  17. Junio C HamanoAug 10, 2018
  18. Johannes SchindelinAug 10, 2018
  19. Johannes SixtAug 10, 2018
  20. Jeff HostetlerAug 10, 2018
  21. Jeff HostetlerAug 10, 2018
  22. Johannes SixtAug 10, 2018
  23. Junio C HamanoAug 10, 2018
  24. Jeff KingAug 10, 2018
  25. mingw: enable atomic O_APPENDJohannes Sixt, Aug 13, 2018
  26. Junio C HamanoAug 13, 2018
  27. Johannes SixtAug 13, 2018
  28. Ævar Arnfjörð BjarmasonAug 13, 2018
  29. Junio C HamanoAug 13, 2018
  30. Jeff KingAug 13, 2018
  31. Ævar Arnfjörð BjarmasonAug 14, 2018
  32. Jeff KingAug 14, 2018
  33. Johannes SixtAug 14, 2018
  34. Jeff KingAug 14, 2018
  35. Jeff HostetlerAug 14, 2018
  36. Junio C HamanoAug 14, 2018
  37. 0/4 t5552: fix flakiness by introducing proper locking for GIT_TRACEJohannes Schindelin via GitGitGadget, Aug 10, 2018
  38. 1/4 Introduce a function to lock/unlock file descriptors when appendingJohannes Schindelin via GitGitGadget, Aug 10, 2018
  39. Junio C HamanoAug 10, 2018
  40. Johannes SchindelinAug 10, 2018
  41. 2/4 mingw: implement lock_or_unlock_fd_for_appending()Johannes Schindelin via GitGitGadget, Aug 10, 2018
  42. 3/4 trace: lock the trace file to avoid racy trace_write() callsJohannes Schindelin via GitGitGadget, Aug 10, 2018
  43. 4/4 trace: verify that locking worksJohannes Schindelin via GitGitGadget, Aug 10, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.