Re: [PATCH] t5510: run auto-gc in the foreground

From: SZEDER Gábor <szeder@ira.uka.de>
Date: May 2, 2016, 23:55 UTC
Message-ID: <20160503015526.Horde.e0uZ0P4BqpNnwx_zmhu3WfE@webmail.informatik.kit.edu>
In-Reply-To: <alpine.DEB.2.20.1605020859131.9313@virtualbox>

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

Show 19 quoted lines

> Hi Gábor,
>
> On Sun, 1 May 2016, SZEDER Gábor wrote:
>
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index 38321d19efbe..454d896390c0 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does
>> not lock up' '
>> 	(
>> 		cd auto-gc &&
>> 		git config gc.autoPackLimit 1 &&
>> +		git config gc.autoDetach false &&
>> 		GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
>> 		! grep "Should I try again" fetch.out
>> 	)
>
> Sounds good to me.

There is something still bothering me, though.

I take this was a Windows-specific issue; deleting open files on Linux is no brainer. According to a comment on the original Git for Windows issue at github[1], 'git gc' runs in the background by default on Windows as well.

Now, it's 'git gc --auto' that's trying to delete pack and index files that became redundant after repacking, and when it can't delete those files, then complains. I.e. those "Should I try again" questions the test looks for come from 'git gc', not from 'git fetch'. So far so good.

Let's assume that someone inconsiderately removes that closed_all_pack() call added to cmd_fetch(), basically reverting the fix in 0898c9628104. The test added in the same commit (i.e. not including my fix here) should still be able to catch it, but I think it's possible that it sometimes remains unnoticed. Consider the following sequence:

    1. 'git fetch' does its thing, including opening pack and index files.
    2. 'git fetch' launches 'git gc --auto' which then goes into background.
    3. The scheduler happens to decide that it's 'git fetch's turn again,
       and it finishes, including closing all opened pack and index files.
    4. 'git gc' gets a chance to proceed, repacks and then manages to delete
       all redundant pack and index files successfully.
    5. 'grep' doesn't find the string it looks for.
    6. The test succeeds.

And the background 'git gc --auto' doesn't even have to be delayed that long, because 'git fetch' exits immediately after launching it. That's considerably shorter than the delay necessary for the 'rm -rf' error I described in the commit message, because for the latter 'git fetch' must finish, 'grep' must run, and 'test_done' must write the test results and start 'rm -rf $trash' while 'git gc' is still running in the background.

So, if I'm right, then my fix is not just about avoiding a sporadic error from the test harness, but it's also important for the test's correctness. But am I right? Alas I don't have a Git for Windows dev environment to play around with this.

[1] - https://github.com/git-for-windows/git/issues/500#issuecomment-149933531

Gábor

Show 5 quoted lines

> Alternatively, we could consider passing `-c gc.autoDetach=false` instead,
> to limit the scope. I am not insisting on it, of course ;-)
>
> Ciao,
> Dscho