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

[Junio C Hamano <gitster@pobox.com>]

Eric Ju <eric.peijian@gmail.com> writes:

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 69ea642dc6..47fd2a777b 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -27,6 +27,18 @@
>  #include "promisor-remote.h"
>  #include "mailmap.h"
>  #include "write-or-die.h"
> +#include "alias.h"
> +#include "remote.h"
> +#include "transport.h"
> +
> +/* Maximum length for a remote URL. While no universal standard exists,
> + * 8K is assumed to be a reasonable limit.
> + */

Style.  Our multi-line comment begins with slash-asterisk and ends
with asterisk-slash both on their own line without anything else.

> +#define MAX_REMOTE_URL_LEN (8*1024)

Here and ...

> +/* Maximum number of objects allowed in a single remote-object-info request. */
> +#define MAX_ALLOWED_OBJ_LIMIT 10000

... here, please have a blank line.

> +/* Maximum input size permitted for the remote-object-info command. */
> +#define MAX_REMOTE_OBJ_INFO_LINE (MAX_REMOTE_URL_LEN + MAX_ALLOWED_OBJ_LIMIT * (GIT_MAX_HEXSZ + 1))

This is an overly long line.

> @@ -579,6 +593,61 @@ 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.
> +	*/

Style.  Closing asterisk-slash aligns with the asterisk on the
previous line.

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

Pointless mixing of "size_t" and "int".  We have declared "int
argc", which is perfectly a sensible type, since we know that the
value of it would not exceed MAX_ALLOWED_OBJ_LIMIT, which is 10000.

> +		if (get_oid_hex(argv[i], &oid))
> +			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) {
> +		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);
> +
> +		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);
> +		}
> +	} else {
> +		retval = -1;
> +	}

Minor style nit, but when everything else is equal, writing the side
of smaller body first would make it easier to follow if/else, i.e.


	gtransport = transport_get(remote, NULL);
	if (!gtransport->smart_options) {
        	/* error */
		retval = -1;
	} else {
		... a lot of real code here ...
	}

> +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;
> +
> +	if (strlen(line) >= MAX_REMOTE_OBJ_INFO_LINE)
> +		die(_("remote-object-info command input overflow "
> +			"(no more than %d objects are allowed)"),
> +			MAX_ALLOWED_OBJ_LIMIT);

Nobody guarantees this user gave a request for more than 10000
objects; after all it may have been an overly long URL that busted
the line length limit, no?

> +	line_to_split = xstrdup(line);
> +	count = split_cmdline(line_to_split, &argv);
> +	if (count < 0)
> +		die(_("split remote-object-info command"));

Here, the code could check if count busts MAX_ALLOWED_OBJ_LIMIT, but
it doesn't.