On Mon, Nov 25, 2024 at 4:51 AM Patrick Steinhardt <ps@xxxxxx> wrote:
>
> On Mon, Nov 25, 2024 at 12:36:16AM -0500, Eric Ju wrote:
> > diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
> > index d5890ae368..6a2f9fd752 100644
> > --- a/Documentation/git-cat-file.txt
> > +++ b/Documentation/git-cat-file.txt
> > @@ -314,7 +323,10 @@ newline. The available atoms are:
> > line) are output in place of the `%(rest)` atom.
> >
> > 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.
> > +WARNING: When "%(objecttype)" is supported, the default format WILL be unified, so
> > +DO NOT RELY on the current the default format to stay the same!!!
>
> Is this stale or do we still not support `%(objecttype)`? I thought we
> wanted to support that, as well, so that we don't have to change the
> default format.
>
Please see my next reply.
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index 5db55fabc4..ad17be69b0 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -576,6 +582,59 @@ 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
> > + */
> > + if (!opt->format)
> > + opt->format = "%(objectname) %(objectsize)";
>
> Seems like it isn't stale. Hum.
>
No, this isn’t stale. As I mentioned in my response to Junio in
https://lore.kernel.org/git/CAN2LT1Cmsw3RB1kbRBvoeLs8WaQeZWqrG96EQfMkMe_jdKaO4g@xxxxxxxxxxxxxx/,
adding type support is planned for the next patch series. Based on the
documentation at https://git-scm.com/docs/protocol-v2#_object_info, it
seems type isn’t yet supported on the server side either. My plan is
to implement the logic for both server and client in the next series.
Unless the reviewers feel strongly that this must be included now, I’d
prefer to stick to the original plan.
> > + 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))
> > + die(_("Not a valid object name %s"), argv[i]);
> > + oid_array_append(&object_info_oids, &oid);
> > + }
>
> Should we return an error when the user didn't pass any object IDs?
>
Thank you. Revising in v8 and also adding a new test case to cover it.
> > @@ -667,6 +726,45 @@ static void parse_cmd_info(struct batch_options *opt,
> > batch_one_object(line, output, opt, data);
> > }
> >
> > +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 = xstrdup_or_null(line);
> > + count = split_cmdline(line_to_split, &argv);
> > + if (get_remote_info(opt, count, argv))
> > + goto cleanup;
> > +
> > + opt->use_remote_info = 1;
> > + data->skip_object_info = 1;
> > + for (size_t i = 0; i < object_info_oids.nr; i++) {
> > +
>
> Nit: empty newline at the start of a block.
>
Thank you. Fixing in v8.
> > diff --git a/t/lib-cat-file.sh b/t/lib-cat-file.sh
> > new file mode 100644
> > index 0000000000..9fb20be308
> > --- /dev/null
> > +++ b/t/lib-cat-file.sh
>
> I think it would make sense to split the introduction of
> "lib-cat-file.sh" into a separate commit.
>
Thank you. Will split it into a separate commit in v8.
> Patrick
