On Tue, May 16, 2017 at 08:35:46PM +0200, Johannes Sixt wrote: > Am 16.05.2017 um 00:24 schrieb Jeff King: > > 4. There is something racy and unportable about both programs writing > > to the same trace file. It's opened with O_APPEND, which means that > > each write should atomically position the pointer at the end of the > > file. Is it possible there's a problem with that in the way > > O_APPEND works on Windows? > > > > If that was the case, though, I'd generally expect a sheared write > > or a partial overwrite. The two origin/HEAD lines from the two > > programs are the exact same length, but I'd find it more likely for > > the overwrite to happen with one of the follow-on lines. > > Good guesswork! O_APPEND is not atomic on Windows, in particular, it is > emulated as lseek(SEEK_END) followed by write(). > > I ran the test a few more times, and it fails in different ways (different > missing lines) and also succeeds in a minority of the cases. OK, that definitely explains it, then. > Windows is capable of writing atomically in the way O_APPEND requires > (keyword: FILE_APPEND_DATA), but I do not see a way to wedge this into the > emulation layer. For the time being, I think I have to skip the test case. I wonder if there's a way we could convince the trace for the two programs to go to separate locations. We don't care about receive-pack's trace at all. So maybe: diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh index 3331e0f53..d375d7110 100755 --- a/t/t5400-send-pack.sh +++ b/t/t5400-send-pack.sh @@ -288,7 +288,10 @@ test_expect_success 'receive-pack de-dupes .have lines' ' $shared .have EOF - GIT_TRACE_PACKET=$(pwd)/trace git push fork HEAD:foo && + GIT_TRACE_PACKET=$(pwd)/trace \ + git push \ + --receive-pack="unset GIT_TRACE_PACKET; git-receive-pack" \ + fork HEAD:foo && extract_ref_advertisement <trace >refs && test_cmp expect refs ' > The question remains how endangered our uses of O_APPEND are when the POSIX > semantics are not obeyed precisely: > > * trace.c: It is about debugging. Not critical. Agreed. > * sequencer.c: It writes the "done" file. No concurrent accesses are > expected: Not critial. Agreed. > * refs/files-backend.c: There are uses in functions open_or_create_logfile() > and log_ref_setup(). Sounds like it is in reflogs. Sounds like this is about > reflogs. If there are concurrent accesses, there is a danger that a reflog > is not recorded correctly. Then reflogs may be missing and objects may be > pruned earlier than expected. That's something to worry about, but I would > not count it as mission critical. We should always hold the matching ref lock already when modifying a reflog. I seem to recall there was a case that missed this (I think maybe modifying the HEAD reflog without holding HEAD.lock), but it was fixed in the last few versions. So I think we're probably OK, but I agree it's an interesting gotcha that may bite us in the future. -Peff