Re: [PATCH v3 3/3] cat-file: add --batch-command mode
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 8, 2022, 23:59 UTC
- Message-ID
- <xmqqwni4ap0t.fsf@gitster.g>
- In-Reply-To
- <1ab5524ee874fc7af65efaca37fb7cd933ea8913.1644353884.git.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 14 quoted lines
> 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".
Show 29 quoted lines
> @@ -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.
Show 7 quoted lines
> +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.
Show 41 quoted lines
> + 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[].