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

From: John Cai <johncai86@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: John Cai via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, me@ttaylorr.com, phillip.wood123@gmail.com,
	avarab@gmail.com, e@80x24.org, bagasdotme@gmail.com,
	Eric Sunshine <sunshine@sunshineco.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	Christian Couder <christian.couder@gmail.com>
Subject: Re: [PATCH v7 4/4] cat-file: add --batch-command mode
Date: Tue, 15 Feb 2022 21:48:31 -0500	[thread overview]
Message-ID: <BE7471F8-0141-49ED-A34B-04D69BE18D96@gmail.com> (raw)
In-Reply-To: <xmqqfsojy50b.fsf@gitster.g>

On 15 Feb 2022, at 20:28, Junio C Hamano wrote:

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +static void dispatch_calls(struct batch_options *opt,
>> +		struct strbuf *output,
>> +		struct expand_data *data,
>> +		struct queued_cmd *cmd,
>> +		size_t *nr)
>> +{
>> +	int i;
>> +
>> +	if (!opt->buffer_output)
>> +		die(_("flush is only for --buffer mode"));
>> +
>> +	for (i = 0; i < *nr; i++) {
>
> If you updated the max number of items *nr to size_t, don't you need
> to use 'i' with the same type to count up to it?
>
>> +		cmd[i].fn(opt, cmd[i].line, output, data);
>> +		free(cmd[i].line);
>> +	}
>> +
>> +	*nr = 0;
>> +	fflush(stdout);
>> +}
>
> Wouldn't it be easier to reason about what the caller/callee are
> responsible for, if the function signature looked more like:
>
>     static size_t dispatch_calls(struct batch_options *opt,
>     			     ...
>     			     struct queued_cmd cmd[], size_t nr)
>     {
>     	size_t i;
>
>     	for (i = 0; i < nr; i++)
>     		... do stuff ...;
>
>     	return updated_nr;
>     }
>
> and make the caller do
>
>     nr = dispatch_calls(opt, ..., nr);
>
> or if the function *never* leaves anything in the queue when it
> returns, then
>
>     static void dispatch_calls(struct batch_options *opt,
>     			     ...
>     			     struct queued_cmd cmd[], size_t nr)
>     {
>     	size_t i;
>
>     	for (i = 0; i < nr; i++)
>     		... do stuff ...;
>     }
>
> 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/