Patchwork context: follow all branches in blockdescendants()

login
register
mail settings
Submitter Denis Laxalde
Date April 14, 2017, 6:57 a.m.
Message ID <0bed2dff50d5bc32820c.1492153041@marimba>
Download mbox | patch
Permalink /patch/20193/
State Accepted
Headers show

Comments

Denis Laxalde - April 14, 2017, 6:57 a.m.
# HG changeset patch
# User Denis Laxalde <denis@laxalde.org>
# Date 1492152918 -7200
#      Fri Apr 14 08:55:18 2017 +0200
# Node ID 0bed2dff50d5bc32820c973b7057f32dd97dd89d
# Parent  f23d579a5a04e44f5e0370ba13ad20dd626e8ad7
context: follow all branches in blockdescendants()

In the initial implementation of blockdescendants (and thus followlines(...,
descend=True) revset), only the first branch encountered in descending
direction was followed.

Update the algorithm so that all children of a revision ('x' in code) are
considered. Accordingly, we need to prevent a child revision to be yielded
multiple times when it gets visited through different path, so we skip 'i'
when this occurs. Finally, since we now consider all parents of a possible
child touching a given line range, we take care of yielding the child if it
has a diff in specified line range with at least one of its parent (same logic
as blockancestors()).
Yuya Nishihara - April 14, 2017, 11:07 a.m.
On Fri, 14 Apr 2017 08:57:21 +0200, Denis Laxalde wrote:
> # HG changeset patch
> # User Denis Laxalde <denis@laxalde.org>
> # Date 1492152918 -7200
> #      Fri Apr 14 08:55:18 2017 +0200
> # Node ID 0bed2dff50d5bc32820c973b7057f32dd97dd89d
> # Parent  f23d579a5a04e44f5e0370ba13ad20dd626e8ad7
> context: follow all branches in blockdescendants()

Looks better, queued, thanks.

A couple of comments follow.

> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -1216,17 +1216,21 @@ def blockdescendants(fctx, fromline, tol
>      fl = fctx.filelog()
>      seen = {fctx.filerev(): (fctx, (fromline, toline))}

Perhaps the start revision itself should be yielded if the file is changed
in that revision?

>      for i in fl.descendants([fctx.filerev()]):
> +        if i in seen:
> +            continue

I might be wrong, but I'm not sure how this would happen.

>          c = fctx.filectx(i)
> +        inrange = False
>          for x in fl.parentrevs(i):
>              try:
> -                p, linerange2 = seen.pop(x)
> +                p, linerange2 = seen[x]
>              except KeyError:
>                  # nullrev or other branch
>                  continue
> -            inrange, linerange1 = _changesrange(c, p, linerange2, diffopts)
> -            if inrange:
> -                yield c, linerange1
> +            inrangep, linerange1 = _changesrange(c, p, linerange2, diffopts)
> +            inrange = inrange or inrangep
>              seen[i] = c, linerange1

Does this mean the merge revision 'i' may have two separate lineranges coming
from 'x's, but one is lost?

> +        if inrange:
> +            yield c, linerange1
Denis Laxalde - April 14, 2017, 11:45 a.m.
Yuya Nishihara a écrit :
> On Fri, 14 Apr 2017 08:57:21 +0200, Denis Laxalde wrote:
>> # HG changeset patch
>> # User Denis Laxalde <denis@laxalde.org>
>> # Date 1492152918 -7200
>> #      Fri Apr 14 08:55:18 2017 +0200
>> # Node ID 0bed2dff50d5bc32820c973b7057f32dd97dd89d
>> # Parent  f23d579a5a04e44f5e0370ba13ad20dd626e8ad7
>> context: follow all branches in blockdescendants()
>
> Looks better, queued, thanks.
>
> A couple of comments follow.
>
>> diff --git a/mercurial/context.py b/mercurial/context.py
>> --- a/mercurial/context.py
>> +++ b/mercurial/context.py
>> @@ -1216,17 +1216,21 @@ def blockdescendants(fctx, fromline, tol
>>      fl = fctx.filelog()
>>      seen = {fctx.filerev(): (fctx, (fromline, toline))}
>
> Perhaps the start revision itself should be yielded if the file is changed
> in that revision?

Yes, that'd make sense since descendants() actually does that (didn't
noticed before).

>>      for i in fl.descendants([fctx.filerev()]):
>> +        if i in seen:
>> +            continue
>
> I might be wrong, but I'm not sure how this would happen.

Yes, that's right. It does not happen in tests at least.

>>          c = fctx.filectx(i)
>> +        inrange = False
>>          for x in fl.parentrevs(i):
>>              try:
>> -                p, linerange2 = seen.pop(x)
>> +                p, linerange2 = seen[x]
>>              except KeyError:
>>                  # nullrev or other branch
>>                  continue
>> -            inrange, linerange1 = _changesrange(c, p, linerange2, diffopts)
>> -            if inrange:
>> -                yield c, linerange1
>> +            inrangep, linerange1 = _changesrange(c, p, linerange2, diffopts)
>> +            inrange = inrange or inrangep
>>              seen[i] = c, linerange1
>
> Does this mean the merge revision 'i' may have two separate lineranges coming
> from 'x's, but one is lost?

Yes. I can add:

   assert i not in seen or seen[i] == (c, linerange1)

before overriding seen[i] (this happens with revision 20 in tests). If
the assertion fails (it doesn't in current tests), that would indicate a
problem in the algorithm instead of maybe producing wrong results. What
do you think?

>> +        if inrange:
>> +            yield c, linerange1

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1216,17 +1216,21 @@  def blockdescendants(fctx, fromline, tol
     fl = fctx.filelog()
     seen = {fctx.filerev(): (fctx, (fromline, toline))}
     for i in fl.descendants([fctx.filerev()]):
+        if i in seen:
+            continue
         c = fctx.filectx(i)
+        inrange = False
         for x in fl.parentrevs(i):
             try:
-                p, linerange2 = seen.pop(x)
+                p, linerange2 = seen[x]
             except KeyError:
                 # nullrev or other branch
                 continue
-            inrange, linerange1 = _changesrange(c, p, linerange2, diffopts)
-            if inrange:
-                yield c, linerange1
+            inrangep, linerange1 = _changesrange(c, p, linerange2, diffopts)
+            inrange = inrange or inrangep
             seen[i] = c, linerange1
+        if inrange:
+            yield c, linerange1
 
 class committablectx(basectx):
     """A committablectx object provides common functionality for a context that
diff --git a/tests/test-annotate.t b/tests/test-annotate.t
--- a/tests/test-annotate.t
+++ b/tests/test-annotate.t
@@ -600,7 +600,28 @@  we are missing the branch with rename wh
   $ hg log -T '{rev}: {desc}\n' -r 'followlines(baz, 5:7, startrev=25, descend=True)'
   26: baz:3+->3-
 
+we follow all branches in descending direction
+  $ hg up 22 --quiet
+  $ sed 's/3/+3/' baz > baz.new
+  $ mv baz.new baz
+  $ hg ci -m 'baz:3->+3'
+  created new head
+  $ hg log -T '{rev}: {desc}\n' -r 'followlines(baz, 3:5, startrev=16, descend=True)' --graph
+  @  29: baz:3->+3
+  :
+  : o  26: baz:3+->3-
+  : :
+  : o  23: baz:3->3+
+  :/
+  o    20: baz:4
+  |\
+  | ~
+  o  19: baz:3
+  |
+  ~
+
 check error cases
+  $ hg up 23 --quiet
   $ hg log -r 'followlines()'
   hg: parse error: followlines takes at least 1 positional arguments
   [255]