Re: [PATCH v7 6/6] cat-file: add remote-object-info to batch-command

[Patrick Steinhardt <ps@pks.im>]

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.

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

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

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

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

Patrick