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

["SZEDER Gábor" <szeder@ira.uka.de>]

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

> 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


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