Re: [RFC PATCH 02/10] range-diff.c: don't use st_mult() for signed "int"

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

On Sat, Dec 11 2021, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Fri, 10 Dec 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> But I'll happily admit ignorance on how the actual guts of range-diff
>> work, I just wanted to fix a segfault I kept running into locally at
>> some point, and figured I'd submit this RFC.
>
> I understand that it is super tempting to avoid spending the time to
> understand how range-diff works and simply make changes until the
> segmentation fault is gone, and then shoot off several iterations of the
> patch series in the hopes that it gets merged at some point, and that
> maybe reviewers who do spend the time to become familiar with the logic
> help avoid introduce new bugs.
>
> However, as a reviewer I am totally unsympathetic of this approach. I do
> not want to review patches that even go so far as renaming functions when
> they claim to "just fix a segfault" and the author even admits that
> they're unfamiliar with what the code is supposed to do, without any
> indication that they're inclined to invest the effort to change that.

What you're eliding here is the context where I say that I must not be
getting something because you're apparently endorsing the WIP
s/int/intmax_t/g patch Jeff King inlined upthread without a
corresponding change to COST_MAX.

Don't those two go hand-in-hand, and changing one without the other
would lead to a subtle bug?

> If all you want to do is to fix the segmentation fault, and want to skip
> the due diligence of studying the business logic, then just fix that
> segmentation fault (I strongly suspect that using `COST()` after modifying
> it to use `st_*()` would accomplish that).

Well, this is an RFC series for a bug that I encountered & which seems
to be fixed by these changes, but in an area which I'll happily admit
that I'm not confident enough to say that this is *the* right fix, and I
think both the "RFC" label and both cover letters make that clear.

> No need to inflate that to 5
> patches. Unless you're thinking of the commit-per-author count as some
> sort of scoreboard where you want to win. In which case I am even less
> interested in reviewing the patches.

Can you mention specific things you'd like to have squashed? I do think
this split-up makes thinsg easier to review.

E.g. if we're using the COST() macro in range-diff.c then splitting 4/5
from 5/5 means you don't need to spend as much time mentally splitting
the meaningful changes from a variable rename (which is required for
using that macro).

I agree that 1-3/5 aren't strictly necessary. I did try to do this
without those, but found e.g. reasoning about changing the
one-giant-function in linear-assignment.c harder when it came to the
segfault fix, and likewise the mechanical change from "int" to "size_t"
is (I think) easier to review & reason about.