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

From: Junio C Hamano <[email-masked]>
Date: Mon, 09 May 2016 14:05:04 -0700

David Turner <[email-masked]> writes:

> On Fri, 2016-05-06 at 18:13 +0200, Michael Haggerty wrote:
>> Thanks to David, Junio, and Peff for their comments on v1 of this
>> patch series [1]. I think I have addressed all of the points that
>> were
>> brought up. Plus I fixed a pre-existing bug that I noticed myself
>> while adding some more tests; see the first bullet point below for
>> more information.
>> 
>> Changes between v1 and v2:
>> 
>> * Prefixed the patch series with three new patches that demonstrate
>>   and fix some bugs in reference resolution that I noticed while
>>   inspecting this series:
>> 
>>   * "t1404: demonstrate a bug resolving references" -- this
>>     demonstrates a bug that has existed since at least 2.5.0.
>>   * "read_raw_ref(): don't get confused by an empty directory"
>>   * "commit_ref(): if there is an empty dir in the way, delete it"
>
> 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).