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

On Fri, Oct 14 2022, Phillip Wood wrote:

Hi, thanks for taking a look again.

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/

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.

Glad you like it!