gitk changing line color for no reason after merge

gitk changing line color for no reason after merge

[Pavel Roskin]

Hello, Paul!

I have noticed that gitk changes the line color at a commit following a
merge (i.e. a commit with many parents).  I made this screenshot to
demonstrate the problem:

http://red-bean.com/proski/gitk/gitk.png

If you follow the leftmost line, you'll see how green becomes red, gray
becomes brown and red becomes blue.

I think it would be much better if line colors only change at
non-trivial nodes, i.e. those with more than one child or parent.

The fix is trivial:

diff --git a/gitk b/gitk
index f12b3ce..14ff570 100755
--- a/gitk
+++ b/gitk
@@ -770,7 +770,7 @@ proc assigncolor {id} {
 
     if [info exists colormap($id)] return
     set ncolors [llength $colors]
-    if {$nparents($id) <= 1 && $nchildren($id) == 1} {
+    if {$nchildren($id) == 1} {
 	set child [lindex $children($id) 0]
 	if {[info exists colormap($child)]
 	    && $nparents($child) == 1} {


I checked the history of gitk and could trace this code back to the
first version of gitk in the git repository.  Maybe it was intended this
way, but I think it would be better to fix it.

-- 
Regards,
Pavel Roskin

Re: gitk changing line color for no reason after merge

[Pavel Roskin]

Hello!

Sorry for replying to myself, but there is nobody else to reply to.

> I think it would be much better if line colors only change at
> non-trivial nodes, i.e. those with more than one child or parent.

I didn't realize that the colors correspond to nodes, not branches.
Every node has one color that is used for lines to all of its children.
It would be much better to assign colors to "branches" consisting of
individual lines connecting nodes, but changing that would require many
changes in gitk.

> diff --git a/gitk b/gitk
> index f12b3ce..14ff570 100755
> --- a/gitk
> +++ b/gitk
> @@ -770,7 +770,7 @@ proc assigncolor {id} {
>  
>      if [info exists colormap($id)] return
>      set ncolors [llength $colors]
> -    if {$nparents($id) <= 1 && $nchildren($id) == 1} {
> +    if {$nchildren($id) == 1} {
>  	set child [lindex $children($id) 0]
>  	if {[info exists colormap($child)]
>  	    && $nparents($child) == 1} {
> 
> 

I still stand behind this patch because it eliminates color changes on
the nodes that have exactly one child and parent.  $nparents($id) is
irrelevant here, because it characterizes the current node, but the code
decides whether the line should change color at the child node.

However, further changes to reduce color changes didn't produce nice
results for me.  If I try to keep one color running as long as possible,
I get branches of the same color because, as I said, gitk uses the same
color for connections to all children.  So, every node on the branch
spurs branching lines of the same color, which can intersect or run
side-by-side.

I can submit this patch formally, but I hope to get some comments first.

-- 
Regards,
Pavel Roskin

Re: gitk changing line color for no reason after merge

[Paul Mackerras]

Pavel Roskin writes:
> Hello!

> I didn't realize that the colors correspond to nodes, not branches.
> Every node has one color that is used for lines to all of its children.
> It would be much better to assign colors to "branches" consisting of
> individual lines connecting nodes, but changing that would require many

Why would that be better?

> > -    if {$nparents($id) <= 1 && $nchildren($id) == 1} {
> > +    if {$nchildren($id) == 1} {
> >  	set child [lindex $children($id) 0]
> >  	if {[info exists colormap($child)]
> >  	    && $nparents($child) == 1} {
> > 
> > 
> 
> I still stand behind this patch because it eliminates color changes on
> the nodes that have exactly one child and parent.  $nparents($id) is

Ummm... I don't see that you have changed anything for nodes that have
exactly one parent?

> However, further changes to reduce color changes didn't produce nice
> results for me.  If I try to keep one color running as long as possible,
> I get branches of the same color because, as I said, gitk uses the same
> color for connections to all children.  So, every node on the branch
> spurs branching lines of the same color, which can intersect or run
> side-by-side.

The colors are really just to make the lines visually distinct.  They
(the colors) have no semantic meaning.  (I did try coloring the lines
according to the committer, but I just ended up with an awful lot of
lines being the same color - corresponding to one Linus Torvalds. :)

Paul.

Re: gitk changing line color for no reason after merge

[Junio C Hamano]

Pavel Roskin <proski@gnu.org> writes:

> I still stand behind this patch because it eliminates color changes on
> the nodes that have exactly one child and parent.  $nparents($id) is
> irrelevant here, because it characterizes the current node, but the code
> decides whether the line should change color at the child node.

It all depends on what you are trying to achieve with colours.
Being a bit colour-challenged, I am not in a good position to
comment on how much gitk's use of different colours is helping
the readability, but I suspect not very much.  Use of too many
colours just makes things distracting.  Especially weaker
colours like light yellow are very hard to see on a white
background, at least to me.

Trying to differenciate "trunk" with "side branches" may be
sometimes useful in a small one-man project, but it quickly
breaks down once you start merging from all over the place,
since merges in distributed development do not have inherent
distinction between "trunk" and "side branches".

One thing it _could_ do is to assign different colours to edges
coming into a merge node, and match the colour of the edge that
leads to a parent to the colour in which the diff text from
that parent is shown.  Then the colouring becomes somewhat
meaningful.

Maybe gitk is already doing it, but it makes me suspect it
doesn't, to read the way the code initialises "$ctext tag conf
m$N" and uses the m$N tag for each diff output line, which is
done pretty much independently from the procedure that paint
edges (the one you touch in your patch).

Re: gitk changing line color for no reason after merge

[Pavel Roskin]

On Tue, 2006-02-07 at 02:04 -0800, Junio C Hamano wrote:
> Pavel Roskin <proski@gnu.org> writes:
> 
> > I still stand behind this patch because it eliminates color changes on
> > the nodes that have exactly one child and parent.  $nparents($id) is
> > irrelevant here, because it characterizes the current node, but the code
> > decides whether the line should change color at the child node.
> 
> It all depends on what you are trying to achieve with colours.

I'm trying to make it easier to follow a line.  It's easier if its color
is not changing, especially on trivial nodes (one parent, one child).

> Being a bit colour-challenged, I am not in a good position to
> comment on how much gitk's use of different colours is helping
> the readability, but I suspect not very much.  Use of too many
> colours just makes things distracting.  Especially weaker
> colours like light yellow are very hard to see on a white
> background, at least to me.

My point is not to use to many colors.  There is one case when gitk
changes line color for no reason, and that case should be fixed.  Since
the colors rotate as gitk picks them, extra color switch means that the
colors repeat more often.

By the way, there are some nice colors to add (lightblue darkcyan pink),
but let's use effectively what we've got.

> Trying to differenciate "trunk" with "side branches" may be
> sometimes useful in a small one-man project, but it quickly
> breaks down once you start merging from all over the place,
> since merges in distributed development do not have inherent
> distinction between "trunk" and "side branches".

It's not my point.  My point is to make it easier to follow the lines,
without making any assumptions about "real" branches.

> One thing it _could_ do is to assign different colours to edges
> coming into a merge node, and match the colour of the edge that
> leads to a parent to the colour in which the diff text from
> that parent is shown.  Then the colouring becomes somewhat
> meaningful.

Yes, that would be great.

> Maybe gitk is already doing it, but it makes me suspect it
> doesn't, to read the way the code initialises "$ctext tag conf
> m$N" and uses the m$N tag for each diff output line, which is
> done pretty much independently from the procedure that paint
> edges (the one you touch in your patch).

Correct.

To illustrate my point, I put some examples online:

http://red-bean.com/proski/gitk/gitk-before.png - what gitk is showing
now.  There are color changes on trivial nodes both below and above
merges.

http://red-bean.com/proski/gitk/gitk-after.png - with my patch.  Color
changes on trivial nodes only happen below merges.

http://red-bean.com/proski/gitk/gitk-ideal.png - made in GIMP.  Trivial
nodes never change line color, because it changes as soon as the line
forks.

-- 
Regards,
Pavel Roskin

Re: gitk changing line color for no reason after merge

[Pavel Roskin]

On Tue, 2006-02-07 at 19:56 +1100, Paul Mackerras wrote:
> Pavel Roskin writes:
> > Hello!
> 
> > I didn't realize that the colors correspond to nodes, not branches.
> > Every node has one color that is used for lines to all of its children.
> > It would be much better to assign colors to "branches" consisting of
> > individual lines connecting nodes, but changing that would require many
> 
> Why would that be better?

Because you would get consistent colors between line forks.  The color
would help to tack the line.  Anyway, I don't think I'll ever implement
it myself.

> > > -    if {$nparents($id) <= 1 && $nchildren($id) == 1} {
> > > +    if {$nchildren($id) == 1} {
> > >  	set child [lindex $children($id) 0]
> > >  	if {[info exists colormap($child)]
> > >  	    && $nparents($child) == 1} {
> > > 
> > > 
> > 
> > I still stand behind this patch because it eliminates color changes on
> > the nodes that have exactly one child and parent.  $nparents($id) is
> 
> Ummm... I don't see that you have changed anything for nodes that have
> exactly one parent?

Nodes with one parent are fine.  It's nodes with multiple parents that
are the problem - they fail to inherit the color assigned to their
child.

Let's see what assigncolor does:

|  ----------- color assigned to the child, possibly to be inherited
|
child
|
|  ----------- we are assigning color to this line
|
id ----------- we are processing this node
|\
| \--------\
|           |
parent1   parent2

Why should color at the child change because its single parent has
several parents?

> > However, further changes to reduce color changes didn't produce nice
> > results for me.  If I try to keep one color running as long as possible,
> > I get branches of the same color because, as I said, gitk uses the same
> > color for connections to all children.  So, every node on the branch
> > spurs branching lines of the same color, which can intersect or run
> > side-by-side.
> 
> The colors are really just to make the lines visually distinct.  They
> (the colors) have no semantic meaning.  (I did try coloring the lines
> according to the committer, but I just ended up with an awful lot of
> lines being the same color - corresponding to one Linus Torvalds. :)

I guess I wasn't clear about my intentions.  My only intention is
exactly that - "to make the lines visually distinct".  There is one
corner case where the color changes for no reason.  There are cases that
are harder to fix, but this one is easy, and I have found a working
solution.

-- 
Regards,
Pavel Roskin

Re: gitk changing line color for no reason after merge

[Paul Mackerras]

Pavel Roskin writes:

> I'm trying to make it easier to follow a line.  It's easier if its color
> is not changing, especially on trivial nodes (one parent, one child).

OK, you're using "line" to mean something a bit different from the
connection between a commit and its children, which is how I use it.
You seem to be using it more as a "line of development", or as a
series of related patches.  Which is fine, if you can find a way to
identify lines of development automatically.  (I know it looks obvious
when you look at the gitk display, but that's a lot different from
writing down an algorithm to do it.)

> http://red-bean.com/proski/gitk/gitk-ideal.png - made in GIMP.  Trivial
> nodes never change line color, because it changes as soon as the line
> forks.

My problem with that is that it isn't clear that e.g. the green and
brown lines near the bottom actually represent the same parent - and
that will get worse with more complex graphs.

Paul.

Re: gitk changing line color for no reason after merge

[Pavel Roskin]

On Wed, 2006-02-08 at 13:30 +1100, Paul Mackerras wrote:
> Pavel Roskin writes:
> 
> > I'm trying to make it easier to follow a line.  It's easier if its color
> > is not changing, especially on trivial nodes (one parent, one child).
> 
> OK, you're using "line" to mean something a bit different from the
> connection between a commit and its children, which is how I use it.

I see.  Actually, your choice seems to me quite random and
non-intuitive.  You group together changes that have the same parent,
likely made independently by different people.  In fact, only those
changes are shown that would lead to the current revision of the
repository, unless "--all" is used.  Changes on unmerged branches are
not shown.

If you prefer "horizontal" grouping, it would be more logical to turn it
upside down, i.e. group commits with their parents.  In this case, the
line group would represent one act of merging, performed by one person.
No parents are hidden from view even without "--all".

> You seem to be using it more as a "line of development", or as a
> series of related patches.  Which is fine, if you can find a way to
> identify lines of development automatically.  (I know it looks obvious
> when you look at the gitk display, but that's a lot different from
> writing down an algorithm to do it.)

As usually, let's go from the newest commits to the root of the tree.
The idea is to assign branch ID to changesets, i.e. to combinations of
sha1 and parent number.  Branch ID should be inherited from the children
by the first parent.  Other parents get new branch ID.  There should be
a list of active branches, i.e. those branch ID with yet to be seen
parents.  Color should be assigned to branch ID at the creation time.
The color should be selected according to two rules, whenever possible.
It should be unique among the already assigned colors for the same
child, and is should avoid colors of the active branches.

Actually, qgit does a pretty reasonable thing.  I haven't used gitk for
months, but I had to inspect a Mercurial repository using hgk.  I was
surprised by its "crazy" color changes (or so it seemed to me after
qgit), then I found that gitk had the same problem, then I fixed it and
started this thread :-)

> > http://red-bean.com/proski/gitk/gitk-ideal.png - made in GIMP.  Trivial
> > nodes never change line color, because it changes as soon as the line
> > forks.
> 
> My problem with that is that it isn't clear that e.g. the green and
> brown lines near the bottom actually represent the same parent - and
> that will get worse with more complex graphs.

You are right.  qgit only uses vertical and horizontal lines, so it's
easier to find the parent.

-- 
Regards,
Pavel Roskin