archive mirror
 help / color / mirror / Atom feed
From: Jonathan Nieder <>
To: Johannes Schindelin <>
Cc: Johannes Schindelin via GitGitGadget <>,, Thomas Rast <>,
	Junio C Hamano <>
Subject: Re: [PATCH 1/4] line-log: demonstrate a bug with nearly-overlapping ranges
Date: Mon, 6 Aug 2018 07:47:26 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

Hi Dscho,

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

>> 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

- 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

I'm beyond words.


  reply	other threads:[~2018-08-06 14:47 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-04 22:18 [PATCH 0/4] line-log: be more careful when adjusting multiple line ranges Johannes Schindelin via GitGitGadget
2018-08-04 22:18 ` [PATCH 1/4] line-log: demonstrate a bug with nearly-overlapping ranges Johannes Schindelin via GitGitGadget
2018-08-05  1:59   ` Jonathan Nieder
2018-08-06 10:27     ` Johannes Schindelin
2018-08-06 14:47       ` Jonathan Nieder [this message]
2018-08-06 15:33         ` Jonathan Nieder
2018-08-04 22:18 ` [PATCH 2/4] line-log: adjust start/end of ranges individually Johannes Schindelin via GitGitGadget
2018-08-05 10:14   ` Eric Sunshine
2018-08-05 10:57     ` Eric Sunshine
2018-08-06 12:52     ` Johannes Schindelin
2018-08-04 22:18 ` [PATCH 3/4] line-log: optimize ranges by joining them when possible Johannes Schindelin via GitGitGadget
2018-08-05  6:11   ` Junio C Hamano
2018-08-05  8:45   ` Andrei Rybak
2018-08-05 10:31     ` Eric Sunshine
2018-08-04 22:18 ` [PATCH 4/4] line-log: convert an assertion to a full BUG() call Johannes Schindelin via GitGitGadget
2018-08-05 10:42   ` Eric Sunshine
2018-08-06 13:14     ` Johannes Schindelin
2018-08-07  9:09       ` Eric Sunshine
2018-08-07 22:00         ` Eric Sunshine
2018-08-05 10:39 ` [PATCH 0/4] line-log: be more careful when adjusting multiple line ranges Eric Sunshine

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).