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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> From: Junio C Hamano [mailto:gitster@xxxxxxxxx]
> 
> Ben Peart <peartben@xxxxxxxxx> writes:
> 
> > 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'll go ahead and do it that way.
 
> 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.





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]