RE: [PATCH v1 2/3] sub-process: refactor the filter process code into a reusable module

From: Ben Peart <ben.peart@microsoft.com>
Date: Mar 24, 2017, 12:39 UTC
Message-ID: <BL2PR03MB32308D0F2BB1C7D6F04141BF43E0@BL2PR03MB323.namprd03.prod.outlook.com>
In-Reply-To: <xmqqk27gfrga.fsf@gitster.mtv.corp.google.com>

Show 27 quoted lines

> -----Original Message-----
> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Thursday, March 23, 2017 2:17 AM
> To: Ben Peart <peartben@gmail.com>
> Cc: git@vger.kernel.org; Ben Peart <Ben.Peart@microsoft.com>;
> christian.couder@gmail.com; larsxschneider@gmail.com
> Subject: Re: [PATCH v1 2/3] sub-process: refactor the filter process code into
> a reusable module
> 
> Ben Peart <peartben@gmail.com> writes:
> 
> > This code is refactored from:
> >
> > 	Commit edcc85814c ("convert: add filter.<driver>.process option",
> 2016-10-16)
> > 	keeps the external process running and processes all commands
> 
> I am afraid that this organization of the patch series is making it harder than
> necessary to review, by duplicating the same code first _WITH_ renaming of
> symbols, etc., in this step and then updaing the original callers while
> removing the now-stale original callee implementation in a separate next
> step.
> 
> Would it perhaps make it clearer to understand what is going on if you
> instead started to update the code in convert.c in place to make it more
> suitable fro reuse as the patch title advertises, and then move the resulting
> cleaned-up code to a separate file, I wonder.

I'm not entirely sure what you're asking for here but I think the challenge may be that by splitting the refactoring into two separate commits, it's hard to see the before and after when looking at the commit in isolation. By splitting them, it's more "a bunch of new code" followed by "using new code" than it is a refactoring of existing code.

How about I squash the last two patches together so that its more apparent that it's just a refactoring of existing code with the before and after in a single patch?