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

[John Cai <johncai86@gmail.com>]

On 16 Feb 2022, at 12:25, Eric Sunshine wrote:

> On Wed, Feb 16, 2022 at 12:15 PM Junio C Hamano <gitster@pobox.com> wrote:
>> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> As it seems that this code structure and division of responsibility
>> between the caller and the callee is confusing even to the author of
>> this code, it may make sense to make the caller responsible for
>> freeing.
>>
>> Then the caller becomes
>>
>>> +             if (!strcmp(cmd->name, "flush")) {
>>> +                     dispatch_calls(opt, output, data, queued_cmd, nr);
>>
>>                         for (i = 0; i < nr; i++)
>>                                 free(queued_cmd[i].line);
>>
>>> +                     nr = 0;
>>> +                     continue;
>>> +             }
>>
>> which is not too bad.  And then we'd free the array itself at the
>> end ...
>>
>>> ...
>>> +             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);
>>
>> ... which may be easier to see what is going on.
>
> I agree that it is easier to see what is going on when the caller is
> responsible for freeing `line`. It _may_ make sense to factor out the
> free-line-loop to a separate function since the caller will have to do
> so after both calls to dispatch_calls(), not just the one inside the
> strbuf_getline() loop, but also the one after the loop. A separate
> function might be overkill for this two-line loop; on the other hand,
> it may clue in future readers that resource management needs to be
> taken into account.

Both of these suggestions sound good to me. Thanks for the help down to these details. These are
all valuable learning points!