Re: [PATCH GSoC v2 3/6] protocol-caps: add type support to object-info

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 1, 2026, 04:55 UTC
Message-ID: <xmqqecgia181.fsf@gitster.g>
In-Reply-To: <20260731-objecttype-support-v2-3-af577461ed57@gmail.com>

Pablo Sabater <pabloosabaterr@gmail.com> writes:

Show 27 quoted lines

> Teach the server-side object-info handler to accept type as a requested
> field. When the client includes type in its object-info request, the
> server returns the requested object type.
>
> While touching send_info(), wrap an over-long line and fix the bit field
> style of requested_info.size.
>
> Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  protocol-caps.c      | 21 ++++++++++++++++++---
>  t/t5701-git-serve.sh | 27 +++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/protocol-caps.c b/protocol-caps.c
> index 02261be14d..27e0f85b10 100644
> --- a/protocol-caps.c
> +++ b/protocol-caps.c
> @@ -11,7 +11,8 @@
>  #include "strbuf.h"
>  
>  struct requested_info {
> -	unsigned size : 1;
> +	unsigned size:1;
> +	unsigned type:1;
>  };

OK.  This matches this bit in our .clang-format file:

    # Add no space around the bit field
    # unsigned bf:2;
    BitFieldColonSpacing: None

Show 32 quoted lines

> @@ -73,15 +74,20 @@ static void send_info(struct repository *r, struct packet_writer *writer,
>  	if (info->size)
>  		packet_writer_write(writer, "size");
>  
> +	if (info->type)
> +		packet_writer_write(writer, "type");
> +
>  	for_each_string_list_item (item, oid_str_list) {
>  		const char *oid_str = item->string;
> +		enum object_type object_type;
>  		struct object_id oid;
>  		size_t object_size;
>  
>  		if (get_oid_hex_algop(oid_str, &oid, r->hash_algo) < 0) {
>  			packet_writer_error(
>  				writer,
> -				"object-info: protocol error, expected to get oid, not '%s'",
> +				"object-info: protocol error, expected to get "
> +				"oid, not '%s'",
>  				oid_str);
>  			continue;
>  		}
> @@ -93,7 +99,8 @@ static void send_info(struct repository *r, struct packet_writer *writer,
>  		 * If an object is not recognized by the server append SP to
>  		 * the response.
>  		 */
> -		if (get_object_info(r->objects, &oid, &object_size) <= OBJ_NONE) {
> +		object_type = get_object_info(r->objects, &oid, &object_size);
> +		if (object_type <= OBJ_NONE) {
>  			strbuf_addstr(&send_buffer, " ");
>  			goto write;
>  		}

We were already learning the object type as part of the existence check anyway, so we will ...

Show 7 quoted lines

> @@ -103,6 +110,9 @@ static void send_info(struct repository *r, struct packet_writer *writer,
>  				    (uintmax_t)object_size);
>  		}
>  
> +		if (info->type)
> +			strbuf_addf(&send_buffer, " %s", type_name(object_type));
> +

... add it to the payload.

>  write:
>  		packet_writer_write(writer, "%s", send_buffer.buf);
>  		strbuf_reset(&send_buffer);

ANd then the payload is sent in one go.

Show 33 quoted lines

> @@ -124,6 +134,11 @@ int cap_object_info(struct repository *r, struct packet_reader *request)
>  			continue;
>  		}
>  
> +		if (!strcmp("type", request->line)) {
> +			info.type = 1;
> +			continue;
> +		}
> +
>  		if (parse_oid(request->line, &oid_str_list))
>  			continue;
>  
> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
> index b4d6beef11..d7445571b1 100755
> --- a/t/t5701-git-serve.sh
> +++ b/t/t5701-git-serve.sh
> @@ -366,6 +366,33 @@ test_expect_success 'basics of object-info' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'object-info supports type' '
> +	test_config transfer.advertiseObjectInfo true &&
> +
> +	test-tool pkt-line pack >in <<-EOF &&
> +	command=object-info
> +	object-format=$(test_oid algo)
> +	0001
> +	size
> +	type
> +	oid $(git rev-parse two:two.t)
> +	oid $(git rev-parse two:two.t)
> +	0000
> +	EOF

This is not something we can change in the middle of this topic, but the input format looks rather curious. We tell the other side that we are going to ask about size and type but on two separate lines, and then throw each object one by one.

Show 7 quoted lines

> +	cat >expect <<-EOF &&
> +	size
> +	type
> +	$(git rev-parse two:two.t) $(test_file_size two.t) blob
> +	$(git rev-parse two:two.t) $(test_file_size two.t) blob
> +	0000
> +	EOF

And the output format is even more curious. Again, we say size and type on two separate lines, but (object name, size, type) come on a single line. I would probably have designed the "these are the fields" declaration at the beginning to also be on a single line, in both directions. It is not like we are afraid that a line would grow too long. If we were worried that placing these "size" and "type" labels on the same line would make the line too long, we would certainly be showing object name, size, and type on separate lines.

Anyway, the code change looks like what anybody would expect to see in a change to add support of "type" to a codebase that supports "size". As an incremental change, I didn't see anything wrong in it, even though the basic protocol design smelled a bit strange.

Thanks.

Show 7 quoted lines

> +	test-tool serve-v2 --stateless-rpc <in >out &&
> +	test-tool pkt-line unpack <out >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'bare OID request' '
>  	test_config transfer.advertiseObjectInfo true &&