John Cai <johncai86@xxxxxxxxx> writes:
>>> + 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.
>>> +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).
