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

>
> 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/
>

> > +       This command may be combined with `--buffer`.
>
> [...]
>
> >  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/
>

> >  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/
>

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

> >  };
>
> > @@ -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./
>

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