Re: [PATCH v3 7/8] fetch-pack: put shallow info in output parameter
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Jun 25, 2018, 18:03 UTC
- Message-ID
- <20180625180353.227420-1-jonathantanmy@google.com>
- In-Reply-To
- <20180620213235.10952-8-bmwill@google.com>
> static void update_shallow(struct fetch_pack_args *args, > - struct ref **sought, int nr_sought, > + struct ref *refs,
update_shallow() now takes in a linked list of refs instead of an array. I see that the translation of this function is straightforward - occasionally, we need to iterate through the linked list and count up from 0 at the same time, but that is not a problem.
Show 6 quoted lines
> struct shallow_info *si)
> {
> struct oid_array ref = OID_ARRAY_INIT;
> int *status;
> - int i;
> + int i = 0;Remove the " = 0" - I've verified that it does not need to be there, and it might inhibit useful "unintialized variable" warnings if others were to change the code later.
Optional: I would also remove this declaration and declare "int i;" in each of the blocks that need it.
Show 21 quoted lines
> static int fetch_refs_via_pack(struct transport *transport,
> - int nr_heads, struct ref **to_fetch)
> + int nr_heads, struct ref **to_fetch,
> + struct ref **fetched_refs)
> {
> int ret = 0;
> struct git_transport_data *data = transport->data;
> @@ -354,8 +356,12 @@ static int fetch_refs_via_pack(struct transport *transport,
> if (report_unmatched_refs(to_fetch, nr_heads))
> ret = -1;
>
> + if (fetched_refs)
> + *fetched_refs = refs;
> + else
> + free_refs(refs);
> +
> free_refs(refs_tmp);
> - free_refs(refs);
> free(dest);
> return ret;
> }Instead of just freeing the linked list, we return it if requested by the client. This makes sense.
Show 43 quoted lines
> -int transport_fetch_refs(struct transport *transport, struct ref *refs)
> +int transport_fetch_refs(struct transport *transport, struct ref *refs,
> + struct ref **fetched_refs)
> {
> int rc;
> int nr_heads = 0, nr_alloc = 0, nr_refs = 0;
> struct ref **heads = NULL;
> + struct ref *nop_head = NULL, **nop_tail = &nop_head;
> struct ref *rm;
>
> for (rm = refs; rm; rm = rm->next) {
> nr_refs++;
> if (rm->peer_ref &&
> !is_null_oid(&rm->old_oid) &&
> - !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid))
> + !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid)) {
> + /*
> + * These need to be reported as fetched, but we don't
> + * actually need to fetch them.
> + */
> + if (fetched_refs) {
> + struct ref *nop_ref = copy_ref(rm);
> + *nop_tail = nop_ref;
> + nop_tail = &nop_ref->next;
> + }
> continue;
> + }
> ALLOC_GROW(heads, nr_heads + 1, nr_alloc);
> heads[nr_heads++] = rm;
> }
> @@ -1245,7 +1263,11 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)
> heads[nr_heads++] = rm;
> }
>
> - rc = transport->vtable->fetch(transport, nr_heads, heads);
> + rc = transport->vtable->fetch(transport, nr_heads, heads, fetched_refs);
> + if (fetched_refs && nop_head) {
> + *nop_tail = *fetched_refs;
> + *fetched_refs = nop_head;
> + }
>
> free(heads);
> return rc;And sometimes, even if we are merely simulating the fetching of refs, we still need to report those refs in fetched_refs. This is correct.
I also see that t5703 now passes.
Besides enabling the writing of subsequent patches, I see that this also makes the API clearer in that the input refs to transport_fetch_refs() are not overloaded to output shallow information. Other than the " = 0" change above, this patch looks good to me.