Re: [PATCH 1/4] line-log: demonstrate a bug with nearly-overlapping ranges

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 6, 2018, 14:47 UTC
Message-ID: <20180806144726.GB97564@aiede.svl.corp.google.com>
In-Reply-To: <nycvar.QRO.7.76.6.1808061216060.71@tvgsbejvaqbjf.bet>

Hi Dscho,

Johannes Schindelin wrote:

> On Sat, 4 Aug 2018, Jonathan Nieder wrote:

Show 11 quoted lines

>> Alternatively, could it be squashed in with the patch that fixes it?
>
> There is this really awful trend on this mailing list to suggest
> conflating the demonstration of a bug with the bug fix.
>
> It is really, really important to realize how valuable it is to have the
> regression test as an individual patch that can be used to verify that
> there is a bug, to pinpoint where it was introduced, to test alternative
> fixes, to keep records separate, and I could go on and on and on. Please
> do not ignore these very good reasons, and please refrain from
> recommending such conflation in the future.

If you want to propose changing the project's style to always separate tests from the patch that fixes a bug, that's a discussion we can have, in a separate thread.

Today, we do allow and encourage putting the test with the patch that fixes it, and that has real advantages:

- the tests are easier to understand when found with "git log" because
  they are in context

- as the patch evolves, it is easier to remember to update the test at
  the same time

- newcomers imitating existing patches have a clear hint to write
  tests

- the beginning of a patch series can be applied and merged down while
  the end is still under review, without either leaving out the tests
  or applying a test that doesn't pass and accomplishes little

I've never found it difficult to use the test from a patch to verify that there is a bug or pinpoint where it was introduced. Tests are separate from the application code since they're in the t/ directory; this is a very easy thing to do. That isn't to say that a patch that only adds a (passing or expected-failure) test isn't valuable, even without a fix. It is valuable, precisely when it is self-explanatory.

More importantly, I am a bit surprised that instead of accepting the feedback, you are basically calling a reviewer complicit, for pointing out a pretty normal possible improvement that follows the project's conventions.

I'm beyond words.

Jonathan