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

[Jeff King <peff@peff.net>]

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

> 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