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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 16, 2022, 03:01 UTC
Message-ID: <CAPig+cQUwErXaZR-xZ8u9iz4kpEkE4G5QaxNRCuUx_Xt0rQ_-g@mail.gmail.com>
In-Reply-To: <BE7471F8-0141-49ED-A34B-04D69BE18D96@gmail.com>

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

Show 11 quoted lines

> On 15 Feb 2022, at 20:28, Junio C Hamano wrote:
> > and make the caller do
> >
> >     dispatch_calls(opt, ..., nr);
> >     nr = 0;
> >
> > instead of passing a pointer to nr like the posted patch?
>
> Yeah, this is what I had before but there was discussion about separation of concerns in [1]. But perhaps it's preferable compared to passing a pointer to nr.
>
> 1. https://lore.kernel.org/git/CAPig+cTwLhn1GZ_=6s0FXL0z=Q=p1w9ZGK0hAV8wfK9RsQYjnA@mail.gmail.com/

My biggest concern when mentioning it during review was that if a caller forgets to do `nr = 0`, then a sequence such as:

    dispatch_calls(...);
    ...
    dispatch_calls(...);

will send dangling pointers to the command handlers in the second dispatch_calls() invocation because the first call to dispatch_calls() did free(cmd[i].line). In that sense, it's an accident waiting to happen if people modifying this code in the future aren't paying close attention.

That said, I don't feel too strongly about it and mentioned in my review that it might be "good enough" as-is (with the caller having to remember to `nr = 0`) since it's a local helper function.