Re: [PATCH v2 1/3] refs: keep track of unresolved reference value in iterator
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 8, 2024, 23:02 UTC
- Message-ID
- <xmqq34rv791a.fsf@gitster.g>
- In-Reply-To
- <6adc9dd26da4459d246591ce148c960b33bde336.1712597893.git.gitgitgadget@gmail.com>
Show 16 quoted lines
> 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.
Show 14 quoted lines
> @@ -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?