Patchwork [4,of,4] log: add TODO comments about --line-range processing

login
register
mail settings
Submitter Yuya Nishihara
Date Feb. 12, 2018, 1:05 a.m.
Message ID <3dd9becb635b9e077b53.1518397500@mimosa>
Download mbox | patch
Permalink /patch/27603/
State New
Headers show

Comments

Yuya Nishihara - Feb. 12, 2018, 1:05 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1516521882 -32400
#      Sun Jan 21 17:04:42 2018 +0900
# Node ID 3dd9becb635b9e077b53f140c65f349cb0776991
# Parent  e5ba72e0361bb859b1688ab1bc620c6ca870d951
log: add TODO comments about --line-range processing
Denis Laxalde - Feb. 12, 2018, 7:20 a.m.
The series looks good to me, thanks for doing that work!

Just a few notes about these todos.

Yuya Nishihara a écrit :
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1516521882 -32400
> #      Sun Jan 21 17:04:42 2018 +0900
> # Node ID 3dd9becb635b9e077b53f140c65f349cb0776991
> # Parent  e5ba72e0361bb859b1688ab1bc620c6ca870d951
> log: add TODO comments about --line-range processing
> 
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -3414,6 +3414,7 @@ def log(ui, repo, *pats, **opts):
>           raise error.Abort(_('--line-range requires --follow'))
>   
>       if linerange and pats:
> +        # TODO: take pats as patterns with no line-range filter

Do you mean handling "--line-range file1,from:to file2", where "file2" 
would be "a pattern with no line-range filter"?

>           raise error.Abort(
>               _('FILE arguments are not compatible with --line-range option')
>           )
> @@ -3421,6 +3422,8 @@ def log(ui, repo, *pats, **opts):
>       repo = scmutil.unhidehashlikerevs(repo, opts.get('rev'), 'nowarn')
>       revs, differ = logcmdutil.getrevs(repo, pats, opts)
>       if linerange:
> +        # TODO: should follow file history from logcmdutil._initialrevs(),
> +        # then filter the result by logcmdutil._makerevset() and --limit

I remember having tried something like that earlier but it got too 
complicated. Maybe your recent refactorings would make this easier now.

>           revs, differ = logcmdutil.getlinerangerevs(repo, revs, opts)
>   
>       getrenamed = None
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - Feb. 12, 2018, 10:34 a.m.
On Mon, 12 Feb 2018 08:20:29 +0100, Denis Laxalde wrote:
> >       if linerange and pats:
> > +        # TODO: take pats as patterns with no line-range filter
> 
> Do you mean handling "--line-range file1,from:to file2", where "file2" 
> would be "a pattern with no line-range filter"?

Yes. I think that is the only useful interpretation.

> > @@ -3421,6 +3422,8 @@ def log(ui, repo, *pats, **opts):
> >       repo = scmutil.unhidehashlikerevs(repo, opts.get('rev'), 'nowarn')
> >       revs, differ = logcmdutil.getrevs(repo, pats, opts)
> >       if linerange:
> > +        # TODO: should follow file history from logcmdutil._initialrevs(),
> > +        # then filter the result by logcmdutil._makerevset() and --limit
> 
> I remember having tried something like that earlier but it got too
> complicated. Maybe your recent refactorings would make this easier now.

I hope so. Currently --limit doesn't work as expected.

This is totally unrelated topic, but how would we do if we want to support
non-contiguous range?

  -L file,a:b,c:d

is ambiguous because file may contain ",".
Denis Laxalde - Feb. 12, 2018, 11:04 a.m.
Yuya Nishihara a écrit :
> This is totally unrelated topic, but how would we do if we want to support
> non-contiguous range?
> 
>   -L file,a:b,c:d
> 
> is ambiguous because file may contain ",".

I guess we could iteratively rsplit(",", 1) the file pattern and try to
parse "from:to" until it fails, meaning that the remaining would be the
file name. Slightly more complicated than it is now, but still doable I
think.
Yuya Nishihara - Feb. 12, 2018, 12:28 p.m.
On Mon, 12 Feb 2018 12:04:21 +0100, Denis Laxalde wrote:
> Yuya Nishihara a écrit :
> > This is totally unrelated topic, but how would we do if we want to support
> > non-contiguous range?
> > 
> >   -L file,a:b,c:d
> > 
> > is ambiguous because file may contain ",".
> 
> I guess we could iteratively rsplit(",", 1) the file pattern and try to
> parse "from:to" until it fails, meaning that the remaining would be the
> file name. Slightly more complicated than it is now, but still doable I
> think.

That doesn't sound nice because only reason we've introduced -L was to reliably
split linerange from file pattern. "1:2" looks odd, but is a valid filename on
non-Windows.
Denis Laxalde - Feb. 12, 2018, 12:42 p.m.
Yuya Nishihara a écrit :
> On Mon, 12 Feb 2018 12:04:21 +0100, Denis Laxalde wrote:
>> Yuya Nishihara a écrit :
>>> This is totally unrelated topic, but how would we do if we want to support
>>> non-contiguous range?
>>>
>>>   -L file,a:b,c:d
>>>
>>> is ambiguous because file may contain ",".
>>
>> I guess we could iteratively rsplit(",", 1) the file pattern and try to
>> parse "from:to" until it fails, meaning that the remaining would be the
>> file name. Slightly more complicated than it is now, but still doable I
>> think.
> 
> That doesn't sound nice because only reason we've introduced -L was to reliably
> split linerange from file pattern. "1:2" looks odd, but is a valid filename on
> non-Windows.

I'm afraid I don't follow, could you elaborate a bit?

By the way, I worked on this a bit earlier, just after your first email
today:

  https://hg.logilab.org/users/dlaxalde/hg/rev/6d8c75041b21

It seems to be working, but I may have missed something of course.
Yuya Nishihara - Feb. 12, 2018, 2:24 p.m.
On Mon, 12 Feb 2018 13:42:54 +0100, Denis Laxalde wrote:
> Yuya Nishihara a écrit :
> > On Mon, 12 Feb 2018 12:04:21 +0100, Denis Laxalde wrote:
> >> Yuya Nishihara a écrit :
> >>> This is totally unrelated topic, but how would we do if we want to support
> >>> non-contiguous range?
> >>>
> >>>   -L file,a:b,c:d
> >>>
> >>> is ambiguous because file may contain ",".
> >>
> >> I guess we could iteratively rsplit(",", 1) the file pattern and try to
> >> parse "from:to" until it fails, meaning that the remaining would be the
> >> file name. Slightly more complicated than it is now, but still doable I
> >> think.
> > 
> > That doesn't sound nice because only reason we've introduced -L was to reliably
> > split linerange from file pattern. "1:2" looks odd, but is a valid filename on
> > non-Windows.
> 
> I'm afraid I don't follow, could you elaborate a bit?
> 
> By the way, I worked on this a bit earlier, just after your first email
> today:
> 
>   https://hg.logilab.org/users/dlaxalde/hg/rev/6d8c75041b21
> 
> It seems to be working, but I may have missed something of course.

I meant "1:2,3:4,5:6" could be parsed as either

 file="1:2,3:4" linerange="5:6"

or

 file="1:2" linerange="3:4,5:6"

because "1:2,3:4" is a valid filename. This would get more complicated if
we want to support other types of linerange specifier seen in git.
Denis Laxalde - Feb. 12, 2018, 8:32 p.m.
Yuya Nishihara a écrit :
> On Mon, 12 Feb 2018 13:42:54 +0100, Denis Laxalde wrote:
>> Yuya Nishihara a écrit :
>>> On Mon, 12 Feb 2018 12:04:21 +0100, Denis Laxalde wrote:
>>>> Yuya Nishihara a écrit :
>>>>> This is totally unrelated topic, but how would we do if we want to support
>>>>> non-contiguous range?
>>>>>
>>>>>    -L file,a:b,c:d
>>>>>
>>>>> is ambiguous because file may contain ",".
>>>>
>>>> I guess we could iteratively rsplit(",", 1) the file pattern and try to
>>>> parse "from:to" until it fails, meaning that the remaining would be the
>>>> file name. Slightly more complicated than it is now, but still doable I
>>>> think.
>>>
>>> That doesn't sound nice because only reason we've introduced -L was to reliably
>>> split linerange from file pattern. "1:2" looks odd, but is a valid filename on
>>> non-Windows.
>>
>> I'm afraid I don't follow, could you elaborate a bit?
>>
>> By the way, I worked on this a bit earlier, just after your first email
>> today:
>>
>>    https://hg.logilab.org/users/dlaxalde/hg/rev/6d8c75041b21
>>
>> It seems to be working, but I may have missed something of course.
> 
> I meant "1:2,3:4,5:6" could be parsed as either
> 
>   file="1:2,3:4" linerange="5:6"
> 
> or
> 
>   file="1:2" linerange="3:4,5:6"
> 
> because "1:2,3:4" is a valid filename. This would get more complicated if
> we want to support other types of linerange specifier seen in git.

Ok, I see. Any separation character (apart from / maybe) would lead to 
such ambiguities I guess. Maybe we can live without this and specify -L 
multiple times? I have no better idea at the moment.
Yuya Nishihara - Feb. 13, 2018, 11:56 a.m.
On Mon, 12 Feb 2018 21:32:15 +0100, Denis Laxalde wrote:
> Yuya Nishihara a écrit :
> > On Mon, 12 Feb 2018 13:42:54 +0100, Denis Laxalde wrote:
> >> Yuya Nishihara a écrit :
> >>> On Mon, 12 Feb 2018 12:04:21 +0100, Denis Laxalde wrote:
> >>>> Yuya Nishihara a écrit :
> >>>>> This is totally unrelated topic, but how would we do if we want to support
> >>>>> non-contiguous range?
> >>>>>
> >>>>>    -L file,a:b,c:d
> >>>>>
> >>>>> is ambiguous because file may contain ",".
> >>>>
> >>>> I guess we could iteratively rsplit(",", 1) the file pattern and try to
> >>>> parse "from:to" until it fails, meaning that the remaining would be the
> >>>> file name. Slightly more complicated than it is now, but still doable I
> >>>> think.
> >>>
> >>> That doesn't sound nice because only reason we've introduced -L was to reliably
> >>> split linerange from file pattern. "1:2" looks odd, but is a valid filename on
> >>> non-Windows.
> >>
> >> I'm afraid I don't follow, could you elaborate a bit?
> >>
> >> By the way, I worked on this a bit earlier, just after your first email
> >> today:
> >>
> >>    https://hg.logilab.org/users/dlaxalde/hg/rev/6d8c75041b21
> >>
> >> It seems to be working, but I may have missed something of course.
> > 
> > I meant "1:2,3:4,5:6" could be parsed as either
> > 
> >   file="1:2,3:4" linerange="5:6"
> > 
> > or
> > 
> >   file="1:2" linerange="3:4,5:6"
> > 
> > because "1:2,3:4" is a valid filename. This would get more complicated if
> > we want to support other types of linerange specifier seen in git.
> 
> Ok, I see. Any separation character (apart from / maybe) would lead to 
> such ambiguities I guess.

As long as we stick to a comma as both file-linerange and linerange-linerange
separators.

> Maybe we can live without this and specify -L
> multiple times? I have no better idea at the moment.

Seems fine.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -3414,6 +3414,7 @@  def log(ui, repo, *pats, **opts):
         raise error.Abort(_('--line-range requires --follow'))
 
     if linerange and pats:
+        # TODO: take pats as patterns with no line-range filter
         raise error.Abort(
             _('FILE arguments are not compatible with --line-range option')
         )
@@ -3421,6 +3422,8 @@  def log(ui, repo, *pats, **opts):
     repo = scmutil.unhidehashlikerevs(repo, opts.get('rev'), 'nowarn')
     revs, differ = logcmdutil.getrevs(repo, pats, opts)
     if linerange:
+        # TODO: should follow file history from logcmdutil._initialrevs(),
+        # then filter the result by logcmdutil._makerevset() and --limit
         revs, differ = logcmdutil.getlinerangerevs(repo, revs, opts)
 
     getrenamed = None