On Mon, Feb 24, 2025 at 3:46 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Eric Ju <eric.peijian@xxxxxxxxx> 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.
