Re: [PATCH v4 6/8] transport: add object-info fallback to fetch

[Jonathan Tan <jonathantanmy@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.)

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

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

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

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