Re: [PATCH v3 5/8] commit-reach: introduce struct paint_state with per-side counters

From: Kristofer Karlsson <krka@spotify.com>
Date: Jun 26, 2026, 21:57 UTC
Message-ID: <CAL71e4O470P7i55C1yHyS9zjmDw2fY8J19KbywFgQkfvenBe1g@mail.gmail.com>
In-Reply-To: <bd37b80d-9eff-496d-8f1f-436594968678@web.de>

On Fri, 26 Jun 2026 at 23:13, René Scharfe <l.s.r@web.de> wrote:

Show 9 quoted lines

>
> > +struct paint_state {
> > +     struct prio_queue queue;
> > +     int p1_count;
> > +     int p2_count;
> > +     int pending_merge_bases;
> > +};
> Can they become negative?  Wouldn't size_t be a more natural fit,
> matching nr from struct prio_queue?

Negative would be a clear indication of a bug though that's not checked right now anyway. And since it's not checked we might as well use size_t instead - and it would technically be more correct though I struggle to imagine a case where the number of active elements in the frontier exceeds 2^31 or whatever a signed int would give.

I am happy to change to size_t.

Show 10 quoted lines

> And some bikeshedding:
>
> Why abbreviate?  parent1_count and parent2_count would be slightly
> easier to read and associate with PARENT1 and PARENT2.
>
> And pending_merge_bases is a counter as well.  Why not call it
> like that, pending_merge_base_count?   Well, that's pretty long.
> both_count?  That's quite generic and nondescript.  Call the other
> counters parents1 and parents2?  Nah.  Or parent1s and parent2s?
> Not sure why this inconsistency bothers me to begin with.

Fair point, I was thinking that the surrounding context is so small that the naming almost doesn't matter - the terms don't escape paint_down_to_common.

I am happy to change to something like: parent1_count, parent2_count, mb_candidate_count to make it more consistent.

It seems the mb_ prefix is already used for merge bases in some files - best example is perhaps builtin/diff.c

I see in the codebase that we are using multiple styles, perhaps depending on specific context.

- nr_ prefix: nr_objects, nr_paths_watching
- num_ prefix: num_commits, num_hashes, num_workers
- _count suffix: entry_count, max_count, skip_count

so I think _count suffix is a good choice at least - it matches other usages where we typically just increment or decrement.

Thanks, Kristofer