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.
