On Tue, Feb 15, 2022 at 5:58 PM John Cai [off-list ref] wrote:

On 15 Feb 2022, at 14:39, Eric Sunshine wrote:

quoted

On Mon, Feb 14, 2022 at 1:23 PM John Cai via GitGitGadget
[off-list ref] wrote:

quoted

+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.

quoted

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.

quoted

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"))`.