{"thread":{"id":"49085","subject":"[PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","startedAt":"2018-08-09T17:35:27Z","lastAt":"2018-08-14T19:17:12Z","messageCount":43,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Jeff King","Johannes Sixt","Johannes Schindelin","Jeff Hostetler","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"355013","messageId":"pull.17.git.gitgitgadget@gmail.com","threadId":"49085","inReplyTo":null,"subject":"[PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-08-09T17:35:24Z","receivedAt":"2018-08-09T17:35:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I reported a couple of times that t5552 is not passing reliably. It has now\nreached next, and will no doubt infect master soon.\n\nTurns out that it is not a Windows-specific issue, even if it occurs a lot \nmore often on Windows than elsewhere.\n\nThe culprit is that two processes try simultaneously to write to the same\nfile specified via GIT_TRACE_PACKET, and it is not well defined how that\nshould work, even on Linux.\n\nThis patch series addresses that by locking the trace fd. I chose to use \nflock() instead of fcntl() because the Win32 API LockFileEx()\n[https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-lockfileex] \n(which does exactly what I want in this context) has much more similar\nsemantics to the former than the latter.\n\nOf course, I have to admit that I am not super solid on flock() semantics,\nand I also do not know which conditional blocks in config.mak.uname should\ngrow a HAVE_FLOCK = YesWeDo line, still. Reviewers knowledgeable in flock() \nsemantics: I would be much indebted if you helped me there. Also: is it safe\nto call flock() on file descriptors referring not to files, but, say, pipes\nor an interactive terminal?\n\nJohannes Schindelin (4):\n Introduce a function to lock/unlock file descriptors when appending\n mingw: implement lock_or_unlock_fd_for_appending()\n trace: lock the trace file to avoid racy trace_write() calls\n trace: verify that locking works\n\n Makefile | 1 +\n compat/mingw.c | 19 ++++++\n compat/mingw.h | 3 +\n config.mak.uname | 3 +\n git-compat-util.h | 2 +\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/helper/test-trace.c | 130 +++++++++++++++++++++++++++++++++++++++++\n t/t0070-fundamental.sh | 6 ++\n trace.c | 11 +++-\n wrapper.c | 14 +++++\n 11 files changed, 190 insertions(+), 1 deletion(-)\n create mode 100644 t/helper/test-trace.c\n\n\nbase-commit: 42cc7485a2ec49ecc440c921d2eb0cae4da80549\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-17%2Fdscho%2Ffetch-negotiator-skipping-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-17/dscho/fetch-negotiator-skipping-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/17\n-- \ngitgitgadget\n"},{"id":"355014","messageId":"e449ed75fe3705692175017f98438815aeccf0fb.1533836122.git.gitgitgadget@gmail.com","threadId":"49085","inReplyTo":"pull.17.git.gitgitgadget@gmail.com","subject":"[PATCH 1/4] Introduce a function to lock/unlock file descriptors when appending","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-08-09T17:35:25Z","receivedAt":"2018-08-09T17:35:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis function will be used to make write accesses in trace_write() a bit\nsafer.\n\nNote: this patch takes a very different approach for cross-platform\nsupport than Git is historically taking: the original approach is to\nfirst implement everything on Linux, using the functions available on\nLinux, and then trying to emulate these functions on platforms that do\nnot have those functions such as Windows.\n\nThis leads to all kinds of undesirable quirks in Git's source code (and\nperformance characteristics) because of the lack of a proper abstraction\nlayer: instead of declaring functions for the functionality we\n*actually* need, we abuse POSIX functions to say what we need, even if\nthose functions serve much broader purposes (and do not make at all\nclear what the caller wants of them).\n\nFor example, when Git wants to determine whether a path refers to a\nsymbolic link, it calls lstat(). That POSIX function has to be emulated\non Windows, painstakingly filling all the information lstat() would,\nonly for the caller to throw away everything except that one bit of\ninformation, and all of the time figuring out the mtime/atime/ctime and\nfile size and whatnot was spent in vain.\n\nIf we were to follow that approach, we would extend the fcntl()\nemulation in compat/mingw.c after this commit, to support the function\nadded in this commit.\n\nBut fcntl() allows a lot more versatile file region locking that we\nactually need, to by necessity the fcntl() emulation would be quite\ncomplex: To support the `l_whence = SEEK_CUR` (which we would use, if it\ndid not require so much book-keeping due to our writing between locking\nand unlocking the file), we would have to call `SetFilePointerEx()`\n(after obtaining the `HANDLE` on which all Win32 API functions work\ninstead of the integer file descriptors used by all POSIX functions).\nMultiplying the number of Win32 API round-trips. Of course, we could\nimplement an incomplete emulation of fcntl()'s F_WRLCK, but that would\nbe unsatisfying.\n\nAn alternative approach would be to use the `flock()` function, whose\nsemantics are much closer to existing Win32 API. But `flock()` is not\neven a POSIX standard, so we would have to implement emulation of\n`flock()` for *other* platforms. And it would again be the wrong thing\nto do, as we would once again fail to have an abstraction that clearly\nsays what *exact*functionality we want to use.\n\nTo set a precedent for a better approach, let's introduce a proper\nabstraction: a function that says in its name precisely what Git\nwants it to do (as opposed to *how* it does it on Linux):\nlock_or_unlock_fd_for_appending().\n\nThe next commit will provide a Windows-specific implementation of this\nfunction/functionality.\n\nSigned-off-by: Johannes Schindelin \n\nsquash! Introduce a function to lock/unlock file descriptors when appending\n---\n git-compat-util.h | 2 ++\n wrapper.c | 14 ++++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9a64998b2..13b83bade 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1202,6 +1202,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);\n #define getc_unlocked(fh) getc(fh)\n #endif\n \n+extern int lock_or_unlock_fd_for_appending(int fd, int lock_it);\n+\n /*\n * Our code often opens a path to an optional file, to work on its\n * contents when we can successfully open it. We can ignore a failure\ndiff --git a/wrapper.c b/wrapper.c\nindex e4fa9d84c..6c2116272 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -690,3 +690,17 @@ int xgethostname(char *buf, size_t len)\n \t\tbuf[len - 1] = 0;\n \treturn ret;\n }\n+\n+#ifndef GIT_WINDOWS_NATIVE\n+int lock_or_unlock_fd_for_appending(int fd, int lock_it)\n+{\n+\tstruct flock flock;\n+\n+\tflock.l_type = lock_it ? F_WRLCK : F_UNLCK;\n+\tflock.l_whence = SEEK_SET;\n+\tflock.l_start = 0;\n+\tflock.l_len = 0xffffffff; /* arbitrary number of bytes */\n+\n+\treturn fcntl(fd, F_SETLKW, &flock);\n+}\n+#endif\n-- \ngitgitgadget\n\n"},{"id":"355015","messageId":"8dda2d5303952306520bc90c2cf2b25b83fdd31a.1533836122.git.gitgitgadget@gmail.com","threadId":"49085","inReplyTo":"pull.17.git.gitgitgadget@gmail.com","subject":"[PATCH 2/4] mingw: implement lock_or_unlock_fd_for_appending()","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-08-09T17:35:27Z","receivedAt":"2018-08-09T17:35:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nFor some reason, t/t5552-skipping-fetch-negotiator.sh fails particularly\noften on Windows due to racy tracing where the `git upload-pack` and the\n`git fetch` processes compete for the same file.\n\nWe just introduced a remedy that uses fcntl(), but Windows does not have\nfcntl(). So let's implement an alternative.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 19 +++++++++++++++++++\n compat/mingw.h | 3 +++\n config.mak.uname | 3 +++\n 3 files changed, 25 insertions(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 6ded1c859..6da9ce861 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -514,6 +514,25 @@ int mingw_chmod(const char *filename, int mode)\n \treturn _wchmod(wfilename, mode);\n }\n \n+int mingw_lock_or_unlock_fd_for_appending(int fd, int lock_it)\n+{\n+\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n+\tOVERLAPPED overlapped = { 0 };\n+\tDWORD err;\n+\n+\tif (!lock_it && UnlockFileEx(handle, 0, -1, 0, &overlapped))\n+\t\treturn 0;\n+\tif (lock_it &&\n+\t LockFileEx(handle, LOCKFILE_EXCLUSIVE_LOCK, 0, -1, 0, &overlapped))\n+\t\treturn 0;\n+\n+\terr = GetLastError();\n+\t/* LockFileEx() cannot lock pipes */\n+\terrno = err == ERROR_INVALID_FUNCTION ?\n+\t\tEBADF : err_win_to_posix(GetLastError());\n+\treturn -1;\n+}\n+\n /*\n * The unit of FILETIME is 100-nanoseconds since January 1, 1601, UTC.\n * Returns the 100-nanoseconds (\"hekto nanoseconds\") since the epoch.\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 571019d0b..0f76d89a9 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -397,6 +397,9 @@ HANDLE winansi_get_osfhandle(int fd);\n * git specific compatibility\n */\n \n+int mingw_lock_or_unlock_fd_for_appending(int fd, int lock_it);\n+#define lock_or_unlock_fd_for_appending mingw_lock_or_unlock_fd_for_appending\n+\n #define has_dos_drive_prefix(path) \\\n \t(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)\n int mingw_skip_dos_drive_prefix(char **path);\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 684fc5bf0..159b7da81 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -376,6 +376,7 @@ ifeq ($(uname_S),Windows)\n \tNO_POSIX_GOODIES = UnfortunatelyYes\n \tNATIVE_CRLF = YesPlease\n \tDEFAULT_HELP_FORMAT = html\n+\tHAVE_FLOCK = YesWeEmulate\n \n \tCC = compat/vcbuild/scripts/clink.pl\n \tAR = compat/vcbuild/scripts/lib.pl\n@@ -523,6 +524,8 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n \tNO_INET_NTOP = YesPlease\n \tNO_POSIX_GOODIES = UnfortunatelyYes\n \tDEFAULT_HELP_FORMAT = html\n+\tHAVE_FLOCK = YesWeEmulate\n+\n \tCOMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32\n \tCOMPAT_CFLAGS += -DSTRIP_EXTENSION=\\\".exe\\\"\n \tCOMPAT_OBJS += compat/mingw.o compat/winansi.o \\\n-- \ngitgitgadget\n\n"},{"id":"355016","messageId":"a53e72198ad690a968c12d22e9f2639130a36d78.1533836122.git.gitgitgadget@gmail.com","threadId":"49085","inReplyTo":"pull.17.git.gitgitgadget@gmail.com","subject":"[PATCH 3/4] trace: lock the trace file to avoid racy trace_write() calls","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-08-09T17:35:28Z","receivedAt":"2018-08-09T17:35:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen multiple processes try to write to the same file, it is not\nguaranteed that everything works as expected: those writes can overlap,\nand in the worst case even lose messages.\n\nThis happens in t/t5552-skipping-fetch-negotiator.sh, where we abuse the\n`GIT_TRACE` facility to write traces of two concurrent processes (`git\nfetch` and `git upload-pack`) to the same file, and then verify that the\ntrace contains certain expected breadcrumbs.\n\nTo remedy this, let's lock the file descriptors for exclusive writing,\nusing the function we just introduced in the previous commit.\n\nSigned-off-by: Johannes Schindelin \n---\n trace.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/trace.c b/trace.c\nindex fc623e91f..6f97dde27 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -114,11 +114,20 @@ static int prepare_trace_line(const char *file, int line,\n \n static void trace_write(struct trace_key *key, const void *buf, unsigned len)\n {\n-\tif (write_in_full(get_trace_fd(key), buf, len) < 0) {\n+\tint fd = get_trace_fd(key), locked;\n+\n+\tlocked = !lock_or_unlock_fd_for_appending(fd, 1);\n+\tif (!locked && errno != EBADF)\n+\t\twarning(\"unable to lock file descriptor for %s: %s\",\n+\t\t\tkey->key, strerror(errno));\n+\tif (write_in_full(fd, buf, len) < 0) {\n \t\twarning(\"unable to write trace for %s: %s\",\n \t\t\tkey->key, strerror(errno));\n \t\ttrace_disable(key);\n \t}\n+\tif (locked && lock_or_unlock_fd_for_appending(fd, 0) < 0)\n+\t\twarning(\"failed to remove lock on fd for %s: %s\",\n+\t\t\tkey->key, strerror(errno));\n }\n \n void trace_verbatim(struct trace_key *key, const void *buf, unsigned len)\n-- \ngitgitgadget\n\n"},{"id":"355017","messageId":"f57234154fb29d0b169442c44e4e683fe1cc3e6c.1533836122.git.gitgitgadget@gmail.com","threadId":"49085","inReplyTo":"pull.17.git.gitgitgadget@gmail.com","subject":"[PATCH 4/4] trace: verify that locking works","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-08-09T17:35:30Z","receivedAt":"2018-08-09T17:35:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nRecently, t5552 introduced a pattern where two processes try to write to\nthe same GIT_TRACE file in parallel. This is not safe, as the two\nprocesses fighting over who gets to append to the file can cause garbled\nlines and may even result in data loss on Windows (where buffers are\nwritten to before they are flushed).\n\nTo remedy this, we introduced the lock_or_unlock_fd_for_appending()\nfunction. And to make sure that this works, this commit introduces a\nregression test.\n\nSigned-off-by: Johannes Schindelin \n---\n Makefile | 1 +\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/helper/test-trace.c | 130 +++++++++++++++++++++++++++++++++++++++++\n t/t0070-fundamental.sh | 6 ++\n 5 files changed, 139 insertions(+)\n create mode 100644 t/helper/test-trace.c\n\ndiff --git a/Makefile b/Makefile\nindex 617475622..2e3fb5b8d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -729,6 +729,7 @@ TEST_BUILTINS_OBJS += test-strcmp-offset.o\n TEST_BUILTINS_OBJS += test-string-list.o\n TEST_BUILTINS_OBJS += test-submodule-config.o\n TEST_BUILTINS_OBJS += test-subprocess.o\n+TEST_BUILTINS_OBJS += test-trace.o\n TEST_BUILTINS_OBJS += test-urlmatch-normalization.o\n TEST_BUILTINS_OBJS += test-wildmatch.o\n TEST_BUILTINS_OBJS += test-write-cache.o\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 805a45de9..7adce872b 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -39,6 +39,7 @@ static struct test_cmd cmds[] = {\n \t{ \"string-list\", cmd__string_list },\n \t{ \"submodule-config\", cmd__submodule_config },\n \t{ \"subprocess\", cmd__subprocess },\n+\t{ \"trace\", cmd__trace },\n \t{ \"urlmatch-normalization\", cmd__urlmatch_normalization },\n \t{ \"wildmatch\", cmd__wildmatch },\n \t{ \"write-cache\", cmd__write_cache },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 7116ddfb9..c462ac924 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -33,6 +33,7 @@ int cmd__strcmp_offset(int argc, const char **argv);\n int cmd__string_list(int argc, const char **argv);\n int cmd__submodule_config(int argc, const char **argv);\n int cmd__subprocess(int argc, const char **argv);\n+int cmd__trace(int argc, const char **argv);\n int cmd__urlmatch_normalization(int argc, const char **argv);\n int cmd__wildmatch(int argc, const char **argv);\n int cmd__write_cache(int argc, const char **argv);\ndiff --git a/t/helper/test-trace.c b/t/helper/test-trace.c\nnew file mode 100644\nindex 000000000..1cc88b030\n--- /dev/null\n+++ b/t/helper/test-trace.c\n@@ -0,0 +1,130 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+#include \"run-command.h\"\n+\n+static struct child_process children[2] = {\n+\tCHILD_PROCESS_INIT,\n+\tCHILD_PROCESS_INIT,\n+};\n+\n+#define SAY(child, what) \\\n+\tif (write_in_full(children[child].in, \\\n+\t\t\t what \"\\n\", strlen(what) + 1) < 0) \\\n+\t\tdie(\"Failed to tell child process #%d to %s\", child, what)\n+\n+#define LISTEN(child, what) \\\n+\tif (strbuf_getwholeline_fd(&buf, children[child].out, '\\n') < 0) \\\n+\t\tdie(\"Child process #%d failed to acknowledge %s\", child, what)\n+\n+#define ACK(what) \\\n+\tif (write_in_full(1, what \": ACK\\n\", strlen(what) + 6) < 0) \\\n+\t\tdie_errno(\"'%s': %s ACK\", child_name, what)\n+\n+static void contention_orchestrator(const char *argv0)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint i;\n+\n+\t/* Spawn two children and simulate write contention */\n+\ttrace_printf(\"start\");\n+\n+\tfor (i = 0; i < 2; i++) {\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addf(&buf, \"child #%d\", i);\n+\t\targv_array_pushl(&children[i].args,\n+\t\t\t\t\targv0, \"trace\", \"lock\", buf.buf, NULL);\n+\t\tchildren[i].in = children[i].out = -1;\n+\t\tif (start_command(&children[i]) < 0)\n+\t\t\tdie(\"Could not spawn child process #%d\", i);\n+\t}\n+\n+\tSAY(1, \"lock\");\n+\tLISTEN(1, \"lock\");\n+\n+\tSAY(0, \"trace delayed\");\n+\tSAY(1, \"trace while-locked\");\n+\tLISTEN(1, \"trace\");\n+\n+\tSAY(1, \"unlock\");\n+\tLISTEN(1, \"unlock\");\n+\tLISTEN(0, \"trace\");\n+\n+\tSAY(0, \"quit\");\n+\tSAY(1, \"quit\");\n+\n+\tif (finish_command(&children[0]) < 0 ||\n+\t\tfinish_command(&children[1]) < 0)\n+\t\tdie(\"Child process failed to finish\");\n+\n+\tstrbuf_release(&buf);\n+}\n+\n+static void child(const char *child_name)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd, locked = 0;\n+\tconst char *p;\n+\n+\t/* This is the child process */\n+\ttrace_printf(\"child start: '%s'\", child_name);\n+\tfd = trace_default_key.fd;\n+\tif (fd <= 0)\n+\t\tdie(\"child process: not tracing...\");\n+\twhile (!strbuf_getwholeline_fd(&buf, 0, '\\n')) {\n+\t\tstrbuf_rtrim(&buf);\n+\t\tif (!strcmp(\"lock\", buf.buf)) {\n+\t\t\tif (lock_or_unlock_fd_for_appending(fd, 1) < 0 &&\n+\t\t\t errno != EBADF)\n+\t\t\t\tdie_errno(\"'%s': lock\", child_name);\n+\t\t\tACK(\"lock\");\n+\t\t\tlocked = 1;\n+\t\t} else if (!strcmp(\"unlock\", buf.buf)) {\n+\t\t\tif (lock_or_unlock_fd_for_appending(fd, 0) < 0 &&\n+\t\t\t errno != EBADF)\n+\t\t\t\tdie_errno(\"'%s': unlock\", child_name);\n+\t\t\tACK(\"unlock\");\n+\t\t\tlocked = 0;\n+\t\t} else if (skip_prefix(buf.buf, \"trace \", &p)) {\n+\t\t\tif (!locked)\n+\t\t\t\ttrace_printf(\"%s: %s\", child_name, p);\n+\t\t\telse {\n+\t\t\t\tchar *p2 = xstrdup(p);\n+\n+\t\t\t\t/* Give the racy process a run for its money. */\n+\t\t\t\tsleep_millisec(50);\n+\n+\t\t\t\tstrbuf_reset(&buf);\n+\t\t\t\tstrbuf_addf(&buf, \"%s: %s\\n\",\n+\t\t\t\t\t child_name, p2);\n+\t\t\t\tfree(p2);\n+\n+\t\t\t\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n+\t\t\t\t\tdie_errno(\"'%s': trace\", child_name);\n+\t\t\t}\n+\t\t\tACK(\"trace\");\n+\t\t} else if (!strcmp(\"quit\", buf.buf)) {\n+\t\t\tclose(0);\n+\t\t\tclose(1);\n+\t\t\tbreak;\n+\t\t} else\n+\t\t\tdie(\"Unhandled command: '%s'\", buf.buf);\n+\n+\t}\n+\n+\tstrbuf_release(&buf);\n+}\n+\n+int cmd__trace(int argc, const char **argv)\n+{\n+\tconst char *argv0 = argv[-1];\n+\n+\tif (argc > 1 && !strcmp(\"lock\", argv[1])) {\n+\t\tif (argc > 2)\n+\t\t\tchild(argv[2]);\n+\t\telse\n+\t\t\tcontention_orchestrator(argv0);\n+\t} else\n+\t\tdie(\"Usage: %s %s lock []\", argv0, argv[0]);\n+\n+\treturn 0;\n+}\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 23fbe6434..57f7a1246 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -34,4 +34,10 @@ test_expect_success 'check for a bug in the regex routines' '\n \ttest-tool regex --bug\n '\n \n+test_expect_success 'multiple processes can GIT_TRACE to the same file' '\n+\tGIT_TRACE=\"$(pwd)/trace\" test-tool trace lock &&\n+\tsed -n \"/while-locked/,\\$s/.*delayed$/YES/p\" actual &&\n+\ttest YES = \"$(cat actual)\"\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"355024","messageId":"xmqqtvo3bc0g.fsf@gitster-ct.c.googlers.com","threadId":"49085","inReplyTo":"e449ed75fe3705692175017f98438815aeccf0fb.1533836122.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] Introduce a function to lock/unlock file descriptors when appending","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T19:01:03Z","receivedAt":"2018-08-09T19:01:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> This function will be used to make write accesses in trace_write() a bit\n> safer.\n> ...\n> To set a precedent for a better approach, let's introduce a proper\n> abstraction: a function that says in its name precisely what Git\n> wants it to do (as opposed to *how* it does it on Linux):\n> lock_or_unlock_fd_for_appending().\n>\n> The next commit will provide a Windows-specific implementation of this\n> function/functionality.\n>\n> Signed-off-by: Johannes Schindelin \n>\n> squash! Introduce a function to lock/unlock file descriptors when appending\n\nIf we can keep the custom, narrow and easy-to-port API (like this\npatch introduces) focused and resist feature creep over time, then\nit would be worth spending effort to come up with such a custom\nhelper that is easy to port. So I agree with the approach in\ngeneral but I tend to think \"set a precedent for a better approach\"\nis a way-too-early and wishful verdict. We do not know if we can\nreally keep that custom API easy-to-port-and-maintain yet.\n\nIn short, even though I agree with the approach, most of the\nverbiage above is unnecessary and mere distraction.\n\n> ---\n> git-compat-util.h | 2 ++\n> wrapper.c | 14 ++++++++++++++\n> 2 files changed, 16 insertions(+)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 9a64998b2..13b83bade 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -1202,6 +1202,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);\n> #define getc_unlocked(fh) getc(fh)\n> #endif\n> \n> +extern int lock_or_unlock_fd_for_appending(int fd, int lock_it);\n> +\n> /*\n> * Our code often opens a path to an optional file, to work on its\n> * contents when we can successfully open it. We can ignore a failure\n> diff --git a/wrapper.c b/wrapper.c\n> index e4fa9d84c..6c2116272 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -690,3 +690,17 @@ int xgethostname(char *buf, size_t len)\n> \t\tbuf[len - 1] = 0;\n> \treturn ret;\n> }\n> +\n> +#ifndef GIT_WINDOWS_NATIVE\n> +int lock_or_unlock_fd_for_appending(int fd, int lock_it)\n> +{\n> +\tstruct flock flock;\n> +\n> +\tflock.l_type = lock_it ? F_WRLCK : F_UNLCK;\n> +\tflock.l_whence = SEEK_SET;\n> +\tflock.l_start = 0;\n> +\tflock.l_len = 0xffffffff; /* arbitrary number of bytes */\n\nIf this can be an arbitrary range, do we need to cover this many (or\nonly this few, depending on your point of view) bytes?\n\nWould it be sufficient to cover just the first one byte instead? Or\nperhaps give l_len==0 to cover all no matter how large the file\ngrows, which sounds like a better range specification.\n\n> +\treturn fcntl(fd, F_SETLKW, &flock);\n> +}\n> +#endif\n"},{"id":"355029","messageId":"20180809194712.GC32376@sigill.intra.peff.net","threadId":"49085","inReplyTo":"pull.17.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T19:47:12Z","receivedAt":"2018-08-09T19:47:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 09, 2018 at 10:35:24AM -0700, Johannes Schindelin via GitGitGadget wrote:\n\n> The culprit is that two processes try simultaneously to write to the same\n> file specified via GIT_TRACE_PACKET, and it is not well defined how that\n> should work, even on Linux.\n\nAre you sure that it's not well-defined? We open the path with O_APPEND,\nwhich means every write() will be atomically positioned at the end of\nfile. So we would never lose or overwrite data.\n\nWe do our own buffering in a strbuf, writing the result out in a single\nwrite() call (modulo the OS returning a short write, but that should not\ngenerally happen when writing short strings to a file). So we should get\nindividual trace lines as atomic units.\n\nThe order of lines from the two processes is undefined, of course.\n\n> This patch series addresses that by locking the trace fd. I chose to use \n> flock() instead of fcntl() because the Win32 API LockFileEx()\n> [https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-lockfileex] \n> (which does exactly what I want in this context) has much more similar\n> semantics to the former than the latter.\n\nI must admit I'm not excited to see us adding extra locking and\ncomplexity when it is not necessarily needed. Is this a feature we\nactually care about delivering for normal users of GIT_TRACE, or do we\njust care about solving the test flakiness here?\n\nBecause we should be able to do the latter with the patch below.\n\nI also wonder if we should be clearing GIT_TRACE as part of clearing\nrepo-env. It would do what we want automatically in this case, though\nthere are definitely cases where I prefer the current behavior (because\nI really do want to see the upload-pack side of it). Of course I could\nalways use \"--upload-pack 'GIT_TRACE_PACKET=... upload-pack'\", so this\nis really just a default.\n\ndiff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\nindex 3b60bd44e0..5ad5bece55 100755\n--- a/t/t5552-skipping-fetch-negotiator.sh\n+++ b/t/t5552-skipping-fetch-negotiator.sh\n@@ -28,6 +28,19 @@ have_not_sent () {\n \tdone\n }\n \n+# trace_fetch [args]\n+#\n+# Trace the packet output of fetch, but make sure we disable the variable\n+# in the child upload-pack, so we don't combine the results in the same file.\n+trace_fetch () {\n+\tclient=$1; shift\n+\tserver=$1; shift\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" \\\n+\tgit -C \"$client\" fetch \\\n+\t --upload-pack 'unset GIT_TRACE_PACKET; git-upload-pack' \\\n+\t \"$server\" \"$@\"\n+}\n+\n test_expect_success 'commits with no parents are sent regardless of skip distance' '\n \tgit init server &&\n \ttest_commit -C server to_fetch &&\n@@ -42,7 +55,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc\n \t# \"c1\" has no parent, it is still sent as \"have\" even though it would\n \t# normally be skipped.\n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n+\ttrace_fetch client \"$(pwd)/server\" &&\n \thave_sent c7 c5 c2 c1 &&\n \thave_not_sent c6 c4 c3\n '\n@@ -88,7 +101,7 @@ test_expect_success 'when two skips collide, favor the larger one' '\n \t# the next \"have\" sent will be \"c1\" (from \"c6\" skip 4) and not \"c4\"\n \t# (from \"c5side\" skip 1).\n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n+\ttrace_fetch client \"$(pwd)/server\" &&\n \thave_sent c5side c11 c9 c6 c1 &&\n \thave_not_sent c10 c8 c7 c5 c4 c3 c2\n '\n@@ -114,7 +127,7 @@ test_expect_success 'use ref advertisement to filter out commits' '\n \t# not need to send any ancestors of \"c3\", but we still need to send \"c3\"\n \t# itself.\n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch origin to_fetch &&\n+\ttrace_fetch client origin to_fetch &&\n \thave_sent c5 c4^ c2side &&\n \thave_not_sent c4 c4^^ c4^^^\n '\n@@ -144,7 +157,7 @@ test_expect_success 'handle clock skew' '\n \t# and sent, because (due to clock skew) its only parent has already been\n \t# popped off the priority queue.\n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n+\ttrace_fetch client \"$(pwd)/server\" &&\n \thave_sent c2 c1 old4 old2 old1 &&\n \thave_not_sent old3\n '\n@@ -176,7 +189,7 @@ test_expect_success 'do not send \"have\" with ancestors of commits that server AC\n \ttest_commit -C server commit-on-b1 &&\n \n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" to_fetch &&\n+\ttrace_fetch client \"$(pwd)/server\" to_fetch &&\n \tgrep \" fetch\" trace &&\n \n \t# fetch-pack sends 2 requests each containing 16 \"have\" lines before\n"},{"id":"355034","messageId":"xmqqo9ebb6z3.fsf@gitster-ct.c.googlers.com","threadId":"49085","inReplyTo":"20180809194712.GC32376@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T20:49:52Z","receivedAt":"2018-08-09T20:49:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Are you sure that it's not well-defined? We open the path with O_APPEND,\n> which means every write() will be atomically positioned at the end of\n> file. So we would never lose or overwrite data.\n>\n> We do our own buffering in a strbuf, writing the result out in a single\n> write() call (modulo the OS returning a short write, but that should not\n> generally happen when writing short strings to a file). So we should get\n> individual trace lines as atomic units.\n>\n> The order of lines from the two processes is undefined, of course.\n\nCorrect. But I am more worried about the \"mixed/overwriting\"\nbreakage, if there is one; it means we may need to be prepared for\nsystems that lack O_APPEND that works correctly. I initially just\nassumed that it was what Dscho was seeing, but after re-reading his\nmessage, I am not sure anymore.\n\nI think the \"do not trace the other side\" approach you suggest for\nthese tests that only care about one side is more appropriate\nsolution for this particular case. We then do not have to worry\nabout overwriting or output from both sides mixed randomly.\n\n> diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\n> index 3b60bd44e0..5ad5bece55 100755\n> --- a/t/t5552-skipping-fetch-negotiator.sh\n> +++ b/t/t5552-skipping-fetch-negotiator.sh\n> @@ -28,6 +28,19 @@ have_not_sent () {\n> \tdone\n> }\n> \n> +# trace_fetch [args]\n> +#\n> +# Trace the packet output of fetch, but make sure we disable the variable\n> +# in the child upload-pack, so we don't combine the results in the same file.\n> +trace_fetch () {\n> +\tclient=$1; shift\n> +\tserver=$1; shift\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" \\\n> +\tgit -C \"$client\" fetch \\\n> +\t --upload-pack 'unset GIT_TRACE_PACKET; git-upload-pack' \\\n> +\t \"$server\" \"$@\"\n> +}\n> +\n> test_expect_success 'commits with no parents are sent regardless of skip distance' '\n> \tgit init server &&\n> \ttest_commit -C server to_fetch &&\n> @@ -42,7 +55,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc\n> \t# \"c1\" has no parent, it is still sent as \"have\" even though it would\n> \t# normally be skipped.\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n> +\ttrace_fetch client \"$(pwd)/server\" &&\n> \thave_sent c7 c5 c2 c1 &&\n> \thave_not_sent c6 c4 c3\n> '\n> @@ -88,7 +101,7 @@ test_expect_success 'when two skips collide, favor the larger one' '\n> \t# the next \"have\" sent will be \"c1\" (from \"c6\" skip 4) and not \"c4\"\n> \t# (from \"c5side\" skip 1).\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n> +\ttrace_fetch client \"$(pwd)/server\" &&\n> \thave_sent c5side c11 c9 c6 c1 &&\n> \thave_not_sent c10 c8 c7 c5 c4 c3 c2\n> '\n> @@ -114,7 +127,7 @@ test_expect_success 'use ref advertisement to filter out commits' '\n> \t# not need to send any ancestors of \"c3\", but we still need to send \"c3\"\n> \t# itself.\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch origin to_fetch &&\n> +\ttrace_fetch client origin to_fetch &&\n> \thave_sent c5 c4^ c2side &&\n> \thave_not_sent c4 c4^^ c4^^^\n> '\n> @@ -144,7 +157,7 @@ test_expect_success 'handle clock skew' '\n> \t# and sent, because (due to clock skew) its only parent has already been\n> \t# popped off the priority queue.\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n> +\ttrace_fetch client \"$(pwd)/server\" &&\n> \thave_sent c2 c1 old4 old2 old1 &&\n> \thave_not_sent old3\n> '\n> @@ -176,7 +189,7 @@ test_expect_success 'do not send \"have\" with ancestors of commits that server AC\n> \ttest_commit -C server commit-on-b1 &&\n> \n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" to_fetch &&\n> +\ttrace_fetch client \"$(pwd)/server\" to_fetch &&\n> \tgrep \" fetch\" trace &&\n> \n> \t# fetch-pack sends 2 requests each containing 16 \"have\" lines before\n"},{"id":"355037","messageId":"xmqqbmabb63b.fsf@gitster-ct.c.googlers.com","threadId":"49085","inReplyTo":"xmqqo9ebb6z3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T21:08:56Z","receivedAt":"2018-08-09T21:09:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> Are you sure that it's not well-defined? We open the path with O_APPEND,\n>> which means every write() will be atomically positioned at the end of\n>> file. So we would never lose or overwrite data.\n>>\n>> We do our own buffering in a strbuf, writing the result out in a single\n>> write() call (modulo the OS returning a short write, but that should not\n>> generally happen when writing short strings to a file). So we should get\n>> individual trace lines as atomic units.\n>>\n>> The order of lines from the two processes is undefined, of course.\n>\n> Correct. But I am more worried about the \"mixed/overwriting\"\n> breakage, if there is one; it means we may need to be prepared for\n> systems that lack O_APPEND that works correctly. I initially just\n> assumed that it was what Dscho was seeing, but after re-reading his\n> message, I am not sure anymore.\n>\n> I think the \"do not trace the other side\" approach you suggest for\n> these tests that only care about one side is more appropriate\n> solution for this particular case. We then do not have to worry\n> about overwriting or output from both sides mixed randomly.\n\nA concluding sentence I forgot to add, after saying \"this is simpler\nand better to fix test breakage\", was\n\n\tBut if we really are seeing O_APPEND breakage, a mandatory\n\tlocking mechanism like this one might be necessary to work\n\taround it (I seriously hope we do not have to, though).\n\nSorry for an additional noise.\n"},{"id":"355043","messageId":"20180809213201.GB11342@sigill.intra.peff.net","threadId":"49085","inReplyTo":"xmqqbmabb63b.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T21:32:01Z","receivedAt":"2018-08-09T21:32:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 09, 2018 at 02:08:56PM -0700, Junio C Hamano wrote:\n\n> > Correct. But I am more worried about the \"mixed/overwriting\"\n> > breakage, if there is one; it means we may need to be prepared for\n> > systems that lack O_APPEND that works correctly. I initially just\n> > assumed that it was what Dscho was seeing, but after re-reading his\n> > message, I am not sure anymore.\n> >\n> > I think the \"do not trace the other side\" approach you suggest for\n> > these tests that only care about one side is more appropriate\n> > solution for this particular case. We then do not have to worry\n> > about overwriting or output from both sides mixed randomly.\n> \n> A concluding sentence I forgot to add, after saying \"this is simpler\n> and better to fix test breakage\", was\n> \n> \tBut if we really are seeing O_APPEND breakage, a mandatory\n> \tlocking mechanism like this one might be necessary to work\n> \taround it (I seriously hope we do not have to, though).\n> \n> Sorry for an additional noise.\n\nYeah, my earlier email said \"if we just care about this test, then...\".\n\nBut if we do care about making this multiple-tracing case work all the\ntime, then we'd need some solution. If there's something that can work\nlike O_APPEND, even if we have to make some system-specific call, I'd\nprefer that to locking.\n\nI did some searching for atomic append on Windows and got mixed reports\non whether their FILE_DATA_APPEND does what we want or not. Knowing\nnothing about msys, I'd _assume_ that O_APPEND would get translated to\nthat flag, but I got lost trying to find it in\ngit-for-windows/msys2-runtime.\n\nWouldn't it be wonderful if the solution were as simple as making sure\nthat flag was plumbed through? I seriously doubt it will be that easy,\nthough. :)\n\n-Peff\n"},{"id":"355079","messageId":"20180810140908.GA23507@sigill.intra.peff.net","threadId":"49085","inReplyTo":"xmqqo9ebb6z3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-10T14:09:08Z","receivedAt":"2018-08-10T14:09:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 09, 2018 at 01:49:52PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Are you sure that it's not well-defined? We open the path with O_APPEND,\n> > which means every write() will be atomically positioned at the end of\n> > file. So we would never lose or overwrite data.\n> >\n> > We do our own buffering in a strbuf, writing the result out in a single\n> > write() call (modulo the OS returning a short write, but that should not\n> > generally happen when writing short strings to a file). So we should get\n> > individual trace lines as atomic units.\n> >\n> > The order of lines from the two processes is undefined, of course.\n> \n> Correct. But I am more worried about the \"mixed/overwriting\"\n> breakage, if there is one; it means we may need to be prepared for\n> systems that lack O_APPEND that works correctly. I initially just\n> assumed that it was what Dscho was seeing, but after re-reading his\n> message, I am not sure anymore.\n> \n> I think the \"do not trace the other side\" approach you suggest for\n> these tests that only care about one side is more appropriate\n> solution for this particular case. We then do not have to worry\n> about overwriting or output from both sides mixed randomly.\n\nHere it is as a patch on top of jt/fetch-negotiator-skipping, which lets\nus pursue any fix for interleaved trace output on Windows without the\npressure of an impending flaky test.\n\nMy gut says that looking into making O_APPEND work there is going to be\nthe nicest solution, but my gut is not very well versed in Windows\nsubtleties. ;)\n\n-- >8 --\nSubject: [PATCH] t5552: suppress upload-pack trace output\n\nThe t5552 test script uses GIT_TRACE_PACKET to monitor what\ngit-fetch sends and receives. However, because we're\naccessing a local repository, the child upload-pack also\nsends trace output to the same file.\n\nOn Linux, this works out OK. We open the trace file with\nO_APPEND, so all writes are atomically positioned at the end\nof the file. No data can be overwritten or omitted. And\nsince we prepare our small writes in a strbuf and write them\nwith a single write(), we should see each line as an atomic\nunit. The order of lines between the two processes is\nundefined, but the test script greps only for \"fetch>\" or\n\"fetch<\" lines. So under Linux, the test results are\ndeterministic.\n\nThe test fails intermittently on Windows, however,\nreportedly even overwriting bits of the output file (i.e.,\nO_APPEND does not seem to give us an atomic position+write).\n\nSince the test only cares about the trace output from fetch,\nwe can just disable the output from upload-pack. That\ndoesn't solve the greater question of O_APPEND/trace issues\nunder Windows, but it easily fixes the flakiness from this\ntest.\n\nReported-by: Johannes Schindelin \nSigned-off-by: Jeff King \n---\nI'm assuming that this really isn't triggerable on Linux. I tried and\ncouldn't manage to get it to fail, and the reasoning above explains why.\nBut I wasn't 100% clear that Dscho hadn't seen it fail on non-Windows.\n\n t/t5552-skipping-fetch-negotiator.sh | 23 ++++++++++++++++++-----\n 1 file changed, 18 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\nindex 0a8e0e42ed..0ad50dd839 100755\n--- a/t/t5552-skipping-fetch-negotiator.sh\n+++ b/t/t5552-skipping-fetch-negotiator.sh\n@@ -28,6 +28,19 @@ have_not_sent () {\n \tdone\n }\n \n+# trace_fetch [args]\n+#\n+# Trace the packet output of fetch, but make sure we disable the variable\n+# in the child upload-pack, so we don't combine the results in the same file.\n+trace_fetch () {\n+\tclient=$1; shift\n+\tserver=$1; shift\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" \\\n+\tgit -C \"$client\" fetch \\\n+\t --upload-pack 'unset GIT_TRACE_PACKET; git-upload-pack' \\\n+\t \"$server\" \"$@\"\n+}\n+\n test_expect_success 'commits with no parents are sent regardless of skip distance' '\n \tgit init server &&\n \ttest_commit -C server to_fetch &&\n@@ -42,7 +55,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc\n \t# \"c1\" has no parent, it is still sent as \"have\" even though it would\n \t# normally be skipped.\n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n+\ttrace_fetch client \"$(pwd)/server\" &&\n \thave_sent c7 c5 c2 c1 &&\n \thave_not_sent c6 c4 c3\n '\n@@ -65,7 +78,7 @@ test_expect_success 'when two skips collide, favor the larger one' '\n \t# the next \"have\" sent will be \"c1\" (from \"c6\" skip 4) and not \"c4\"\n \t# (from \"c5side\" skip 1).\n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n+\ttrace_fetch client \"$(pwd)/server\" &&\n \thave_sent c5side c11 c9 c6 c1 &&\n \thave_not_sent c10 c8 c7 c5 c4 c3 c2\n '\n@@ -91,7 +104,7 @@ test_expect_success 'use ref advertisement to filter out commits' '\n \t# not need to send any ancestors of \"c3\", but we still need to send \"c3\"\n \t# itself.\n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch origin to_fetch &&\n+\ttrace_fetch client origin to_fetch &&\n \thave_sent c5 c4^ c2side &&\n \thave_not_sent c4 c4^^ c4^^^\n '\n@@ -121,7 +134,7 @@ test_expect_success 'handle clock skew' '\n \t# and sent, because (due to clock skew) its only parent has already been\n \t# popped off the priority queue.\n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n+\ttrace_fetch client \"$(pwd)/server\" &&\n \thave_sent c2 c1 old4 old2 old1 &&\n \thave_not_sent old3\n '\n@@ -153,7 +166,7 @@ test_expect_success 'do not send \"have\" with ancestors of commits that server AC\n \ttest_commit -C server commit-on-b1 &&\n \n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" to_fetch &&\n+\ttrace_fetch client \"$(pwd)/server\" to_fetch &&\n \tgrep \" fetch\" trace &&\n \n \t# fetch-pack sends 2 requests each containing 16 \"have\" lines before\n-- \n2.18.0.1058.g0926f0b71f\n\n"},{"id":"355084","messageId":"xmqq36vm9psu.fsf@gitster-ct.c.googlers.com","threadId":"49085","inReplyTo":"20180810140908.GA23507@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-10T15:58:25Z","receivedAt":"2018-08-10T15:58:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Here it is as a patch on top of jt/fetch-negotiator-skipping, which lets\n> us pursue any fix for interleaved trace output on Windows without the\n> pressure of an impending flaky test.\n>\n> My gut says that looking into making O_APPEND work there is going to be\n> the nicest solution, but my gut is not very well versed in Windows\n> subtleties. ;)\n\nAs we know these tests are only interested in \"fetch\" lines and not\nrecord of communication in the other direction, I think this is a\nnice clean-up that is worth doing.\n\nFor only two grep's it may not look worth doing, but it would be\nalso a good clean-up to give an got_acked helper that sits next to\nhave_sent and have_not_sent helpers for readability. That is of\ncourse outside the scope of this change.\n\nWill queue. Thansk.\n\n> -- >8 --\n> Subject: [PATCH] t5552: suppress upload-pack trace output\n>\n> The t5552 test script uses GIT_TRACE_PACKET to monitor what\n> git-fetch sends and receives. However, because we're\n> accessing a local repository, the child upload-pack also\n> sends trace output to the same file.\n>\n> On Linux, this works out OK. We open the trace file with\n> O_APPEND, so all writes are atomically positioned at the end\n> of the file. No data can be overwritten or omitted. And\n> since we prepare our small writes in a strbuf and write them\n> with a single write(), we should see each line as an atomic\n> unit. The order of lines between the two processes is\n> undefined, but the test script greps only for \"fetch>\" or\n> \"fetch<\" lines. So under Linux, the test results are\n> deterministic.\n>\n> The test fails intermittently on Windows, however,\n> reportedly even overwriting bits of the output file (i.e.,\n> O_APPEND does not seem to give us an atomic position+write).\n>\n> Since the test only cares about the trace output from fetch,\n> we can just disable the output from upload-pack. That\n> doesn't solve the greater question of O_APPEND/trace issues\n> under Windows, but it easily fixes the flakiness from this\n> test.\n>\n> Reported-by: Johannes Schindelin \n> Signed-off-by: Jeff King \n> ---\n> I'm assuming that this really isn't triggerable on Linux. I tried and\n> couldn't manage to get it to fail, and the reasoning above explains why.\n> But I wasn't 100% clear that Dscho hadn't seen it fail on non-Windows.\n>\n> t/t5552-skipping-fetch-negotiator.sh | 23 ++++++++++++++++++-----\n> 1 file changed, 18 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\n> index 0a8e0e42ed..0ad50dd839 100755\n> --- a/t/t5552-skipping-fetch-negotiator.sh\n> +++ b/t/t5552-skipping-fetch-negotiator.sh\n> @@ -28,6 +28,19 @@ have_not_sent () {\n> \tdone\n> }\n> \n> +# trace_fetch [args]\n> +#\n> +# Trace the packet output of fetch, but make sure we disable the variable\n> +# in the child upload-pack, so we don't combine the results in the same file.\n> +trace_fetch () {\n> +\tclient=$1; shift\n> +\tserver=$1; shift\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" \\\n> +\tgit -C \"$client\" fetch \\\n> +\t --upload-pack 'unset GIT_TRACE_PACKET; git-upload-pack' \\\n> +\t \"$server\" \"$@\"\n> +}\n> +\n> test_expect_success 'commits with no parents are sent regardless of skip distance' '\n> \tgit init server &&\n> \ttest_commit -C server to_fetch &&\n> @@ -42,7 +55,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc\n> \t# \"c1\" has no parent, it is still sent as \"have\" even though it would\n> \t# normally be skipped.\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n> +\ttrace_fetch client \"$(pwd)/server\" &&\n> \thave_sent c7 c5 c2 c1 &&\n> \thave_not_sent c6 c4 c3\n> '\n> @@ -65,7 +78,7 @@ test_expect_success 'when two skips collide, favor the larger one' '\n> \t# the next \"have\" sent will be \"c1\" (from \"c6\" skip 4) and not \"c4\"\n> \t# (from \"c5side\" skip 1).\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n> +\ttrace_fetch client \"$(pwd)/server\" &&\n> \thave_sent c5side c11 c9 c6 c1 &&\n> \thave_not_sent c10 c8 c7 c5 c4 c3 c2\n> '\n> @@ -91,7 +104,7 @@ test_expect_success 'use ref advertisement to filter out commits' '\n> \t# not need to send any ancestors of \"c3\", but we still need to send \"c3\"\n> \t# itself.\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch origin to_fetch &&\n> +\ttrace_fetch client origin to_fetch &&\n> \thave_sent c5 c4^ c2side &&\n> \thave_not_sent c4 c4^^ c4^^^\n> '\n> @@ -121,7 +134,7 @@ test_expect_success 'handle clock skew' '\n> \t# and sent, because (due to clock skew) its only parent has already been\n> \t# popped off the priority queue.\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n> +\ttrace_fetch client \"$(pwd)/server\" &&\n> \thave_sent c2 c1 old4 old2 old1 &&\n> \thave_not_sent old3\n> '\n> @@ -153,7 +166,7 @@ test_expect_success 'do not send \"have\" with ancestors of commits that server AC\n> \ttest_commit -C server commit-on-b1 &&\n> \n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" to_fetch &&\n> +\ttrace_fetch client \"$(pwd)/server\" to_fetch &&\n> \tgrep \" fetch\" trace &&\n> \n> \t# fetch-pack sends 2 requests each containing 16 \"have\" lines before\n"},{"id":"355085","messageId":"xmqq1sb69psp.fsf@gitster-ct.c.googlers.com","threadId":"49085","inReplyTo":"20180810140908.GA23507@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-10T15:58:30Z","receivedAt":"2018-08-10T15:58:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Here it is as a patch on top of jt/fetch-negotiator-skipping, which lets\n> us pursue any fix for interleaved trace output on Windows without the\n> pressure of an impending flaky test.\n>\n> My gut says that looking into making O_APPEND work there is going to be\n> the nicest solution, but my gut is not very well versed in Windows\n> subtleties. ;)\n\nAs we know these tests are only interested in \"fetch\" lines and not\nrecord of communication in the other direction, I think this is a\nnice clean-up that is worth doing.\n\nFor only two grep's it may not look worth doing, but it would be\nalso a good clean-up to give an got_acked helper that sits next to\nhave_sent and have_not_sent helpers for readability. That is of\ncourse outside the scope of this change.\n\nWill queue. Thanks.\n\n> -- >8 --\n> Subject: [PATCH] t5552: suppress upload-pack trace output\n>\n> The t5552 test script uses GIT_TRACE_PACKET to monitor what\n> git-fetch sends and receives. However, because we're\n> accessing a local repository, the child upload-pack also\n> sends trace output to the same file.\n>\n> On Linux, this works out OK. We open the trace file with\n> O_APPEND, so all writes are atomically positioned at the end\n> of the file. No data can be overwritten or omitted. And\n> since we prepare our small writes in a strbuf and write them\n> with a single write(), we should see each line as an atomic\n> unit. The order of lines between the two processes is\n> undefined, but the test script greps only for \"fetch>\" or\n> \"fetch<\" lines. So under Linux, the test results are\n> deterministic.\n>\n> The test fails intermittently on Windows, however,\n> reportedly even overwriting bits of the output file (i.e.,\n> O_APPEND does not seem to give us an atomic position+write).\n>\n> Since the test only cares about the trace output from fetch,\n> we can just disable the output from upload-pack. That\n> doesn't solve the greater question of O_APPEND/trace issues\n> under Windows, but it easily fixes the flakiness from this\n> test.\n>\n> Reported-by: Johannes Schindelin \n> Signed-off-by: Jeff King \n> ---\n> I'm assuming that this really isn't triggerable on Linux. I tried and\n> couldn't manage to get it to fail, and the reasoning above explains why.\n> But I wasn't 100% clear that Dscho hadn't seen it fail on non-Windows.\n>\n> t/t5552-skipping-fetch-negotiator.sh | 23 ++++++++++++++++++-----\n> 1 file changed, 18 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\n> index 0a8e0e42ed..0ad50dd839 100755\n> --- a/t/t5552-skipping-fetch-negotiator.sh\n> +++ b/t/t5552-skipping-fetch-negotiator.sh\n> @@ -28,6 +28,19 @@ have_not_sent () {\n> \tdone\n> }\n> \n> +# trace_fetch [args]\n> +#\n> +# Trace the packet output of fetch, but make sure we disable the variable\n> +# in the child upload-pack, so we don't combine the results in the same file.\n> +trace_fetch () {\n> +\tclient=$1; shift\n> +\tserver=$1; shift\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" \\\n> +\tgit -C \"$client\" fetch \\\n> +\t --upload-pack 'unset GIT_TRACE_PACKET; git-upload-pack' \\\n> +\t \"$server\" \"$@\"\n> +}\n> +\n> test_expect_success 'commits with no parents are sent regardless of skip distance' '\n> \tgit init server &&\n> \ttest_commit -C server to_fetch &&\n> @@ -42,7 +55,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc\n> \t# \"c1\" has no parent, it is still sent as \"have\" even though it would\n> \t# normally be skipped.\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n> +\ttrace_fetch client \"$(pwd)/server\" &&\n> \thave_sent c7 c5 c2 c1 &&\n> \thave_not_sent c6 c4 c3\n> '\n> @@ -65,7 +78,7 @@ test_expect_success 'when two skips collide, favor the larger one' '\n> \t# the next \"have\" sent will be \"c1\" (from \"c6\" skip 4) and not \"c4\"\n> \t# (from \"c5side\" skip 1).\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n> +\ttrace_fetch client \"$(pwd)/server\" &&\n> \thave_sent c5side c11 c9 c6 c1 &&\n> \thave_not_sent c10 c8 c7 c5 c4 c3 c2\n> '\n> @@ -91,7 +104,7 @@ test_expect_success 'use ref advertisement to filter out commits' '\n> \t# not need to send any ancestors of \"c3\", but we still need to send \"c3\"\n> \t# itself.\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch origin to_fetch &&\n> +\ttrace_fetch client origin to_fetch &&\n> \thave_sent c5 c4^ c2side &&\n> \thave_not_sent c4 c4^^ c4^^^\n> '\n> @@ -121,7 +134,7 @@ test_expect_success 'handle clock skew' '\n> \t# and sent, because (due to clock skew) its only parent has already been\n> \t# popped off the priority queue.\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n> +\ttrace_fetch client \"$(pwd)/server\" &&\n> \thave_sent c2 c1 old4 old2 old1 &&\n> \thave_not_sent old3\n> '\n> @@ -153,7 +166,7 @@ test_expect_success 'do not send \"have\" with ancestors of commits that server AC\n> \ttest_commit -C server commit-on-b1 &&\n> \n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" to_fetch &&\n> +\ttrace_fetch client \"$(pwd)/server\" to_fetch &&\n> \tgrep \" fetch\" trace &&\n> \n> \t# fetch-pack sends 2 requests each containing 16 \"have\" lines before\n"},{"id":"355088","messageId":"811ded48-6f33-c46e-7bae-b9f7c7e8764c@kdbg.org","threadId":"49085","inReplyTo":"pull.17.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-08-10T16:15:36Z","receivedAt":"2018-08-10T16:15:41Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 09.08.2018 um 19:35 schrieb Johannes Schindelin via GitGitGadget:\n> I reported a couple of times that t5552 is not passing reliably. It has now\n> reached next, and will no doubt infect master soon.\n> \n> Turns out that it is not a Windows-specific issue, even if it occurs a lot\n> more often on Windows than elsewhere.\n> \n> The culprit is that two processes try simultaneously to write to the same\n> file specified via GIT_TRACE_PACKET, and it is not well defined how that\n> should work, even on Linux.\n\nThanks for digging down to the root cause. As has been said, the \nbehavior of O_APPEND is well-defined under POSIX, but last time I looked \nfor equivalent feature on Windows, I did not find any.\n\nLast time was when I worked around the same failure in \nt5503-tagfollow.sh in my private builds:\n\nhttps://github.com/j6t/git/commit/9a447a6844b50b43746d9765b3ac809e2793d742\n\nIt is basically the same as Peff suggests: log only one side of the fetch.\n\nAs this buglet looks like a recurring theme, and a proper fix is \npreferable over repeated work-arounds. To me it looks like we need some \nsort of locking on Windows. Unless your friends at Microsoft have an ace \nin their sleeves that lets us have atomic O_APPEND the POSIX way...\n\n-- Hannes\n"},{"id":"355092","messageId":"nycvar.QRO.7.76.6.1808101833330.71@tvgsbejvaqbjf.bet","threadId":"49085","inReplyTo":"20180810140908.GA23507@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-10T16:43:07Z","receivedAt":"2018-08-10T16:43:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 10 Aug 2018, Jeff King wrote:\n\n> On Thu, Aug 09, 2018 at 01:49:52PM -0700, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > Are you sure that it's not well-defined? We open the path with O_APPEND,\n> > > which means every write() will be atomically positioned at the end of\n> > > file. So we would never lose or overwrite data.\n> > >\n> > > We do our own buffering in a strbuf, writing the result out in a single\n> > > write() call (modulo the OS returning a short write, but that should not\n> > > generally happen when writing short strings to a file). So we should get\n> > > individual trace lines as atomic units.\n> > >\n> > > The order of lines from the two processes is undefined, of course.\n> > \n> > Correct. But I am more worried about the \"mixed/overwriting\"\n> > breakage, if there is one; it means we may need to be prepared for\n> > systems that lack O_APPEND that works correctly. I initially just\n> > assumed that it was what Dscho was seeing, but after re-reading his\n> > message, I am not sure anymore.\n> > \n> > I think the \"do not trace the other side\" approach you suggest for\n> > these tests that only care about one side is more appropriate\n> > solution for this particular case. We then do not have to worry\n> > about overwriting or output from both sides mixed randomly.\n\nJust so everybody knows: I never received the above two mails. Something\nis seriously rotten in GMX. This seems to have started a week or two ago.\n\n> Here it is as a patch on top of jt/fetch-negotiator-skipping, which lets\n> us pursue any fix for interleaved trace output on Windows without the\n> pressure of an impending flaky test.\n> \n> My gut says that looking into making O_APPEND work there is going to be\n> the nicest solution, but my gut is not very well versed in Windows\n> subtleties. ;)\n\nYour patch seems to be a patch in the original sense of the word: it is\nnot a solution.\n\nAnd let me spell it out very clearly (as I have mentioned to Jonathan a\ncouple times, without any response): t5552 fails regularly, way more than\nrarely, see for yourself:\n\nhttps://git-for-windows.visualstudio.com/git/_build/index?definitionId=1&_a=history\n\nAnd the reason it fails was analyzed by me and described in the commit\nmessage (I am sorry that the cover letter was still stale and talked\nabout flock(), which I had decided was not the right approach after\nfighting with Linux over it).\n\nWhat you cannot see easily, unless you go the route that I offered\nJonathan (open a PR on gitgitgadget/git which will automatically run the\ntest suite on Windows, macOS and Linux, and of course you can do anything\nin a PR, including narrowing down what tests are run) is that sometimes\nthose lines in the `trace` file were clearly *incomplete*. That is, even\nif both processes tried to write atomically, one managed to overwrite the\nother's buffer in flight.\n\nThis is a pretty serious thing, even worse than the failing test suite,\nand I don't think that your patch acknowledges how serious this is.\n\nAnd please don't give me that \"but it works on Linux\" response. Seriously.\nSheesh.\n\nEven if you have not managed to trigger it, the POSIX standard seems not\nto define clearly what the behavior is of two competing write() calls,\nunless you lock the files appropriately, as my patch series does.\n\nSo unless you are willing to ignore, to willfully keep this breakage, I\nwould suggest not to introduce the ugliness of an overridden upload-pack\nfor the sole purpose of disabling the tracing on one side, but instead to\nget this here bug fixed, by helping me with this here patch series.\n\nWe don't have to rush this, you know? We have to fix it, though.\n\nThank you,\nDscho\n\n> -- >8 --\n> Subject: [PATCH] t5552: suppress upload-pack trace output\n> \n> The t5552 test script uses GIT_TRACE_PACKET to monitor what\n> git-fetch sends and receives. However, because we're\n> accessing a local repository, the child upload-pack also\n> sends trace output to the same file.\n> \n> On Linux, this works out OK. We open the trace file with\n> O_APPEND, so all writes are atomically positioned at the end\n> of the file. No data can be overwritten or omitted. And\n> since we prepare our small writes in a strbuf and write them\n> with a single write(), we should see each line as an atomic\n> unit. The order of lines between the two processes is\n> undefined, but the test script greps only for \"fetch>\" or\n> \"fetch<\" lines. So under Linux, the test results are\n> deterministic.\n> \n> The test fails intermittently on Windows, however,\n> reportedly even overwriting bits of the output file (i.e.,\n> O_APPEND does not seem to give us an atomic position+write).\n> \n> Since the test only cares about the trace output from fetch,\n> we can just disable the output from upload-pack. That\n> doesn't solve the greater question of O_APPEND/trace issues\n> under Windows, but it easily fixes the flakiness from this\n> test.\n> \n> Reported-by: Johannes Schindelin \n> Signed-off-by: Jeff King \n> ---\n> I'm assuming that this really isn't triggerable on Linux. I tried and\n> couldn't manage to get it to fail, and the reasoning above explains why.\n> But I wasn't 100% clear that Dscho hadn't seen it fail on non-Windows.\n> \n> t/t5552-skipping-fetch-negotiator.sh | 23 ++++++++++++++++++-----\n> 1 file changed, 18 insertions(+), 5 deletions(-)\n> \n> diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\n> index 0a8e0e42ed..0ad50dd839 100755\n> --- a/t/t5552-skipping-fetch-negotiator.sh\n> +++ b/t/t5552-skipping-fetch-negotiator.sh\n> @@ -28,6 +28,19 @@ have_not_sent () {\n> \tdone\n> }\n> \n> +# trace_fetch [args]\n> +#\n> +# Trace the packet output of fetch, but make sure we disable the variable\n> +# in the child upload-pack, so we don't combine the results in the same file.\n> +trace_fetch () {\n> +\tclient=$1; shift\n> +\tserver=$1; shift\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" \\\n> +\tgit -C \"$client\" fetch \\\n> +\t --upload-pack 'unset GIT_TRACE_PACKET; git-upload-pack' \\\n> +\t \"$server\" \"$@\"\n> +}\n> +\n> test_expect_success 'commits with no parents are sent regardless of skip distance' '\n> \tgit init server &&\n> \ttest_commit -C server to_fetch &&\n> @@ -42,7 +55,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc\n> \t# \"c1\" has no parent, it is still sent as \"have\" even though it would\n> \t# normally be skipped.\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n> +\ttrace_fetch client \"$(pwd)/server\" &&\n> \thave_sent c7 c5 c2 c1 &&\n> \thave_not_sent c6 c4 c3\n> '\n> @@ -65,7 +78,7 @@ test_expect_success 'when two skips collide, favor the larger one' '\n> \t# the next \"have\" sent will be \"c1\" (from \"c6\" skip 4) and not \"c4\"\n> \t# (from \"c5side\" skip 1).\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n> +\ttrace_fetch client \"$(pwd)/server\" &&\n> \thave_sent c5side c11 c9 c6 c1 &&\n> \thave_not_sent c10 c8 c7 c5 c4 c3 c2\n> '\n> @@ -91,7 +104,7 @@ test_expect_success 'use ref advertisement to filter out commits' '\n> \t# not need to send any ancestors of \"c3\", but we still need to send \"c3\"\n> \t# itself.\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch origin to_fetch &&\n> +\ttrace_fetch client origin to_fetch &&\n> \thave_sent c5 c4^ c2side &&\n> \thave_not_sent c4 c4^^ c4^^^\n> '\n> @@ -121,7 +134,7 @@ test_expect_success 'handle clock skew' '\n> \t# and sent, because (due to clock skew) its only parent has already been\n> \t# popped off the priority queue.\n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n> +\ttrace_fetch client \"$(pwd)/server\" &&\n> \thave_sent c2 c1 old4 old2 old1 &&\n> \thave_not_sent old3\n> '\n> @@ -153,7 +166,7 @@ test_expect_success 'do not send \"have\" with ancestors of commits that server AC\n> \ttest_commit -C server commit-on-b1 &&\n> \n> \ttest_config -C client fetch.negotiationalgorithm skipping &&\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" to_fetch &&\n> +\ttrace_fetch client \"$(pwd)/server\" to_fetch &&\n> \tgrep \" fetch\" trace &&\n> \n> \t# fetch-pack sends 2 requests each containing 16 \"have\" lines before\n> -- \n> 2.18.0.1058.g0926f0b71f\n> \n> \n"},{"id":"355093","messageId":"70b25098-bf96-d362-56cd-2bb17cadf162@jeffhostetler.com","threadId":"49085","inReplyTo":"811ded48-6f33-c46e-7bae-b9f7c7e8764c@kdbg.org","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-08-10T16:51:23Z","receivedAt":"2018-08-10T16:51:27Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/10/2018 12:15 PM, Johannes Sixt wrote:\n> Am 09.08.2018 um 19:35 schrieb Johannes Schindelin via GitGitGadget:\n>> I reported a couple of times that t5552 is not passing reliably. It \n>> has now\n>> reached next, and will no doubt infect master soon.\n>>\n>> Turns out that it is not a Windows-specific issue, even if it occurs a \n>> lot\n>> more often on Windows than elsewhere.\n>>\n>> The culprit is that two processes try simultaneously to write to the same\n>> file specified via GIT_TRACE_PACKET, and it is not well defined how that\n>> should work, even on Linux.\n> \n> Thanks for digging down to the root cause. As has been said, the \n> behavior of O_APPEND is well-defined under POSIX, but last time I looked \n> for equivalent feature on Windows, I did not find any.\n> \n> Last time was when I worked around the same failure in \n> t5503-tagfollow.sh in my private builds:\n> \n> https://github.com/j6t/git/commit/9a447a6844b50b43746d9765b3ac809e2793d742\n> \n> It is basically the same as Peff suggests: log only one side of the fetch.\n> \n> As this buglet looks like a recurring theme, and a proper fix is \n> preferable over repeated work-arounds. To me it looks like we need some \n> sort of locking on Windows. Unless your friends at Microsoft have an ace \n> in their sleeves that lets us have atomic O_APPEND the POSIX way...\n\nThe official Windows CRT for open() does document an O_APPEND, but after\nlooking at the distributed source, I'm not sure I trust it.\n\nI have not looked at the MSYS version of the CRT yet so I cannot comment\non it.\n\nI did find that the following code does what we want. Notice the use of\nFILE_APPEND_DATA in place of the usual GENERIC_READ. (And yes, the docs\nare very vague here.)\n\nBefore we try a locking solution, perhaps we should tweak mingw_open()\nto use something like this to get a proper HANDLE directly and then use\n_open_osfhandle() to get a \"fd\" for it.\n\nJeff\n\n---------------------------------------------------------------------\n\n#include \n\nint main()\n{\n\tHANDLE h = CreateFileW(L\"foo.txt\",\n\t\tFILE_APPEND_DATA,\n\t\tFILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,\n\t\tNULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);\n\n\tconst char * buf = \"this is a test\\n\";\n\tDWORD len = strlen(buf);\n\tDWORD nbw;\n\n\tWriteFile(h, buf, len, &nbw, NULL);\n\tCloseHandle(h);\n\n\treturn 0;\n}\n"},{"id":"355116","messageId":"5773077b-8912-c6be-9017-1fc1f3e13b02@jeffhostetler.com","threadId":"49085","inReplyTo":"70b25098-bf96-d362-56cd-2bb17cadf162@jeffhostetler.com","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-08-10T16:57:34Z","receivedAt":"2018-08-10T16:57:37Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/10/2018 12:51 PM, Jeff Hostetler wrote:\n> \n> \n> On 8/10/2018 12:15 PM, Johannes Sixt wrote:\n>> Am 09.08.2018 um 19:35 schrieb Johannes Schindelin via GitGitGadget:\n>>> I reported a couple of times that t5552 is not passing reliably. It \n>>> has now\n>>> reached next, and will no doubt infect master soon.\n>>>\n>>> Turns out that it is not a Windows-specific issue, even if it occurs \n>>> a lot\n>>> more often on Windows than elsewhere.\n>>>\n>>> The culprit is that two processes try simultaneously to write to the \n>>> same\n>>> file specified via GIT_TRACE_PACKET, and it is not well defined how that\n>>> should work, even on Linux.\n>>\n>> Thanks for digging down to the root cause. As has been said, the \n>> behavior of O_APPEND is well-defined under POSIX, but last time I \n>> looked for equivalent feature on Windows, I did not find any.\n>>\n>> Last time was when I worked around the same failure in \n>> t5503-tagfollow.sh in my private builds:\n>>\n>> https://github.com/j6t/git/commit/9a447a6844b50b43746d9765b3ac809e2793d742 \n>>\n>>\n>> It is basically the same as Peff suggests: log only one side of the \n>> fetch.\n>>\n>> As this buglet looks like a recurring theme, and a proper fix is \n>> preferable over repeated work-arounds. To me it looks like we need \n>> some sort of locking on Windows. Unless your friends at Microsoft have \n>> an ace in their sleeves that lets us have atomic O_APPEND the POSIX \n>> way...\n> \n> The official Windows CRT for open() does document an O_APPEND, but after\n> looking at the distributed source, I'm not sure I trust it.\n> \n> I have not looked at the MSYS version of the CRT yet so I cannot comment\n> on it.\n> \n> I did find that the following code does what we want.  Notice the use of\n> FILE_APPEND_DATA in place of the usual GENERIC_READ.  (And yes, the docs\n> are very vague here.)\n\nd'oh\n\ns/GENERIC_READ/GENERIC_WRITE/\n\n> \n> Before we try a locking solution, perhaps we should tweak mingw_open()\n> to use something like this to get a proper HANDLE directly and then use\n> _open_osfhandle() to get a \"fd\" for it.\n> \n> Jeff\n> \n> ---------------------------------------------------------------------\n> \n> #include \n> \n> int main()\n> {\n>     HANDLE h = CreateFileW(L\"foo.txt\",\n>         FILE_APPEND_DATA,\n>         FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,\n>         NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);\n> \n>     const char * buf = \"this is a test\\n\";\n>     DWORD len = strlen(buf);\n>     DWORD nbw;\n> \n>     WriteFile(h, buf, len, &nbw, NULL);\n>     CloseHandle(h);\n> \n>     return 0;\n> }\n"},{"id":"355118","messageId":"0eb43493-ca10-dd09-efdb-93783cc01611@kdbg.org","threadId":"49085","inReplyTo":"70b25098-bf96-d362-56cd-2bb17cadf162@jeffhostetler.com","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-08-10T17:08:40Z","receivedAt":"2018-08-10T17:08:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.08.2018 um 18:51 schrieb Jeff Hostetler:\n> \n> \n> On 8/10/2018 12:15 PM, Johannes Sixt wrote:\n>> Am 09.08.2018 um 19:35 schrieb Johannes Schindelin via GitGitGadget:\n>>> I reported a couple of times that t5552 is not passing reliably. It \n>>> has now\n>>> reached next, and will no doubt infect master soon.\n>>>\n>>> Turns out that it is not a Windows-specific issue, even if it occurs \n>>> a lot\n>>> more often on Windows than elsewhere.\n>>>\n>>> The culprit is that two processes try simultaneously to write to the \n>>> same\n>>> file specified via GIT_TRACE_PACKET, and it is not well defined how that\n>>> should work, even on Linux.\n>>\n>> Thanks for digging down to the root cause. As has been said, the \n>> behavior of O_APPEND is well-defined under POSIX, but last time I \n>> looked for equivalent feature on Windows, I did not find any.\n>>\n>> Last time was when I worked around the same failure in \n>> t5503-tagfollow.sh in my private builds:\n>>\n>> https://github.com/j6t/git/commit/9a447a6844b50b43746d9765b3ac809e2793d742 \n>>\n>>\n>> It is basically the same as Peff suggests: log only one side of the \n>> fetch.\n>>\n>> As this buglet looks like a recurring theme, and a proper fix is \n>> preferable over repeated work-arounds. To me it looks like we need \n>> some sort of locking on Windows. Unless your friends at Microsoft have \n>> an ace in their sleeves that lets us have atomic O_APPEND the POSIX \n>> way...\n> \n> The official Windows CRT for open() does document an O_APPEND, but after\n> looking at the distributed source, I'm not sure I trust it.\n\nI looked at the CRT code, too, and no, we can't trust it.\n\n> I have not looked at the MSYS version of the CRT yet so I cannot comment\n> on it.\n> \n> I did find that the following code does what we want.  Notice the use of\n> FILE_APPEND_DATA in place of the usual GENERIC_READ.  (And yes, the docs\n> are very vague here.)\n\nAs far as I understand, FILE_APPEND_DATA is just a permission flag (I'm \nnot sure why that would be necessary), but at least the documentation \nstates that it is still necessary for the caller to set the file pointer.\n\nBut I haven't tried your code below, yet. Should it append the line of \ntext on each invocation? And if so, is it an atomic operation?\n\n> Before we try a locking solution, perhaps we should tweak mingw_open()\n> to use something like this to get a proper HANDLE directly and then use\n> _open_osfhandle() to get a \"fd\" for it.\n> \n> Jeff\n> \n> ---------------------------------------------------------------------\n> \n> #include \n> \n> int main()\n> {\n>     HANDLE h = CreateFileW(L\"foo.txt\",\n>         FILE_APPEND_DATA,\n>         FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,\n>         NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);\n> \n>     const char * buf = \"this is a test\\n\";\n>     DWORD len = strlen(buf);\n>     DWORD nbw;\n> \n>     WriteFile(h, buf, len, &nbw, NULL);\n>     CloseHandle(h);\n> \n>     return 0;\n> }\n> \n\n"},{"id":"355119","messageId":"20180810171546.GA32713@sigill.intra.peff.net","threadId":"49085","inReplyTo":"nycvar.QRO.7.76.6.1808101833330.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-10T17:15:46Z","receivedAt":"2018-08-10T17:15:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 10, 2018 at 06:43:07PM +0200, Johannes Schindelin wrote:\n\n> > > Correct. But I am more worried about the \"mixed/overwriting\"\n> > > breakage, if there is one; it means we may need to be prepared for\n> > > systems that lack O_APPEND that works correctly. I initially just\n> > > assumed that it was what Dscho was seeing, but after re-reading his\n> > > message, I am not sure anymore.\n> > > \n> > > I think the \"do not trace the other side\" approach you suggest for\n> > > these tests that only care about one side is more appropriate\n> > > solution for this particular case. We then do not have to worry\n> > > about overwriting or output from both sides mixed randomly.\n> \n> Just so everybody knows: I never received the above two mails. Something\n> is seriously rotten in GMX. This seems to have started a week or two ago.\n\nI think it may just have been that they went to the gitgitgadget\naddress; I explicitly added you as a cc when I sent the patch with a\ncommit message, which was when I first noticed you weren't cc'd on the\nearlier bits.\n\n> What you cannot see easily, unless you go the route that I offered\n> Jonathan (open a PR on gitgitgadget/git which will automatically run the\n> test suite on Windows, macOS and Linux, and of course you can do anything\n> in a PR, including narrowing down what tests are run) is that sometimes\n> those lines in the `trace` file were clearly *incomplete*. That is, even\n> if both processes tried to write atomically, one managed to overwrite the\n> other's buffer in flight.\n> \n> This is a pretty serious thing, even worse than the failing test suite,\n> and I don't think that your patch acknowledges how serious this is.\n\nI don't see anything in what you wrote above or in your original cover\nletter or series that contradicts the notion that O_APPEND is simply not\natomic on Windows. Overwriting the same byte ranges in the file is what\nI would expect on Linux, as well, if the file were opened without\nO_APPEND.\n\nI have mixed reports from searching online whether an equivalent to\nO_APPEND exists. Some sources seem to say that FILE_APPEND_DATA is. I\ncouldn't find anything definitive in Microsoft documentation about\nwhether it actually provides atomicity. From my (admittedly limited)\ndigging, that does not seem to be used by the msys2 runtime, so it might\nbe worth exploring.\n\n> And please don't give me that \"but it works on Linux\" response. Seriously.\n> Sheesh.\n\nBut it does. And it's designed to. There is literally nothing to fix on\nLinux.\n\n> Even if you have not managed to trigger it, the POSIX standard seems not\n> to define clearly what the behavior is of two competing write() calls,\n> unless you lock the files appropriately, as my patch series does.\n\nPOSIX says:\n\n If the O_APPEND flag of the file status flags is set, the file offset\n shall be set to the end of the file prior to each write and no\n intervening file modification operation shall occur between changing\n the file offset and the write operation.\n\nand:\n\n This volume of POSIX.1-2017 does not specify the behavior of\n concurrent writes to a regular file from multiple threads, except that\n each write is atomic.\n\nI admit the writing is a little turgid, but I think between the two it\nis promising the behavior that we expect (and I think that\ninterpretation is pretty common in the Unix world).\n\n> So unless you are willing to ignore, to willfully keep this breakage, I\n> would suggest not to introduce the ugliness of an overridden upload-pack\n> for the sole purpose of disabling the tracing on one side, but instead to\n> get this here bug fixed, by helping me with this here patch series.\n\nI'm OK if you want to live with the broken test in the interim. But\nafter your complaining about the imminence of it \"infecting\" master, I\nthought you might be happy to see a solution that was more immediate.\n\nI have tried to help with the actual problem, by identifying the root\ncause (that the trace code's strategy is not fundamentally broken, but\nthat it relies on O_APPEND whose guarantees are obviously not portable\nto Windows) and exploring possible solutions there (FILE_APPEND_DATA).\n\nLacking any kind of Windows development environment, I'd just as soon\nstop there and let more knowledgeable people take over. But please don't\ncharacterize my presence in this thread as some kind of willful harm or\nignoring of the problem.\n\n-Peff\n"},{"id":"355126","messageId":"nycvar.QRO.7.76.6.1808101843520.71@tvgsbejvaqbjf.bet","threadId":"49085","inReplyTo":"xmqqtvo3bc0g.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/4] Introduce a function to lock/unlock file descriptors when appending","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-10T18:32:07Z","receivedAt":"2018-08-10T18:32:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 9 Aug 2018, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > From: Johannes Schindelin \n> >\n> > This function will be used to make write accesses in trace_write() a bit\n> > safer.\n> > ...\n> > To set a precedent for a better approach, let's introduce a proper\n> > abstraction: a function that says in its name precisely what Git\n> > wants it to do (as opposed to *how* it does it on Linux):\n> > lock_or_unlock_fd_for_appending().\n> >\n> > The next commit will provide a Windows-specific implementation of this\n> > function/functionality.\n> >\n> > Signed-off-by: Johannes Schindelin \n> >\n> > squash! Introduce a function to lock/unlock file descriptors when appending\n> \n> If we can keep the custom, narrow and easy-to-port API (like this\n> patch introduces) focused and resist feature creep over time, then\n> it would be worth spending effort to come up with such a custom\n> helper that is easy to port. So I agree with the approach in\n> general but I tend to think \"set a precedent for a better approach\"\n> is a way-too-early and wishful verdict. We do not know if we can\n> really keep that custom API easy-to-port-and-maintain yet.\n> \n> In short, even though I agree with the approach, most of the\n> verbiage above is unnecessary and mere distraction.\n\nI disagree that it is a distraction, because the commit messages are the\nvery location where I am supposed to detail my rationale, motivation, and\nconsiderations that are not obvious from the diff alone.\n\nOf course, I cannot force you to take this commit message, you can\noverrule me and edit it. But you would do this against my express wish.\n\n> > ---\n> > git-compat-util.h | 2 ++\n> > wrapper.c | 14 ++++++++++++++\n> > 2 files changed, 16 insertions(+)\n> >\n> > diff --git a/git-compat-util.h b/git-compat-util.h\n> > index 9a64998b2..13b83bade 100644\n> > --- a/git-compat-util.h\n> > +++ b/git-compat-util.h\n> > @@ -1202,6 +1202,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);\n> > #define getc_unlocked(fh) getc(fh)\n> > #endif\n> > \n> > +extern int lock_or_unlock_fd_for_appending(int fd, int lock_it);\n> > +\n> > /*\n> > * Our code often opens a path to an optional file, to work on its\n> > * contents when we can successfully open it. We can ignore a failure\n> > diff --git a/wrapper.c b/wrapper.c\n> > index e4fa9d84c..6c2116272 100644\n> > --- a/wrapper.c\n> > +++ b/wrapper.c\n> > @@ -690,3 +690,17 @@ int xgethostname(char *buf, size_t len)\n> > \t\tbuf[len - 1] = 0;\n> > \treturn ret;\n> > }\n> > +\n> > +#ifndef GIT_WINDOWS_NATIVE\n> > +int lock_or_unlock_fd_for_appending(int fd, int lock_it)\n> > +{\n> > +\tstruct flock flock;\n> > +\n> > +\tflock.l_type = lock_it ? F_WRLCK : F_UNLCK;\n> > +\tflock.l_whence = SEEK_SET;\n> > +\tflock.l_start = 0;\n> > +\tflock.l_len = 0xffffffff; /* arbitrary number of bytes */\n> \n> If this can be an arbitrary range, do we need to cover this many (or\n> only this few, depending on your point of view) bytes?\n\nIt can be an arbitrary range, but it does not matter at this point because\nwe expect only appending callers. Therefore any range will do, as long as\nit covers the range of bytes to be written by the trace functions. And\nwith 0-0xffffffff, I am fairly certain we got it.\n\nTechnically, we could even lock the range 0-1, as all of our callers would\nagree on that, and block each other. Other Git implementations might not,\nthough. So 0-0xffffffff is my best bet and cheap.\n\n> Would it be sufficient to cover just the first one byte instead?\n\nAs I said, this would depend on no other software trying to append to the\ntrace file, including alternative Git implementations.\n\nIn other words: it would be \"too clever\". Clever, but asking for problems.\n\n> Or perhaps give l_len==0 to cover all no matter how large the file\n> grows, which sounds like a better range specification.\n\nI must have overlooked that option in the documentation.\n\n*clicketyclick*\n\nIndeed,\nhttp://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html\nspells it out pretty clearly:\n\n> A lock shall be set to extend to the largest possible value of the file\n> offset for that file by setting l_len to 0. If such a lock also has\n> l_start set to 0 and l_whence is set to SEEK_SET, the whole file shall\n> be locked.\n\nSorry about missing this. I will change the implementation to this.\n\n> > +\treturn fcntl(fd, F_SETLKW, &flock);\n> > +}\n> > +#endif\n\nThanks for helping me improve the patch,\nDscho\n"},{"id":"355127","messageId":"xmqqin4i83zg.fsf@gitster-ct.c.googlers.com","threadId":"49085","inReplyTo":"811ded48-6f33-c46e-7bae-b9f7c7e8764c@kdbg.org","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-10T18:34:59Z","receivedAt":"2018-08-10T18:35:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> As this buglet looks like a recurring theme, and a proper fix is\n> preferable over repeated work-arounds. To me it looks like we need\n> some sort of locking on Windows. Unless your friends at Microsoft have\n> an ace in their sleeves that lets us have atomic O_APPEND the POSIX\n> way...\n\nJust to put the severity of the issue in context, we use O_APPEND in\na few codepaths, and the trace thing for debugging is the only thing\nthat could have multiple writers. Other users of O_APPEND are:\n\n * refs/files-backend.c uses it so that a reflog entry can be\n appended at the end, but because update to each ref is protected\n from racing at a lot higher layer with a lock, no two people\n would try to append to the same reflog file, so atomicity of\n O_APPEND does not matter here.\n\n * sequencer.c wants to use it when moving one insn from the todo\n list to the 'done' list when it finishes one operation. If you\n are running two sequences in a single repository, intermixed\n 'done' list would be the least of your problem, so presumably we\n are fine here.\n\nIt may make sense to allow GIT_TRACE to have a placeholder\n(e.g. \"/tmp/traceout.$$\") to help debuggers arrange to give\ndifferent processes their own trace output file, which perhaps may\nbe a simpler and less impactful to the performance solution than\nhaving to make locks at an application layer.\n\n\n"},{"id":"355129","messageId":"xmqqeff683qv.fsf@gitster-ct.c.googlers.com","threadId":"49085","inReplyTo":"20180810171546.GA32713@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-10T18:40:08Z","receivedAt":"2018-08-10T18:40:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I have tried to help with the actual problem, by identifying the root\n> cause (that the trace code's strategy is not fundamentally broken, but\n> that it relies on O_APPEND whose guarantees are obviously not portable\n> to Windows) and exploring possible solutions there (FILE_APPEND_DATA).\n>\n> Lacking any kind of Windows development environment, I'd just as soon\n> stop there and let more knowledgeable people take over. But please don't\n> characterize my presence in this thread as some kind of willful harm or\n> ignoring of the problem.\n\nThanks.\n\nIt seems like the other Johannes and the other Jeff are also\ninterested, so we may hopefully see a properly fixed O_APPEND,\nwhich would be the real solution that would help appenders\nthat are *not* tracing subsystem, too.\n\n\n\n"},{"id":"355132","messageId":"20180810185618.GA9749@sigill.intra.peff.net","threadId":"49085","inReplyTo":"xmqqin4i83zg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-10T18:56:18Z","receivedAt":"2018-08-10T18:56:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 10, 2018 at 11:34:59AM -0700, Junio C Hamano wrote:\n\n> Johannes Sixt writes:\n> \n> > As this buglet looks like a recurring theme, and a proper fix is\n> > preferable over repeated work-arounds. To me it looks like we need\n> > some sort of locking on Windows. Unless your friends at Microsoft have\n> > an ace in their sleeves that lets us have atomic O_APPEND the POSIX\n> > way...\n> \n> Just to put the severity of the issue in context, we use O_APPEND in\n> a few codepaths, and the trace thing for debugging is the only thing\n> that could have multiple writers. Other users of O_APPEND are:\n> \n> * refs/files-backend.c uses it so that a reflog entry can be\n> appended at the end, but because update to each ref is protected\n> from racing at a lot higher layer with a lock, no two people\n> would try to append to the same reflog file, so atomicity of\n> O_APPEND does not matter here.\n\nJust an interesting side note, but I think one of the reasons we use\ndot-locks and not flock() is that the former generally works on network\nfilesystems. I think O_APPEND also is not reliable for non-local files,\nthough, so short of actually dot-locking trace files (yuck) I don't\nthink there's a great solution there.\n\n> It may make sense to allow GIT_TRACE to have a placeholder\n> (e.g. \"/tmp/traceout.$$\") to help debuggers arrange to give\n> different processes their own trace output file, which perhaps may\n> be a simpler and less impactful to the performance solution than\n> having to make locks at an application layer.\n\nHeh, I actually wrote that patch several years ago, as part of an\nabandoned effort to have config-triggered tracing (e.g., on the server\nside where you're getting hundreds of requests and you want to enable\ntracing on a repo for a slice of time).\n\nOne annoying thing for a case like the test in question is that you\ndon't actually know the pid of the process you're interested in. So we'd\ngenerate two trace files, and then you'd have to figure out which one is\nwhich. In my earlier work, I coupled it with some magic to allow\nper-command config, like:\n\n [trace \"fetch\"]\n packet = /tmp/trace.%p\n\nbut you could also imagine a \"%n\" which uses the command name.\n\nI don't remember why I abandoned it, but I think a lot had to do with\nviolating the \"don't look at config\" rules for our repo startup code. A\nlot of that has been fixed since then, but I haven't really needed it.\nIf anybody is really interested, the patches are at:\n\n https://github.com/peff/git jk/trace-stdin\n\n(my ultimate goal was to record stdin for pack-objects to replay slow\nfetches, but I ended up hacking together a patch to pack-objects\ninstead).\n\n-Peff\n"},{"id":"355138","messageId":"nycvar.QRO.7.76.6.1808102129570.71@tvgsbejvaqbjf.bet","threadId":"49085","inReplyTo":"20180810171546.GA32713@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-10T19:34:41Z","receivedAt":"2018-08-10T19:34:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 10 Aug 2018, Jeff King wrote:\n\n> On Fri, Aug 10, 2018 at 06:43:07PM +0200, Johannes Schindelin wrote:\n> \n> > So unless you are willing to ignore, to willfully keep this breakage,\n> > I would suggest not to introduce the ugliness of an overridden\n> > upload-pack for the sole purpose of disabling the tracing on one side,\n> > but instead to get this here bug fixed, by helping me with this here\n> > patch series.\n> \n> I'm OK if you want to live with the broken test in the interim.\n\nI realize that I failed to tell you that I basically spent 2.5 days worth\nof worktime to figure this out and come up with three iterations of the\npatch series (you only saw the latest).\n\nI want this patch series in git.git, maybe not in the current form, but in\none form or another. I don't want to spend any more minute on trying to\nfigure out the same problem with any other regression test (which might\nnot even be as easily worked around as with a semi-simple --upload-pack\noption).\n\nThank you for wanting to help. Please accept my apologies for expressing\nin a poor way that I appreciate your eagerness to provide a patch, but\nthat I think nevertheless that it would be better to work on the GIT_TRACE\nconcurrency problem and its resolution via file locks. It would solve that\nclass of problems, instead of that single regression test being flakey.\n\nSorry,\nDscho\n"},{"id":"355141","messageId":"pull.17.v2.git.gitgitgadget@gmail.com","threadId":"49085","inReplyTo":"pull.17.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-08-10T19:47:18Z","receivedAt":"2018-08-10T19:47:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I reported a couple of times that t5552 is not passing reliably. It has now\nreached next, and will no doubt infect master soon.\n\nTurns out that it is not a Windows-specific issue, even if it occurs a lot \nmore often on Windows than elsewhere. (And even if it is apparently\nimpossible to trigger on Linux.)\n\nThe culprit is that two processes try to write simultaneously to the same\nfile specified via GIT_TRACE_PACKET, and it is not well defined how that\nshould work, even when only looking at the POSIX specification (the\ndocumentation of write()\n[http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html] says\n\"Applications should use some form of concurrency control.\").\n\nThis patch series addresses that by locking the trace fd. I chose to use \nfcntl() for the Unix(-y) platforms we support instead of flock() (even if\nthe latter has much simpler semantics) because fcntl() is in POSIX while \nflock() is not. On Windows, I use the Win32 API function LockFileEx()\n[https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-lockfileex]\n.\n\nOf course, I have to admit that I am not super solid on the fcntl() \nsemantics. Junio was nice enough to educate me on l_len = 0 meaning the\nentire file. If anybody has more experience with locking file descriptors\nreferring not to files, but, say, to pipes or to an interactive terminal, I\nwould be very thankful for help (while the POSIX documentation states that\nthe errno should be EINVAL on a file descriptor that cannot be locked,\napparently macOS sets errno to EBADF when trying to lock a redirected stdout\n)?\n\nChanges since v1:\n\n * Touched up the cover letter and the first commit message (in particular,\n removed the bogus squash! line giving away just how much I rely on the \n --autosquash feature these days).\n * Now we're locking the entire file via l_len = 0 (except on Windows).\n * To cover all bases, EINVAL is now also treated as \"cannot lock this fd\"\n (in addition to EBADF).\n * Removed some superfluous flock()-related left-overs from a previous\n attempt (that caused a lot of me fighting with Linux).\n\nJohannes Schindelin (4):\n Introduce a function to lock/unlock file descriptors when appending\n mingw: implement lock_or_unlock_fd_for_appending()\n trace: lock the trace file to avoid racy trace_write() calls\n trace: verify that locking works\n\n Makefile | 1 +\n compat/mingw.c | 19 ++++++\n compat/mingw.h | 3 +\n git-compat-util.h | 2 +\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/helper/test-trace.c | 130 +++++++++++++++++++++++++++++++++++++++++\n t/t0070-fundamental.sh | 6 ++\n trace.c | 11 +++-\n wrapper.c | 16 +++++\n 10 files changed, 189 insertions(+), 1 deletion(-)\n create mode 100644 t/helper/test-trace.c\n\n\nbase-commit: 42cc7485a2ec49ecc440c921d2eb0cae4da80549\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-17%2Fdscho%2Ffetch-negotiator-skipping-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-17/dscho/fetch-negotiator-skipping-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/17\n\nRange-diff vs v1:\n\n 1: e449ed75f ! 1: a19904682 Introduce a function to lock/unlock file descriptors when appending\n @@ -30,7 +30,7 @@\n added in this commit.\n \n But fcntl() allows a lot more versatile file region locking that we\n - actually need, to by necessity the fcntl() emulation would be quite\n + actually need, so by necessity the fcntl() emulation would be quite\n complex: To support the `l_whence = SEEK_CUR` (which we would use, if it\n did not require so much book-keeping due to our writing between locking\n and unlocking the file), we would have to call `SetFilePointerEx()`\n @@ -47,18 +47,16 @@\n to do, as we would once again fail to have an abstraction that clearly\n says what *exact*functionality we want to use.\n \n - To set a precedent for a better approach, let's introduce a proper\n - abstraction: a function that says in its name precisely what Git\n - wants it to do (as opposed to *how* it does it on Linux):\n - lock_or_unlock_fd_for_appending().\n + This commit expressly tries to set a precedent for a better approach:\n + Let's introduce a proper abstraction: a function that says in its name\n + precisely what Git wants it to do (as opposed to *how* it does it on\n + Linux): lock_or_unlock_fd_for_appending().\n \n The next commit will provide a Windows-specific implementation of this\n function/functionality.\n \n Signed-off-by: Johannes Schindelin \n \n - squash! Introduce a function to lock/unlock file descriptors when appending\n -\n diff --git a/git-compat-util.h b/git-compat-util.h\n --- a/git-compat-util.h\n +++ b/git-compat-util.h\n @@ -86,9 +84,11 @@\n +\tstruct flock flock;\n +\n +\tflock.l_type = lock_it ? F_WRLCK : F_UNLCK;\n ++\n ++\t/* (un-)lock the whole file */\n +\tflock.l_whence = SEEK_SET;\n +\tflock.l_start = 0;\n -+\tflock.l_len = 0xffffffff; /* arbitrary number of bytes */\n ++\tflock.l_len = 0;\n +\n +\treturn fcntl(fd, F_SETLKW, &flock);\n +}\n 2: 8dda2d530 ! 2: fa0c282aa mingw: implement lock_or_unlock_fd_for_appending()\n @@ -54,24 +54,3 @@\n #define has_dos_drive_prefix(path) \\\n \t(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)\n int mingw_skip_dos_drive_prefix(char **path);\n -\n -diff --git a/config.mak.uname b/config.mak.uname\n ---- a/config.mak.uname\n -+++ b/config.mak.uname\n -@@\n - \tNO_POSIX_GOODIES = UnfortunatelyYes\n - \tNATIVE_CRLF = YesPlease\n - \tDEFAULT_HELP_FORMAT = html\n -+\tHAVE_FLOCK = YesWeEmulate\n - \n - \tCC = compat/vcbuild/scripts/clink.pl\n - \tAR = compat/vcbuild/scripts/lib.pl\n -@@\n - \tNO_INET_NTOP = YesPlease\n - \tNO_POSIX_GOODIES = UnfortunatelyYes\n - \tDEFAULT_HELP_FORMAT = html\n -+\tHAVE_FLOCK = YesWeEmulate\n -+\n - \tCOMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32\n - \tCOMPAT_CFLAGS += -DSTRIP_EXTENSION=\\\".exe\\\"\n - \tCOMPAT_OBJS += compat/mingw.o compat/winansi.o \\\n 3: a53e72198 ! 3: 1f5c15c7e trace: lock the trace file to avoid racy trace_write() calls\n @@ -14,6 +14,17 @@\n To remedy this, let's lock the file descriptors for exclusive writing,\n using the function we just introduced in the previous commit.\n \n + Note: while the POSIX documentation of fcntl() at\n + http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html\n + suggests that the `errno` is set to `EINVAL` when being asked to\n + lock a file descriptor that cannot be locked, on macOS it results in\n + `EBADF` when trying to lock a redirected `stdout` (which the\n + documentation claims should indicate that the file descriptor is not\n + valid for writing).\n +\n + To cover all our bases, we simply treat both `EINVAL` and `EBADF` as\n + indicators that we cannot lock/unlock this file descriptor.\n +\n Signed-off-by: Johannes Schindelin \n \n diff --git a/trace.c b/trace.c\n @@ -27,7 +38,7 @@\n +\tint fd = get_trace_fd(key), locked;\n +\n +\tlocked = !lock_or_unlock_fd_for_appending(fd, 1);\n -+\tif (!locked && errno != EBADF)\n ++\tif (!locked && errno != EBADF && errno != EINVAL)\n +\t\twarning(\"unable to lock file descriptor for %s: %s\",\n +\t\t\tkey->key, strerror(errno));\n +\tif (write_in_full(fd, buf, len) < 0) {\n 4: f57234154 ! 4: 38358ac74 trace: verify that locking works\n @@ -131,13 +131,13 @@\n +\t\tstrbuf_rtrim(&buf);\n +\t\tif (!strcmp(\"lock\", buf.buf)) {\n +\t\t\tif (lock_or_unlock_fd_for_appending(fd, 1) < 0 &&\n -+\t\t\t errno != EBADF)\n ++\t\t\t errno != EBADF && errno != EINVAL)\n +\t\t\t\tdie_errno(\"'%s': lock\", child_name);\n +\t\t\tACK(\"lock\");\n +\t\t\tlocked = 1;\n +\t\t} else if (!strcmp(\"unlock\", buf.buf)) {\n +\t\t\tif (lock_or_unlock_fd_for_appending(fd, 0) < 0 &&\n -+\t\t\t errno != EBADF)\n ++\t\t\t errno != EBADF && errno != EINVAL)\n +\t\t\t\tdie_errno(\"'%s': unlock\", child_name);\n +\t\t\tACK(\"unlock\");\n +\t\t\tlocked = 0;\n\n-- \ngitgitgadget\n"},{"id":"355142","messageId":"a19904682f6984de21fbf77240ff5fb6eed1b744.1533930437.git.gitgitgadget@gmail.com","threadId":"49085","inReplyTo":"pull.17.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] Introduce a function to lock/unlock file descriptors when appending","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-08-10T19:47:20Z","receivedAt":"2018-08-10T19:47:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis function will be used to make write accesses in trace_write() a bit\nsafer.\n\nNote: this patch takes a very different approach for cross-platform\nsupport than Git is historically taking: the original approach is to\nfirst implement everything on Linux, using the functions available on\nLinux, and then trying to emulate these functions on platforms that do\nnot have those functions such as Windows.\n\nThis leads to all kinds of undesirable quirks in Git's source code (and\nperformance characteristics) because of the lack of a proper abstraction\nlayer: instead of declaring functions for the functionality we\n*actually* need, we abuse POSIX functions to say what we need, even if\nthose functions serve much broader purposes (and do not make at all\nclear what the caller wants of them).\n\nFor example, when Git wants to determine whether a path refers to a\nsymbolic link, it calls lstat(). That POSIX function has to be emulated\non Windows, painstakingly filling all the information lstat() would,\nonly for the caller to throw away everything except that one bit of\ninformation, and all of the time figuring out the mtime/atime/ctime and\nfile size and whatnot was spent in vain.\n\nIf we were to follow that approach, we would extend the fcntl()\nemulation in compat/mingw.c after this commit, to support the function\nadded in this commit.\n\nBut fcntl() allows a lot more versatile file region locking that we\nactually need, so by necessity the fcntl() emulation would be quite\ncomplex: To support the `l_whence = SEEK_CUR` (which we would use, if it\ndid not require so much book-keeping due to our writing between locking\nand unlocking the file), we would have to call `SetFilePointerEx()`\n(after obtaining the `HANDLE` on which all Win32 API functions work\ninstead of the integer file descriptors used by all POSIX functions).\nMultiplying the number of Win32 API round-trips. Of course, we could\nimplement an incomplete emulation of fcntl()'s F_WRLCK, but that would\nbe unsatisfying.\n\nAn alternative approach would be to use the `flock()` function, whose\nsemantics are much closer to existing Win32 API. But `flock()` is not\neven a POSIX standard, so we would have to implement emulation of\n`flock()` for *other* platforms. And it would again be the wrong thing\nto do, as we would once again fail to have an abstraction that clearly\nsays what *exact*functionality we want to use.\n\nThis commit expressly tries to set a precedent for a better approach:\nLet's introduce a proper abstraction: a function that says in its name\nprecisely what Git wants it to do (as opposed to *how* it does it on\nLinux): lock_or_unlock_fd_for_appending().\n\nThe next commit will provide a Windows-specific implementation of this\nfunction/functionality.\n\nSigned-off-by: Johannes Schindelin \n---\n git-compat-util.h | 2 ++\n wrapper.c | 16 ++++++++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9a64998b2..13b83bade 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1202,6 +1202,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);\n #define getc_unlocked(fh) getc(fh)\n #endif\n \n+extern int lock_or_unlock_fd_for_appending(int fd, int lock_it);\n+\n /*\n * Our code often opens a path to an optional file, to work on its\n * contents when we can successfully open it. We can ignore a failure\ndiff --git a/wrapper.c b/wrapper.c\nindex e4fa9d84c..5aecbda34 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -690,3 +690,19 @@ int xgethostname(char *buf, size_t len)\n \t\tbuf[len - 1] = 0;\n \treturn ret;\n }\n+\n+#ifndef GIT_WINDOWS_NATIVE\n+int lock_or_unlock_fd_for_appending(int fd, int lock_it)\n+{\n+\tstruct flock flock;\n+\n+\tflock.l_type = lock_it ? F_WRLCK : F_UNLCK;\n+\n+\t/* (un-)lock the whole file */\n+\tflock.l_whence = SEEK_SET;\n+\tflock.l_start = 0;\n+\tflock.l_len = 0;\n+\n+\treturn fcntl(fd, F_SETLKW, &flock);\n+}\n+#endif\n-- \ngitgitgadget\n\n"},{"id":"355143","messageId":"fa0c282aab98cd857ea37afd5a00ce8600b77bb1.1533930437.git.gitgitgadget@gmail.com","threadId":"49085","inReplyTo":"pull.17.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] mingw: implement lock_or_unlock_fd_for_appending()","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-08-10T19:47:21Z","receivedAt":"2018-08-10T19:47:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nFor some reason, t/t5552-skipping-fetch-negotiator.sh fails particularly\noften on Windows due to racy tracing where the `git upload-pack` and the\n`git fetch` processes compete for the same file.\n\nWe just introduced a remedy that uses fcntl(), but Windows does not have\nfcntl(). So let's implement an alternative.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 19 +++++++++++++++++++\n compat/mingw.h | 3 +++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 6ded1c859..6da9ce861 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -514,6 +514,25 @@ int mingw_chmod(const char *filename, int mode)\n \treturn _wchmod(wfilename, mode);\n }\n \n+int mingw_lock_or_unlock_fd_for_appending(int fd, int lock_it)\n+{\n+\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n+\tOVERLAPPED overlapped = { 0 };\n+\tDWORD err;\n+\n+\tif (!lock_it && UnlockFileEx(handle, 0, -1, 0, &overlapped))\n+\t\treturn 0;\n+\tif (lock_it &&\n+\t LockFileEx(handle, LOCKFILE_EXCLUSIVE_LOCK, 0, -1, 0, &overlapped))\n+\t\treturn 0;\n+\n+\terr = GetLastError();\n+\t/* LockFileEx() cannot lock pipes */\n+\terrno = err == ERROR_INVALID_FUNCTION ?\n+\t\tEBADF : err_win_to_posix(GetLastError());\n+\treturn -1;\n+}\n+\n /*\n * The unit of FILETIME is 100-nanoseconds since January 1, 1601, UTC.\n * Returns the 100-nanoseconds (\"hekto nanoseconds\") since the epoch.\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 571019d0b..0f76d89a9 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -397,6 +397,9 @@ HANDLE winansi_get_osfhandle(int fd);\n * git specific compatibility\n */\n \n+int mingw_lock_or_unlock_fd_for_appending(int fd, int lock_it);\n+#define lock_or_unlock_fd_for_appending mingw_lock_or_unlock_fd_for_appending\n+\n #define has_dos_drive_prefix(path) \\\n \t(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)\n int mingw_skip_dos_drive_prefix(char **path);\n-- \ngitgitgadget\n\n"},{"id":"355144","messageId":"1f5c15c7ed3914b733ccf8a4457fef50d2eff1c5.1533930437.git.gitgitgadget@gmail.com","threadId":"49085","inReplyTo":"pull.17.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] trace: lock the trace file to avoid racy trace_write() calls","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-08-10T19:47:23Z","receivedAt":"2018-08-10T19:47:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen multiple processes try to write to the same file, it is not\nguaranteed that everything works as expected: those writes can overlap,\nand in the worst case even lose messages.\n\nThis happens in t/t5552-skipping-fetch-negotiator.sh, where we abuse the\n`GIT_TRACE` facility to write traces of two concurrent processes (`git\nfetch` and `git upload-pack`) to the same file, and then verify that the\ntrace contains certain expected breadcrumbs.\n\nTo remedy this, let's lock the file descriptors for exclusive writing,\nusing the function we just introduced in the previous commit.\n\nNote: while the POSIX documentation of fcntl() at\nhttp://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html\nsuggests that the `errno` is set to `EINVAL` when being asked to\nlock a file descriptor that cannot be locked, on macOS it results in\n`EBADF` when trying to lock a redirected `stdout` (which the\ndocumentation claims should indicate that the file descriptor is not\nvalid for writing).\n\nTo cover all our bases, we simply treat both `EINVAL` and `EBADF` as\nindicators that we cannot lock/unlock this file descriptor.\n\nSigned-off-by: Johannes Schindelin \n---\n trace.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/trace.c b/trace.c\nindex fc623e91f..16abdb816 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -114,11 +114,20 @@ static int prepare_trace_line(const char *file, int line,\n \n static void trace_write(struct trace_key *key, const void *buf, unsigned len)\n {\n-\tif (write_in_full(get_trace_fd(key), buf, len) < 0) {\n+\tint fd = get_trace_fd(key), locked;\n+\n+\tlocked = !lock_or_unlock_fd_for_appending(fd, 1);\n+\tif (!locked && errno != EBADF && errno != EINVAL)\n+\t\twarning(\"unable to lock file descriptor for %s: %s\",\n+\t\t\tkey->key, strerror(errno));\n+\tif (write_in_full(fd, buf, len) < 0) {\n \t\twarning(\"unable to write trace for %s: %s\",\n \t\t\tkey->key, strerror(errno));\n \t\ttrace_disable(key);\n \t}\n+\tif (locked && lock_or_unlock_fd_for_appending(fd, 0) < 0)\n+\t\twarning(\"failed to remove lock on fd for %s: %s\",\n+\t\t\tkey->key, strerror(errno));\n }\n \n void trace_verbatim(struct trace_key *key, const void *buf, unsigned len)\n-- \ngitgitgadget\n\n"},{"id":"355145","messageId":"38358ac74dcb10a7908f2ee236d544b1fcb6626c.1533930437.git.gitgitgadget@gmail.com","threadId":"49085","inReplyTo":"pull.17.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] trace: verify that locking works","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-08-10T19:47:24Z","receivedAt":"2018-08-10T19:47:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nRecently, t5552 introduced a pattern where two processes try to write to\nthe same GIT_TRACE file in parallel. This is not safe, as the two\nprocesses fighting over who gets to append to the file can cause garbled\nlines and may even result in data loss on Windows (where buffers are\nwritten to before they are flushed).\n\nTo remedy this, we introduced the lock_or_unlock_fd_for_appending()\nfunction. And to make sure that this works, this commit introduces a\nregression test.\n\nSigned-off-by: Johannes Schindelin \n---\n Makefile | 1 +\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/helper/test-trace.c | 130 +++++++++++++++++++++++++++++++++++++++++\n t/t0070-fundamental.sh | 6 ++\n 5 files changed, 139 insertions(+)\n create mode 100644 t/helper/test-trace.c\n\ndiff --git a/Makefile b/Makefile\nindex 617475622..2e3fb5b8d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -729,6 +729,7 @@ TEST_BUILTINS_OBJS += test-strcmp-offset.o\n TEST_BUILTINS_OBJS += test-string-list.o\n TEST_BUILTINS_OBJS += test-submodule-config.o\n TEST_BUILTINS_OBJS += test-subprocess.o\n+TEST_BUILTINS_OBJS += test-trace.o\n TEST_BUILTINS_OBJS += test-urlmatch-normalization.o\n TEST_BUILTINS_OBJS += test-wildmatch.o\n TEST_BUILTINS_OBJS += test-write-cache.o\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 805a45de9..7adce872b 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -39,6 +39,7 @@ static struct test_cmd cmds[] = {\n \t{ \"string-list\", cmd__string_list },\n \t{ \"submodule-config\", cmd__submodule_config },\n \t{ \"subprocess\", cmd__subprocess },\n+\t{ \"trace\", cmd__trace },\n \t{ \"urlmatch-normalization\", cmd__urlmatch_normalization },\n \t{ \"wildmatch\", cmd__wildmatch },\n \t{ \"write-cache\", cmd__write_cache },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 7116ddfb9..c462ac924 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -33,6 +33,7 @@ int cmd__strcmp_offset(int argc, const char **argv);\n int cmd__string_list(int argc, const char **argv);\n int cmd__submodule_config(int argc, const char **argv);\n int cmd__subprocess(int argc, const char **argv);\n+int cmd__trace(int argc, const char **argv);\n int cmd__urlmatch_normalization(int argc, const char **argv);\n int cmd__wildmatch(int argc, const char **argv);\n int cmd__write_cache(int argc, const char **argv);\ndiff --git a/t/helper/test-trace.c b/t/helper/test-trace.c\nnew file mode 100644\nindex 000000000..04159c77a\n--- /dev/null\n+++ b/t/helper/test-trace.c\n@@ -0,0 +1,130 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+#include \"run-command.h\"\n+\n+static struct child_process children[2] = {\n+\tCHILD_PROCESS_INIT,\n+\tCHILD_PROCESS_INIT,\n+};\n+\n+#define SAY(child, what) \\\n+\tif (write_in_full(children[child].in, \\\n+\t\t\t what \"\\n\", strlen(what) + 1) < 0) \\\n+\t\tdie(\"Failed to tell child process #%d to %s\", child, what)\n+\n+#define LISTEN(child, what) \\\n+\tif (strbuf_getwholeline_fd(&buf, children[child].out, '\\n') < 0) \\\n+\t\tdie(\"Child process #%d failed to acknowledge %s\", child, what)\n+\n+#define ACK(what) \\\n+\tif (write_in_full(1, what \": ACK\\n\", strlen(what) + 6) < 0) \\\n+\t\tdie_errno(\"'%s': %s ACK\", child_name, what)\n+\n+static void contention_orchestrator(const char *argv0)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint i;\n+\n+\t/* Spawn two children and simulate write contention */\n+\ttrace_printf(\"start\");\n+\n+\tfor (i = 0; i < 2; i++) {\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addf(&buf, \"child #%d\", i);\n+\t\targv_array_pushl(&children[i].args,\n+\t\t\t\t\targv0, \"trace\", \"lock\", buf.buf, NULL);\n+\t\tchildren[i].in = children[i].out = -1;\n+\t\tif (start_command(&children[i]) < 0)\n+\t\t\tdie(\"Could not spawn child process #%d\", i);\n+\t}\n+\n+\tSAY(1, \"lock\");\n+\tLISTEN(1, \"lock\");\n+\n+\tSAY(0, \"trace delayed\");\n+\tSAY(1, \"trace while-locked\");\n+\tLISTEN(1, \"trace\");\n+\n+\tSAY(1, \"unlock\");\n+\tLISTEN(1, \"unlock\");\n+\tLISTEN(0, \"trace\");\n+\n+\tSAY(0, \"quit\");\n+\tSAY(1, \"quit\");\n+\n+\tif (finish_command(&children[0]) < 0 ||\n+\t\tfinish_command(&children[1]) < 0)\n+\t\tdie(\"Child process failed to finish\");\n+\n+\tstrbuf_release(&buf);\n+}\n+\n+static void child(const char *child_name)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd, locked = 0;\n+\tconst char *p;\n+\n+\t/* This is the child process */\n+\ttrace_printf(\"child start: '%s'\", child_name);\n+\tfd = trace_default_key.fd;\n+\tif (fd <= 0)\n+\t\tdie(\"child process: not tracing...\");\n+\twhile (!strbuf_getwholeline_fd(&buf, 0, '\\n')) {\n+\t\tstrbuf_rtrim(&buf);\n+\t\tif (!strcmp(\"lock\", buf.buf)) {\n+\t\t\tif (lock_or_unlock_fd_for_appending(fd, 1) < 0 &&\n+\t\t\t errno != EBADF && errno != EINVAL)\n+\t\t\t\tdie_errno(\"'%s': lock\", child_name);\n+\t\t\tACK(\"lock\");\n+\t\t\tlocked = 1;\n+\t\t} else if (!strcmp(\"unlock\", buf.buf)) {\n+\t\t\tif (lock_or_unlock_fd_for_appending(fd, 0) < 0 &&\n+\t\t\t errno != EBADF && errno != EINVAL)\n+\t\t\t\tdie_errno(\"'%s': unlock\", child_name);\n+\t\t\tACK(\"unlock\");\n+\t\t\tlocked = 0;\n+\t\t} else if (skip_prefix(buf.buf, \"trace \", &p)) {\n+\t\t\tif (!locked)\n+\t\t\t\ttrace_printf(\"%s: %s\", child_name, p);\n+\t\t\telse {\n+\t\t\t\tchar *p2 = xstrdup(p);\n+\n+\t\t\t\t/* Give the racy process a run for its money. */\n+\t\t\t\tsleep_millisec(50);\n+\n+\t\t\t\tstrbuf_reset(&buf);\n+\t\t\t\tstrbuf_addf(&buf, \"%s: %s\\n\",\n+\t\t\t\t\t child_name, p2);\n+\t\t\t\tfree(p2);\n+\n+\t\t\t\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n+\t\t\t\t\tdie_errno(\"'%s': trace\", child_name);\n+\t\t\t}\n+\t\t\tACK(\"trace\");\n+\t\t} else if (!strcmp(\"quit\", buf.buf)) {\n+\t\t\tclose(0);\n+\t\t\tclose(1);\n+\t\t\tbreak;\n+\t\t} else\n+\t\t\tdie(\"Unhandled command: '%s'\", buf.buf);\n+\n+\t}\n+\n+\tstrbuf_release(&buf);\n+}\n+\n+int cmd__trace(int argc, const char **argv)\n+{\n+\tconst char *argv0 = argv[-1];\n+\n+\tif (argc > 1 && !strcmp(\"lock\", argv[1])) {\n+\t\tif (argc > 2)\n+\t\t\tchild(argv[2]);\n+\t\telse\n+\t\t\tcontention_orchestrator(argv0);\n+\t} else\n+\t\tdie(\"Usage: %s %s lock []\", argv0, argv[0]);\n+\n+\treturn 0;\n+}\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 23fbe6434..57f7a1246 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -34,4 +34,10 @@ test_expect_success 'check for a bug in the regex routines' '\n \ttest-tool regex --bug\n '\n \n+test_expect_success 'multiple processes can GIT_TRACE to the same file' '\n+\tGIT_TRACE=\"$(pwd)/trace\" test-tool trace lock &&\n+\tsed -n \"/while-locked/,\\$s/.*delayed$/YES/p\" actual &&\n+\ttest YES = \"$(cat actual)\"\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"355147","messageId":"xmqqzhxu6l81.fsf@gitster-ct.c.googlers.com","threadId":"49085","inReplyTo":"a19904682f6984de21fbf77240ff5fb6eed1b744.1533930437.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/4] Introduce a function to lock/unlock file descriptors when appending","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-10T20:05:34Z","receivedAt":"2018-08-10T20:05:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> +\n> +#ifndef GIT_WINDOWS_NATIVE\n> +int lock_or_unlock_fd_for_appending(int fd, int lock_it)\n> +{\n> +\tstruct flock flock;\n> +\n> +\tflock.l_type = lock_it ? F_WRLCK : F_UNLCK;\n> +\n> +\t/* (un-)lock the whole file */\n> +\tflock.l_whence = SEEK_SET;\n> +\tflock.l_start = 0;\n> +\tflock.l_len = 0;\n> +\n> +\treturn fcntl(fd, F_SETLKW, &flock);\n> +}\n> +#endif\n\nI think people already told you that this is not needed on systems\nwith properly working O_APPEND [*1*]\n\n\tSide note #1: and with network filesystems where O_APPEND\n may not work reliably, fcntl based range locking would not\n work either, so having this would not help.\n\nI saw other Johannes and other Jeff peeking into fixing O_APPEND;\nI do not know how well that effort goes, but it would be preferrable\nif we can successfully go that route. \n\nAs I said in my review of the first patch in v1 series, I am not\nfundamentally opposed to a few \"lock here to work around lack of\nO_APPEND\" and \"unlock here for the same reason\" calls to limited\ncodepaths as a workaround, as the damage is limited (that is why I\nearlier looked at our use of O_APPEND), but that would be the last\nresort if O_APPEND cannot be made to work reliably on Windows.\n\nBut even if we end up doing so, on systems with POSIX O_APPEND\nworking, I think that function should be\n\n #define lock_or_unlock_for_appending(fd, lock) 0 /* nothing to do */\n\n\n\n"},{"id":"355161","messageId":"nycvar.QRO.7.76.6.1808102327080.71@tvgsbejvaqbjf.bet","threadId":"49085","inReplyTo":"xmqqzhxu6l81.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/4] Introduce a function to lock/unlock file descriptors when appending","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-10T21:31:14Z","receivedAt":"2018-08-10T21:31:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 10 Aug 2018, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > +\n> > +#ifndef GIT_WINDOWS_NATIVE\n> > +int lock_or_unlock_fd_for_appending(int fd, int lock_it)\n> > +{\n> > +\tstruct flock flock;\n> > +\n> > +\tflock.l_type = lock_it ? F_WRLCK : F_UNLCK;\n> > +\n> > +\t/* (un-)lock the whole file */\n> > +\tflock.l_whence = SEEK_SET;\n> > +\tflock.l_start = 0;\n> > +\tflock.l_len = 0;\n> > +\n> > +\treturn fcntl(fd, F_SETLKW, &flock);\n> > +}\n> > +#endif\n> \n> I think people already told you that this is not needed on systems\n> with properly working O_APPEND [*1*]\n\nI also wanted to safeguard against other (well-behaved) programs writing\nto the trace file.\n\nGranted, this is a bit theoretical, but it is quite possible that other\nGit implementations do things differently than our trace.c. But with the\nfile locking that even that POSIX man page I quoted recommends, all this\nis not an issue.\n\nAnd this patch series implements that locking.\n\n> \tSide note #1: and with network filesystems where O_APPEND\n> may not work reliably, fcntl based range locking would not\n> work either, so having this would not help.\n\nAt least on Windows, it would help, though.\n\nAnd if there *is* a way to lock via NFS (which is the only network\nfilesystem I am aware of that has these locking issues, at least *some*\nothers are just fine), we would at least already have that function where\nto implement it.\n\n> I saw other Johannes and other Jeff peeking into fixing O_APPEND;\n> I do not know how well that effort goes, but it would be preferrable\n> if we can successfully go that route. \n\nAs I pointed out previously, my mail provider is losing mails left and\nright for me. Could I ask for a pointer?\n\n> As I said in my review of the first patch in v1 series, I am not\n> fundamentally opposed to a few \"lock here to work around lack of\n> O_APPEND\" and \"unlock here for the same reason\" calls to limited\n> codepaths as a workaround, as the damage is limited (that is why I\n> earlier looked at our use of O_APPEND), but that would be the last\n> resort if O_APPEND cannot be made to work reliably on Windows.\n> \n> But even if we end up doing so, on systems with POSIX O_APPEND\n> working, I think that function should be\n> \n> #define lock_or_unlock_for_appending(fd, lock) 0 /* nothing to do */\n\nI think that that would be only appropriate if there were no other Git\nimplementations.\n\nCiao,\nDscho\n"},{"id":"355451","messageId":"bc9132ab-3030-07e3-03d1-d2ddae6a1b45@kdbg.org","threadId":"49085","inReplyTo":"xmqqin4i83zg.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] mingw: enable atomic O_APPEND","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-08-13T19:02:50Z","receivedAt":"2018-08-13T19:02:54Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The Windows CRT implements O_APPEND \"manually\": on write() calls, the\nfile pointer is set to EOF before the data is written. Clearly, this is\nnot atomic. And in fact, this is the root cause of failures observed in\nt5552-skipping-fetch-negotiator.sh and t5503-tagfollow.sh, where\ndifferent processes write to the same trace file simultanously; it also\noccurred in t5400-send-pack.sh, but there it was worked around in\n71406ed4d6 (\"t5400: avoid concurrent writes into a trace file\",\n2017-05-18).\n\nFortunately, Windows does support atomic O_APPEND semantics using the\nfile access mode FILE_APPEND_DATA. Provide an implementation that does.\n\nThis implementation is minimal in such a way that it only implements\nthe open modes that are actually used in the Git code base. Emulation\nfor other modes can be added as necessary later. To become aware of\nthe necessity early, the unusal error ENOSYS is reported if an\nunsupported mode is encountered.\n\nDiagnosed-by: Johannes Schindelin \nHelped-by: Jeff Hostetler \nSigned-off-by: Johannes Sixt \n---\n compat/mingw.c | 41 +++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 39 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 6ded1c859f..858ca14a57 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -341,12 +341,44 @@ int mingw_mkdir(const char *path, int mode)\n \treturn ret;\n }\n \n+static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)\n+{\n+\tHANDLE handle;\n+\tint fd;\n+\tDWORD create = (oflags & O_CREAT) ? OPEN_ALWAYS : OPEN_EXISTING;\n+\n+\t/* only these flags are supported */\n+\tif ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))\n+\t\treturn errno = ENOSYS, -1;\n+\n+\t/*\n+\t * FILE_SHARE_WRITE is required to permit child processes\n+\t * to append to the file.\n+\t */\n+\thandle = CreateFileW(wfilename, FILE_APPEND_DATA,\n+\t\t\tFILE_SHARE_WRITE | FILE_SHARE_READ,\n+\t\t\tNULL, create, FILE_ATTRIBUTE_NORMAL, NULL);\n+\tif (handle == INVALID_HANDLE_VALUE)\n+\t\treturn errno = err_win_to_posix(GetLastError()), -1;\n+\t/*\n+\t * No O_APPEND here, because the CRT uses it only to reset the\n+\t * file pointer to EOF on write(); but that is not necessary\n+\t * for a file created with FILE_APPEND_DATA.\n+\t */\n+\tfd = _open_osfhandle((intptr_t)handle, O_BINARY);\n+\tif (fd < 0)\n+\t\tCloseHandle(handle);\n+\treturn fd;\n+}\n+\n int mingw_open (const char *filename, int oflags, ...)\n {\n+\ttypedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...);\n \tva_list args;\n \tunsigned mode;\n \tint fd;\n \twchar_t wfilename[MAX_PATH];\n+\topen_fn_t open_fn;\n \n \tva_start(args, oflags);\n \tmode = va_arg(args, int);\n@@ -355,9 +387,14 @@ int mingw_open (const char *filename, int oflags, ...)\n \tif (filename && !strcmp(filename, \"/dev/null\"))\n \t\tfilename = \"nul\";\n \n+\tif (oflags & O_APPEND)\n+\t\topen_fn = mingw_open_append;\n+\telse\n+\t\topen_fn = _wopen;\n+\n \tif (xutftowcs_path(wfilename, filename) < 0)\n \t\treturn -1;\n-\tfd = _wopen(wfilename, oflags, mode);\n+\tfd = open_fn(wfilename, oflags, mode);\n \n \tif (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {\n \t\tDWORD attrs = GetFileAttributesW(wfilename);\n@@ -375,7 +412,7 @@ int mingw_open (const char *filename, int oflags, ...)\n \t\t * CREATE_ALWAYS flag of CreateFile()).\n \t\t */\n \t\tif (fd < 0 && errno == EACCES)\n-\t\t\tfd = _wopen(wfilename, oflags & ~O_CREAT, mode);\n+\t\t\tfd = open_fn(wfilename, oflags & ~O_CREAT, mode);\n \t\tif (fd >= 0 && set_hidden_flag(wfilename, 1))\n \t\t\twarning(\"could not mark '%s' as hidden.\", filename);\n \t}\n-- \n2.18.0.rc0.112.g5f42c6ebd3\n"},{"id":"355470","messageId":"xmqqr2j23tnb.fsf@gitster-ct.c.googlers.com","threadId":"49085","inReplyTo":"bc9132ab-3030-07e3-03d1-d2ddae6a1b45@kdbg.org","subject":"Re: [PATCH] mingw: enable atomic O_APPEND","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-13T20:20:56Z","receivedAt":"2018-08-13T20:21:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> The Windows CRT implements O_APPEND \"manually\": on write() calls, the\n> file pointer is set to EOF before the data is written. Clearly, this is\n> not atomic. And in fact, this is the root cause of failures observed in\n> t5552-skipping-fetch-negotiator.sh and t5503-tagfollow.sh, where\n> different processes write to the same trace file simultanously; it also\n> occurred in t5400-send-pack.sh, but there it was worked around in\n> 71406ed4d6 (\"t5400: avoid concurrent writes into a trace file\",\n> 2017-05-18).\n>\n> Fortunately, Windows does support atomic O_APPEND semantics using the\n> file access mode FILE_APPEND_DATA. Provide an implementation that does.\n>\n> This implementation is minimal in such a way that it only implements\n> the open modes that are actually used in the Git code base. Emulation\n> for other modes can be added as necessary later. To become aware of\n> the necessity early, the unusal error ENOSYS is reported if an\n> unsupported mode is encountered.\n>\n> Diagnosed-by: Johannes Schindelin \n> Helped-by: Jeff Hostetler \n> Signed-off-by: Johannes Sixt \n> ---\n> compat/mingw.c | 41 +++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 39 insertions(+), 2 deletions(-)\n\nNice.\n\nI wonder how much more expensive using this implementation is\ncompared with the original \"race susceptible\" open(), when raciness\nis known not to be an issue (e.g. there is higher level lock that\nprotects the appending).\n\nIf it turns out to be quite more costly, I do not think we'd mind\nintroducing a thin wrapper\n\n#ifndef race_safe_append_open\n#ifndef race_safe_append_open(fn) open(fn, O_WRONLY|O_APPEND|O_CREAT, 0666)\n#endif\n\nin git-compat-util.h after it includes \"compat/mingw.h\" and replace\nthe call to open(... O_APPEND ...) in trace.c::get_trace_fd() with a\ncall to that wrapper. That way, other codepaths that use O_APPEND\n(namely, reflog and todo-list writers) can avoid the additional\ncost, if any.\n\nSome may find it beneficial from code readability POV because that\napproach marks the codepath that needs to have non-racy fd more\nexplicitly.\n\nI am assuming that in this case other users of O_APPEND are not\nperformance critical, so hopefully the following is only theoretical\nand not necessary.\n\n git-compat-util.h | 5 +++++\n trace.c | 2 +-\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9a64998b24..a981c50800 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -218,6 +218,11 @@\n #else\n #include \n #endif\n+\n+#ifndef race_safe_append_open\n+#define race_safe_append_open(fn) open(fn, O_WRONLY|O_APPEND|O_CREAT, 0666)\n+#endif\n+\n #ifdef NO_INTPTR_T\n /*\n * On I16LP32, ILP32 and LP64 \"long\" is the save bet, however\ndiff --git a/trace.c b/trace.c\nindex fc623e91fd..bef9cee1da 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -47,7 +47,7 @@ static int get_trace_fd(struct trace_key *key)\n \telse if (strlen(trace) == 1 && isdigit(*trace))\n \t\tkey->fd = atoi(trace);\n \telse if (is_absolute_path(trace)) {\n-\t\tint fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);\n+\t\tint fd = race_safe_append_open(trace);\n \t\tif (fd == -1) {\n \t\t\twarning(\"could not open '%s' for tracing: %s\",\n \t\t\t\ttrace, strerror(errno));\n"},{"id":"355479","messageId":"84c749fd-23d2-0bc5-225b-74f8d31502b6@kdbg.org","threadId":"49085","inReplyTo":"xmqqr2j23tnb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] mingw: enable atomic O_APPEND","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-08-13T21:05:31Z","receivedAt":"2018-08-13T21:05:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 13.08.2018 um 22:20 schrieb Junio C Hamano:\n> Johannes Sixt writes:\n> \n>> The Windows CRT implements O_APPEND \"manually\": on write() calls, the\n>> file pointer is set to EOF before the data is written. Clearly, this is\n>> not atomic. And in fact, this is the root cause of failures observed in\n>> t5552-skipping-fetch-negotiator.sh and t5503-tagfollow.sh, where\n>> different processes write to the same trace file simultanously; it also\n>> occurred in t5400-send-pack.sh, but there it was worked around in\n>> 71406ed4d6 (\"t5400: avoid concurrent writes into a trace file\",\n>> 2017-05-18).\n>>\n>> Fortunately, Windows does support atomic O_APPEND semantics using the\n>> file access mode FILE_APPEND_DATA. Provide an implementation that does.\n>>\n>> This implementation is minimal in such a way that it only implements\n>> the open modes that are actually used in the Git code base. Emulation\n>> for other modes can be added as necessary later. To become aware of\n>> the necessity early, the unusal error ENOSYS is reported if an\n>> unsupported mode is encountered.\n>>\n>> Diagnosed-by: Johannes Schindelin \n>> Helped-by: Jeff Hostetler \n>> Signed-off-by: Johannes Sixt \n>> ---\n>> compat/mingw.c | 41 +++++++++++++++++++++++++++++++++++++++--\n>> 1 file changed, 39 insertions(+), 2 deletions(-)\n> \n> Nice.\n> \n> I wonder how much more expensive using this implementation is\n> compared with the original \"race susceptible\" open(), when raciness\n> is known not to be an issue (e.g. there is higher level lock that\n> protects the appending).\n\nCertainly, the former way that uses two syscalls \n(SetFilePointer+WriteFile) is more costly than this new way with just \none syscall (WriteFile). Of course, I don't know how atomic append would \nbe implemented in the kernel, but I can't think of a reason why it \nshould be slow on Windows, but fast on POSIX.\n\n(But I can't provide numbers to back up my gut feeling...)\n\n(And I also assume that you are not worried about the performance of \nopen() itself.)\n\n> ...[define race_safe_append_open]... and replace\n> the call to open(... O_APPEND ...) in trace.c::get_trace_fd() with a\n> call to that wrapper. That way, other codepaths that use O_APPEND\n> (namely, reflog and todo-list writers) can avoid the additional\n> cost, if any.\n> \n> Some may find it beneficial from code readability POV because that\n> approach marks the codepath that needs to have non-racy fd more\n> explicitly.\n\nO_APPEND is POSIX and means race-free append. If you mark some call \nsites with O_APPEND, then that must be the ones that need race-free \nappend. Hence, you would have to go the other route: Mark those call \nsites that do _not_ need race-free append with some custom \nfunction/macro. (Or mark both with different helpers and avoid writing \ndown O_APPEND.)\n\n-- Hannes\n"},{"id":"355486","messageId":"87eff2rmgt.fsf@evledraar.gmail.com","threadId":"49085","inReplyTo":"84c749fd-23d2-0bc5-225b-74f8d31502b6@kdbg.org","subject":"Re: [PATCH] mingw: enable atomic O_APPEND","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-13T21:22:10Z","receivedAt":"2018-08-13T21:22:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Aug 13 2018, Johannes Sixt wrote:\n\n> Am 13.08.2018 um 22:20 schrieb Junio C Hamano:\n>> Johannes Sixt writes:\n>>\n>>> The Windows CRT implements O_APPEND \"manually\": on write() calls, the\n>>> file pointer is set to EOF before the data is written. Clearly, this is\n>>> not atomic. And in fact, this is the root cause of failures observed in\n>>> t5552-skipping-fetch-negotiator.sh and t5503-tagfollow.sh, where\n>>> different processes write to the same trace file simultanously; it also\n>>> occurred in t5400-send-pack.sh, but there it was worked around in\n>>> 71406ed4d6 (\"t5400: avoid concurrent writes into a trace file\",\n>>> 2017-05-18).\n>>>\n>>> Fortunately, Windows does support atomic O_APPEND semantics using the\n>>> file access mode FILE_APPEND_DATA. Provide an implementation that does.\n>>>\n>>> This implementation is minimal in such a way that it only implements\n>>> the open modes that are actually used in the Git code base. Emulation\n>>> for other modes can be added as necessary later. To become aware of\n>>> the necessity early, the unusal error ENOSYS is reported if an\n>>> unsupported mode is encountered.\n>>>\n>>> Diagnosed-by: Johannes Schindelin \n>>> Helped-by: Jeff Hostetler \n>>> Signed-off-by: Johannes Sixt \n>>> ---\n>>> compat/mingw.c | 41 +++++++++++++++++++++++++++++++++++++++--\n>>> 1 file changed, 39 insertions(+), 2 deletions(-)\n>>\n>> Nice.\n>>\n>> I wonder how much more expensive using this implementation is\n>> compared with the original \"race susceptible\" open(), when raciness\n>> is known not to be an issue (e.g. there is higher level lock that\n>> protects the appending).\n>\n> Certainly, the former way that uses two syscalls\n> (SetFilePointer+WriteFile) is more costly than this new way with just\n> one syscall (WriteFile). Of course, I don't know how atomic append\n> would be implemented in the kernel, but I can't think of a reason why\n> it should be slow on Windows, but fast on POSIX.\n>\n> (But I can't provide numbers to back up my gut feeling...)\n>\n> (And I also assume that you are not worried about the performance of\n> open() itself.)\n>\n>> ...[define race_safe_append_open]... and replace\n>> the call to open(... O_APPEND ...) in trace.c::get_trace_fd() with a\n>> call to that wrapper. That way, other codepaths that use O_APPEND\n>> (namely, reflog and todo-list writers) can avoid the additional\n>> cost, if any.\n>>\n>> Some may find it beneficial from code readability POV because that\n>> approach marks the codepath that needs to have non-racy fd more\n>> explicitly.\n>\n> O_APPEND is POSIX and means race-free append. If you mark some call\n> sites with O_APPEND, then that must be the ones that need race-free\n> append. Hence, you would have to go the other route: Mark those call\n> sites that do _not_ need race-free append with some custom\n> function/macro. (Or mark both with different helpers and avoid writing\n> down O_APPEND.)\n\nO_APPEND in POSIX is race-free only up to PIPE_MAX bytes written at a\ntime, which is e.g. 2^12 by default on linux, after that all bets are\noff and the kernel is free to interleave different write calls.\n\nI've written code (not for git.git) that implements such a\n\"write_non_racy\" function in the past, and the first thing it needs to\ndo is to assert that the length of the buffer being written doesn't\nexceed PIPE_MAX.\n\nSo there's still a use for a race_safe_append_open() wrapper function,\nto O_APPEND and do the PIPE_MAX assertion. Otherwise you're calling a\n\"safe\" function which isn't safe at all anymore.\n\nI have no idea what the equivalent of that PIPE_MAX caveat is on\nnon-POSIX (e.g. Windows), but would be interested to find out.\n"},{"id":"355492","messageId":"xmqqo9e62aos.fsf@gitster-ct.c.googlers.com","threadId":"49085","inReplyTo":"87eff2rmgt.fsf@evledraar.gmail.com","subject":"Re: [PATCH] mingw: enable atomic O_APPEND","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-13T21:55:47Z","receivedAt":"2018-08-13T21:55:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> O_APPEND in POSIX is race-free only up to PIPE_MAX bytes written at a\n> time, which is e.g. 2^12 by default on linux, after that all bets are\n> off and the kernel is free to interleave different write calls.\n\nWe are only interested in the use of race-safe append in the\ntracing, so we should be OK in practice. but then ...\n\n> So there's still a use for a race_safe_append_open() wrapper function,\n> to O_APPEND and do the PIPE_MAX assertion. Otherwise you're calling a\n> \"safe\" function which isn't safe at all anymore.\n\n... having an assert that would never trigger is not all that bad\n;-)\n"},{"id":"355493","messageId":"20180813223701.GC16006@sigill.intra.peff.net","threadId":"49085","inReplyTo":"87eff2rmgt.fsf@evledraar.gmail.com","subject":"Re: [PATCH] mingw: enable atomic O_APPEND","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-13T22:37:01Z","receivedAt":"2018-08-13T22:37:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 13, 2018 at 11:22:10PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > O_APPEND is POSIX and means race-free append. If you mark some call\n> > sites with O_APPEND, then that must be the ones that need race-free\n> > append. Hence, you would have to go the other route: Mark those call\n> > sites that do _not_ need race-free append with some custom\n> > function/macro. (Or mark both with different helpers and avoid writing\n> > down O_APPEND.)\n> \n> O_APPEND in POSIX is race-free only up to PIPE_MAX bytes written at a\n> time, which is e.g. 2^12 by default on linux, after that all bets are\n> off and the kernel is free to interleave different write calls.\n\nThis is a claim I've run across often, but I've never seen a good\ncitation for it.\n\nCertainly atomic writes to _pipes_ are determined by PIPE_BUF (which\nIIRC is not even a constant on Linux, but can be changed at run-time).\nBut is it relevant for regular-file writes?\n\nAnother gem I found while digging on this O_APPEND/FILE_APPEND_DATA\nstuff the other day: somebody claimed that the max atomic-append size on\nLinux is 4096 and 1024 on Windows. But their experimental script was\ndone in bash! So I suspect they were really just measuring the size of\nstdio buffers.\n\nHere's my attempt at a test setup. This C program forces two processes\nto write simultaneously to the same file with O_APPEND:\n\n-- >8 --\n#include \n#include \n#include \n#include \n#include \n#include \n\nstatic void doit(int size, const char *fn, char c)\n{\n\tint fd;\n\tchar *buf;\n\n\tfd = open(fn, O_WRONLY|O_APPEND|O_CREAT, 0666);\n\tif (fd < 0) {\n\t\tperror(\"open\");\n\t\treturn;\n\t}\n\n\tbuf = malloc(size);\n\tmemset(buf, c, size);\n\n\twhile (1)\n\t\twrite(fd, buf, size);\n}\n\nint main(int argc, const char **argv)\n{\n\tint size = atoi(argv[1]);\n\n\tif (fork())\n\t\tdoit(size, argv[2], '1');\n\telse\n\t\tdoit(size, argv[2], '2');\n\treturn 0;\n}\n-- 8< --\n\nand then this program checks that we saw atomic units of the correct\nsize:\n\n-- >8 --\n#include \n#include \n#include \n\nint main(int argc, const char **argv)\n{\n\tint size = atoi(argv[1]);\n\tchar *buf;\n\n\tbuf = malloc(size);\n\twhile (1) {\n\t\tint i;\n\t\t/* assume atomic reads, i.e., no signals */\n\t\tint r = read(0, buf, size);\n\t\tif (!r)\n\t\t\tbreak;\n\t\tfor (i = 1; i < size; i++) {\n\t\t\tif (buf[i] != buf[0]) {\n\t\t\t\tfprintf(stderr, \"overlap\\n\");\n\t\t\t\treturn 1;\n\t\t\t}\n\t\t}\n\t}\n\treturn 0;\n}\n-- 8< --\n\nAnd then you can do something like:\n\n for size in 4097 8193 16385 32769 65537 131073 262145 524289 1048577; do\n >out ;# clean up from last run\n echo \"Trying $size...\"\n timeout 5 ./write $size out\n if ! ./check $size The Windows CRT implements O_APPEND \"manually\": on write() calls, the\n> file pointer is set to EOF before the data is written. Clearly, this is\n> not atomic. And in fact, this is the root cause of failures observed in\n> t5552-skipping-fetch-negotiator.sh and t5503-tagfollow.sh, where\n> different processes write to the same trace file simultanously; it also\n> occurred in t5400-send-pack.sh, but there it was worked around in\n> 71406ed4d6 (\"t5400: avoid concurrent writes into a trace file\",\n> 2017-05-18).\n> \n> Fortunately, Windows does support atomic O_APPEND semantics using the\n> file access mode FILE_APPEND_DATA. Provide an implementation that does.\n> \n> This implementation is minimal in such a way that it only implements\n> the open modes that are actually used in the Git code base. Emulation\n> for other modes can be added as necessary later. To become aware of\n> the necessity early, the unusal error ENOSYS is reported if an\n> unsupported mode is encountered.\n> \n> Diagnosed-by: Johannes Schindelin \n> Helped-by: Jeff Hostetler \n> Signed-off-by: Johannes Sixt \n> ---\n> compat/mingw.c | 41 +++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 39 insertions(+), 2 deletions(-)\n> \n\n[...]\n\nThis looks good. Thanks for following up on this.\n\nJeff\n"},{"id":"355539","messageId":"87a7pprred.fsf@evledraar.gmail.com","threadId":"49085","inReplyTo":"20180813223701.GC16006@sigill.intra.peff.net","subject":"Re: [PATCH] mingw: enable atomic O_APPEND","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-14T13:47:54Z","receivedAt":"2018-08-14T13:48:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Aug 13 2018, Jeff King wrote:\n\n> On Mon, Aug 13, 2018 at 11:22:10PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > O_APPEND is POSIX and means race-free append. If you mark some call\n>> > sites with O_APPEND, then that must be the ones that need race-free\n>> > append. Hence, you would have to go the other route: Mark those call\n>> > sites that do _not_ need race-free append with some custom\n>> > function/macro. (Or mark both with different helpers and avoid writing\n>> > down O_APPEND.)\n>>\n>> O_APPEND in POSIX is race-free only up to PIPE_MAX bytes written at a\n>> time, which is e.g. 2^12 by default on linux, after that all bets are\n>> off and the kernel is free to interleave different write calls.\n\n[I should have said PIPE_BUF, not PIPE_MAX]\n\n> This is a claim I've run across often, but I've never seen a good\n> citation for it.\n\nTo clarify I'm claiming that this is not a guarantee POSIX or linux\nsupport. Not that in practice it doesn't work on some systems.\n\nThe relevant POSIX docs are here:\nhttp://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html\n\n Write requests of {PIPE_BUF} bytes or less shall not be interleaved\n with data from other processes doing writes on the same pipe. Writes\n of greater than {PIPE_BUF} bytes may have data interleaved, on\n arbitrary boundaries, with writes by other processes, whether or not\n the O_NONBLOCK flag of the file status flags is set.\n\nAnd the Linux docs at http://man7.org/linux/man-pages/man7/pipe.7.html\nsay something similar:\n\n Writes of more than PIPE_BUF bytes may be nonatomic: the kernel may\n interleave the data with data written by other processes. POSIX.1\n requires PIPE_BUF to be at least 512 bytes. (On Linux, PIPE_BUF is\n 4096 bytes.)\n\n> Certainly atomic writes to _pipes_ are determined by PIPE_BUF (which\n> IIRC is not even a constant on Linux, but can be changed at run-time).\n> But is it relevant for regular-file writes?\n\nI believe it's hardcoded at PIPE_BUF which is defined as PAGE_SIZE on\nlinux. I think you may be thinking of /proc/sys/fs/pipe-max-pages which\nis the number of pages that a pipe will take before filling up, but I\nmay be wrong.\n\n> Another gem I found while digging on this O_APPEND/FILE_APPEND_DATA\n> stuff the other day: somebody claimed that the max atomic-append size on\n> Linux is 4096 and 1024 on Windows. But their experimental script was\n> done in bash! So I suspect they were really just measuring the size of\n> stdio buffers.\n\nYes, and some tests for this are wrong because they use e.g. \"print\" in\nsome higher-level language which'll always split stuff into writes of\n1024 or something.\n\n> Here's my attempt at a test setup. This C program forces two processes\n> to write simultaneously to the same file with O_APPEND:\n>\n> -- >8 --\n> #include \n> #include \n> #include \n> #include \n> #include \n> #include \n>\n> static void doit(int size, const char *fn, char c)\n> {\n> \tint fd;\n> \tchar *buf;\n>\n> \tfd = open(fn, O_WRONLY|O_APPEND|O_CREAT, 0666);\n> \tif (fd < 0) {\n> \t\tperror(\"open\");\n> \t\treturn;\n> \t}\n>\n> \tbuf = malloc(size);\n> \tmemset(buf, c, size);\n>\n> \twhile (1)\n> \t\twrite(fd, buf, size);\n> }\n>\n> int main(int argc, const char **argv)\n> {\n> \tint size = atoi(argv[1]);\n>\n> \tif (fork())\n> \t\tdoit(size, argv[2], '1');\n> \telse\n> \t\tdoit(size, argv[2], '2');\n> \treturn 0;\n> }\n> -- 8< --\n>\n> and then this program checks that we saw atomic units of the correct\n> size:\n>\n> -- >8 --\n> #include \n> #include \n> #include \n>\n> int main(int argc, const char **argv)\n> {\n> \tint size = atoi(argv[1]);\n> \tchar *buf;\n>\n> \tbuf = malloc(size);\n> \twhile (1) {\n> \t\tint i;\n> \t\t/* assume atomic reads, i.e., no signals */\n> \t\tint r = read(0, buf, size);\n> \t\tif (!r)\n> \t\t\tbreak;\n> \t\tfor (i = 1; i < size; i++) {\n> \t\t\tif (buf[i] != buf[0]) {\n> \t\t\t\tfprintf(stderr, \"overlap\\n\");\n> \t\t\t\treturn 1;\n> \t\t\t}\n> \t\t}\n> \t}\n> \treturn 0;\n> }\n> -- 8< --\n>\n> And then you can do something like:\n>\n> for size in 4097 8193 16385 32769 65537 131073 262145 524289 1048577; do\n> >out ;# clean up from last run\n> echo \"Trying $size...\"\n> timeout 5 ./write $size out\n> if ! ./check $size echo \"$size failed\"\n> break\n> fi\n> done\n>\n> On my Linux system, each of those seems to write several gigabytes\n> without overlapping. I did manage to hit some failing cases, but they\n> were never sheared writes, but rather cases where there was an\n> incomplete write at the end-of-file.\n\nYeah I can't make that fail experimentally either, except in the case\nyou mentioned. I also checked on a NetBSD & OpenBSD and OpenBSD box I\nhave access to, same thing.\n\n> So obviously this is all a bit of a tangent. I'd be fine declaring that\n> trace output is generally small enough not to worry about this in the\n> first place. But those results show that it shouldn't matter even if\n> we're writing 1MB trace lines on Linux. I wouldn't be at all surprised\n> to see different results on other operating systems, though.\n\nI don't know how this works internally in the kernel, but I'd be very\ncareful to make that assertion. Most likely this in practice depends on\nwhat FS you're using, its mount options, whether fsync() is involved,\nI/O pressure etc.\n\nFWIW this is something I've ran into in the past on Linux as a practical\nmatter, but that was many kernel versions ago, so maybe the semantics\nchanged.\n\nWe had an ad-hoc file format with each chunk a \"\\n\" format (and the was\nguaranteed not to contain \"\\n\"). These would be written to the same file\nby N workers. We would get corrupt data because of this in cases where\nwe were writing more than PIPE_BUF, e.g. start markers for unrelated\npayloads interleaved with content, lines that were incorrectly formed\netc.\n\nBut yeah, whether this is a practical concern for git trace output is\nanother matter. I just wanted to chime in to note that atomic appends to\nfiles are only portable on POSIX up to PIPE_BUF.\n"},{"id":"355545","messageId":"xmqqftzh2ets.fsf@gitster-ct.c.googlers.com","threadId":"49085","inReplyTo":"5a7e7ed1-da0b-20a4-147d-a24ca7da5832@jeffhostetler.com","subject":"Re: [PATCH] mingw: enable atomic O_APPEND","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-14T14:38:39Z","receivedAt":"2018-08-14T14:38:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> On 8/13/2018 3:02 PM, Johannes Sixt wrote:\n>>\n>> Fortunately, Windows does support atomic O_APPEND semantics using the\n>> file access mode FILE_APPEND_DATA. Provide an implementation that does.\n>> ...\n>>\n>> Diagnosed-by: Johannes Schindelin \n>> Helped-by: Jeff Hostetler \n>> Signed-off-by: Johannes Sixt \n>> ---\n>\n> This looks good. Thanks for following up on this.\n\nThanks, all. Will queue.\n"},{"id":"355547","messageId":"20180814145344.GC3441@sigill.intra.peff.net","threadId":"49085","inReplyTo":"87a7pprred.fsf@evledraar.gmail.com","subject":"Re: [PATCH] mingw: enable atomic O_APPEND","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-14T14:53:44Z","receivedAt":"2018-08-14T14:53:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 14, 2018 at 03:47:54PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> The relevant POSIX docs are here:\n> http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html\n> \n> Write requests of {PIPE_BUF} bytes or less shall not be interleaved\n> with data from other processes doing writes on the same pipe. Writes\n> of greater than {PIPE_BUF} bytes may have data interleaved, on\n> arbitrary boundaries, with writes by other processes, whether or not\n> the O_NONBLOCK flag of the file status flags is set.\n\nRight, this is the part I've seen, but it's pretty clearly only about\npipes, not regular files.\n\n> > Certainly atomic writes to _pipes_ are determined by PIPE_BUF (which\n> > IIRC is not even a constant on Linux, but can be changed at run-time).\n> > But is it relevant for regular-file writes?\n> \n> I believe it's hardcoded at PIPE_BUF which is defined as PAGE_SIZE on\n> linux. I think you may be thinking of /proc/sys/fs/pipe-max-pages which\n> is the number of pages that a pipe will take before filling up, but I\n> may be wrong.\n\nYeah, you're probably right.\n\n> > So obviously this is all a bit of a tangent. I'd be fine declaring that\n> > trace output is generally small enough not to worry about this in the\n> > first place. But those results show that it shouldn't matter even if\n> > we're writing 1MB trace lines on Linux. I wouldn't be at all surprised\n> > to see different results on other operating systems, though.\n> \n> I don't know how this works internally in the kernel, but I'd be very\n> careful to make that assertion. Most likely this in practice depends on\n> what FS you're using, its mount options, whether fsync() is involved,\n> I/O pressure etc.\n\nDefinitely it depends on the filesystem (and I'm pretty sure that at\nleast old versions of NFS could not possibly do O_APPEND correctly,\nbecause the protocol did not support an atomic seek+write).\n\nI agree that the experiment I did doesn't really tell us anything for\nsure. It _seems_ to work, but the machine was not under any kind of\nmemory or I/O pressure.\n\nI'd feel pretty confident that writes under a page are always going to\nbe fine, but anything else is \"seems to work\". To me that's enough for\ntracing, as the absolute worst case is jumbled output, not an on-disk\ncorruption.\n\n> FWIW this is something I've ran into in the past on Linux as a practical\n> matter, but that was many kernel versions ago, so maybe the semantics\n> changed.\n> \n> We had an ad-hoc file format with each chunk a \" marker>\\n\" format (and the was\n> guaranteed not to contain \"\\n\"). These would be written to the same file\n> by N workers. We would get corrupt data because of this in cases where\n> we were writing more than PIPE_BUF, e.g. start markers for unrelated\n> payloads interleaved with content, lines that were incorrectly formed\n> etc.\n\nInteresting. I wonder if it is because of PIPE_BUF, or it is simply the\npage size, which also happens to be the value of PIPE_BUF.\n\n> But yeah, whether this is a practical concern for git trace output is\n> another matter. I just wanted to chime in to note that atomic appends to\n> files are only portable on POSIX up to PIPE_BUF.\n\nI still think POSIX doesn't say anything either way here. The PIPE_BUF\nbits are _just_ about pipes. At any rate, I think we have a decent\nhandle on what systems actually _do_, which is more important than POSIX\nanyway.\n\n-Peff\n"},{"id":"355585","messageId":"313beb50-f5bd-ace6-8d6d-5f2a9e7cb6e7@kdbg.org","threadId":"49085","inReplyTo":"20180813223701.GC16006@sigill.intra.peff.net","subject":"Re: [PATCH] mingw: enable atomic O_APPEND","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-08-14T18:29:04Z","receivedAt":"2018-08-14T18:29:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 14.08.2018 um 00:37 schrieb Jeff King:\n> And then you can do something like:\n> \n> for size in 4097 8193 16385 32769 65537 131073 262145 524289 1048577; do\n> >out ;# clean up from last run\n> echo \"Trying $size...\"\n> timeout 5 ./write $size out\n> if ! ./check $size echo \"$size failed\"\n> break\n> fi\n> done\n> \n> On my Linux system, each of those seems to write several gigabytes\n> without overlapping. I did manage to hit some failing cases, but they\n> were never sheared writes, but rather cases where there was an\n> incomplete write at the end-of-file.\n\nI used your programs with necessary adjustments (as fork() is not \navailable), and did similar tests with concurrent processes. With packet \nsizes 1025, 4093, 7531 (just to include some odd number), and 8193 I did \nnot observe any overlapping or short writes.\n\nI'm now very confident that we are on the safe side for our purposes.\n\n-- Hannes\n"},{"id":"355597","messageId":"20180814191709.GB28452@sigill.intra.peff.net","threadId":"49085","inReplyTo":"313beb50-f5bd-ace6-8d6d-5f2a9e7cb6e7@kdbg.org","subject":"Re: [PATCH] mingw: enable atomic O_APPEND","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-14T19:17:09Z","receivedAt":"2018-08-14T19:17:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 14, 2018 at 08:29:04PM +0200, Johannes Sixt wrote:\n\n> Am 14.08.2018 um 00:37 schrieb Jeff King:\n> > And then you can do something like:\n> > \n> > for size in 4097 8193 16385 32769 65537 131073 262145 524289 1048577; do\n> > >out ;# clean up from last run\n> > echo \"Trying $size...\"\n> > timeout 5 ./write $size out\n> > if ! ./check $size > echo \"$size failed\"\n> > break\n> > fi\n> > done\n> > \n> > On my Linux system, each of those seems to write several gigabytes\n> > without overlapping. I did manage to hit some failing cases, but they\n> > were never sheared writes, but rather cases where there was an\n> > incomplete write at the end-of-file.\n> \n> I used your programs with necessary adjustments (as fork() is not\n> available), and did similar tests with concurrent processes. With packet\n> sizes 1025, 4093, 7531 (just to include some odd number), and 8193 I did not\n> observe any overlapping or short writes.\n> \n> I'm now very confident that we are on the safe side for our purposes.\n\nGreat, thanks for testing!\n\nRe-reading what I wrote about end-of-file above and thinking about the\nconversation with Ævar elsewhere in the thread, I suspect it _is_ easy\nto get overlapping writes if the processes are receiving signals (since\nclearly the TERM signal caused a partial write).\n\nMy experiment doesn't simulate that at all. I suppose the parent process\ncould send SIGUSR1 to the child in each loop, and the child would catch\nit but keep going.\n\nHmm, that was easy enough to do (programs below for reference), but\nsurprisingly it didn't fail for me (except for the normal end-of-file\ntruncation). It's like the OS is willing to truncate the write of a\ndying program but not one for a signal that is getting handled. Which is\ngreat for us, since it's exactly what we want, but makes me even more\nsuspicious that a non-Linux kernel might behave completely differently.\n\nI still think we're fine in practice, as I'd expect any kernel to be\natomic under the page size. So this was mostly just for my own\nedification.\n\n-Peff\n\n-- >8 --\n/* check.c, with separate short-read reporting */\n#include \n#include \n#include \n\nint main(int argc, const char **argv)\n{\n\tint size = atoi(argv[1]);\n\tint block = 0;\n\tchar *buf;\n\n\tbuf = malloc(size);\n\twhile (1) {\n\t\tint i;\n\t\t/* assume atomic reads */\n\t\tint r = read(0, buf, size);\n\t\tif (!r)\n\t\t\tbreak;\n\t\tif (r < size) {\n\t\t\tfprintf(stderr, \"short read\\n\");\n\t\t\treturn 1;\n\t\t}\n\t\tfor (i = 1; i < size; i++) {\n\t\t\tif (buf[i] != buf[0]) {\n\t\t\t\tfprintf(stderr, \"overlap in block %d\\n\", block);\n\t\t\t\treturn 1;\n\t\t\t}\n\t\t}\n\t\tblock++;\n\t}\n}\n-- >8 --\n\n-- >8 --\n/* write.c with signals; you can also confirm via strace\n that each write is atomic */\n#include \n#include \n#include \n#include \n#include \n#include \n#include \n\nvoid handle_signal(int sig)\n{\n\t/* do nothing */\n}\n\nstatic void doit(int size, const char *fn, char c, pid_t pid)\n{\n\tint fd;\n\tchar *buf;\n\n\tfd = open(fn, O_WRONLY|O_APPEND|O_CREAT, 0666);\n\tif (fd < 0) {\n\t\tperror(\"open\");\n\t\treturn;\n\t}\n\n\tbuf = malloc(size);\n\tmemset(buf, c, size);\n\n\twhile (1) {\n\t\tif (pid)\n\t\t\tkill(pid, SIGUSR1);\n\t\twrite(fd, buf, size);\n\t}\n}\n\nint main(int argc, const char **argv)\n{\n\tint size = atoi(argv[1]);\n\tpid_t pid;\n\n\tsignal(SIGUSR1, handle_signal);\n\n\tpid = fork();\n\tif (pid)\n\t\tdoit(size, argv[2], '1', pid);\n\telse\n\t\tdoit(size, argv[2], '2', pid);\n\treturn 0;\n}\n-- >8 --\n"}]}