Re: [PATCH 3/4] replace-objects: evaluate replacement refs without using the object store

From: Jeff King <peff@peff.net>
Date: Sep 13, 2017, 11:15 UTC
Message-ID: <20170913111501.cosukqzf76oy65ow@sigill.intra.peff.net>
In-Reply-To: <fe956802-41ab-783f-62b3-f7c4b8292e8c@alum.mit.edu>

On Wed, Sep 13, 2017 at 10:03:58AM +0200, Michael Haggerty wrote:

Show 22 quoted lines

> On 09/12/2017 07:31 PM, Jonathan Nieder wrote:
> > From: Stefan Beller <sbeller@google.com>
> > 
> > Pass DO_FOR_EACH_INCLUDE_BROKEN when iterating over replacement refs
> > so that the iteration does not require opening the named objects from
> > the object store. This avoids a dependency cycle between object access
> > and replace ref iteration.
> > 
> > Moreover the ref subsystem has not been migrated yet to access the
> > object store via passed in repository objects.  As a result, without
> > this patch, iterating over replace refs in a repository other than
> > the_repository it produces errors:
> > 
> >    error: refs/replace/3afabef75c627b894cccc3bcae86837abc7c32fe does not point to a valid object!
> > 
> > Noticed while adapting the object store (and in particular its
> > evaluation of replace refs) to handle arbitrary repositories.
> 
> Have you checked that consumers of this API can handle broken
> references? Aside from missing values, broken references can have
> illegal names (though hopefully not unsafe in the sense of causing
> filesystem traversal outside of `$GITDIR`).

It looks like there are only two callers. One complains about names that aren't 40-hex. The other ("git replace -l") parses the 40-hex in "long" mode, but will print anything in short mode (not that printing is very dangerous).

I do have to wonder if for_each_replace_ref() should just do the 40-hex syntactic check itself (and issue a warning for other refs). It seems like that would be the point of calling for_each_replace_ref() instead of just for_each_ref_in("refs/replace") yourself, and both of the callers would benefit.

-Peff