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

>
> On Sat, Jul 20, 2024 at 5:44 AM Eric Ju <eric.peijian@gmail.com> wrote:
>
> > +remote-object-info <remote> <object>...::
> > +       Print object info for object references `<object>` at specified <remote> without
> > +       downloading objects from remote. If the object-info capability is not
> > +       supported by the server, the objects will be downloaded instead.
> > +       Error when no object references is provided.
>
> Maybe s/is provided/are provided/

>
>
> > +       This command may be combined with `--buffer`.
>
> >  `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.
> > +       below. Not supported by `remote-object-info`.
> >
> >  `rest`::
> >         If this atom is used in the output string, input lines are split
> > @@ -314,7 +323,9 @@ newline. The available atoms are:
> >         line) are output in place of the `%(rest)` atom.
> >
> >  If no format is specified, the default format is `%(objectname)
> > -%(objecttype) %(objectsize)`.
> > +%(objecttype) %(objectsize)`, except remote-object-info command who uses
>
> s/except remote-object-info command who uses/except for
> `remote-object-info` commands which use/

>
>
> > +`%(objectname) %(objectsize)` for now because "%(objecttype)" is not supported yet.
> > +When "%(objecttype)" is supported, default format should be unified.
> >
> >  If `--batch` is specified, or if `--batch-command` is used with the `contents`
> >  command, the object information is followed by the object contents (consisting
> > @@ -396,6 +407,10 @@ scripting purposes.
> >  CAVEATS
> >  -------
> >
> > +Note that since objecttype, objectsize:disk and deltabase are currently not supported by the
>
> s/objecttype, objectsize:disk and deltabase/%(objecttype),
> %(objectsize:disk) and %(deltabase)/
>

>
> > +remote-object-info, git will error and exit when they are in the format string.
>
> s//remote-object-info, git /`remote-object-info` command, we/
>

>
> > +
> > +
>
> Maybe a single blank line is enough.

>
>
> >  Note that the sizes of objects on disk are reported accurately, but care
> >  should be taken in drawing conclusions about which refs or objects are
> >  responsible for disk usage. The size of a packed non-delta object may be
>
> [...]
>
> > +       gtransport = transport_get(remote, NULL);
> > +       if (gtransport->smart_options) {
> > +               CALLOC_ARRAY(remote_object_info, object_info_oids.nr);
> > +               gtransport->smart_options->object_info = 1;
> > +               gtransport->smart_options->object_info_oids = &object_info_oids;
> > +               /*
> > +                * 'size' is the only option currently supported.
> > +                * Other options that are passed in the format will exit with error.
> > +                */
> > +               if (strstr(opt->format, "%(objectsize)")) {
> > +                       string_list_append(&object_info_options, "size");
> > +               } else {
> > +                       die(_("%s is currently not supported with remote-object-info"), opt->format);
> > +               }
>
> Something like the following might be a bit shorter and simpler:
>
>   /* 'objectsize' is the only option currently supported */
>   if (!strstr(opt->format, "%(objectsize)"))
>           die(_("%s is currently not supported with
> remote-object-info"), opt->format);
>
>   string_list_append(&object_info_options, "size");

>
>
> > +               if (object_info_options.nr > 0) {
> > +                       gtransport->smart_options->object_info_options = &object_info_options;
> > +                       gtransport->smart_options->object_info_data = remote_object_info;
> > +                       retval = transport_fetch_refs(gtransport, NULL);
> > +               }
> > +       } else {
> > +               retval = -1;
> > +       }
>
> [...]
>
> > +       opt->use_remote_info = 1;
> > +       data->skip_object_info = 1;
> > +       for (size_t i = 0; i < object_info_oids.nr; i++) {
> > +
> > +               data->oid = object_info_oids.oid[i];
> > +
> > +               if (remote_object_info[i].sizep) {
> > +                       data->size = *remote_object_info[i].sizep;
> > +               } else {
> > +                       /*
> > +                        * When reaching here, it means remote-object-info can't retrive
>
> s/retrive/retrieve/
>

>
> > +                        * infomation from server withoug downloading them, and the objects
>
> s/infomation from server withoug/information from server without/
>

>
>
> > +                        */
> > +                       data->skip_object_info = 0;
> > +               }
> > +
> > +               opt->batch_mode = BATCH_MODE_INFO;
> > +               batch_object_write(argv[i+1], output, opt, data, NULL, 0);
> > +
> > +       }
> > +       opt->use_remote_info = 0;
> > +       data->skip_object_info = 0;
> > +
> > +cleanup:
> > +       for (size_t i = 0; i < object_info_oids.nr; i++)
> > +               free_object_info_contents(&remote_object_info[i]);
> > +       free(line_to_split);
> > +       free(argv);
> > +       free(remote_object_info);
> > +}
> > +
> >  static void dispatch_calls(struct batch_options *opt,
> >                 struct strbuf *output,
> >                 struct expand_data *data,
> > @@ -696,9 +803,10 @@ static const struct parse_cmd {
> >         parse_cmd_fn_t fn;
> >         unsigned takes_args;
> >  } commands[] = {
> > -       { "contents", parse_cmd_contents, 1},
> > -       { "info", parse_cmd_info, 1},
> > -       { "flush", NULL, 0},
> > +       { "contents", parse_cmd_contents, 1 },
> > +       { "info", parse_cmd_info, 1 },
> > +       { "remote-object-info", parse_cmd_remote_object_info, 1 },
> > +       { "flush", NULL, 0 },
>
> I am not sure it's a good thing to add a space before "}".
>