Re: [PATCH v9 8/8] cat-file: add remote-object-info to batch-command

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 10, 2025, 11:20 UTC
Message-ID: <CAP8UFD0TBYi0CGOfNYH9FtL3odrqaxz0w1q36RK3gS-Z3pJAmw@mail.gmail.com>
In-Reply-To: <20250108183740.67022-9-eric.peijian@gmail.com>

On Wed, Jan 8, 2025 at 7:39 PM Eric Ju <eric.peijian@gmail.com> wrote:

>
> Since the `info` command in cat-file --batch-command prints object info

Nit: Everywhere in this commit message, it would be a bit clearer and
easier to read with:

s/cat-file --batch-command/`cat-file --batch-command`/

> for a given object, it is natural to add another command in cat-file
> --batch-command to print object info for a given object from a remote.
>
> Add `remote-object-info` to cat-file --batch-command.

s/`remote-object-info`/a new `remote-object-info` command/

[...]

> To summarize, `remote-object-info` gets object info from the remote and
> then loop through the object info passed in, printing the info.

s/loop/loops/

> +remote-object-info <remote> <object>...::
> +       Print object info for object references `<object>` at specified
> +       `<remote>` without downloading objects from the remote.
> +       Error when the `object-info` capability is not supported by the server.

I think it's more grammatically correct to use "Error out when..." or "Raise an error when..." than just "Error when..."

Also maybe: s/server/remote/

> +       Error when no object references are provided.

Here also "Error out when..." or "Raise an error when..."

> +       This command may be combined with `--buffer`.

[...]

Show 6 quoted lines

>  If no format is specified, the default format is `%(objectname)
> -%(objecttype) %(objectsize)`.
> +%(objecttype) %(objectsize)`, except for `remote-object-info` commands which use
> +`%(objectname) %(objectsize)` for now because "%(objecttype)" is not supported yet.
> +WARNING: When "%(objecttype)" is supported, the default format WILL be unified, so
> +DO NOT RELY on the current the default format to stay the same!!!

s/current the default/current default/

Show 5 quoted lines

>  CAVEATS
>  -------
>
> +Note that since %(objecttype), %(objectsize:disk) and %(deltabase) are
> +currently not supported by the `remote-object-info` command, we will error

s/error/raise an error/

or maybe:

s/error and exit/error out/

> +and exit when they are in the format string.

s/are/appear/

Show 5 quoted lines

> @@ -45,9 +48,12 @@ struct batch_options {
>         char input_delim;
>         char output_delim;
>         const char *format;
> +       int use_remote_info;

"unsigned int" might be a bit better for bool fields like this.

Actually it seems to me that this field is set to 0 and 1 in some places but we never read it, so I wonder if it's actually useful.

>  };

Show 16 quoted lines

> @@ -579,6 +585,61 @@ static void batch_one_object(const char *obj_name,
>         object_context_release(&ctx);
>  }
>
> +static int get_remote_info(struct batch_options *opt, int argc, const char **argv)
> +{
> +       int retval = 0;
> +       struct remote *remote = NULL;
> +       struct object_id oid;
> +       struct string_list object_info_options = STRING_LIST_INIT_NODUP;
> +       static struct transport *gtransport;
> +
> +       /*
> +        * Change the format to "%(objectname) %(objectsize)" when
> +        * remote-object-info command is used. Once we start supporting objecttype
> +        * the default format should change to DEFAULT_FORMAT

s/DEFAULT_FORMAT/DEFAULT_FORMAT./

Show 17 quoted lines

> +       */
> +       if (!opt->format)
> +               opt->format = "%(objectname) %(objectsize)";
> +
> +       remote = remote_get(argv[0]);
> +       if (!remote)
> +               die(_("must supply valid remote when using remote-object-info"));
> +
> +       oid_array_clear(&object_info_oids);
> +       for (size_t i = 1; i < argc; i++) {
> +               if (get_oid_hex(argv[i], &oid))
> +                       die(_("Not a valid object name %s"), argv[i]);
> +               oid_array_append(&object_info_oids, &oid);
> +       }
> +       if (object_info_oids.nr == 0) {
> +               die(_("remote-object-info requires objects"));
> +       }

We prefer to drop '{' and '}' and use "!X" instead of "X == 0" when possible, so:

       if (!object_info_oids.nr)
               die(_("remote-object-info requires objects"));

Thanks.