Re: [PATCH v2 00/33] Yet more preparation for reference backends
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 9, 2016, 21:05 UTC
- Message-ID
- <xmqqinynt01r.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1462824758.24478.37.camel@twopensource.com>
David Turner <dturner@twopensource.com> writes:
Show 22 quoted lines
> 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).