Re: [PATCH v3 0/8] refactor the filter process code into a reusable module

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 29, 2017, 19:59 UTC
Message-ID: <xmqqr31fam77.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170329155330.12860-1-benpeart@microsoft.com>

Ben Peart <peartben@gmail.com> writes:

Show 10 quoted lines

> Ben Peart (8):
>   pkt-line: add packet_writel() and packet_read_line_gently()
>   convert: Update convert to use new packet_writel() function
>   convert: Split start_multi_file_filter into two separate functions
>   convert: Separate generic structures and variables from the filter
>     specific ones
>   convert: Update generic functions to only use generic data structures
>   convert: rename reusable sub-process functions
>   sub-process: move sub-process functions into separate files
>   convert: Update subprocess_read_status to not die on EOF

This presentation is much easier to digest, compared to the large ball of wax we saw previously. It highlights the key modification that cmd2process is now "subclassed" from subprocess_entry which is a more generic structure by embedding the latter at the beginning, and have its user start_multi_file_filter_fn() explicitly downcast the latter to the former around patches 4/8 and 5/8.

If I were doing this series, I would organize the first two slightly differently, namely:

 * 1/8 just adds packet_read_line_gently().

 * 2/8 moves packet_write_line() from convert.c to pkt-line.c while
   renaming it, with the justification that this function must be
   made more widely available.  It would naturally involves
   adjusting existing callers.

because write and read done in your 1/8 are independent and orthogonal changes, and doing it that way also avoids needless temporary duplication of the same function.

I may later have further comments on 3-8/8 after giving them another read, but I haven't seen anything questionable in them so far.

Thanks.