Re: [PATCH v2 1/3] refs: keep track of unresolved reference value in iterator

[Junio C Hamano <gitster@pobox.com>]

> diff --git a/refs.h b/refs.h
> index 298caf6c618..2e740c692ac 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -71,9 +71,10 @@ struct pack_refs_opts {
>  	struct ref_exclusions *exclusions;
>  	struct string_list *includes;
>  };
> -
>  const char *refs_resolve_ref_unsafe(struct ref_store *refs,
> +
>  				    const char *refname,
> +				    char **referent,
>  				    int resolve_flags,
>  				    struct object_id *oid,
>  				    int *flags);

If referent is meant to be an out-parameter, it should sit next to
oid that is also an out-parameter.  And as a late-comer sibling, it
should sit after its elder brother.

Also, I do not see the reason for the shuffling of blank lines.
Shouldn't it be the other way around, even?  After an unrelated
definition of "struct pack_refs_opts", there is (and should be)
a blank line, then the first line of the declaration of function.

Perhaps some fat-fingering.

> @@ -1928,6 +1928,7 @@ int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,
>  
>  const char *refs_resolve_ref_unsafe(struct ref_store *refs,
>  				    const char *refname,
> +				    char **referent,
>  				    int resolve_flags,
>  				    struct object_id *oid,
>  				    int *flags)
> @@ -1989,6 +1990,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
>  		}
>  
>  		*flags |= read_flags;
> +		if (referent && read_flags & REF_ISSYMREF && sb_refname.len > 0)
> +			*referent = sb_refname.buf;

Is this safe?  After this assignment, which "return" in this loop
are you expecting to return from this function?  If you fail to
return from the function during this iteration, you'll clobber the
same strbuf with the next refs_read_raw_ref(), but I do not see how
you are ensuring that you'll return from the function without such
corruption happening.

This assignment happens only when read_flags has REF_ISSYMREF set,
so the next "if it is not, then return refname" would not even
trigger.  If RESOLVE_REF_NO_RECURSE bit is on in resolve_flags,
then we'd return without further dereferencing, but if that is the
only safe exit from the function, shouldn't you be guarding the
function with something like

	if (referent && !(resolve_flags & RESOLVE_REF_NO_RECURSE))
		BUG("recursive dereference can will clobber *referent");

to protect its callers from their own mistakes?

Another return before we start the next iteration of the loop and
clobber sb_refname with another call to refs_read_raw_ref() is the
error return codepath at the end of the loop, but that is a totally
uninteresting case.

Or am I totally confused?