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

On 06/14, Jonathan Tan wrote:

quoted

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

quoted

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

quoted

-		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