Straighten multiple segments, including when there are commits #11059

mstv · 2023-06-18T14:58:27Z

Fixes #5782
Improves on #9050
The essence of #10778 by @o-kloster including the very useful test utility

Proposed changes

As of now, segments in the revision graph are straightened only of they contain no intermediate commits (i.e. it's just one segment). This change allows straightening also when there are intermediate commits and/or merges.

Screenshots

Before

After

Test methodology

Manual with GE repo
Unit tests verify the graph layout

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).

✒️ I contribute this code under The Developer Certificate of Origin.

RussKie

The tests are 🔥
Few nits

GitUI/UserControls/RevisionGrid/Graph/RevisionGraphRow.cs

RussKie · 2023-06-19T01:09:46Z

GitUI/UserControls/RevisionGrid/Graph/RevisionGraph.cs

@@ -508,9 +508,11 @@ static void StraightenLanes(int startIndex, int lastStraightenIndex, int lastLoo

                        int straightenedCurrentLane = currentLane + 1;
                        int lookaheadLane = currentLane;


Suggested change

int lookaheadLane = currentLane;

int lookAheadLane = currentLane;

- int lookaheadLane = currentLane;
+ int lookAheadLane = currentLane;

Then we would need to change the capitalization of the existing lookaheadIndex, too.

RussKie · 2023-06-19T01:09:59Z

GitUI/UserControls/RevisionGrid/Graph/RevisionGraph.cs

                        for (int lookaheadIndex = currentIndex + 1; lookaheadLane == currentLane && lookaheadIndex <= Math.Min(currentIndex + _straightenLanesLookAhead, lastLookaheadIndex); ++lookaheadIndex)
                        {
-                            lookaheadLane = localOrderedRowCache[lookaheadIndex].GetLaneForSegment(revisionGraphSegment).Index;
+                            RevisionGraphRow lookaheadRow = localOrderedRowCache[lookaheadIndex];


Suggested change

RevisionGraphRow lookaheadRow = localOrderedRowCache[lookaheadIndex];

RevisionGraphRow lookAheadRow = localOrderedRowCache[lookaheadIndex];

RussKie · 2023-06-19T01:12:46Z

UnitTests/GitUI.Tests/UserControls/RevisionGrid/Graph/RevisionGraphTests.cs

+        private static void AssertGraphLayout(RevisionGraph revisionGraph, string expectedLayout)
+        {
+            string expectedGraph = expectedLayout.Split('\n', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries).Join("\n");
+            string actualGraph = AsciiGraphFor(revisionGraph).Join("\n");
+            try
+            {
+                Assert.AreEqual(expectedGraph, actualGraph);
+            }
+            catch
+            {
+                Console.WriteLine(actualGraph);
+            }
+        }


Have you considered using snapshot testing instead (i.e., Verify)? This way the diff would be visual

The display (of Console.WriteLine output) in the test explorer is not perfect, but was good enough for me when using o-kloster's test utility.
Though it is very useful to have the graph definition and the expected result in one place:

RevisionGraph revisionGraph = CreateGraph("R 4:R 3:R 2:R,3 1:2 0:1,4"); AssertGraphLayout(revisionGraph, @" 0 |\ 1 | | \ 2 | |\ | | 3 | | | | | | 4 |/-´ R ");

The display (of Console.WriteLine output) in the test explorer is not perfect, but was good enough for me when using o-kloster's test utility. Though it is very useful to have the graph definition and the expected result in one place:

RevisionGraph revisionGraph = CreateGraph("R 4:R 3:R 2:R,3 1:2 0:1,4"); AssertGraphLayout(revisionGraph, @" 0 |\ 1 | | \ 2 | |\ | | 3 | | | | | | 4 |/-´ R ");

I am still convinced that this is better tp be kept in one place than fiddling around with lots of .verified.txt files.

than fiddling around with lots of .verified.txt files.

That's the key point - there's no fiddling required, the verified files are updated automatically. First, if the test result is different from the expectation, you'll see it in a diff tool. Then, if the change is expected, then the new version is applied in the diff tool.

gerhardol

This works well, but the scenario below is not handled, also in 4.1 though.
Not sure I have understood the details in the implementation, but it seems reasonable.

mstv · 2023-07-05T20:57:01Z

but the scenario below is not handled, also in 4.1 though.

I do net see a difference in the screenshots (which seem to have been taken with this PR). What would you expect?

gerhardol · 2023-07-06T20:53:28Z

I do net see a difference in the screenshots (which seem to have been taken with this PR). What would you expect?

Bad screenshot, updated.

I expected the gap to be filled, there is nothing at all in that column.
But it is the same in 4.1.

mstv · 2023-07-06T21:09:55Z

Straightened lanes are bought by inserting gaps.
This PR straightens also the lane with commits in it. So the gap is moved one to the left in this case.
That's exactly how this PR is intended to work.

Co-authored-by: Igor Velikorossov <russkie@gmail.com>

ghost assigned mstv Jun 18, 2023

RussKie reviewed Jun 19, 2023

View reviewed changes

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jun 19, 2023

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jun 19, 2023

mstv force-pushed the feature/straighten_segments branch from 042821e to ab8b642 Compare June 30, 2023 21:53

gerhardol approved these changes Jul 5, 2023

View reviewed changes

mstv requested a review from RussKie August 8, 2023 13:45

RussKie approved these changes Aug 12, 2023

View reviewed changes

Straighten multiple segments, including when there are commits

80fde7e

Co-authored-by: Igor Velikorossov <russkie@gmail.com>

mstv force-pushed the feature/straighten_segments branch from 2214231 to 80fde7e Compare August 13, 2023 07:40

mstv merged commit ae93fbc into gitextensions:master Aug 13, 2023
3 of 4 checks passed

mstv deleted the feature/straighten_segments branch August 13, 2023 07:41

ghost added this to the vNext milestone Aug 13, 2023

mstv mentioned this pull request Oct 31, 2023

Implement RevisionGraphRow.FirstParentOrSelf without LaneSharing #11316

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Straighten multiple segments, including when there are commits #11059

Straighten multiple segments, including when there are commits #11059

mstv commented Jun 18, 2023 •

edited

RussKie left a comment

RussKie Jun 19, 2023

mstv Jun 19, 2023

RussKie Jun 24, 2023

mstv Jul 1, 2023

RussKie Jun 19, 2023

RussKie Jun 19, 2023

mstv Jun 19, 2023

mstv Aug 12, 2023

RussKie Aug 13, 2023

gerhardol left a comment •

edited

mstv commented Jul 5, 2023

gerhardol commented Jul 6, 2023

mstv commented Jul 6, 2023

		@@ -508,9 +508,11 @@ static void StraightenLanes(int startIndex, int lastStraightenIndex, int lastLoo

		int straightenedCurrentLane = currentLane + 1;
		int lookaheadLane = currentLane;

	RevisionGraphRow lookaheadRow = localOrderedRowCache[lookaheadIndex];
	RevisionGraphRow lookAheadRow = localOrderedRowCache[lookaheadIndex];

Straighten multiple segments, including when there are commits #11059

Straighten multiple segments, including when there are commits #11059

Conversation

mstv commented Jun 18, 2023 • edited

Proposed changes

Screenshots

Before

After

Test methodology

Merge strategy

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol left a comment • edited

Choose a reason for hiding this comment

mstv commented Jul 5, 2023

gerhardol commented Jul 6, 2023

mstv commented Jul 6, 2023

mstv commented Jun 18, 2023 •

edited

gerhardol left a comment •

edited