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

[Junio C Hamano <gitster@pobox.com>]

Pablo Sabater <pabloosabaterr@gmail.com> writes:

> 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

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

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

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

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

> +	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 &&