Re: [PATCH v6 4/4] cat-file: add --batch-command mode
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Feb 15, 2022, 19:39 UTC
- Message-ID
- <CAPig+cTwLhn1GZ_=6s0FXL0z=Q=p1w9ZGK0hAV8wfK9RsQYjnA@mail.gmail.com>
- In-Reply-To
- <a6dd5d72fce8cec430c0ec5da2dd9f0969a7fd91.1644862989.git.gitgitgadget@gmail.com>
On Mon, Feb 14, 2022 at 1:23 PM John Cai via GitGitGadget <gitgitgadget@gmail.com> wrote:
> Add a new flag --batch-command that accepts commands and arguments > from stdin, similar to git-update-ref --stdin.
Some relatively minor comments below. Not sure any of them are serious enough to warrant a reroll...
Show 5 quoted lines
> The contents command takes an <object> argument and prints out the object > contents. > > The info command takes a <object> argument and prints out the object > metadata.
s/a <object>/an <object>/
Show 11 quoted lines
> Signed-off-by: John Cai <johncai86@gmail.com> > --- > diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt > @@ -96,6 +96,33 @@ OPTIONS > +contents `<object>`:: > + Print object contents for object reference `<object>`. This corresponds to > + the output of `--batch`. > + > +info `<object>`:: > + Print object info for object reference `<object>`. This corresponds to the > + output of `--batch-check`.
Sorry if I wasn't clear in my earlier review, but when I suggested s/<object>/`<object>`/, I was referring only to the body of each item, not to the item itself (for which we do not -- I think -- ever use `<...>`). So:
content <object>::
Print object contents ... `<object>`. ...As mentioned in my earlier review, I think the SYNOPSIS also needs an update to mention --batch-command.
Show 11 quoted lines
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> @@ -513,6 +514,129 @@ static int batch_unordered_packed(const struct object_id *oid,
> +static void dispatch_calls(struct batch_options *opt,
> + struct strbuf *output,
> + struct expand_data *data,
> + struct queued_cmd *cmd,
> + int nr)
> +{
> + int i;
> +
> + for (i = 0; i < nr; i++){Style: for (...) {Show 6 quoted lines
> + cmd[i].fn(opt, cmd[i].line, output, data); > + free(cmd[i].line); > + } > + > + fflush(stdout); > +}
If I recall correctly, Junio suggested calling free() within this loop, but this feels like an incorrect separation of concerns since it doesn't also reset the caller's `nr` to 0. If (for some reason), dispatch_calls() is invoked twice in a row without resetting `nr` to 0 in between the calls, then the dispatched commands will be called with a pointer to freed memory.
One somewhat ugly way to fix this potential problem would be for this function to clear `nr`:
static void dispatch_calls(..., int *nr)
{
for (...) {
cmd[i].fn(...);
free(cmd[i].line);
}
*nr = 0;
flush(stdout);
}But, as this is a private helper, the code as presented in the patch may be "good enough" even though it's a bit fragile.
Show 15 quoted lines
> +static void batch_objects_command(struct batch_options *opt,
> + struct strbuf *output,
> + struct expand_data *data)
> +{
> + while (!strbuf_getline(&input, stdin)) {
> + 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"));I didn't articulate it in my previous review since the thought was only half-formed, but given "flushify", this will incorrectly complain that "flush takes no arguments" instead of complaining "unknown command: flushify" as is done below (or it will incorrectly complain "flush is only for --buffer mode" if --buffer wasn't specified).
If I'm reading the code correctly, it seems as if these problems could be avoided by treating `flush` as just another parse_cmd::commands[] item so that it gets all the same parsing/checking as the other commands rather than parsing it separately here.
Show 20 quoted lines
> + dispatch_calls(opt, output, data, queued_cmd, nr);
> + nr = 0;
> + continue;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(commands); i++) {
> + if (!skip_prefix(input.buf, commands[i].prefix, &cmd_end))
> + continue;
> +
> + cmd = &commands[i];
> + if (cmd->takes_args) {
> + if (*cmd_end != ' ')
> + die(_("%s requires arguments"),
> + commands[i].prefix);
> +
> + p = cmd_end + 1;
> + } else if (*cmd_end) {
> + die(_("%s takes no arguments"),
> + commands[i].prefix);
> + }Good. Appears to be correctly handling the full matrix of command-requires-arguments and the actual input having or not having arguments.
Show 5 quoted lines
> + break;
> + }
> +
> + if (!cmd)
> + die(_("unknown command: '%s'"), input.buf);If you treat `flush` as just another parse_cmd::commands[], then right here is where you would handle it (I think):
if (strcmp(cmd->prefix, "flush")) {
dispatch_calls(opt, output, data, queued_cmd, nr);
nr = 0;
continue;
}Show 19 quoted lines
> + if (!opt->buffer_output) {
> + cmd->fn(opt, p, output, data);
> + continue;
> + }
> +
> + ALLOC_GROW(queued_cmd, nr + 1, alloc);
> + call.fn = cmd->fn;
> + call.line = xstrdup_or_null(p);
> + queued_cmd[nr++] = call;
> + }
> +
> + if (opt->buffer_output &&
> + nr &&
> + !git_env_bool("GIT_TEST_CAT_FILE_NO_FLUSH_ON_EXIT", 0))
> + dispatch_calls(opt, output, data, queued_cmd, nr);
> +
> + free(queued_cmd);
> + strbuf_release(&input);
> +}