Re: [PATCH GSoC v14 11/13] cat-file: add remote-object-info to batch-command

>
> Pablo Sabater <pabloosabaterr@gmail.com> 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 <object>::
> >       Print object info for object reference `<object>`. This corresponds to the
> >       output of `--batch-check`.
> >
> > +remote-object-info <remote> <object>...::
> > +     Print object info for object references `<object>` at specified
> > +     `<remote>` 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.

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

>
> [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"

>
> > +      * 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?

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

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