On Fri, Jan 10, 2025 at 6:21 AM Christian Couder
<christian.couder@xxxxxxxxx> wrote:
>
> On Wed, Jan 8, 2025 at 7:39 PM Eric Ju <eric.peijian@xxxxxxxxx> 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`/
>
Thank you. Fixed in v10.
> > 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/
>
Thank you. Fixed in v10.
> [...]
>
> > 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/
>
Thank you. Fixed in v10.
> > +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/
>
Thank you. Fixed in v10. I will use "Raise an error when..."
> > + Error when no object references are provided.
>
> Here also "Error out when..." or "Raise an error when..."
>
Thank you. Fixed in v10.
> > + 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/
>
Thank you. Fixed in v10.
> > 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/
>
Thank you. Replaced with "raise an error".
> > +and exit when they are in the format string.
>
> s/are/appear/
>
Thank you. Fixed in v10.
> > @@ -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.
>
Thank you. It is used at all and removed in v10.
> > };
>
> > @@ -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./
>
Thank you. Fixed in v10.
> > + */
> > + 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.
Thank you. Fixed in v10.
