Re: [PATCH GSoC v20 00/13] cat-file: add remote-object-info to batch-command
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jul 21, 2026, 20:12 UTC
- Message-ID
- <CAOLa=ZT77L39-jSZxgrvVFwdY6_rapBmKv8gCxr7QpdkCsEzXg@mail.gmail.com>
- In-Reply-To
- <20260718-ps-eric-work-rebase-v20-0-0c13962ac532@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
[snip]
Show 7 quoted lines
> 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]
Show 53 quoted lines
> 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?
Show 47 quoted lines
> `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.)
Show 19 quoted lines
> 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: 44de1520f08d1dfebc3ab2d9f644208eaa5ac925I reviewed v18 last, but the diff here looks good to me. Thanks!