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

[Karthik Nayak <karthik.188@gmail.com>]

On 05/31/2015 01:59 PM, Eric Sunshine wrote:
> 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.)
>
Thanks for clearing it up.

-- 
Regards,
Karthik