Re: [WIP/PATCH v4 8/8] ref-filter: add 'ref-filter.h'

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, May 31, 2015 at 4:19 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On 05/31/2015 09:13 AM, Eric Sunshine wrote:
>> On Sat, May 30, 2015 at 1:53 PM, Karthik Nayak <karthik.188@gmail.com>
>> wrote:
>>>
>>> Create 'ref-filter.h', also add ref-filter to the Makefile.
>>> This completes movement of creation of 'ref-filter' from
>>> 'for-each-ref'.
>>
>> It's important that the project can be built successfully and function
>> correctly at each stage of a patch series. Unfortunately, the way this
>> series is organized, the build breaks after application of patch 7/8
>> since for-each-ref.c is trying to access functionality which was moved
>> to ref-filter.c, but there is no header at that stage which advertises
>> the functionality. Fixing this may be as simple as swapping patches
>> 7/8 and 8/8, along with whatever minor adjustments they might need to
>> keep them sane. (The alternative would be to combine patches 7/8 and
>> 8/8, however, Matthieu didn't seem to favor that approach[1].)
>
> That is kind of a problem, If I need to swap those commits also, I'd have to
> add the part where ref-filter is added to the Makefile with the code
> movement from for-each-ref to ref-filter. This again will not just be Code
> movement.
> But I guess thats a fair trade-off, will change it. Thanks

Don't take the "code movement-only" recommendation too literally. What
people mean is that you shouldn't make changes to the lines you're
moving from one location to another (other than absolutely mandatory
changes to support the movement) since it's difficult to spot and
review changes in lines which are being moved.

The recommendation is not saying that the patch itself can't include
any other changes (though, it's often best to minimize other changes
in the same patch). In this case, the Makefile change is logically
related to that particular code movement, so it's correct to include
it in the patch. (Also, the Makefile change is so minor that it's not
a burden upon reviewers.)