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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 12, 2021, 17:44 UTC
Message-ID: <211212.861r2hbtb5.gmgdl@evledraar.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2112111453040.90@tvgsbejvaqbjf.bet>

On Sat, Dec 11 2021, Johannes Schindelin wrote:

Show 20 quoted lines

> 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.