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

[Peijian Ju <eric.peijian@gmail.com>]

On Mon, Feb 24, 2025 at 3:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Ju <eric.peijian@gmail.com> writes:
>
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index 69ea642dc6..47fd2a777b 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -27,6 +27,18 @@
> >  #include "promisor-remote.h"
> >  #include "mailmap.h"
> >  #include "write-or-die.h"
> > +#include "alias.h"
> > +#include "remote.h"
> > +#include "transport.h"
> > +
> > +/* Maximum length for a remote URL. While no universal standard exists,
> > + * 8K is assumed to be a reasonable limit.
> > + */
>
> Style.  Our multi-line comment begins with slash-asterisk and ends
> with asterisk-slash both on their own line without anything else.
>

Thank you. All the style-related comments will be fixed in the next patch.

> > +#define MAX_REMOTE_URL_LEN (8*1024)
>
> Here and ...
>
> > +/* Maximum number of objects allowed in a single remote-object-info request. */
> > +#define MAX_ALLOWED_OBJ_LIMIT 10000
>
> ... here, please have a blank line.
>
> > +/* Maximum input size permitted for the remote-object-info command. */
> > +#define MAX_REMOTE_OBJ_INFO_LINE (MAX_REMOTE_URL_LEN + MAX_ALLOWED_OBJ_LIMIT * (GIT_MAX_HEXSZ + 1))
>
> This is an overly long line.
>
> > @@ -579,6 +593,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.
> > +     */
>
> Style.  Closing asterisk-slash aligns with the asterisk on the
> previous line.
>
> > +     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++) {
>
> Pointless mixing of "size_t" and "int".  We have declared "int
> argc", which is perfectly a sensible type, since we know that the
> value of it would not exceed MAX_ALLOWED_OBJ_LIMIT, which is 10000.
>

Thank you. Change to "int" instead.

> > +             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)
> > +             die(_("remote-object-info requires objects"));
> > +
> > +     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;
> > +
> > +             /* '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;
> > +     }
>
> Minor style nit, but when everything else is equal, writing the side
> of smaller body first would make it easier to follow if/else, i.e.
>
>
>         gtransport = transport_get(remote, NULL);
>         if (!gtransport->smart_options) {
>                 /* error */
>                 retval = -1;
>         } else {
>                 ... a lot of real code here ...
>         }
>
> > +static void parse_cmd_remote_object_info(struct batch_options *opt,
> > +                                      const char *line, struct strbuf *output,
> > +                                      struct expand_data *data)
> > +{
> > +     int count;
> > +     const char **argv;
> > +     char *line_to_split;
> > +
> > +     if (strlen(line) >= MAX_REMOTE_OBJ_INFO_LINE)
> > +             die(_("remote-object-info command input overflow "
> > +                     "(no more than %d objects are allowed)"),
> > +                     MAX_ALLOWED_OBJ_LIMIT);
>
> Nobody guarantees this user gave a request for more than 10000
> objects; after all it may have been an overly long URL that busted
> the line length limit, no?

Yes, the error message is indeed misleading here.
In the next patch, the behavior will be updated as follows:

1. If strlen(line) >= MAX_REMOTE_OBJ_INFO_LINE, we will die with a clear error:
"remote-object-info input too long".

2. After step 1, when calling get_remote_info(opt, count, argv), we
will check object_info_oids.nr.
If `object_info_oids.nr > MAX_ALLOWED_OBJ_LIMIT`, we will die with:
"no more than %d objects are allowed", MAX_ALLOWED_OBJ_LIMIT.

>
> > +     line_to_split = xstrdup(line);
> > +     count = split_cmdline(line_to_split, &argv);
> > +     if (count < 0)
> > +             die(_("split remote-object-info command"));
>
> Here, the code could check if count busts MAX_ALLOWED_OBJ_LIMIT, but
> it doesn't.

Yes, a check is needed here. Please see the previous reply.