Patchwork Proposing compact graph log

login
register
mail settings
Submitter Santiago Payà i Miralta
Date June 1, 2014, 8:58 p.m.
Message ID <CABmqNJUkY16gHr+Wu47DPoXEL5UAH70Ne2DhvxfzUhMwy1YTGw@mail.gmail.com>
Download mbox | patch
Permalink /patch/4910/
State Changes Requested
Headers show

Comments

Santiago Payà i Miralta - June 1, 2014, 8:58 p.m.
Hello,

I would ask for a review of the --graph log option. When using --graph with
a --template the resulting DAG chain always shows the changesets connected
by a row of vertical edges '|', regardless of using '\n' or not in the
--template parameter:

$ hg log -G --template={rev} -l 3
@  21644
|
o  21643
|
o  21642
|

And what is proposed is:

$ hg log -G --template={rev} -l 3
@  21644
o  21643
o  21642

You can see my point of view detailed in this blog [1].

There are some problems with this change:

* Breaks some (57) tests :(
* Could depend from an option to the log command, some as --compact or
similar, in this way the previous behavior would not be broken.
* Works filtering the resulting graph, may be should work in the graph
generation (?). About the code be aware that, in my opinion, the comment in
line graphmod:327[2] is not true, the `shift_interline' list contains all
non-vertical _and_ vertical edges.

Well, this is just a proposal that if deemed correct should be developed.
Was it discussed at some point when the graphlog extension ?

Best regards,

Santiago

[1] http://print--hello--world.blogspot.com.es/2014_05_01_archive.html
[2] http://selenic.com/hg/file/9c35f3a8cac4/mercurial/graphmod.py#l327

- - - -
Mads Kiilerich - June 1, 2014, 10:22 p.m.
On 06/01/2014 10:58 PM, Santiago Payà i Miralta wrote:
>
> Hello,
>
> I would ask for a review of the --graph log option. When using --graph 
> with a --template the resulting DAG chain always shows the changesets 
> connected by a row of vertical edges '|', regardless of using '\n' or 
> not in the --template parameter:
>
> $ hg log -G --template={rev} -l 3
> @  21644
> |
> o  21643
> |
> o  21642
> |
>
> And what is proposed is:
>
> $ hg log -G --template={rev} -l 3
> @  21644
> o  21643
> o  21642
>

The example shows the trivial case. The interesting case is the 
non-trivial one where you also have non-vertical lines - that is what 
ultimately should be in the patch description.

> You can see my point of view detailed in this blog [1].
>
> There are some problems with this change:
>
> * Breaks some (57) tests :(
>

It would also break existing uses of Mercurial. I doubt it can be 
changed now. The default must stay the same.

> * Could depend from an option to the log command, some as --compact or 
> similar, in this way the previous behavior would not be broken.
>

Yes, it could perhaps be added to Mercurial if controlled this way.

> * Works filtering the resulting graph, may be should work in the graph 
> generation (?).
>

Yes. Probably just by looking at coldiff.

> About the code be aware that, in my opinion, the comment in line 
> graphmod:327[2] is not true, the `shift_interline' list contains all 
> non-vertical _and_ vertical edges.
>

The comment doesn't say that there is no other edges. It just says that 
the line is there to handle the non-vertical edges - that is what makes 
it special.

> Well, this is just a proposal that if deemed correct should be 
> developed. Was it discussed at some point when the graphlog extension ?
>

I would be -0.1 for making the output more inconsistent and another -0.1 
for adding another option ... and to that I would add +0.2 because I can 
see how it could be convenient even though it is harder to read.

I suggest looking at 
http://mercurial.selenic.com/wiki/ContributingChanges and contributing a 
full patch so we know exactly what we are discussing.

A second patch could perhaps also skip the extra padding lines in this 
compact mode.

/Mads


> Best regards,
>
> Santiago
>
> [1] http://print--hello--world.blogspot.com.es/2014_05_01_archive.html
> [2] http://selenic.com/hg/file/9c35f3a8cac4/mercurial/graphmod.py#l327
>
> - - - -
>
> diff -r 76a347bcdb33 -r 9c5a8e65386a mercurial/graphmod.py
> --- a/mercurial/graphmod.py Thu May 29 16:01:39 2014 -0700
> +++ b/mercurial/graphmod.py Fri May 30 22:09:31 2014 +0200
> @@ -346,7 +346,14 @@
>      lines = [nodeline]
>      if add_padding_line:
>          lines.append(_getpaddingline(idx, ncols, edges))
> -    lines.append(shift_interline)
> +
> +    # do not add shift_interline if are all vertical edges '|'
> +    add_shift_interline = False
> +    for token in shift_interline:
> +        if token not in ['|', ' ']:
> +            add_shift_interline = True
> +    if add_shift_interline:
> +        lines.append(shift_interline)
>

It could probably just be something like

if util.any(c not in '| ' for c in shift_interline):
     lines.append(shift_interline)

/Mads
Greg Ward - June 2, 2014, 12:05 p.m.
On 01 June 2014, Santiago Payà i Miralta said:
> Hello,
> 
> I would ask for a review of the --graph log option. When using --graph with
> a --template the resulting DAG chain always shows the changesets connected
> by a row of vertical edges '|', regardless of using '\n' or not in the
> --template parameter:
> 
> $ hg log -G --template={rev} -l 3
> @  21644
> |
> o  21643
> |
> o  21642
> |
> 
> And what is proposed is:
> 
> $ hg log -G --template={rev} -l 3
> @  21644
> o  21643
> o  21642
> 
> You can see my point of view detailed in this blog [1].

That blog post doesn't really explain things, it just shows a bunch of
examples. You'll get better feedback if you 1) explain exactly what
you want to change and 2) provide a patch.

That said, backwards incompatible changes to Mercurial are not done
lightly. You might be better off writing an extension to show to the
world that your way is better.

       Greg
Gregory Szorc - June 2, 2014, 4:16 p.m.
On 6/1/14, 1:58 PM, Santiago Payà i Miralta wrote:
> Hello,
>
> I would ask for a review of the --graph log option. When using --graph
> with a --template the resulting DAG chain always shows the changesets
> connected by a row of vertical edges '|', regardless of using '\n' or
> not in the --template parameter:
>
> $ hg log -G --template={rev} -l 3
> @  21644
> |
> o  21643
> |
> o  21642
> |
>
> And what is proposed is:
>
> $ hg log -G --template={rev} -l 3
> @  21644
> o  21643
> o  21642
>
> You can see my point of view detailed in this blog [1].
>
> There are some problems with this change:
>
> * Breaks some (57) tests :(
> * Could depend from an option to the log command, some as --compact or
> similar, in this way the previous behavior would not be broken.
> * Works filtering the resulting graph, may be should work in the graph
> generation (?). About the code be aware that, in my opinion, the comment
> in line graphmod:327[2] is not true, the `shift_interline' list contains
> all non-vertical _and_ vertical edges.
>
> Well, this is just a proposal that if deemed correct should be
> developed. Was it discussed at some point when the graphlog extension ?
>
> Best regards,
>
> Santiago
>
> [1] http://print--hello--world.blogspot.com.es/2014_05_01_archive.html
> [2] http://selenic.com/hg/file/9c35f3a8cac4/mercurial/graphmod.py#l327

I've long wanted a more compact graph view like you've proposed. The 
examples on your blog are exactly what I'm looking for!

I hear the concerns that this breaks backwards compatibility. 
Personally, I'm not sure why machines would attempt to parse the 
graphical output. Surely any machine that cares about the graph 
structure of a repository is parsing the output of a custom template 
that prints {parents} or some such? But, backwards compatibility is 
backwards compatibility. Perhaps we should land this behind a config flag.
Santiago Payà i Miralta - June 2, 2014, 10:10 p.m.
Hello,

Many thanks for your replies and guidance. If I not misunderstood I would
have to:

* Open an entry in bugzilla (feature) explaining in detail the intentions
of this contribution.

* Trying to write a patch that does not break the backwards compatibility
nor tests. With an option to the log command or with a config flag in hgrc.

Some code to have fun :)

Best regards,

Santiago


On Mon, Jun 2, 2014 at 6:16 PM, Gregory Szorc <gregory.szorc@gmail.com>
wrote:

> On 6/1/14, 1:58 PM, Santiago Payà i Miralta wrote:
>
>> Hello,
>>
>> I would ask for a review of the --graph log option. When using --graph
>> with a --template the resulting DAG chain always shows the changesets
>> connected by a row of vertical edges '|', regardless of using '\n' or
>> not in the --template parameter:
>>
>> $ hg log -G --template={rev} -l 3
>> @  21644
>> |
>> o  21643
>> |
>> o  21642
>> |
>>
>> And what is proposed is:
>>
>> $ hg log -G --template={rev} -l 3
>> @  21644
>> o  21643
>> o  21642
>>
>> You can see my point of view detailed in this blog [1].
>>
>> There are some problems with this change:
>>
>> * Breaks some (57) tests :(
>> * Could depend from an option to the log command, some as --compact or
>> similar, in this way the previous behavior would not be broken.
>> * Works filtering the resulting graph, may be should work in the graph
>> generation (?). About the code be aware that, in my opinion, the comment
>> in line graphmod:327[2] is not true, the `shift_interline' list contains
>> all non-vertical _and_ vertical edges.
>>
>> Well, this is just a proposal that if deemed correct should be
>> developed. Was it discussed at some point when the graphlog extension ?
>>
>> Best regards,
>>
>> Santiago
>>
>> [1] http://print--hello--world.blogspot.com.es/2014_05_01_archive.html
>> [2] http://selenic.com/hg/file/9c35f3a8cac4/mercurial/graphmod.py#l327
>>
>
> I've long wanted a more compact graph view like you've proposed. The
> examples on your blog are exactly what I'm looking for!
>
> I hear the concerns that this breaks backwards compatibility. Personally,
> I'm not sure why machines would attempt to parse the graphical output.
> Surely any machine that cares about the graph structure of a repository is
> parsing the output of a custom template that prints {parents} or some such?
> But, backwards compatibility is backwards compatibility. Perhaps we should
> land this behind a config flag.
>
Mads Kiilerich - June 2, 2014, 10:24 p.m.
On 06/03/2014 12:10 AM, Santiago Payà i Miralta wrote:
> Hello,
>
> Many thanks for your replies and guidance. If I not misunderstood I 
> would have to:
>
> * Open an entry in bugzilla (feature) explaining in detail the 
> intentions of this contribution.

No. The intentions should be in the patch description.

>
> * Trying to write a patch that does not break the backwards 
> compatibility nor tests. With an option to the log command or with a 
> config flag in hgrc.

No config flag, please. If someone wants this by default, they should 
define an alias like "logg = log -G --compact" and use that.

... unless you can convince "us" that it should the default. Perhaps 
controlled indirectly by whether the template string (doesn't) contain a 
newline.

/Mads
Greg Ward - June 2, 2014, 11:10 p.m.
On 03 June 2014, Santiago Payà i Miralta said:
> Hello,
> 
> Many thanks for your replies and guidance. If I not misunderstood I would
> have to:
> 
> * Open an entry in bugzilla (feature) explaining in detail the intentions
> of this contribution.

No need for a Bugzilla entry. In fact, that's probably the worst place
to propose a new feature. This mailing list right here is the best
place to propose and discuss new features. It's where you'll get the
most informed audience and best feedback.

> * Trying to write a patch that does not break the backwards compatibility
> nor tests. With an option to the log command or with a config flag in hgrc.

Treating templates without \n more consistently would be nice. I'm not
sure how to do it sanely though. If you can figure it out, great!

Also, I agree that backwards compatibility is probably not a huge
concern here. Anyone parsing "log -G" output is doing it wrong. Very,
very wrong. ;-)

       Greg

Patch

diff -r 76a347bcdb33 -r 9c5a8e65386a mercurial/graphmod.py
--- a/mercurial/graphmod.py Thu May 29 16:01:39 2014 -0700
+++ b/mercurial/graphmod.py Fri May 30 22:09:31 2014 +0200
@@ -346,7 +346,14 @@ 
     lines = [nodeline]
     if add_padding_line:
         lines.append(_getpaddingline(idx, ncols, edges))
-    lines.append(shift_interline)
+
+    # do not add shift_interline if are all vertical edges '|'
+    add_shift_interline = False
+    for token in shift_interline:
+        if token not in ['|', ' ']:
+            add_shift_interline = True
+    if add_shift_interline:
+        lines.append(shift_interline)

     # make sure that there are as many graph lines as there are
     # log strings