Re: [PATCH v8 1/5] Refactor parse_loc

> Thomas Rast <trast@student.ethz.ch> writes:
>
>> From: Bo Yang <struggleyb.nku@gmail.com>
>>
>> We want to use the same style of -L n,m argument for 'git log -L' as
>> for git-blame.  Refactor the argument parsing of the range arguments
>> from builtin/blame.c to the (new) file that will hold the 'git log -L'
>> logic.
>>
>> To accommodate different data structures in blame and log -L, the file
>> contents are abstracted away; parse_range_arg takes a callback that it
>> uses to get the contents of a line of the (notional) file.
>>
>> The new test is for a case that made me pause during debugging: the
>> 'blame -L with invalid end' test was the only one that noticed an
>> outright failure to parse the end *at all*.  So make a more explicit
>> test for that.
>>
>> Signed-off-by: Bo Yang <struggleyb.nku@gmail.com>
>> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
>> ---
>>  Documentation/blame-options.txt     |  19 +------
>>  Documentation/line-range-format.txt |  18 +++++++
>>  Makefile                            |   2 +
>>  builtin/blame.c                     |  99 +++-------------------------------
>>  line-log.c                          | 105 ++++++++++++++++++++++++++++++++++++
>>  line-log.h                          |  23 ++++++++
>
> Was this churn necessary?  
>
> It is strange to move existing functions that will be tweaked to be
> shared by two different codepaths (blame and line-log) to the new
> user.
>
> The only effect this has, as opposed to tweaking the functions in
> place and making them extern, is to make it harder to see the tweaks
> made while moving the lines, and also make it more cumbersome to
> determine the lineage of the code later.
>  
> It would have been understandable if they were moved to a new
> library-ish file (perhaps "line-range.[ch]"); even though that
> approach shares the same downsides, at least it would have a better
> excuse "We will share this, so let's move it to a neutral third
> place to allow us to hide the implementation details from both
> users".  The arrangement this patch series makes does not even have
> that excuse.  The final implementation still stay with one of the
> users; the only difference is that it is away from the original user
> and close to the new user.