From: Kristofer Karlsson <krka@spotify.com>
Date: Wed, 24 Jun 2026 14:47:13 GMT
Subject: Re: [PATCH v2 7/7] commit-reach: terminate merge-base walk when one paint side is exhausted
Message-ID: <CAL71e4N1zMz=v9umGdGPTvLP1nF-tNLVQc+vAEBnekt2L0b6zQ@mail.gmail.com>
In-Reply-To: <6b0d81e7-7617-4fb4-9e39-cdf8bc778837@gmail.com>

On Wed, 24 Jun 2026 at 16:02, Derrick Stolee <stolee@gmail.com> wrote:
>
> I see how the previous implementation has a termination condition
> before calling prio_queue_get(), which is technically more
> efficient. It does make this initial diff a bit more complicated
> because we are moving the prio_queue_get() line.

I was thinking the efficiency here does not matter in practice -
prio_queue_get() only returns NULL once, and all other times
where we keep looping we do need the value.

I agree it does get a bit complex though.

> If the introduction of the method in patch 5/7 looked like this:
>
> +static struct commit *paint_queue_get(struct paint_state *state)
> +{
> +       struct commit *commit = prio_queue_get(&state->queue);
> +
> +       if (!commit)
> +               return NULL;
> +
> +       if (!state->p1_count && !state->p2_count &&
> +           !state->pending_merge_bases)
> +               return NULL;
> +
> +       commit->object.flags &= ~ENQUEUED;
> +       paint_count_update(state, commit->object.flags, -1);
> +       return commit;
> +}
>
> Then this diff would look cleaner.
>
> (This is the nittiest of nitpicks so feel free to ignore if this
> doesn't bother you at all.)

That's a good point. It doesn't technically bother me,
but it would be cleaner. The refactor commit would effectively
be looking into the future and prepare for it. I can change it for
the next version - my only thinking was that the current refactor
patch matched my original idea for how to best handle
the halt condition, but that did indeed change after this discussion.

> > -     test_trace2_data paint_down_to_common steps 81 <trace-half.txt
> > +     test_trace2_data paint_down_to_common steps 57 <trace-half.txt
> >  '
> I love to see these steps change. If you take my suggestion to
> update more tests with these checks, then this diff will get bigger
> (but in a deserved way).

I will try to add them to some (but not all) tests since it's more
closely related to performance than correctness and I want to
avoid making too many tests overly fragile.

> Also, when I suggested that 'test_all_modes' creates the trace
> files on our behalf, I forgot to mention that this specific test
> that you added in patch 4/7 simplifies by running the merge-base
> check under 'test_all_modes' and then checking the trace2 data
> on the three well-known files afterwards.

That's a nice bonus, I will try to see if I can manage to utilize it.

Thanks,
Kristofer