Re: [PATCH v4 6/8] transport: add object-info fallback to fetch
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- May 3, 2022, 23:27 UTC
- Message-ID
- <20220503232701.4173948-1-jonathantanmy@google.com>
- In-Reply-To
- <20220502170904.2770649-7-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
> If a v2 server does not support object-info or if the client requests > information that isn't supported by object-info, fetch the objects as if > it were a standard v2 fetch (however without changing any refs).
What do you mean by "however without changing any refs"? (I would have expected that no refs would be changed, so this, to me, implies that we would expect some refs to be changed.)
Show 23 quoted lines
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 506ca28e05..938d082b80 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1639,6 +1639,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> if (args->depth > 0 || args->deepen_since || args->deepen_not)
> args->deepen = 1;
>
> + if (args->object_info)
> + state = FETCH_SEND_REQUEST;
> +
> while (state != FETCH_DONE) {
> switch (state) {
> case FETCH_CHECK_LOCAL:
> @@ -1648,7 +1651,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> /* Filter 'ref' by 'sought' and those that aren't local */
> mark_complete_and_common_ref(negotiator, args, &ref);
> filter_refs(args, &ref, sought, nr_sought);
> - if (!args->refetch && everything_local(args, &ref))
> + if (!args->refetch && !args->object_info && everything_local(args, &ref))
> state = FETCH_DONE;
> else
> state = FETCH_SEND_REQUEST;I think that the purpose of all these changes is to skip certain steps when we know that we're fetching as a fallback for object-info, but I don't think they're necessary - it's fine to not fetch certain objects if we have them present.
Show 13 quoted lines
> @@ -2035,6 +2038,15 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
> args->connectivity_checked = 1;
> }
>
> + if (args->object_info) {
> + struct ref *ref_cpy_reader = ref_cpy;
> + int i = 0;
> + while (ref_cpy_reader) {
> + oid_object_info_extended(the_repository, &ref_cpy_reader->old_oid, &(*args->object_info_data)[i], OBJECT_INFO_LOOKUP_REPLACE);
> + ref_cpy_reader = ref_cpy_reader->next;
> + i++;
> + }
> + }Could this part be done in the same function that falls back (fetch_refs_via_pack(), I believe)? That would make the code easier to understand - here, we don't know why we would need to copy the OIDs, but in the function that falls back, we can look up and see that this is done because we couldn't do something else.
Show 14 quoted lines
> diff --git a/transport.c b/transport.c
> index 08c505e1d0..b85e52d9a8 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -436,10 +436,12 @@ static int fetch_refs_via_pack(struct transport *transport,
> int nr_heads, struct ref **to_fetch)
> {
> int ret = 0;
> + size_t i;
> struct git_transport_data *data = transport->data;
> struct ref *refs = NULL;
> struct fetch_pack_args args;
> struct ref *refs_tmp = NULL;
> + struct ref *wanted_refs = xcalloc(1, sizeof (struct ref));If you only need these new variables in a block, declare them there (and free them at the end of that block).
Show 9 quoted lines
> @@ -489,7 +500,7 @@ static int fetch_refs_via_pack(struct transport *transport,
> else if (data->version <= protocol_v1)
> die_if_server_options(transport);
>
> - if (data->options.acked_commits) {
> + if (data->options.acked_commits && !transport->smart_options->object_info) {
> if (data->version < protocol_v2) {
> warning(_("--negotiate-only requires protocol v2"));
> ret = -1;Is this addition necessary? --negotiate-only and object-info do not work together.