From: Thomas Rast Date: Thu, 28 Feb 2013 19:24:44 GMT Subject: Re: [PATCH v8 1/5] Refactor parse_loc Message-ID: <87lia86y2r.fsf@pctrast.inf.ethz.ch> In-Reply-To: <7vobf4icjh.fsf@alter.siamese.dyndns.org> Junio C Hamano writes: > Thomas Rast writes: > >> From: Bo Yang >> >> 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 >> Signed-off-by: Thomas Rast >> --- >> 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. Even though I am moving from builtin/blame.c to line-log.c? I would otherwise have to call from a rather lib-ish file into a "frontend" file. I always figured I wasn't supposed to do that. -- Thomas Rast trast@{inf,student}.ethz.ch