| 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
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
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
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.
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. >
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
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
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 - - - -