Re: [PATCH v11 8/8] cat-file: add remote-object-info to batch-command
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 24, 2025, 20:46 UTC
- Message-ID
- <xmqqo6yr3wc4.fsf@gitster.g>
- In-Reply-To
- <20250221190451.12536-9-eric.peijian@gmail.com>
Eric Ju <eric.peijian@gmail.com> writes:
Show 15 quoted lines
> 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.
> +#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.
Show 17 quoted lines
> @@ -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.
Show 9 quoted lines
> + 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.
Show 27 quoted lines
> + 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 ...
}Show 12 quoted lines
> +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?
> + 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.