On 05/31/2015 01:59 PM, Eric Sunshine wrote:
On Sun, May 31, 2015 at 4:19 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote: > On 05/31/2015 09:13 AM, Eric Sunshine wrote: >> On Sat, May 30, 2015 at 1:53 PM, Karthik Nayak <karthik.188@xxxxxxxxx> >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html