Re: [PATCH v3 3/3] cat-file: add --batch-command mode

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

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 1c673385868..ec266ff95e9 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -17,15 +17,16 @@
>  #include "object-store.h"
>  #include "promisor-remote.h"
>  
> -enum batch_command {
> -	BATCH_COMMAND_CONTENTS,
> -	BATCH_COMMAND_INFO,
> +enum batch_mode {
> +	BATCH_MODE_CONTENTS,
> +	BATCH_MODE_INFO,

Would have been better to introduce batch_mode at [2/3] insteads of
having to rename it like this, I guess.

> +	BATCH_MODE_PARSE_CMDS,

What the new mode does looks more like queue-and-dispatch, as
opposed to a mode that can only do "info" or another mode that can
only do "contents".

> @@ -513,6 +514,117 @@ static int batch_unordered_packed(const struct object_id *oid,
>  				      data);
>  }
>  
> +typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,
> +			       struct strbuf *, struct expand_data *);
> +
> +struct queued_cmd {
> +	parse_cmd_fn_t fn;
> +	const char *line;
> +};
> +
> +static void parse_cmd_contents(struct batch_options *opt,
> +			     const char *line,
> +			     struct strbuf *output,
> +			     struct expand_data *data)
> +{
> +	opt->batch_mode = BATCH_MODE_CONTENTS;
> +	batch_one_object(line, output, opt, data);
> +}
> +
> +static void parse_cmd_info(struct batch_options *opt,
> +			   const char *line,
> +			   struct strbuf *output,
> +			   struct expand_data *data)
> +{
> +	opt->batch_mode = BATCH_MODE_INFO;
> +	batch_one_object(line, output, opt, data);
> +}

OK.

> +static void flush_batch_calls(struct batch_options *opt,
> +		struct strbuf *output,
> +		struct expand_data *data,
> +		struct queued_cmd *cmds,
> +		int nr)
> +{
> +	int i;

Have a blank line here between the decl(s) and the first statement.

> +	for (i = 0; i < nr; i++)
> +		cmds[i].fn(opt, cmds[i].line, output, data);
> +
> +	fflush(stdout);
> +}
> +
> +static const struct parse_cmd {
> +	const char *prefix;
> +	parse_cmd_fn_t fn;
> +	unsigned takes_args;
> +} commands[] = {
> +	{ "contents", parse_cmd_contents, 1},
> +	{ "info", parse_cmd_info, 1},
> +};
> +
> +static void batch_objects_command(struct batch_options *opt,
> +				    struct strbuf *output,
> +				    struct expand_data *data)
> +{
> +	struct strbuf input = STRBUF_INIT;
> +	struct queued_cmd *cmds = NULL;
> +	size_t alloc = 0, nr = 0;
> +
> +	while (!strbuf_getline(&input, stdin)) {
> +		int i;
> +		const struct parse_cmd *cmd = NULL;
> +		const char *p = NULL, *cmd_end;
> +		struct queued_cmd call = {0};
> +
> +		if (!input.len)
> +			die(_("empty command in input"));
> +		if (isspace(*input.buf))
> +			die(_("whitespace before command: '%s'"), input.buf);
> +
> +		if (skip_prefix(input.buf, "flush", &cmd_end)) {
> +			if (!opt->buffer_output)
> +				die(_("flush is only for --buffer mode"));
> +			if (*cmd_end)
> +				die(_("flush takes no arguments"));
> +			if (!nr)
> +				error(_("nothing to flush"));

I am not sure if "you already gave us flush and haven't given a new
command, saying 'flush' in such a state is an error" is a good
interface.  What does it achieve to punish such a caller like this
(as opposed to just iterate the loop zero times)?

> +			flush_batch_calls(opt, output, data, cmds, nr);

This iterated cmds[] array and called .fn() for each.  For a command
in the cmds[] array that takes an argument, each element in cmds[]
has a pointer that holds memory obtained from xstrdup_or_null().

Rewinding the array with "nr = 0" to reuse the slots we have
allocated is good, but the memory pointed at by the .line member of
these elements must be freed when this happens.

Perhaps flush_batch_calls() should do so while iterating, i.e.

	for (i = 0; i < nr; i++) {
		cmds[i].fn(opt, cmds[i].line, output, data);
		free(cmds[i].line);
	}
	fflush(stdout);

or something like that?

A tangent, but naming an array as singular, cmd[], would work more
natural when the code often works on individual elements of the
array, i.e. work_on(cmd[4]) would name the "4th cmd", which would
not work well if the array were named cmds[].