Re: [PATCH GSoC RFC v12 12/12] cat-file: make remote-object-info allow-list dynamic

>
> The static allow-list in expand_atom() is hardcoded to only allow
> "objectname" and "objectsize" for remote queries. This works because
> up to this point all servers will either support object-info with name
> and size or they do not support them at all, but we cannot expect that
> in a future different servers with different git versions to have the
> same object-info capabilities. Therefore, the allow_list needs to be
> dynamic depending on what does the server advertise.
>
> The client will now:
>
> 1. Request the protocol option that the placeholder refers to (i.e.
>    "size" when "%(objectsize)").
>
> 2. Filters the request in fetch_object_info() dropping any option that
>    the server does not advertise.
>
> 3. After the fetching, the options that haven't been dropped are the ones
>    fetched and supported by the server, these supported options are
>    mapped and remote_allowed_atoms is populated with the placeholders.
>
> 4. expand_atom() checks remote_allowed_atoms with the same behaviour as
>    the static allow_list had.
>
> Move object_info_options out of get_remote_info so the caller which has
> data can select what options will be requested instead of requesting
> always size.
> Move batch_object_write() out so there will always be an output even if
> all the placeholders are not supported by the server (returns an empty
> line).
>
> Include "type" in the object_info_options so once the server supports
> it, the clients know already how to request it.
>
> Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  builtin/cat-file.c  | 85 ++++++++++++++++++++++++++++++++---------------------
>  fetch-object-info.c |  6 ++++
>  2 files changed, 58 insertions(+), 33 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 1166a046b4..055991b5af 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -341,13 +341,10 @@ struct expand_data {
>          * Flags about when an object info is being fetched from remote.
>          */
>         unsigned is_remote:1;
> -};
> -#define EXPAND_DATA_INIT  { .mode = S_IFINVALID, .type = OBJ_BAD }
>
> -static const char *remote_object_info_atoms[] = {
> -       "objectname",
> -       "objectsize",
> +       struct string_list remote_allowed_atoms;
>  };
> +#define EXPAND_DATA_INIT  { .mode = S_IFINVALID, .type = OBJ_BAD, .remote_allowed_atoms = STRING_LIST_INIT_NODUP }
>
>  static int is_atom(const char *atom, const char *s, int slen)
>  {
> @@ -359,17 +356,11 @@ static int expand_atom(struct strbuf *sb, const char *atom, int len,
>                        struct expand_data *data)
>  {
>         if (data->is_remote) {
> -               size_t i, allowed_nr = ARRAY_SIZE(remote_object_info_atoms);
> -               for (i = 0; i < allowed_nr; i++)
> -                       if (is_atom(remote_object_info_atoms[i], atom, len))
> +               size_t i;
> +               for (i = 0; i < data->remote_allowed_atoms.nr; i++)
> +                       if (is_atom(data->remote_allowed_atoms.items[i].string, atom, len))
>                                 break;
> -
> -               /*
> -                * On remote, skip unsupported atoms returning an empty sb,
> -                * honoring how for-each-ref handles known but inapplicable
> -                * atoms (e.g. %(tagger)).
> -                */
> -               if (i == allowed_nr)
> +               if (i == data->remote_allowed_atoms.nr)
>                         return 1;
>         }
>
> @@ -686,12 +677,12 @@ 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)
> +                          struct oid_array *object_info_oids,
> +                          struct string_list *object_info_options)
>  {
>         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;
>
>         /*
> @@ -726,15 +717,12 @@ static int get_remote_info(struct batch_options *opt,
>         gtransport->smart_options->object_info = 1;
>         gtransport->smart_options->object_info_oids = object_info_oids;
>
> -       string_list_append(&object_info_options, "size");
> -
> -       if (object_info_options.nr > 0) {
> -               gtransport->smart_options->object_info_options = &object_info_options;
> +       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;
>  }
> @@ -820,6 +808,21 @@ static void parse_cmd_mailmap(struct batch_options *opt UNUSED,
>                 load_mailmap();
>  }
>
> +struct protocol_placeholder_entry {
> +       const char *option;
> +       const char *atom;
> +};
> +
> +static const struct protocol_placeholder_entry remote_atom_map[] = {
> +       {"size", "objectsize"},
> +       {"type", "objecttype"},
> +       /*
> +        * Add new protocol options here. Even if the server doesn't support
> +        * them the allow_list will drop them if the server doesn't advertise
> +        * them.
> +        */
> +};
> +
>  static void parse_cmd_remote_object_info(struct batch_options *opt,
>                                          const char *line, struct strbuf *output,
>                                          struct expand_data *data)
> @@ -829,6 +832,7 @@ static void parse_cmd_remote_object_info(struct batch_options *opt,
>         char *line_to_split;
>         static struct object_info *remote_object_info;
>         static struct oid_array object_info_oids = OID_ARRAY_INIT;
> +       struct string_list object_info_options = STRING_LIST_INIT_NODUP;
>
>         if (strlen(line) >= MAX_REMOTE_OBJ_INFO_LINE)
>                 die(_("remote-object-info command too long"));
> @@ -841,30 +845,44 @@ static void parse_cmd_remote_object_info(struct batch_options *opt,
>                 die(_("remote-object-info supports at most %d objects"),
>                     MAX_ALLOWED_OBJ_LIMIT);
>
> +       if (data->info.sizep)
> +               string_list_append(&object_info_options, "size");
> +       if (data->info.typep)
> +               string_list_append(&object_info_options, "type");
> +
>         if (get_remote_info(opt, count, argv, &remote_object_info,
> -                           &object_info_oids))
> +                           &object_info_oids, &object_info_options))
>                 goto cleanup;
>
> +       string_list_clear(&data->remote_allowed_atoms, 0);
> +       string_list_append(&data->remote_allowed_atoms, "objectname");
> +       for (size_t i = 0; i < ARRAY_SIZE(remote_atom_map); i++)
> +               if (unsorted_string_list_has_string(&object_info_options, remote_atom_map[i].option))
> +                       string_list_append(&data->remote_allowed_atoms,
> +                                          remote_atom_map[i].atom);
> +
>         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.
> -                        */
> +               /*
> +                * When reaching here, it means remote-object-info can retrieve
> +                * information from server without downloading them.
> +                */
> +               if (remote_object_info[i].sizep)
>                         data->size = *remote_object_info[i].sizep;
> -                       opt->batch_mode = BATCH_MODE_INFO;
> -                       data->is_remote = 1;
> -                       batch_object_write(argv[i + 1], output, opt, data, NULL, 0);
> -                       data->is_remote = 0;
> -               }
> +               if (remote_object_info[i].typep)
> +                       data->type = *remote_object_info[i].typep;
> +               opt->batch_mode = BATCH_MODE_INFO;
> +               data->is_remote = 1;
> +               batch_object_write(argv[i + 1], output, opt, data, NULL, 0);
> +               data->is_remote = 0;
>         }
>         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]);
> +       string_list_clear(&object_info_options, 0);
>         free(line_to_split);
>         free(argv);
>         free(remote_object_info);
> @@ -1177,6 +1195,7 @@ static int batch_objects(struct batch_options *opt)
>   cleanup:
>         strbuf_release(&input);
>         strbuf_release(&output);
> +       string_list_clear(&data.remote_allowed_atoms, 0);
>         warn_on_object_refname_ambiguity = save_warning;
>         return retval;
>  }
> diff --git a/fetch-object-info.c b/fetch-object-info.c
> index 51a898430d..425929a269 100644
> --- a/fetch-object-info.c
> +++ b/fetch-object-info.c
> @@ -39,6 +39,12 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
>         case protocol_v2:
>                 if (!server_supports_v2("object-info"))
>                         die(_("object-info capability is not enabled on the server"));
> +
> +               for (int i = args->object_info_options->nr - 1; i >= 0; i--)

> +                       if (!server_supports_feature("object-info",
> +                                                    args->object_info_options->items[i].string, 0))
> +                               unsorted_string_list_delete_item(args->object_info_options, i, 0);
> +
>                 send_object_info_request(fd_out, args);
>                 break;
>         case protocol_v1:
>
> --
> 2.54.0