"John Cai via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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[].
