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

[Denis Laxalde]

Yuya Nishihara a écrit :
> On Wed, 04 Oct 2017 17:04:02 +0200, Denis Laxalde wrote:
>> # HG changeset patch
>> # User Denis Laxalde <denis.laxalde at logilab.fr>
>> # Date 1507127733 -7200
>> #      Wed Oct 04 16:35:33 2017 +0200
>> # Node ID 0f2d8b304223a8d00163f917fdc18082a88bceae
>> # Parent  9274bdbba0f913672305ed3d91c7e05f38d1eab7
>> # Available At http://hg.logilab.org/users/dlaxalde/hg
>> #              hg pull http://hg.logilab.org/users/dlaxalde/hg -r 0f2d8b304223
>> # EXP-Topic followlines-cli
>> log: add -L/--line-range option to follow file history by line range
>>
>> 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 or
>> regular file pattern in a meaningful way. It also respects --follow semantics.
>> 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.
> 
> Nit: we use ':' in revset.
> 
>> The implementation spreads between commands.log() and cmdutil module.
>> In commands.log(), the main loop now works with tuples of (rev, matchfn,
>> lineranges), where 'rev' and 'matchfn' are similar to before except that
>> 'matchfn' is now built out the loop. Without any -L option, "lineranges" is
>> None. With a -L option, 'matchfn' is a new matcher function built from files
>> specified in -L option(s) and any other regular FILE pattern.
>>
>> The logic to build revisions from -L/--line-range options lives in
>> cmdutil.getloglineranges() and the resulting information is combined with
>> information from --rev option and file patterns in commands.log(). Options
>> --rev and file patterns act as restrictions to the revisions obtained by
>> following line-range history of files specified in -L/--line-range patterns.
> 
> [...]
> 
>> +def getloglineranges(repo, opts):
>> +    """Return a generator of tuples (rev, fpath, [line range]) obtained by
>> +    processing "line-range" log options and walking block ancestors of each
>> +    specified file/line-range.
>> +    """
>> +    wctx = repo[None]
>> +    follow = bool(opts.get('follow'))
>> +    # Two-levels map of "rev -> file path -> [line range]".
>> +    linerangesbyrev = {}
>> +    for fname, (fromline, toline) in _parselinerangelogopt(repo, opts):
>> +        fctx = wctx.filectx(fname)
>> +        for fctx, linerange in dagop.blockancestors(fctx, fromline, toline):
>> +            fpath = fctx.path()
>> +            if not follow and fpath != fname:
>> +                continue
>> +            linerangesbyrev.setdefault(
>> +                fctx.rev(), {}).setdefault(
>> +                    fpath, []).append(linerange)
>> +    return sorted(linerangesbyrev.iteritems(), reverse=follow)
> 
> --follow isn't the option to flip the order. I think --follow should be
> implied (or required) given how line-range works.

Okay, will do.

> 
>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> --- a/mercurial/commands.py
>> +++ b/mercurial/commands.py
>> @@ -3230,6 +3230,8 @@ def locate(ui, repo, *pats, **opts):
>>       ('k', 'keyword', [],
>>        _('do case-insensitive search for a given text'), _('TEXT')),
>>       ('r', 'rev', [], _('show the specified revision or revset'), _('REV')),
>> +    ('L', 'line-range', [], _('follow line range of specified file'),
>> +     _('FILE,RANGE')),
> 
> No idea if we like the short option -L.
> 
>> +    # Build an iterator of (rev, matcher, lineranges) tuples to be consumed
>> +    # later in the main loop.
>> +    if followlines:
>> +        # Filter revisions from --line-range patterns from those not in --rev
>> +        # option and combine any existing filematcher with the one built from
>> +        # "line range" patterns.
>> +        if filematcher is None:
>> +            def buildmatcher(files):
>> +                return scmutil.match(repo[None], files)
>> +        else:
>> +            def buildmatcher(files):
>> +                return scmutil.match(repo[None],
>> +                                     files + filematcher(None).files(),
>> +                                     default='path')
>> +        revsmatchandlineranges = (
>> +            (rev, buildmatcher(list(lrs)), lrs)
>> +            for rev, lrs in linerangesbyrev if rev in revs
>> +        )
> 
> I don't think it's intuitive that '-L PAT1' and 'PAT2' are AND-ed. (Well, I
> didn't carefully read the code, but probably PAT2 would be filtered by revs
> even though PATs are OR-ed.)
> 
> So I think -L PAT and plain PATs should be mutually exclusive. Alternatively,
> -L could be considered pseudo-pair of file pattern.
> 
>    $ hg log FILE1 -L RANGE1 FILE2 -L RANGE2
> 
> I prefer this over -L FILE,RANGE syntax.

I'm fine for making -L PAT and plain PATs mutually exclusive, it makes
sense.

I'm not sure I understand your proposal (how more specifically, how to
implement it). How should I make FILE1 and RANGE1 matching? Is there a
way to enforce a "-L RANGE" to be followed by a "FILE"? Or should I just
assuming that "opts['line_range']" and "pats" will have the same order
and length and zip on the two lists?

> 
>> -        displayer.show(ctx, copies=copies, matchfn=revmatchfn)
>> +        displayer.show(ctx, copies=copies, matchfn=matchfn,
>> +                       lineranges=lineranges)
> 
> Appears that passing matchfn adds extra '\n' in showpatch().
>