Re: [PATCH v2 1/3] index-pack: add --unpack-limit to unpack objects

From: Jeff King <peff@peff.net>
Date: Sep 8, 2013, 06:35 UTC
Message-ID: <20130908063533.GA22949@sigill.intra.peff.net>
In-Reply-To: <CACsJy8DeJnwJ2hxbKs+ks3Rp6h+uedFA+qt8JTTwyen8sF7XMw@mail.gmail.com>

On Sun, Sep 08, 2013 at 01:28:47PM +0700, Nguyen Thai Ngoc Duy wrote:

Show 13 quoted lines

> > From a cursory read, this seems fine. If it were done in complete
> > isolation, I'd say it was a slight regression, just because we are doing
> > more I/O for the unpack case, and it is not really saving us any code
> > (it is not like we can throw away unpack-objects, as I think we would
> > want to keep it as a last resort for getting data out of malformed or
> > otherwise non-indexable packs).
> 
> I can see unpack-objects is more tolerable on corrupt/incomplete
> packs. If index-pack finds something wrong, it aborts the whole
> process. I think we can make index-pack stop at the first bad object,
> adjust nr_objects, and try to recover as much as possible out of the
> good part. Any other reasons to keep unpack-objects (because I still
> want to kill it)?

No, I don't think there is another reason. The command name may hang around for historical reasons, but we can always make it an alias for "index-pack --unpack-limit=0" or whatever.

I do not think index-pack even needs to be particularly clever about trying to recover. It is mainly that we may do some extra sanity checks when writing the index (e.g., the recent discussion on avoiding duplicates in the index), that do not even come up when simply exploding the pack in a linear fashion. But I don't think there is any fundamental reason why index-pack could not learn to be as robust when operating in unpack mode.

As an aside, you cannot just drop the nr_objects that index-pack's generated index says it contains. Packfile readers double-check that the .idx and .pack agree on the number of objects (I tried it as a method for working around duplicate entries :) ).

-Peff