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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 18, 2022, 17:32 UTC
Message-ID: <xmqqpmnk2i8u.fsf@gitster.g>
In-Reply-To: <ECBE48BF-156B-4D8A-9CDE-30AE84BF3630@gmail.com>

John Cai <johncai86@gmail.com> writes:

Show 12 quoted lines

>>> +		if (!strcmp(cmd->name, "flush")) {
>>> +			dispatch_calls(opt, output, data, queued_cmd, nr);
>>> +			free_cmds(queued_cmd, nr);
>>> +			nr = 0;
>>
>> It'd be nice if free_cmds() zeroed nr for us rather than having to remember to do it separately as the two are intimately linked.
>
> This does feel cleaner. Before there was a version where I did this inside of
> dispatch_calls and there was feedback that this wasn't clean. But now that
> free_cmds prepares the queued_cmd array for reuse, then it may make sense to do
> it inside. Though honestly from the back and forth around this, I'm not too sure
> what the best thing to do stylistically would be.

I am not sure about style, but at the semantic level, free_cmds() that "frees" the queued_cmd by releasing the resources it holds and resets its counter to zero would be a more complete "does one thing and one thing well" helper function.

Show 5 quoted lines

>>>  +test_expect_success '--batch-command --buffer with flush for blob info' '
>>> +	echo "$hello_sha1 blob $hello_size" >expect &&
>>> +	test_write_lines "info $hello_sha1" "flush" | \
>>
>> You don't need a '\' after a '|', however it might be better to use the style from the tests above where the '|' is on the beginning of the next line.

Please don't do

	producer \
	| consumer

instead, write

	producer |
	consumer

With two fewer bytes, and is far more common, judging from the output of

    $ git grep -e '^[   ]*| [A-Za-z]' t

i.e. indent with whitespace or tab, pipe, space and alpha (i.e. the beginning of the command, possibly a single-shot environment assignment).