Re: [PATCH v2 7/8] fetch-pack: put shallow info in output parameter

[Brandon Williams <bmwill@google.com>]

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