patcht6026-merge-attr: wait for process to release trash directory

The process spawned in the hook uses the test's trash directory as CWD. As long as it is alive, the directory cannot be removed on Windows. Although the test succeeds, the 'test_done' that follows produces an error message and leaves the trash directory around. Insert a delay to give the hook time to go away.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t6026-merge-attr.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Hi Hannes,

On Mon, 5 Sep 2016, Johannes Sixt wrote:

Maybe we should write a pid file in the sleep command instead, and kill it in the end? Something like this, maybe?

-- snipsnap --

The process spawned in the hook uses the test's trash directory as CWD. As long as it is alive, the directory cannot be removed on Windows. Although the test succeeds, the 'test_done' that follows produces an error message and leaves the trash directory around. Kill the process before the test case advances.

Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 06.09.2016 um 09:25 schrieb Johannes Schindelin:

Yes, that is much better, thank you!

I did not extend the sleep time because it requires to change the file name in the same patch.

I did experiment with 'while sleep 1; do :; done &' to spin indefinitely, but the loop did not terminate in time in my tests on Windows for some unknown reason (most likely because the killed process does not exit with a non-zero code -- remember, Windows is not POSIX, particularly, when it comes to signal handling).

t/t6026-merge-attr.sh | 2 ++
 1 file changed, 2 insertions(+)

Hi Hannes,

On Wed, 7 Sep 2016, Johannes Sixt wrote:

Yeah, I was just concerned that maybe we would take longer than that second to finish the test. But I guess I am just too paranoid here.

Thanks for the patch! Dscho

On Mon, Sep 05, 2016 at 09:03:48PM +0200, Johannes Sixt wrote:

Ugh. I'd love it if we could avoid inserting a sleep, which wastes time in the optimistic case and is insufficient in the pessimistic one.

The fact that the hook is already using sleep is even nastier, as it that's a potential race on a loaded system.

Can we do some signaling with fifos to tell the hook when it is safe to exit? Then we would just need to `wait` for its parent process.

-Peff

Jeff King <peff@peff.net> writes:

Is fifo safe on Windows, though?

With v2 that explicitly kills, I guess we can make the sleep longer without slowing down in the optimistic case?

On Wed, Sep 07, 2016 at 11:39:57AM -0700, Junio C Hamano wrote:

No clue. We seem to use mkfifo unconditionally in lib-daemon, but perhaps people do not run that test on Windows. Other invocations seem to be protected by the PIPE prerequisite. But...

Yeah, I think the v2 one is non-racy (I thought at first we might race with the "echo", but it should be synchronous; the hook will not exit until we have written the pid file, and git will not exit until the hook is done running).

I agree that the sleep could be made longer, to make the test less racy. However, the racy failure mode is that it might pass while testing nothing (i.e., the sleep ends anyway before the hook returns), so I don't think it's a high priority.

-Peff

Jeff King <peff@peff.net> writes:

I do not think it is necessary, either. It was just me wondering aloud, nothing more.

Hi Peff & Junio,

On Wed, 7 Sep 2016, Jeff King wrote:

AFAICT we do not use mkfifo on Windows. Let's see what t/test-lib.sh has to say about the matter:

	test_lazy_prereq PIPE '
		# test whether the filesystem supports FIFOs
		case $(uname -s) in
		CYGWIN*|MINGW*)
			false
			;;
		*)
			rm -f testfifo && mkfifo testfifo
			;;
		esac
	'

So there you go.

The reason it is disabled is that Cygwin/MSYS2 do have a concept of a FIFO. But `git.exe` won't be able to access such a FIFO because it is emulated by the POSIX emulation layer, which Git cannot access.

Please note that Hannes and I discussed this (as I originally suggested to increase it to 10 seconds, and Hannes rightfully pointed out that we would have to change the script name, too, as it says sleep-one-second.sh, and that would have made the patch less readable) and we came to the same conclusion: it's not necessary.

Ciao, Dscho

On Thu, Sep 08, 2016 at 10:05:58AM +0200, Johannes Schindelin wrote:

Regarding my "unconditionally" above: coincidentally, I happened to be looking in lib-git-daemon.sh about an hour ago and noticed that we do indeed check "test_have_prereq PIPE" (just not near the mkfifo, of course, because we are not in a test block).

It seems to have been added by a "Johannes Schindelin". Any relation?

So yeah. It definitely would be a bad idea to use a fifo in a test that is already Windows-specific.

-Peff

Hi Peff,

On Thu, 8 Sep 2016, Jeff King wrote:

Maybe ;-) Dscho