Re: [PATCH GSoC v20 00/13] cat-file: add remote-object-info to batch-command

On Tue Jul 21, 2026 at 10:12 PM CEST, Karthik Nayak wrote:
> Pablo Sabater <[email protected]> writes:
>
> [snip]
>
>> Changes in v20:
>> - Moved the prep patch that fixes hash_algo's type to be before
>>   write_fetch_command_and_capabilities() being moved to 'connect.c'
>> - Reverted git-cat-file.adoc documentation comments about CAVEATS
>> - Fixed style for EXPAND_DATA_INIT
>> - Added more context for comman line die()
>>
>
> [snip]
>
>> Range-diff versus v19:
>>
>>  1:  6df5bcb3bc =  1:  31b02f10b5 transport-helper: fix memory leak of helper on disconnect
>>  2:  141d85a76f =  2:  7d62e0586f cat-file: declare loop counter inside for()
>>  3:  87457a1fe3 =  3:  082d593fb7 t1006: extract helper functions into new 'lib-cat-file.sh'
>>  4:  c26d378931 =  4:  8646b9d7cf fetch-pack: drop the static advertise_sid variable
>>  -:  ---------- >  5:  5531a3c399 fetch-pack: use unsigned int for hash_algo variable
>>  5:  ee16e17228 !  6:  92b4122642 fetch-pack: move write_fetch_command_and_capabilities() to connect.c
>>     @@ connect.c: int server_supports(const char *feature)
>>      +	}
>>      +
>>      +	if (server_feature_v2("object-format", &hash_name)) {
>>     -+		int hash_algo = hash_algo_by_name(hash_name);
>>     ++		const unsigned int hash_algo = hash_algo_by_name(hash_name);
>>      +		if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
>>      +			die(_("mismatched algorithms: client %s; server %s"),
>>      +			    the_hash_algo->name, hash_name);
>>     @@ fetch-pack.c: static int add_haves(struct fetch_negotiator *negotiator,
>>      -	}
>>      -
>>      -	if (server_feature_v2("object-format", &hash_name)) {
>>     --		int hash_algo = hash_algo_by_name(hash_name);
>>     +-		const unsigned int hash_algo = hash_algo_by_name(hash_name);
>>      -		if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
>>      -			die(_("mismatched algorithms: client %s; server %s"),
>>      -			    the_hash_algo->name, hash_name);
>>  6:  bb915f57ad <  -:  ---------- connect: use unsigned int for hash_algo_by_name() calls
>>  7:  b3b06b0cae =  7:  b954a5994a connect: make write_fetch_command_and_capabilities() more generic
>>  8:  79a87c7011 =  8:  627242a7a1 fetch-pack: move fetch initialization
>>  9:  37c3f93975 =  9:  0578594733 protocol-caps: check object existence regardless of the attributes requested
>> 10:  c5062ecaf2 = 10:  816bfa9162 serve: advertise object-info feature
>> 11:  22d72168bc = 11:  2323f45cb2 transport: add client support for object-info
>> 12:  2cf3b24a35 ! 12:  a39975766b cat-file: add remote-object-info to batch-command
>>     @@ Documentation/git-cat-file.adoc: one per line, and print information based on th
>>       You can specify the information shown for each object by using a custom
>>       `<format>`. The `<format>` is copied literally to stdout for each
>>      @@ Documentation/git-cat-file.adoc: newline. The available atoms are:
>>     - 	reports).
>>     -
>>     - `objectsize:disk`::
>>     --	The size, in bytes, that the object takes up on disk. See the
>>     --	note about on-disk sizes in the `CAVEATS` section below.
>>     -+	The size, in bytes, that the object takes up on disk.
>>     -
>>       `deltabase`::
>>       	If the object is stored as a delta on-disk, this expands to the
>>       	full hex representation of the delta base object name.
>>      -	Otherwise, expands to the null OID (all zeroes). See `CAVEATS`
>>     --	below.
>>     -+	Otherwise, expands to the null OID (all zeroes).
>>     ++	Otherwise, expands to the null OID (all zeroes). See `CAVEATS` section
>>     + 	below.
>>
>
> This chagne is still unnecessary, no?

I guess it is unnecessary. I added it because on top, at objectsize:disk
it says "`CAVEATS` section". So I tried to have it consistently.

Is it worth a reroll?

>
>>       `rest`::
>>     - 	If this atom is used in the output string, input lines are split
>>      @@ Documentation/git-cat-file.adoc: newline. The available atoms are:
>>       	after that first run of whitespace (i.e., the "rest" of the
>>       	line) are output in place of the `%(rest)` atom.
>>     @@ builtin/cat-file.c: struct expand_data {
>>      +	 */
>>      +	unsigned is_remote:1;
>>      +};
>>     ++
>>      +#define EXPAND_DATA_INIT  { .mode = S_IFINVALID, .type = OBJ_BAD }
>>      +
>>      +static const char *remote_object_info_atoms[] = {
>>     @@ builtin/cat-file.c: static void parse_cmd_mailmap(struct batch_options *opt UNUS
>>      +	line_to_split = xstrdup(line);
>>      +	count = split_cmdline(line_to_split, &argv);
>>      +	if (count < 0)
>>     -+		die(_("remote-object-info: %s"), split_cmdline_strerror(count));
>>     ++		die(_("remote-object-info: failed to parse command line: %s"),
>>     ++		    split_cmdline_strerror(count));
>>      +	if (count - 1 > MAX_ALLOWED_OBJ_LIMIT)
>>      +		die(_("remote-object-info supports at most %d objects"),
>>      +		    MAX_ALLOWED_OBJ_LIMIT);
>> 13:  ebdfc22fb7 ! 13:  70a11d2aea cat-file: make remote-object-info allow-list adapt to the server
>>     @@ builtin/cat-file.c: struct expand_data {
>>       	 */
>>       	unsigned is_remote:1;
>>      -};
>>     +-
>>      -#define EXPAND_DATA_INIT  { .mode = S_IFINVALID, .type = OBJ_BAD }
>>
>>      -static const char *remote_object_info_atoms[] = {
>>      -	"objectname",
>>      -	"objectsize",
>>     ++	/*
>>     ++	 * List of atoms (i.e. "objectsize") that the server supports. Built
>>     ++	 * from the server's object-info advertised capabilities.
>>     ++	 */
>>      +	struct string_list remote_allowed_atoms;
>>       };
>>     -+#define EXPAND_DATA_INIT  { .mode = S_IFINVALID, .type = OBJ_BAD, \
>>     -+			    .remote_allowed_atoms = STRING_LIST_INIT_NODUP }
>>
>>     ++#define EXPAND_DATA_INIT  { .mode = S_IFINVALID, \
>>     ++			    .type = OBJ_BAD, \
>>     ++			    .remote_allowed_atoms = STRING_LIST_INIT_NODUP }
>>     ++
>
> I guess this is output of running the style check script (which I did
> recommend, but I should've mentioned that you should take it with a
> pinch of salt, it doesn't always provide the best solutions.)

It comes in part from Junio's suggestion [1]. Although I don't think that
it will receive a lot of new fields, I thought that it would leave the
macro clean for that.

>
>>       static int is_atom(const char *atom, const char *s, int slen)
>>       {
>>     + 	int alen = strlen(atom);
>>      @@ builtin/cat-file.c: static int expand_atom(struct strbuf *sb, const char *atom, int len,
>>       		       struct expand_data *data)
>>       {
>>     @@ builtin/cat-file.c: static int expand_atom(struct strbuf *sb, const char *atom,
>>      -			if (is_atom(remote_object_info_atoms[i], atom, len))
>>      +		size_t i;
>>      +		for (i = 0; i < data->remote_allowed_atoms.nr; i++)
>>     -+			if (is_atom(data->remote_allowed_atoms.items[i].string, atom, len))
>>     ++			if (is_atom(data->remote_allowed_atoms.items[i].string,
>>     ++				    atom, len))
>>       				break;
>>      -
>>      -		/*
>>
>> ---
>> base-commit: 44de1520f08d1dfebc3ab2d9f644208eaa5ac925
>
> I reviewed v18 last, but the diff here looks good to me. Thanks!

[1]: https://lore.kernel.org/git/[email protected]/

Thanks,
Pablo