Re: [PATCH 2/4] pack-objects: do not get distracted by stale refs

[Jeff King <peff@peff.net>]

On Thu, Sep 24, 2015 at 11:13:44AM +0200, Johannes Schindelin wrote:

> It is quite possible for, say, a remote HEAD to become stale, e.g. when
> the default branch was renamed.
> 
> We should still be able to pack our objects when such a thing happens;
> simply ignore invalid refs (because they cannot matter for the packing
> process anyway).
> [...]
>  	init_revisions(&revs, NULL);
> +	revs.ignore_missing = 1;

I think this is dangerous, because a repack may drop unreferenced
history. Imagine you have a corrupted repository where you are missing
the commit at the tip of "master", but still have "master^" and the rest
of the history, and "git gc --auto" triggers.

Right now, pack-objects will barf and refuse to pack. Your repo is still
corrupted, but you can use git-fsck to recover the history minus the
single commit. With your patch, the repack will delete the entire
history. Of course there are complications like other branches
referencing the history, reflogs if it is a non-bare repo, and the
2-week expiration time. But there's definitely a potential to make
things worse. I think commands that drop objects (like repack and prune)
need to be more careful than other commands about refusing to run when
there is something fishy in the repository.

I think we would want to introduce a flag to ignore dangling
symbolic refs when running for_each_ref, as those should be harmless. In
fact, I think we could omit symrefs completely for reachability
analysis; we would hit the ref they refer to anyway during the rest of
the traversal.

-Peff