Re: [PATCHv4 1/4] Refactor for_each_ref variants to use for_each_ref_in and avoid magic numbers

[Josh Triplett <josh@joshtriplett.org>]

On Thu, Jun 02, 2011 at 01:36:23PM -0700, Junio C Hamano wrote:
> Jamey Sharp <jamey@minilop.net> writes:
> > Furthermore, for_each_ref and for_each_ref_submodule passed "refs/" but
> > a length of 0, which caused do_for_each_ref to ignore the "refs/".
> 
> I had to read, stop, think for two days, until finally get to the point
> that I _think_ I understand what you wanted to say.
> 
> As we use the same "trim" (meant to say "strip this many bytes from the
> beginning of the full refname when calling the callback") to reject refs
> outside the area we are interested in with the strncmp() at the beginning
> of do_one_ref(), if do_for_each_ref() that is called by for_each_ref() fed
> something outside "refs/" hierarchy to the function, the garbage ref that
> is not a ref (as it is outside "refs/") will _not_ get filtered, which I
> think is what you are trying to say by 'ignore the "refs/"'.
> 
> Which is technically a bug (we should be rejecting anything outside
> "refs/", even when trim is set to 0) that dates as far back as e1e22e3
> (Start handling references internally as a sorted in-memory list,
> 2006-09-11), but it didn't matter an iota because everything we read from
> either loose or packed refs have "refs/" prefix.
> 
> Am I following your train of thought correctly so far?

Yes.  The calls that currently pass base="refs/" and trim=0 do not
filter the refs to those starting with "refs/" because they strncmp with
0 bytes.  We very intentionally ensured that this refactoring commit
made no semantic change to the current behavior.  As you point out,
everything produced from loose or packed refs will always start with
"refs/" anyway.

> > diff --git a/refs.c b/refs.c
> > index e3c0511..60cebe6 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -584,7 +584,7 @@ int read_ref(const char *ref, unsigned char *sha1)
> >  static int do_one_ref(const char *base, each_ref_fn fn, int trim,
> >  		      int flags, void *cb_data, struct ref_list *entry)
> >  {
> > -	if (strncmp(base, entry->name, trim))
> > +	if (prefixcmp(entry->name, base))
> >  		return 0;
> >  
> >  	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {
> > ...
> >  int for_each_ref(each_ref_fn fn, void *cb_data)
> >  {
> > -	return do_for_each_ref(NULL, "refs/", fn, 0, 0, cb_data);
> > +	return for_each_ref_in("", fn, cb_data);
> >  }
> 
> But then this looks like a bad way to fix that issue.  It will be a
> non-issue as long as do-for-each-ref will never give anything outside
> "refs/", but once that happens (say, a contaminated .git/packed-refs
> file), this will show whatever that is outside "refs/", i.e. the issue the
> proposed commit log message claims to address, which is "... which caused
> do_for_each_ref to ignore", is not fixed here at all.

We didn't intend the commit message to suggest that we changed that
behavior; we intended that commit message to document why the commit
*didn't* change the behavior despite changing "refs/" to "".

> Shouldn't you be passing prefix and trim the same way as we have always
> done, but just fixing the strncmp() at the beginning of do_one_ref()?

I still think prefixcmp makes the most sense; if you pass a given base,
you expect do_for_each_ref to use that entire base as the prefix.  If
you want for_each_ref to start filtering out anything that doesn't start
with "refs/", then it could continue passing "refs/" and 0 rather than
calling for_each_ref_in.  It doesn't matter for this patch series either
way; we just didn't want this refactor to change the existing behavior.

- Josh Triplett