On Thu, Jul 28 2022, Johannes Schindelin wrote: > [...] > I see why this might have been suggested, but it definitely made it more > challenging for me to review. You see, it is easy to just fly over a patch > that simply removes the `PERL` prereq, but it is much harder to jump back > and forth over all of these removals when the `.c` version of the filter > is added before them and the `.pl` version is removed after them. So I > find that it was bad advice, but I do not fault you for following it (we > all want reviews to just be over already and therefore sometimes pander to > the reviewers, no matter how much or little sense their feedback makes). > [...] > To illustrate my point: it was a bit of a challenge to find the adjustment > of the "smudge write error at" needle in all of that cruft. It would have > made my life as a reviewer substantially easier had the patch > series been organized this way (which I assume you had before the feedback > you received demanded to squash everything in one hot pile): If you don't think a suggestion of mine makes sense, I'd appreciate it if you just replied me directly, instead of sending this sort of comment to someone else. I find your wording here to be somewhere between snarky and mean-spirited. I didn't demand anything. If this was the first time this sort of thing has occurred I wouldn't say anything about it, but this is far from being the first time. In any case, if you read more than a few words into https://lore.kernel.org/git/220723.86pmhwquie.gmgdl@xxxxxxxxxxxxxxxxxxx/ you'll see that I suggested splitting the removal of the PERL prereq into its own change, which I think would address what you're bringing up here. What I was mainly commenting on was that this series could avoid introducing code in-between the v1 1/2 and 2/2 which is only needed because of that split-up. I.e. the "exec", and needing to quote those arguments. Which I stand by, I think it's much easier to just do a "git show --word-diff" on this than reason about how that "chain-loading" is working, and whether the inter-series state is buggy. But again, the concern you about the associated verbosity is easy to mitigate. On the point of pandering to reviewers I find it really nitpicky to ask for changes to change some working O(N^2)) code in a test-tool to O(N log(N)), or to avoid a few allocations here & there. If it was a new git built-in, then sure, but I think our collective time is much better spend by just letting that sort of thing slide when it comes to test-tools, which are almost always going to be operating on the relatively tiny set of test data we expose them too. Unless Matheus is keenly interested on optimizing this code, that is. > [...] > This does what we want it to do, but it looks as if it was code translated > from a language that does not care one bit about allocations to a language > that cares a lot. FWIW Perl cares a lot about allocations, the sort of code you're commenting on here doesn't involve allocations in Perl in the general case, since it "allocates ahead", similar to how we use alloc_nr() and strbuf_reset() patterns. What it doesn't care about is free()-ing memory, which is an orthagonal thing. But that's just an optimization, the general assumptios in that if your program ever needs X MB of memory it's likely to need at least that much again, or it'll exit() and have the OS clean it up.
