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

On Wed Jul 22, 2026 at 6:13 PM CEST, Karthik Nayak wrote:
> "Pablo Sabater" <[email protected]> writes:
>
>> 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:

[snip]

>>>>
>>>>     @@ 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?
>>
>
> No I don't think a re-roll is necessary. But something to avoid next
> time :)
>

[snip]

>>>
>>>> 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.
>>
>
> Then wouldn't something like this be better?
>
>      #define EXPAND_DATA_INIT  {  \
>              .mode = S_IFINVALID, \
>              .type = OBJ_BAD,     \
>              .remote_allowed_atoms = STRING_LIST_INIT_NODUP, \
>     }
>
> Note the difference:
> 1. The first line no longer has the '.mode' field, this would ensure
> that removal of the field is a diff which only contains the field and
> not movement of other fields to the top line.
> 2. Similar for the last time.
> 3. We also add a comma for the last field, this allows addition of new
> fields without modifying previous lines by adding a missing comma.
>
> I don't think this too warrants a re-roll, but this is something to
> definitely fix incase you do re-roll for other reasons :)

Yeah, this one is better formatted than what I did. Thanks for
the explanation. I'll definetely fix this two hunks if I have to reroll.

[snip]

Thanks,
Pablo