Re: [PATCH v6 4/4] cat-file: add --batch-command mode

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 15, 2022, 23:20 UTC
Message-ID: <CAPig+cR2fymDzEupNW3XvQDBmsd4rtQDQq-2ha6ODzbqoeFQ9A@mail.gmail.com>
In-Reply-To: <1FF9283F-C823-4D26-A39B-6506B900CEB2@gmail.com>

On Tue, Feb 15, 2022 at 5:58 PM John Cai <johncai86@gmail.com> wrote:

Show 39 quoted lines

> On 15 Feb 2022, at 14:39, Eric Sunshine wrote:
> > On Mon, Feb 14, 2022 at 1:23 PM John Cai via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> +static void dispatch_calls(struct batch_options *opt,
> >> +               int nr)
> >> +{
> >> +       for (i = 0; i < nr; i++){
> >> +               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.
>
> What you suggested makes sense from a separation of concerns point of view. I'm
> still learning what looks ugly in C :), but I think this is easier to read
> overall than what I had before.

Even with my suggestion, it's still rather ugly for a "dispatcher" function to be freeing resources allocated by some other entity and for it to be clearing the other entity's `nr` variable, but at least it's less fragile. It would be less ugly, perhaps, if this function was named dispatch_and_free().

A better way to make it less ugly would probably be to introduce a structure which holds the array of batch_options* and the `nr` and `alloc` variables, and then have a dedicated function for clearing/freeing that structure. However, while such an approach is fine for reusable containers but is probably way overkill for this one-off case.

Show 15 quoted lines

> > 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.
>
> This is a good idea. I like the reduced complexity.
>
> > 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;
> >     }

One other point which would make this suggested code even clearer is to rename the parse_cmd::prefix member to `command` or `name` or `token` or something other than "prefix" since it really _is_ the command name, not a prefix of the command. (You're only treating it as a prefix semantically at parse time.) Thus:

    if (strcmp(cmd->name, "flush")) {
        dispatch_calls(opt, output, data, queued_cmd, nr);
        nr = 0;
        continue;
    }

is easier to understand at a glance than `strcmp(cmd->prefix, "flush"))`.