Re: [PATCH v3 00/15] run-command API: pass functions & opts via struct

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Oct 14 2022, Phillip Wood wrote:

> Hi Ævar

Hi, thanks for taking a look again.

> On 12/10/2022 22:02, Ævar Arnfjörð Bjarmason wrote:
>> For a general overview see the v2 CL:
>> https://lore.kernel.org/git/cover-v2-00.22-00000000000-20221012T084850Z-avarab@gmail.com/
>> Changes since v2:
>>   * Ejected various not-to-the-point of converting to the "opts
>> struct"
>>     per feedback about attempting to make this leaner.
>
> We're back to the same number of patches as v1 but you've removed
> three test cleanups and squashed three patches together which means
> there are five new patches in this version - what are they doing?

There's nothing *new* in this version, but per the range-diff of v2 some
of the new ones in that version were kept here. I think that's what
you're taling about.

Mainly it's the 'Only the "ungroup" was here in v1[...]' part of the v2
CL:

1. https://lore.kernel.org/git/cover-v2-00.22-00000000000-20221012T084850Z-avarab@gmail.com/

>>   * I kept the size_t change & the online_cpus() fallback, and updated
>>     a commit message for the latter. For "int" v.s. "size_t" once we're
>>     not handling "-1" to mean "use the default" it makes sense to be
>>     unsigned, and if we're re-doing that at this point we'd need
>>     rewrites for "!processes" assumptions.
>
> I left some comments about this, I think the size_t change is taking
> us in the wrong direction as it is introducing a number of new
> implicit signed->unsigned conversions. I'm still not sure why you need
> the online_cups() changes c.f. 
> https://lore.kernel.org/git/8f95fbdb-b211-56af-8693-0e5a84afebac@gmail.com/
> which has never had a reply

Sorry about the non-reply there, between that & later discussion I tried
to address that in the "Keeping this default behavior just for[...]"
commit message update in this v3 (see range-diff).

But no, those changes are not strictly needed. But it's a trade-off I
decided to take.

In the v1 (linked above) you pointed out that we could simply copy this
field to the "struct pp" (a shorthand for "struct parallel_processes", I
assume).

That's true, but for maintaining & understanding this API I think it's
much easier to reason about it when all of our user options are "const",
and we don't second guess any of those, and the "struct
parallel_processes"

For the v1 I can see what that was easy to miss, as we still kept the
copy of the number of processes in the "struct parallel_processes". In
the v2 and this v3 we get to the point where we can remove that, and
"ungroup", the copies of the callbacks etc.

So, leaving out the "provide a default" seemed worth it in the end, it's
just 4 additional lines in the callers per the 05/15 (most of them had
those already).

You also had a related concern in 04/05 (which I'm taking the liberty of
replying ot here):

	https://lore.kernel.org/git/a7463bc5-9a92-8f0f-c0ee-e72fbbeedc09@dunelm.org.uk/

So, first I disagree with it "going in the wrong direction". We've been
converting more things to size_t. For e.g. an "int nr_processes" we can
expect that we'll want to e.g. have a corresponding "struct string_list"
whose "nr" is a "size_t" (or similar aggregate types).

By mixing the two we're mixing types forever with associated warnings (&
suppressions). We've been changing these sort of things to a "size_t"
elsewhere, e.g. dd38e9e510c (trace2: use size_t alloc,nr_open_regions in
tr2tls_thread_ctx, 2022-10-12) is one recent example..

But yes, we do incur warnings under DEVOPTS=extra-all now because things
outside of the API are still "int" in some cases, just as we do with
e.g. "struct string_list" and its users.

As to your:

	Before this patch we could have caught a negative value of n
	with an assertion. After this patch a negative value passed will
	become a large positive value (you have audited the current
	callers but that does not protect us from future mistakes).

I that's a good point. In this case I thought the likelihood that anyone
would accidentally pass ".processes = -1" or whatever wasn't worth
worrying about. If you think it's worth worrying about I think that
concern would be easily addressed e.g. with:

	if (n == SIZE_MAX)
        	BUG("overflow?");

Or whatever.

A much likelier edge case IMO is that you'd have some pattern where you
init a "processes" with "{ 0 }" or whatever, so it's zero, and then we'd
interpret that zero as online_cpus(), which e.g. for the in-flight
paralellizing of certain "git submodule" operations is probably too high
a number.

Since we don't interpret !n or n < 1 as online_cpus() anymore we can
BUG() out on it, which I think is an improvement. The 4 lines of
additions to the relevant callers to call online_cpus() themselves are
better than having those or a new caller potentially have this DWYMery
from the low-level API.

>>   * Squashed the multi-step introduction of the "opts" struct, per
>>     Phillip's suggestion.
> That's most welcome, thanks

Glad you like it!