From: Karthik Nayak Date: Sun, 31 May 2015 09:12:37 GMT Subject: Re: [WIP/PATCH v4 8/8] ref-filter: add 'ref-filter.h' Message-ID: <556AD085.7080104@gmail.com> In-Reply-To: On 05/31/2015 01:59 PM, Eric Sunshine wrote: > On Sun, May 31, 2015 at 4:19 AM, Karthik Nayak wrote: > > On 05/31/2015 09:13 AM, Eric Sunshine wrote: > >> On Sat, May 30, 2015 at 1:53 PM, Karthik Nayak > >> 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