On Wed, Sep 28 2022, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

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.

quoted

 * 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 :)