Re: [PATCH v4 4/6] transport: add client support for object-info

From: karthik nayak <karthik.188@gmail.com>
Date: Oct 25, 2024, 10:12 UTC
Message-ID: <CAOLa=ZS4XLNBeXewQ8O7tpobQfF9C9LKP--MVYghVR52hcqgWA@mail.gmail.com>
In-Reply-To: <20241024205359.16376-5-eric.peijian@gmail.com>

Eric Ju <eric.peijian@gmail.com> writes:

[snip]

Show 9 quoted lines

> diff --git a/fetch-pack.c b/fetch-pack.c
> index 800505f25f..1a9facc1c0 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1347,7 +1347,6 @@ static void write_command_and_capabilities(struct strbuf *req_buf,
>  	packet_buf_delim(req_buf);
>  }
>
> -

Seems like this was introduced in Patch 1/6, including the function below which is not used in that patch.

Show 13 quoted lines

>  void send_object_info_request(int fd_out, struct object_info_args *args)
>  {
>  	struct strbuf req_buf = STRBUF_INIT;
> @@ -1706,6 +1705,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:

[snip]

Show 10 quoted lines

>  /*
>   * sought represents remote references that should be updated from.
>   * On return, the names that were found on the remote will have been
> @@ -106,4 +114,6 @@ int report_unmatched_refs(struct ref **sought, int nr_sought);
>   */
>  int fetch_pack_fsck_objects(void);
>
> +void send_object_info_request(int fd_out, struct object_info_args *args);
> +
>

Nit: Would be nice to have a comment here explaining what the function does.

Show 12 quoted lines

>  #endif
> diff --git a/transport-helper.c b/transport-helper.c
> index 013ec79dc9..2ff9675984 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -709,8 +709,8 @@ static int fetch_refs(struct transport *transport,
>
>  	/*
>  	 * If we reach here, then the server, the client, and/or the transport
> -	 * helper does not support protocol v2. --negotiate-only requires
> -	 * protocol v2.
> +	 * helper does not support protocol v2. --negotiate-only and cat-file remote-object-info

Nit: could we wrap this comment?

> +	 * require protocol v2.
>  	 */
>  	if (data->transport_options.acked_commits) {
>  		warning(_("--negotiate-only requires protocol v2"));

[snip]

Show 24 quoted lines

>  static struct ref *get_refs_via_connect(struct transport *transport, int for_push,
>  					struct transport_ls_refs_options *options)
>  {
> @@ -418,6 +489,7 @@ static int fetch_refs_via_pack(struct transport *transport,
>  	struct ref *refs = NULL;
>  	struct fetch_pack_args args;
>  	struct ref *refs_tmp = NULL, **to_fetch_dup = NULL;
> +	struct ref *object_info_refs = NULL;
>
>  	memset(&args, 0, sizeof(args));
>  	args.uploadpack = data->options.uploadpack;
> @@ -444,11 +516,36 @@ static int fetch_refs_via_pack(struct transport *transport,
>  	args.server_options = transport->server_options;
>  	args.negotiation_tips = data->options.negotiation_tips;
>  	args.reject_shallow_remote = transport->smart_options->reject_shallow;
> +	args.object_info = transport->smart_options->object_info;
> +
> +	if (transport->smart_options
> +		&& transport->smart_options->object_info
> +		&& transport->smart_options->object_info_oids->nr > 0) {
> +		struct ref *ref_itr = object_info_refs = alloc_ref("");
> +
> +		if (!fetch_object_info(transport, data->options.object_info_data))
> +			goto cleanup;

So if we were successful, we skip to the cleanup. Okay.

> +		args.object_info_data = data->options.object_info_data;
> +		args.quiet = 1;
> +		args.no_progress = 1;

Not sure why we set quiet and no_progress here.

Show 43 quoted lines

> +		for (size_t i = 0; i < transport->smart_options->object_info_oids->nr; i++) {
> +			ref_itr->old_oid = transport->smart_options->object_info_oids->oid[i];
> +			ref_itr->exact_oid = 1;
> +			if (i == transport->smart_options->object_info_oids->nr - 1)
> +				/* last element, no need to allocate to next */
> +				ref_itr->next = NULL;
> +			else
> +				ref_itr->next = alloc_ref("");
>
> -	if (!data->finished_handshake) {
> -		int i;
> +			ref_itr = ref_itr->next;
> +		}
> +
> +		transport->remote_refs = object_info_refs;
> +
> +	} else if (!data->finished_handshake) {
>  		int must_list_refs = 0;
> -		for (i = 0; i < nr_heads; i++) {
> +		for (int i = 0; i < nr_heads; i++) {
>  			if (!to_fetch[i]->exact_oid) {
>  				must_list_refs = 1;
>  				break;
> @@ -494,16 +591,26 @@ static int fetch_refs_via_pack(struct transport *transport,
>  			  &transport->pack_lockfiles, data->version);
>
>  	data->finished_handshake = 0;
> +	if (args.object_info) {
> +		struct ref *ref_cpy_reader = object_info_refs;
> +		for (int i = 0; ref_cpy_reader; i++) {
> +			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;
> +		}
> +	}
> +
>  	data->options.self_contained_and_connected =
>  		args.self_contained_and_connected;
>  	data->options.connectivity_checked = args.connectivity_checked;
>
> -	if (!refs)
> +	if (!refs && !args.object_info)
>  		ret = -1;

This is because, now we don't necessary always fetch the refs, since sometimes we're just happy fetching the object info. Would be nice to have a comment here.

Show 8 quoted lines

>  	if (report_unmatched_refs(to_fetch, nr_heads))
>  		ret = -1;
>
>  cleanup:
> +	free_refs(object_info_refs);
>  	close(data->fd[0]);
>  	if (data->fd[1] >= 0)
>  		close(data->fd[1]);

[snip]