Re: [PATCH v3 3/3] cat-file: add --batch-command mode
From: Junio C Hamano <hidden>
Date: 2022-02-08 23:59:20
"John Cai via GitGitGadget" [off-list ref] writes:
quoted hunk
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".
quoted hunk
@@ -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[].