On Mon, Feb 24, 2025 at 3:46 PM Junio C Hamano [off-list ref] wrote:

Eric Ju [off-list ref] writes:

quoted

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.

quoted

+#define MAX_REMOTE_URL_LEN (8*1024)

Here and ...

quoted

+/* Maximum number of objects allowed in a single remote-object-info request. */
+#define MAX_ALLOWED_OBJ_LIMIT 10000

... here, please have a blank line.

quoted

+/* 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.

quoted

@@ -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.

quoted

+     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.

quoted

+             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 ...
        }

quoted

+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.

quoted

+     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.