Simplify column assignment algorithm #654

Merged
merged 3 commits into from Feb 1, 2017

Conversation

Projects
None yet
2 participants
@laerreal
Contributor

laerreal commented Jan 31, 2017

A graph configuration is found that is not correctly handled using current way to maintain occupied column set. If all children of a fork commit are merges and they are assigned a column before the commit processing then no one of children will be assigned same column. Also, no one of them will left parent's column because of the condition in the main loop. In other configuration the column is left when a non-fork ancestor at the column merges. But, in current configuration, no child remains on the column. As a result, the column 'leaks'. This causes the graph spreading.

Such configuration is not found in Git-Cola graph. I've found one in Qemu graph. After selected fork the 7-th column leaks.

example

Investigating the bug I have realized that fixing up the predicate is possible but it will result in new nested loop. Instead, another way was found. It is described in comments and commit messages.

laerreal added some commits Jan 31, 2017

dag: fix up commetns in commit laying out algorithm
Signed-off-by: Efimov Vasily <real@ispras.ru>
dag: update commit laying out algorithm description
Signed-off-by: Efimov Vasily <real@ispras.ru>
dag: simplify commit laying out algorithm main loop
The loop for parents of merge commit is not actually needed. Columns to leave
can be detected during children processing. See comments for details.

Signed-off-by: Efimov Vasily <real@ispras.ru>
@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Feb 1, 2017

Member

This is great. I wonder if it would be resilient to octopus merges, and octopus over octopus. I don't think I've ever seen those in the wild, though ;-)

One thing I noticed is that the first-parent chain seems to get lost on the right side, and this tiny tweak seems to make keep things left-aligned

diff --git a/cola/widgets/dag.py b/cola/widgets/dag.py
index 3562226a..15ecdbeb 100644
--- a/cola/widgets/dag.py
+++ b/cola/widgets/dag.py
@@ -1112,7 +1112,7 @@ class GraphView(QtWidgets.QGraphicsView, ViewerMixin):
     x_adjust = Commit.commit_radius*4/3
     y_adjust = Commit.commit_radius*4/3
 
-    x_off = 18
+    x_off = -18
     y_off = -24
 
     def __init__(self, notifier, parent):

I'm going to merge this as-is, but might apply that on top later. What do you think?

The negative offset seems to work ok on the qemu repo too. In the cola repo the first-parent history chain become easier to see with a negative x offset. Any downsides?

There also seems to be a rendering artifact from overlapping tags with the commit circle (git dag v2.7.0 in qemu) when jumping up and down between the top two commits. The items might be overlapping. I'll see if I can find what's causing that.

Member

davvid commented Feb 1, 2017

This is great. I wonder if it would be resilient to octopus merges, and octopus over octopus. I don't think I've ever seen those in the wild, though ;-)

One thing I noticed is that the first-parent chain seems to get lost on the right side, and this tiny tweak seems to make keep things left-aligned

diff --git a/cola/widgets/dag.py b/cola/widgets/dag.py
index 3562226a..15ecdbeb 100644
--- a/cola/widgets/dag.py
+++ b/cola/widgets/dag.py
@@ -1112,7 +1112,7 @@ class GraphView(QtWidgets.QGraphicsView, ViewerMixin):
     x_adjust = Commit.commit_radius*4/3
     y_adjust = Commit.commit_radius*4/3
 
-    x_off = 18
+    x_off = -18
     y_off = -24
 
     def __init__(self, notifier, parent):

I'm going to merge this as-is, but might apply that on top later. What do you think?

The negative offset seems to work ok on the qemu repo too. In the cola repo the first-parent history chain become easier to see with a negative x offset. Any downsides?

There also seems to be a rendering artifact from overlapping tags with the commit circle (git dag v2.7.0 in qemu) when jumping up and down between the top two commits. The items might be overlapping. I'll see if I can find what's causing that.

@davvid davvid merged commit 8dcebe5 into git-cola:master Feb 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

davvid added a commit that referenced this pull request Feb 1, 2017

Merge pull request #654 from laerreal/simplify_column_assignment_algo…
…rithm

* laerreal/simplify_column_assignment_algorithm:
  dag: update commit laying out algorithm description
  dag: simplify commit laying out algorithm main loop
  dag: fix up commetns in commit laying out algorithm

Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit that referenced this pull request Feb 1, 2017

dag: offset tags to avoid rendering artifacts
Discussed-in: #654
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit that referenced this pull request Feb 1, 2017

dag: make the graph left-aligned
Make the first-parent history chain easier to see by making the X offset
negative.  This effectively mirrors the graph.

Discussed-in: #654
Signed-off-by: David Aguilar <davvid@gmail.com>
@laerreal

This comment has been minimized.

Show comment
Hide comment
@laerreal

laerreal Feb 1, 2017

Contributor

The problem is detection of 'first' ('main') chain. (I think, 'first-chain' is the one that was 'master' before the merge. Correct me if it is wrong.) If 'master' branch consists of merge commits only then the heuristic in column distribution assigns same column to those merge commits. Because generation of each other is gather than generation of any other child of the fork. But, in real life, 'master' contains regular commits too. It is not clear how to distinguish what branch is 'main'. In fact, same generation commits is assigned random columns. As a result, the first-parent chain is shifted to side.

Contributor

laerreal commented Feb 1, 2017

The problem is detection of 'first' ('main') chain. (I think, 'first-chain' is the one that was 'master' before the merge. Correct me if it is wrong.) If 'master' branch consists of merge commits only then the heuristic in column distribution assigns same column to those merge commits. Because generation of each other is gather than generation of any other child of the fork. But, in real life, 'master' contains regular commits too. It is not clear how to distinguish what branch is 'main'. In fact, same generation commits is assigned random columns. As a result, the first-parent chain is shifted to side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment