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

From: SZEDER Gábor <szeder@ira.uka.de>
Date: May 3, 2016, 11:50 UTC
Message-ID: <20160503135028.Horde.jeJdKT1kb2NTVAS1HpcsQh2@webmail.informatik.kit.edu>
In-Reply-To: <20160503015526.Horde.e0uZ0P4BqpNnwx_zmhu3WfE@webmail.informatik.kit.edu>

Quoting SZEDER Gábor <szeder@ira.uka.de>:

Show 27 quoted lines

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

Ok, having slept on it, it was a false alarm.

Though 'git gc --auto' claims "Auto packing the repository in background for optimum performance." on Windows, it does in fact runs in the foreground.

'git gc --auto' first prints that message, unless gc.autoDetach is disabled, and then calls daemonize() to go to the background. However, daemonize() is basically a no-op on Windows, thus 'git gc' will remain in the foreground and the sequence I described below is impossible. Good.

Perhaps it would be worth updating 'git gc' to not lie about going to the background when we can already know in advance that it won't.

Show 44 quoted lines

> 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