On Wed, Sep 28 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> Victoria: I decided not to go for your suggestion of trimming this >> series down in [1]. Reasons: >> >> * It would take me time I don't have to spend on this, as some of it >> isn't easy to cleanly re-arrange. E.g. the later "make consistent" >> commits rely on earlier whitespace/basic syntax fixes. > > A devil's advocate question. If even the original author feels it > does not deserve his or her time to clean up the series, how does it > possibly deserve reviewers' time to review such a series? So, I'm clearly doing a bad job of explaining this (and I'm not saying I'm not also lazy!), but with the latter part of that I meant that I took pains to optimize this for overall reviewer time. I.e. at the start of the series (made up example, but it'll suffice) we might have stuff like this: *.txt usage: (foo|bar) <file> -h usage: (foo | bar ) <dir> The start of this series fixes a bunch of misc issues like whitespace issues, so we can e.g. turn that into: *.txt usage: (foo | bar) <file> -h usage: (foo | bar) <dir> At *that* point I can include the s/dir/file/ change on one side in a "doc txt & -h consistency" commit, and end up with, say: *.txt usage: (foo | bar) <file> -h usage: (foo | bar) <file> So, I can say for the "doc txt & -h consistency" that we had the "(foo | bar) <file>" version in-tree already, but that's only the case if you'll buy the earlier whitespace-only change. I think that's easier to reason about & review than a bunch of "here I'm changing the label, and some whitespace issues, and blah blah". I.e. the reviewer only has to pay attention to the first change being a whitespace-only change, and can then be assured that post-whitespace change we're just changing one side to be consistent with the other. We then test that consistency at the end of the series. >> * A major advantage of reviewing this in one go is that the 34-35/35 >> tests at the end are asserting everything that came before >> it. > > Yes, but it does not assert anything about the other patches not > doing unrelated things while at it. So tests cannot be blindly > trusted (in other words you have to be also trustworthy, if the > reviewers are expected to swallow this huge series uninspected). > > I'll give it a read-over when I find time. Thanks for working on > it. Right, I didn't mean to say that these could be blindly trusted, just that the series was working towards splitting up little fixes like whatespace fixes, and at *that point* the reader should be assured that one side of the commits with "doc txt & -h consistency" in the subject is in-tree already. Of course that at the minimum needs a review for *which side* we should pick :)
