Re: [PATCH 0/16] make prune mtime-checking more careful

From: Jeff King <peff@peff.net>
Date: Oct 8, 2014, 08:31 UTC
Message-ID: <20141008083109.GA3767@peff.net>
In-Reply-To: <20141006014249.GA11495@peff.net>

On Sun, Oct 05, 2014 at 09:42:49PM -0400, Jeff King wrote:

Show 8 quoted lines

> On Sat, Oct 04, 2014 at 03:22:10PM -0700, Junio C Hamano wrote:
> 
> > This applied on top of 'maint' (which does have c40fdd01) makes the
> > test #9 (prune: do not prune detached HEAD with no reflog) fail.
> 
> I'll fix the bone-headed error returns that René noticed and double
> check that they were the complete culprit in the test failure you saw
> (and not just masking some other problem).

OK, I figured out what is going on. The short of it is that yes, it's the return-value bug René noticed. Read on only if you are really interested. :)

The test runs "git prune" and intends to check that the detached HEAD commit is not in the list. It does so by checking the whole output of "git prune -n", which it expects to be empty (and it generally is, because we've gc'd in previous tests and all objects were either packed or pruned).

However, when the test fails, there is a pruned object. But it is not the HEAD commit that we just wrote, but rather the tree that it points to. When we run "git commit", it tries to write out the tree with write_sha1_file. This in turn calls freshen_packed_object to check whether we have the object packed. We do, but the return-value bug makes us think we do not. As a result, we write out an extra copy of the tree as a loose object, and that is what gets pruned (and not by prune's normal is-it-old-and-unreachable code, but by its call to prune_packed).

So that explains that bug (as a side note, you might think that if we are flipping return values, lots of things would fail when they ask "do we have this packed object" and it erroneously says "yes". But that does not happen. The wrong return value comes from freshening the file, so we only flip "yes" to "no", and never the opposite).

> > If we merge 'dt/cache-tree-repair' (which in turn needs
> > 'jc/reopen-lock-file') to 'maint' and then apply these on top, the
> > said test passes.  But I do not see an apparent reason why X-<.

When dt/cache-tree-repair is merged, we have a valid cache tree when we run "git commit", and we realize that we do not need to write out the tree object at all. Thus we never hit the buggy code, the object isn't created, and the subsequent prune reports that there is nothing to prune.

-Peff