From: Pablo Sabater Date: Sat, 27 Jun 2026 20:51:51 GMT Subject: Re: [PATCH GSoC v14 11/13] cat-file: add remote-object-info to batch-command Message-ID: In-Reply-To: El sáb, 27 jun 2026 a las 15:14, Karthik Nayak () escribió: > > Pablo Sabater writes: > > [snip] > > > diff --git a/Documentation/git-cat-file.adoc b/Documentation/git-cat-file.adoc > > index 86b9181599..aba20eb770 100644 > > --- a/Documentation/git-cat-file.adoc > > +++ b/Documentation/git-cat-file.adoc > > @@ -169,6 +169,13 @@ info :: > > Print object info for object reference ``. This corresponds to the > > output of `--batch-check`. > > > > +remote-object-info ...:: > > + Print object info for object references `` at specified > > + `` without downloading objects from the remote. > > + Raise an error when the `object-info` capability is not supported by the remote. > > + Raise an error when no object references are provided. > > + This command may be combined with `--buffer`. > > + > > flush:: > > Used with `--buffer` to execute all preceding commands that were issued > > since the beginning or since the last flush was issued. When `--buffer` > > @@ -312,7 +319,8 @@ newline. The available atoms are: > > The full hex representation of the object name. > > > > `objecttype`:: > > - The type of the object (the same as `cat-file -t` reports). > > + The type of the object (the same as `cat-file -t` reports). See > > + `CAVEATS` below. Not supported by `remote-object-info`. > > > > Do we have to keep adding 'Not supported by `remote-object-info`' to > each type? Can't we do the inverse and only add 'Supported by > `remote-object-info`' to `objectsize`. This avoid having to add this > line to every new type. Yes, I will do that. > > > If no format is specified, the default format is `%(objectname) > > -%(objecttype) %(objectsize)`. > > +%(objecttype) %(objectsize)`, except for `remote-object-info` commands which use > > +`%(objectname) %(objectsize)` for now because "%(objecttype)" is not supported yet. > > Nit: I would drop the 'for now' here, since we don't know when the changes > for 'objecttype' will land. Okay, I'll drop it. > > [snip] > > > enum batch_mode { > > BATCH_MODE_CONTENTS, > > @@ -633,6 +649,81 @@ 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, > > + struct object_info **remote_object_info, > > + struct oid_array *object_info_oids) > > +{ > > + int retval = 0; > > + struct remote *remote = NULL; > > + struct object_id oid; > > + struct string_list object_info_options = STRING_LIST_INIT_NODUP; > > + struct transport *gtransport; > > + > > + /* > > + * Change the format to "%(objectname) %(objectsize)" when > > Nit: perhaps prepend a "TODO" I'll add it. > > > + * remote-object-info command is used. Once we start supporting objecttype > > + * the default format should change to DEFAULT_FORMAT. > > + */ > > + 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++) { > > + if (get_oid_hex(argv[i], &oid)) { > > + size_t len = strlen(argv[i]); > > + > > + if (len < the_hash_algo->hexsz && len >= 4) { > > + size_t j; > > + for (j = 0; j < len; j++) > > + if (!isxdigit(argv[i][j])) > > + break; > > + if (j == len) > > + die(_("remote-object-info does not support " > > + "short oids, %d characters required"), > > + (int)the_hash_algo->hexsz); > > + } > > + 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) { > > + retval = -1; > > + goto cleanup; > > + } > > + > > + 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); > > + > > Aren't we setting the opt->format ourselves in this function? Why do we > need to check it? We only set `opt->format` when the user does not provide a custom format. The `strstr` check catches cases like `%(objecttype)` alone, but is not sufficient for mixed formats like `%(objecttype) %(objectsize)`. This is fixed in [12/13] with the allow-list. > > > + 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); > > + } > > +cleanup: > > + string_list_clear(&object_info_options, 0); > > + transport_disconnect(gtransport); > > + return retval; > > +} > > + > > struct object_cb_data { > > struct batch_options *opt; > > struct expand_data *expand; > > @@ -714,6 +805,57 @@ static void parse_cmd_mailmap(struct batch_options *opt UNUSED, > > load_mailmap(); > > } > > > > +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; > > + struct object_info *remote_object_info = NULL; > > + struct oid_array object_info_oids = OID_ARRAY_INIT; > > + > > + if (strlen(line) >= MAX_REMOTE_OBJ_INFO_LINE) > > + die(_("remote-object-info command too long")); > > + > > + line_to_split = xstrdup(line); > > + count = split_cmdline(line_to_split, &argv); > > + if (count < 0) > > + die(_("split remote-object-info command")); > > We should be using `split_cmdline_strerror()` here Ok, I'll use it. > > > + if (count - 1 > MAX_ALLOWED_OBJ_LIMIT) > > + die(_("remote-object-info supports at most %d objects"), > > + MAX_ALLOWED_OBJ_LIMIT); > > + > > + if (get_remote_info(opt, count, argv, &remote_object_info, > > + &object_info_oids)) > > + goto cleanup; > > + > > + 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) { > > + /* > > + * When reaching here, it means remote-object-info can retrieve > > + * information from server without downloading them. > > + */ > > + data->size = *remote_object_info[i].sizep; > > + opt->batch_mode = BATCH_MODE_INFO; > > + batch_object_write(argv[i + 1], output, opt, data, NULL, 0); > > + } else { > > + report_object_status(opt, oid_to_hex(&data->oid), &data->oid, "missing"); > > + } > > + } > > + 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); > > + oid_array_clear(&object_info_oids); > > +} > > + > > [snip] > > > diff --git a/t/meson.build b/t/meson.build > > index 3219264fe7..54d21111a3 100644 > > --- a/t/meson.build > > +++ b/t/meson.build > > @@ -170,6 +170,7 @@ integration_tests = [ > > 't1014-read-tree-confusing.sh', > > 't1015-read-index-unmerged.sh', > > 't1016-compatObjectFormat.sh', > > + 't1017-cat-file-remote-object-info.sh', > > 't1020-subdirectory.sh', > > 't1022-read-tree-partial-clone.sh', > > 't1050-large.sh', > > [snip] Thanks for the feedback, Pablo.