Patchwork graphlog: draw multiple edges towards null node (issue5440)

login
register
mail settings
Submitter Yuya Nishihara
Date March 20, 2017, 8:22 a.m.
Message ID <db70a30dde5182e704b0.1489998157@mimosa>
Download mbox | patch
Permalink /patch/19472/
State Accepted
Headers show

Comments

Yuya Nishihara - March 20, 2017, 8:22 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1489978255 -32400
#      Mon Mar 20 11:50:55 2017 +0900
# Node ID db70a30dde5182e704b02b0e47a79d3ecfafdd49
# Parent  cede8eaf55e2fe68434c273cfd066512b5ca112e
graphlog: draw multiple edges towards null node (issue5440)

Before, edge (r, null) was processed only once by newparents. However what
we really need is just stripping the edge (null, null).
Ryan McElroy - March 20, 2017, 11:24 a.m.
On 3/20/17 8:22 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1489978255 -32400
> #      Mon Mar 20 11:50:55 2017 +0900
> # Node ID db70a30dde5182e704b02b0e47a79d3ecfafdd49
> # Parent  cede8eaf55e2fe68434c273cfd066512b5ca112e
> graphlog: draw multiple edges towards null node (issue5440)
>
> Before, edge (r, null) was processed only once by newparents. However what
> we really need is just stripping the edge (null, null).
>
> diff --git a/mercurial/graphmod.py b/mercurial/graphmod.py
> --- a/mercurial/graphmod.py
> +++ b/mercurial/graphmod.py
> @@ -182,6 +182,9 @@ def asciiedges(type, char, lines, state,
>       knownparents = []
>       newparents = []
>       for ptype, parent in parents:
> +        if parent == rev:
> +            # self reference (should only be seen in null rev)
> +            continue
>           if parent in seen:
>               knownparents.append(parent)
>           else:
> @@ -191,8 +194,7 @@ def asciiedges(type, char, lines, state,
>       ncols = len(seen)
>       nextseen = seen[:]
>       nextseen[nodeidx:nodeidx + 1] = newparents
> -    edges = [(nodeidx, nextseen.index(p))
> -             for p in knownparents if p != nullrev]
> +    edges = [(nodeidx, nextseen.index(p)) for p in knownparents]
>   
>       seen[:] = nextseen
>       while len(newparents) > 2:
> diff --git a/tests/test-glog.t b/tests/test-glog.t
> --- a/tests/test-glog.t
> +++ b/tests/test-glog.t
> @@ -3424,3 +3424,39 @@ the right node styles are used (issue517
>        summary:     0
>     
>   
> +  $ cd ..
> +
> +Multiple roots (issue5440):
> +
> +  $ hg init multiroots
> +  $ cd multiroots
> +  $ cat <<EOF > .hg/hgrc
> +  > [ui]
> +  > logtemplate = '{rev} {desc}\n\n'
> +  > EOF
> +
> +  $ touch foo
> +  $ hg ci -Aqm foo
> +  $ hg co -q null
> +  $ touch bar
> +  $ hg ci -Aqm bar
> +
> +  $ hg log -Gr null:
> +  @  1 bar
> +  |
> +  | o  0 foo
> +  |/
> +  o  -1
> +
> +  $ hg log -Gr null+0
> +  o  0 foo
> +  |
> +  o  -1
> +
> +  $ hg log -Gr null+1
> +  @  1 bar
> +  |
> +  o  -1
> +
> +
> +  $ cd ..

I'd like to re-state for the record my wish that for subtle bugfixes 
like this that we could split the fix into two patches: a patch that 
adds a test exhibiting the broken behavior, and then the fix with the 
corrections to the test. That would make it *so much easier* to review a 
patch like this.

For the curious (like me), the previous (broken) behavior was this (note 
the missing /):

    @  1 bar
    |
    | o  0 foo
|
    o  -1

The difference in the test output with this patch is thus:

    @  1 bar
    |
    | o  0 foo
-  |
+  |/
    o  -1

Overall, this change looks good to me.
Augie Fackler - March 21, 2017, 9:33 p.m.
On Mon, Mar 20, 2017 at 11:24:45AM +0000, Ryan McElroy wrote:
> On 3/20/17 8:22 AM, Yuya Nishihara wrote:
> ># HG changeset patch
> ># User Yuya Nishihara <yuya@tcha.org>
> ># Date 1489978255 -32400
> >#      Mon Mar 20 11:50:55 2017 +0900
> ># Node ID db70a30dde5182e704b02b0e47a79d3ecfafdd49
> ># Parent  cede8eaf55e2fe68434c273cfd066512b5ca112e
> >graphlog: draw multiple edges towards null node (issue5440)

queued, thanks

> >
> >Before, edge (r, null) was processed only once by newparents. However what
> >we really need is just stripping the edge (null, null).
> >
>

[...]

> I'd like to re-state for the record my wish that for subtle bugfixes like
> this that we could split the fix into two patches: a patch that adds a test
> exhibiting the broken behavior, and then the fix with the corrections to the
> test. That would make it *so much easier* to review a patch like this.

+1

>
> For the curious (like me), the previous (broken) behavior was this (note the
> missing /):
>
>    @  1 bar
>    |
>    | o  0 foo
> |
>    o  -1
>
> The difference in the test output with this patch is thus:
>
>    @  1 bar
>    |
>    | o  0 foo
> -  |
> +  |/
>    o  -1
>
> Overall, this change looks good to me.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/graphmod.py b/mercurial/graphmod.py
--- a/mercurial/graphmod.py
+++ b/mercurial/graphmod.py
@@ -182,6 +182,9 @@  def asciiedges(type, char, lines, state,
     knownparents = []
     newparents = []
     for ptype, parent in parents:
+        if parent == rev:
+            # self reference (should only be seen in null rev)
+            continue
         if parent in seen:
             knownparents.append(parent)
         else:
@@ -191,8 +194,7 @@  def asciiedges(type, char, lines, state,
     ncols = len(seen)
     nextseen = seen[:]
     nextseen[nodeidx:nodeidx + 1] = newparents
-    edges = [(nodeidx, nextseen.index(p))
-             for p in knownparents if p != nullrev]
+    edges = [(nodeidx, nextseen.index(p)) for p in knownparents]
 
     seen[:] = nextseen
     while len(newparents) > 2:
diff --git a/tests/test-glog.t b/tests/test-glog.t
--- a/tests/test-glog.t
+++ b/tests/test-glog.t
@@ -3424,3 +3424,39 @@  the right node styles are used (issue517
      summary:     0
   
 
+  $ cd ..
+
+Multiple roots (issue5440):
+
+  $ hg init multiroots
+  $ cd multiroots
+  $ cat <<EOF > .hg/hgrc
+  > [ui]
+  > logtemplate = '{rev} {desc}\n\n'
+  > EOF
+
+  $ touch foo
+  $ hg ci -Aqm foo
+  $ hg co -q null
+  $ touch bar
+  $ hg ci -Aqm bar
+
+  $ hg log -Gr null:
+  @  1 bar
+  |
+  | o  0 foo
+  |/
+  o  -1
+  
+  $ hg log -Gr null+0
+  o  0 foo
+  |
+  o  -1
+  
+  $ hg log -Gr null+1
+  @  1 bar
+  |
+  o  -1
+  
+
+  $ cd ..