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

From: Jeff King <peff@peff.net>
Date: Sep 24, 2015, 17:03 UTC
Message-ID: <20150924170320.GA14695@sigill.intra.peff.net>
In-Reply-To: <a3a883ce9d7a8bf4220beb640caca5bed0f7d492.1443085919.git.johannes.schindelin@gmx.de>

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

Show 9 quoted lines

> 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