[PATCH] t6026-merge-attr: wait for process to release trash directory

[PATCH] t6026-merge-attr: wait for process to release trash directory

[Johannes Sixt]

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(-)

diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
index dd8f88d..2c5b138 100755
--- a/t/t6026-merge-attr.sh
+++ b/t/t6026-merge-attr.sh
@@ -191,7 +191,10 @@ test_expect_success 'custom merge does not lock index' '
 		"* merge=ours" "text merge=sleep-one-second" &&
 	test_config merge.ours.driver true &&
 	test_config merge.sleep-one-second.driver ./sleep-one-second.sh &&
-	git merge master
+	git merge master &&
+	# the trash directory cannot be removed on Windows until the hook
+	# does not occupy it for its CWD anymore; wait for it to quit:
+	sleep 1
 '
 
 test_done
-- 
2.10.0.85.gea34e30

Re: [PATCH] t6026-merge-attr: wait for process to release trash directory

[Johannes Schindelin]

Hi Hannes,

On Mon, 5 Sep 2016, Johannes Sixt wrote:

> 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.

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

-- snipsnap --
diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
index dd8f88d..2e2beb5 100755
--- a/t/t6026-merge-attr.sh
+++ b/t/t6026-merge-attr.sh
@@ -184,8 +184,10 @@ test_expect_success 'up-to-date merge without common
ancestor' '
 test_expect_success 'custom merge does not lock index' '
 	git reset --hard anchor &&
 	write_script sleep-one-second.sh <<-\EOF &&
-		sleep 1 &
+		sleep 10 &
+		echo $! >sleep.pid
 	EOF
+	test_when_finished "kill -9 \$(cat sleep.pid)" &&
 
 	test_write_lines >.gitattributes \
 		"* merge=ours" "text merge=sleep-one-second" &&

Re: [PATCH] t6026-merge-attr: wait for process to release trash directory

[Jeff King]

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

> 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.

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

[PATCH v2] t6026-merge-attr: clean up background process at end of test case

[Johannes Sixt]

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:
> Maybe we should write a pid file in the sleep command instead, and kill it
> in the end? Something like this, maybe?

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(+)

diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
index dd8f88d..7a6e33e 100755
--- a/t/t6026-merge-attr.sh
+++ b/t/t6026-merge-attr.sh
@@ -185,7 +185,9 @@ test_expect_success 'custom merge does not lock index' '
 	git reset --hard anchor &&
 	write_script sleep-one-second.sh <<-\EOF &&
 		sleep 1 &
+		echo $! >sleep.pid
 	EOF
+	test_when_finished "kill \$(cat sleep.pid)" &&
 
 	test_write_lines >.gitattributes \
 		"* merge=ours" "text merge=sleep-one-second" &&
-- 
2.10.0.85.gea34e30

Re: [PATCH v2] t6026-merge-attr: clean up background process at end of test case

[Johannes Schindelin]

Hi Hannes,

On Wed, 7 Sep 2016, Johannes Sixt wrote:

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

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

Re: [PATCH] t6026-merge-attr: wait for process to release trash directory

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Sep 05, 2016 at 09:03:48PM +0200, Johannes Sixt wrote:
>
>> 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.
>
> 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.

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?

Re: [PATCH] t6026-merge-attr: wait for process to release trash directory

[Jeff King]

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

> > 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.
> 
> Is fifo safe on Windows, though?

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...

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

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

Re: [PATCH] t6026-merge-attr: wait for process to release trash directory

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

Re: [PATCH] t6026-merge-attr: wait for process to release trash directory

[Johannes Schindelin]

Hi Peff & Junio,

On Wed, 7 Sep 2016, Jeff King wrote:

> On Wed, Sep 07, 2016 at 11:39:57AM -0700, Junio C Hamano wrote:
> 
> > > 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.
> > 
> > Is fifo safe on Windows, though?
> 
> 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...

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.

> > With v2 that explicitly kills, I guess we can make the sleep longer
> > without slowing down in the optimistic case?
> 
> 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).

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

Re: [PATCH] t6026-merge-attr: wait for process to release trash directory

[Jeff King]

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

> > > Is fifo safe on Windows, though?
> > 
> > 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...
> 
> 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.

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

Re: [PATCH] t6026-merge-attr: wait for process to release trash directory

[Johannes Schindelin]

Hi Peff,

On Thu, 8 Sep 2016, Jeff King wrote:

> On Thu, Sep 08, 2016 at 10:05:58AM +0200, Johannes Schindelin wrote:
> 
> > > > Is fifo safe on Windows, though?
> > > 
> > > 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...
> > 
> > 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.
> 
> 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?

Maybe ;-)
Dscho