From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Wed, 30 Nov 2022 10:57:38 GMT
Subject: Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts
Message-ID: <221130.86cz94iu9a.gmgdl@evledraar.gmail.com>
In-Reply-To: <5f6f0181-c251-0a98-a39f-a910b93f8c1a@dunelm.org.uk>


On Wed, Nov 30 2022, Phillip Wood wrote:

> On 30/11/2022 09:53, Ævar Arnfjörð Bjarmason wrote:
>> On Tue, Nov 29 2022, Glen Choo wrote:
>> 
>>> Calvin Wan <calvinwan@google.com> writes:
>>> So this might be more idiomatically written as:
>>>
>>>          int n = strbuf_read_once(&pp->children[i].err,
>>>          			 pp->children[i].process.err, 0);
>>>   +      if (opts->duplicate_output) {
>>>   +          struct strbuf buf = STRBUF_INIT;
>>>   +          strbuf_addbuf(&buf, &pp->children[i].err);
>>>   +        	opts->duplicate_output(&buf, &pp->children[i].err,
>>>   +        		  opts->data,
>>>   +        		  pp->children[i].data);
>>>   +          strbuf_release(&buf);
>>>   +      }
>>>
>>> which also has the nice benefit of not touching the strbuf_read_once()
>>> line.
>> We should also use "size_t n" there, not "int n", which is what it
>> returns.
>
> It returns an ssize_t not size_t, lower down we test `n < 0` so we
> certainly should not be using an unsigned type.

Good catch, I just skimmed it and missed the extra "s". In any case:
let's use the type it's returning, so ssize_t, not int.

(DEVOPTS=extra-all etc. will also catch this, depending on your compiler
etc.)