On Mon, Apr 10, 2017 at 3:58 PM, Jeff King <peff@xxxxxxxx> wrote: > On Mon, Apr 10, 2017 at 02:59:11PM +0200, SZEDER Gábor wrote: > >> While this change doesn't completely eliminate the possibility of >> this race, it significantly and seemingly sufficiently reduces its >> probability. Running t6500 in a loop while my box was under heavy CPU >> and I/O load usually triggered the error under 15 seconds, but with >> this patch it run overnight without incident. >> >> (Alternatively, we could check the presence of the gc.pid file after >> starting the detached auto gc, and wait while it's there. However, >> this would create a different race: the auto gc process creates the >> pid file after it goes to the background, so our check in the >> foreground might racily look for the pid file before it's even >> created, and thus would not wait for the background gc to finish. >> Furthermore, this would open up the possibility of the test hanging if >> something were to go wrong with the gc process and the pid file were >> not removed.) > > Could we just leave open a pipe descriptor that the child inherits, and > wait for it to close? > > Something like: > > git gc --auto 9>&1 | read > > should wait until the background gc process finishes. It depends on our > daemonize() not closing descriptors beyond 0/1/2, but that is certainly > the case now. > > It also loses the exit status of the main "git gc", but that can be > fixed with shell hackery: > > code=$(sh -c 'git gc --auto; echo $?' 9>&1) > test "$code" = 0 Indeed this seems to work, and luckily we don't need that much hackery. When there is a single variable assignment and the expansion of a command substitution is assigned to the variable, then the exit status is that of the command inside the command substitution, i.e. $ v=$(false) ; echo $? 1 This means we can write this simply as: doesnt_matter=$(git gc --auto 9>&1) It's still hackery :) OTOH, this makes it possible to continue the test reliably after the gc finished in the background, so we could also check that there is only a single pack file left, i.e. that the detached gc did what it was supposed to do.