[PATCH 0 of 2] help: RST formatting

[Matt Mackall]

[back on the list]

On Thu, 2012-05-17 at 23:11 +0200, Olav Reinert wrote:
> On 17. maj 2012, at 17.59, Matt Mackall wrote:
> > On Thu, 2012-05-17 at 17:14 +0200, Olav Reinert wrote:
> >> 1) The first column of Field lists are now always 14 characters wide.
> > 
> > Why? This sounds suspiciously like an "unrelated change".
> 
> It happens due to the following:
> 
> 1) I chose to format the command lists using RST field list markup, because it most closely resembles the original format:
> 
> http://docutils.sourceforge.net/docs/user/rst/quickref.html#field-lists
> 
> 2) minirst always formats field lists with a first column 14 characters wide due to this change:
> 
> http://markmail.org/message/krl4cxopsnii7s6z
> 
> Some markup had to be chosen to achieve two-column output, and other options seemed even more intrusive.
> I don't consider this an unrelated change.

That's probably just fine, but my comment is not about your particular
choice here but about the process.

Here, if you isolate the part where you switch to using field list to
its own patch with _nothing else in it_, it becomes completely obvious
why the tests are changing both to me and to future archaeologists. But
in the current patch, it's an unfathomable mystery. No reviewer will
have the time or attention span to comprehend all these changes.

And that's just one of numerous reasons why such a big patch is a
non-starter. Others include lack of bisectability, and the inability of
the reviewer to accept good bits and send others back for rework.

Again, this really needs to be broken into several steps.

> > I have to drop the first one too, as I now have no confidence that
> > you've internalized "don't make unrelated changes" and I have no
> > convenient way of confirming that you've just moved the code without
> > sneaking in tweaks.
> 
> Not even the fact that it makes no changes to any test cases, yet passes all of them, is enough to convince you of that?

The test suite doesn't have perfect coverage, so no, it's not very
strong evidence that you've simply moved the code . And the follow-up
patch is pretty strong evidence that you don't really understand why we
care about doing one easily-reviewed thing at a time yet.

But if you say "no really, I just did the minimum work to move the code
and didn't slip in any other changes because it seemed like a convenient
time to do it" then I'll probably believe you.

(You might be wondering, where did this crazy review discipline come
from? It's basically identical to the Linux kernel's discipline, where I
hacked and reviewed code for over a decade.)

-- 
Mathematics is the supreme nostalgia of our time.