On Tue, Feb 15, 2022 at 5:58 PM John Cai <johncai86@xxxxxxxxx> wrote:
> On 15 Feb 2022, at 14:39, Eric Sunshine wrote:
> > On Mon, Feb 14, 2022 at 1:23 PM John Cai via GitGitGadget
> > <gitgitgadget@xxxxxxxxx> 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.
> > 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"))`.
