Re: t5400 failure on Windows

On Tue, May 16, 2017 at 08:35:46PM +0200, Johannes Sixt wrote:

Am 16.05.2017 um 00:24 schrieb Jeff King:

quoted

   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