Re: [PATCH v2 00/33] Yet more preparation for reference backends

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: May 9, 2016, 21:50 UTC
Message-ID: <5731062B.6080909@alum.mit.edu>
In-Reply-To: <xmqqinynt01r.fsf@gitster.mtv.corp.google.com>

On 05/09/2016 11:05 PM, Junio C Hamano wrote:

Show 19 quoted lines

> David Turner <dturner@twopensource.com> writes:
>> [...]
>> I generally like to put the bug fixes before the tests for those fixes
>> (so that bisect on the complete suite works).  But maybe the git policy
>> is different.
> 
> The Git policy only asks not to break bisection.
> 
> As long as patch that adds a new test that comes before a patch that
> fixes the issue marks the new test with test_expect_failure, and a
> later patch that fixes the issue turns it into test_expect_success,
> bisection would not break.
> 
> The "demonstrate an existing breakage first" order makes it slightly
> easier to review and follow a long series, as it forces the reviewer
> to see the issue first and think about possible avenues to solve it
> for themselves, before seeing a paticular solution.  For a trivial
> single-issue fix, it is not necessary (including a fix and a test to
> protect the fix from future breakage in the same patch is a norm).

I find it useful to add the broken test in a separate patch, because it is then easy to cherry pick that patch to other versions of Git to discover which ones are also affected by the problem. If the addition of the test is combined with the fix, then the patch would more often fail to apply to other versions due to conflicts at the locations of the fix, and even if it applied, you wouldn't learn whether that version of git was broken but the breakage was fixed by the same fix, or whether it wasn't broken in the first place and the fix was unnecessary.

Michael