On 06/14, Jonathan Tan wrote:
> > @@ -1122,6 +1124,7 @@ static int do_fetch(struct transport *transport,
> > int autotags = (transport->remote->fetch_tags == 1);
> > int retcode = 0;
> > const struct ref *remote_refs;
> > + struct ref *new_remote_refs = NULL;
>
> Above, you use the name "updated_remote_refs" - it's probably better to
> standardize on one. I think "updated" is better.
Good catch I'll update the variable name.
>
> (The transport calling it "fetched_refs" is fine, because that's what
> they are from the perspective of the transport. From the perspective of
> fetch-pack, it is indeed a new or updated set of remote refs.)
>
> > - if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
> > {
> > +
> > + if (fetch_refs(transport, ref_map, &new_remote_refs)) {
> > + free_refs(ref_map);
> > + retcode = 1;
> > + goto cleanup;
> > + }
> > + if (new_remote_refs) {
> > + free_refs(ref_map);
> > + ref_map = get_ref_map(transport->remote, new_remote_refs, rs,
> > + tags, &autotags);
> > + free_refs(new_remote_refs);
> > + }
> > + if (consume_refs(transport, ref_map)) {
> > free_refs(ref_map);
> > retcode = 1;
> > goto cleanup;
>
> Here, if we got updated remote refs, we need to regenerate ref_map,
> since it is the source of truth.
>
> Maybe add a comment in the "if (new_remote_refs)" block explaining this
> - something like: Regenerate ref_map using the updated remote refs,
> because the transport would place shallow (and other) information
> there.
That's probably a good idea to give future readers more context into why
this is happening.
>
> > - for (i = 0; i < nr_sought; i++)
> > + for (r = refs; r; r = r->next, i++)
> > if (status[i])
> > - sought[i]->status = REF_STATUS_REJECT_SHALLOW;
> > + r->status = REF_STATUS_REJECT_SHALLOW;
>
> You use i here without initializing it to 0. t5703 also fails with this
> patch - probably related to this, but I didn't check.
Oh yeah that's definitely a bug, thanks for catching that.
>
> If you initialize i here, I don't think you need to initialize it to 0
> at the top of this function.
--
Brandon Williams
