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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 2, 2011, 20:36 UTC
Message-ID: <7vk4d4c6ns.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1306887870-3875-1-git-send-email-jamey@minilop.net>

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?

Show 19 quoted lines

> 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.

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()?