Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 10, 2016, 22:30 UTC
Message-ID: <xmqq60nv55o3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1611102254340.24684@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 6 quoted lines

>> OK.  sleep.pid is a reasonable easy-to-access side effect we can
>> observe to make sure that the sleep-one-second merge driver was
>> indeed invoked, which was missing from the earlier round.
>
> No, this is incorrect. The condition that we need to know applies is that
> the script is still running, and blocking if the bug reappears.

OK, I see what you are saying, and I see a few things wrong in here:

 * First, the test is titled in a misleading way.  In the context of
   a patch that was titled ad65f7e3b7 ("t6026-merge-attr: child
   processes must not inherit index.lock handles", 2016-08-18), it
   might have been clear enough to say "does not lock index", but
   the sleeping is to make sure that we would notice if the fd to
   the index.lock leaked to the child process by mistake, and the
   way to do so is that the child arranges the leaked fd to be kept
   open after it exits (by spawning "sleep").  The test was never
   about "does not lock index" (the driver does not take any lock by
   itself in the first place).

 * There are three possible outcome from this test:

   - 'git merge' fails.

     This is expected to happen only on Windows and if the code gets
     broken and starts leaking the fd.

   - 'git merge' finishes correctly, the sleep is still running when
     test_when_finished goes to cull it.

     In this case, we KNOW there wasn't any fd leak IF we are on
     Windows where a leaked FD would not allow 'git merge' to
     succeed.  But on other platforms, fd leak that may cause
     trouble for Windows friends will not be caught.

   - 'git merge' finishes correctly, the sleep is no longer
     running because the machine was heavily loaded; a workaround is
     to tolerate failure of culling it.

     In this case, we cannot tell anything from the test.  Even if
     the fd was leaked, 'git merge' may have succeeded even on
     Windows.

As everybody knows there is no appropriate timeout value that is good for everybody. I wonder if we can replace the sleep 1 with something like

	( while sleep 3600; do :; done ) &

so that leaked fd will be kept even in any heavily loaded environment instead?