[PATCH 4 of 4 v3] log: add -L/--line-range option to follow file history by line range

Denis Laxalde denis at laxalde.org
Fri Oct 13 04:02:07 EDT 2017


Yuya Nishihara a écrit :
> On Tue, 10 Oct 2017 17:37:27 +0200, Denis Laxalde wrote:
>> # HG changeset patch
>> # User Denis Laxalde <denis.laxalde at logilab.fr>
>> # Date 1507290475 -7200
>> #      Fri Oct 06 13:47:55 2017 +0200
>> # Node ID a05d3b45319a9ec28205f19dd7012b206a2b200f
>> # Parent  86a055d1c06f55daeb5d725187b61522974d24e3
>> # Available At http://hg.logilab.org/users/dlaxalde/hg
>> #              hg pull http://hg.logilab.org/users/dlaxalde/hg -r a05d3b45319a
>> # EXP-Topic followlines-cli/v2
>> log: add -L/--line-range option to follow file history by line range
> 
> The series generally looks good to me in functionality POV. Some nits follow.

Nits addressed in v4 which I can send now or once we agree on the UI.

> 
> So, do we really like this UI?

For the record, there are currently two proposals:

1. The one implemented in this patch that adds a -L option to specify
    both the file and its line range:

      hg log -L file.c,13-23 -L main.c,2-6

2. The idea by Yuya to have pairs of -L FROMLINE-TOLINE options and
    regular FILE arguments

      hg log -L 13-23 file.c -L 2-6 main.c

    The issue with this one (as explained in [1]) is that option parsing
    would not be strict, meaning that (IIUC) we would allow:

      hg log -L 13-23 -L 2-6 file.c main.c

    to work the same as the previous example.

[1] 
https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-October/105858.html


Then, there's the question of using "-" to specify the line range as
"FROMLINE-TOLINE". As said in the commit message quoted below, I'm not
at all attached to this syntax and would be fine with using ":" which is
more consistent with the followlines() revset syntax. Later on, I think
I'd like to add support for an offset mechanism like FROMLINE+OFFSET or
FROMLINE-OFFSET, so perhaps FROMLINE:TOLINE would be better. The reason
I did not pick this in the first place is because I thought it would be
too much ":" when used in combination with file patterns (e.g.
relpath:file.c,12:23) but maybe it's acceptable.


What do others think?


>> We add a -L/--line-range option to 'hg log' taking file patterns along with a
>> line range using the (new) FILE,FROMLINE-TOLINE syntax where FILE may be a
>> pattern (matching exactly one file). The resulting history is similar to what
>> the "followlines" revset except that, if --patch is specified, only diff hunks
>> within specified line range are shown (it's also more convenient to type).
>>
>> Basically, this brings the CLI on par with what currently only exists in hgweb
>> through line selection in "file" and "annotate" views resulting in a file log
>> with filtered patch to only display followed line range.
>>
>> The option may be specified multiple times and can be combined with --rev to
>> futher restrict revisions. Revisions are shown in descending order and
>> renames are followed (sort of implying --follow).
>> Only the --graph option is currently not supported.
>>
>> Some debatable UI choices (I did not think too much about this for now).
>>
>> *   "," as a separator between the FILE and line range information; the idea
>>      is to avoid confusion with file pattern syntax which uses ":".
>>
>> *   "-" in the line range information may not be the best choice; in
>>      particular, we might want to add support for an offset +/- syntax.
> 
> Perhaps ":" would be better for consistency with the followlines() revset.
> 
>> +def getloglinerangerevs(repo, userrevs, opts):
>> +    """Return (revs, filematcher, hunksfilter).
>> +
>> +    "revs" are revisions obtained by processing "line-range" log options and
>> +    walking block ancestors of each specified file/line-range.
>> +
>> +    "filematcher(rev) -> match" is a factory function returning a match object
>> +    for a given revision for file patterns specified in --line-range option.
>> +    If neither --stat nor --patch options are passed, "filematcher" is None.
>> +
>> +    "hunksfilter(rev) -> filterfn(fctx, hunks)" is a factory function
>> +    returning a hunks filtering function.
>> +    If neither --stat nor --patch options are passed, "filterhunks" is None.
>> +    """
>> +    wctx = repo[None]
>> +
>> +    # Two-levels map of "rev -> file ctx -> [line range]".
>> +    linerangesbyrev = {}
>> +    for fname, (fromline, toline) in _parselinerangelogopt(repo, opts):
>> +        fctx = wctx.filectx(fname)
>> +        for fctx, linerange in dagop.blockancestors(fctx, fromline, toline):
>> +            if fctx.rev() not in userrevs:
>> +                continue
>> +            linerangesbyrev.setdefault(
>> +                fctx.rev(), {}).setdefault(
> 
> I'm not sure, but it might be fctx.introrev() since this function seems quite
> similar to _makefollowlogfilematcher().
> 
>> +                    fctx, []).append(linerange)
> 
> Perhaps it's better to not cache fctx for long. IIUC, we only need
> (rev or node, path).
> 



More information about the Mercurial-devel mailing list