Re: [PATCH v7 1/7] run-command: add duplicate_output_fn to run_processes_parallel_opts

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 9, 2023, 20:37 UTC
Message-ID: <a86cda2c-71be-a19a-f269-92d095b2428d@dunelm.org.uk>
In-Reply-To: <CAFySSZDeC-zc7wS7WXE+bAijVQkuin5GwLRnJ2ok7C9PRaH3+g@mail.gmail.com>

Hi Calvin

On 08/02/2023 22:54, Calvin Wan wrote:

Show 18 quoted lines

>>> +                     } else {
>>> +                             if (opts->duplicate_output)
>>> +                                     opts->duplicate_output(&pp->children[i].err,
>>> +                                            strlen(pp->children[i].err.buf) - n,
>>
>> Looking at how this is used in patch 7 I think it would be better to
>> pass a const char*, length pair rather than a struct strbuf*, offset pair.
>> i.e.
>>          opts->duplicate_output(pp->children[i].err.buf +
>> pp->children[i].err.len - n, n, ...)
>>
>> That would make it clear that we do not expect duplicate_output() to
>> alter the buffer and would avoid the duplicate_output() having to add
>> the offset to the start of the buffer to find the new data.
> 
> I don't think that would work since
> pp->children[i].err.buf + pp->children[i].err.len - n
> wouldn't end up as a const char* unless I'm missing something?

You can still pass it to a function that takes a const char* though and change type of the callback to

typedef void (*duplicate_output_fn)(const char *out, size_t offset,
				    void *pp_cb, void *pp_task_cb);

Best Wishes

Phillip