Quoting SZEDER Gábor <szeder@xxxxxxxxxx>:
Quoting Johannes Schindelin <Johannes.Schindelin@xxxxxx>:
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.
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html