From 60fb59a7a0302537b3d2f5bbdc6d1d6216c4e0d1 Mon Sep 17 00:00:00 2001 From: Bram Hagens Date: Mon, 29 Dec 2025 02:53:54 +0100 Subject: [PATCH] fix: display orphan branches separately in commit graph (#10484) Fixes #2705. Fixes #7635. This PR fixes the commit graph showing false connections for orphan/root commits. Connection lines are now shown only when a parent/child relationship exists. Visible relationships are determined using `git log`'s `%P` output by the new `ComputeGlyphConnectivity` function. The SVG template is adapted to render vertical lines conditionally. Unit tests for `ComputeGlyphConnectivity` cover regular linear commit history, orphan commits, merge commits, and non-commit glyphs (`|`, `/`, `\`). Unit tests also cover the changes to the `git log` parsing. The SVG template was verified manually. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10484 Reviewed-by: Gusted Co-authored-by: Bram Hagens Co-committed-by: Bram Hagens --- services/repository/gitgraph/graph.go | 5 +- services/repository/gitgraph/graph_models.go | 127 +++++++++++++---- services/repository/gitgraph/graph_test.go | 139 ++++++++++++++++++- services/repository/gitgraph/parser.go | 3 + templates/repo/graph/svgcontainer.tmpl | 10 +- 5 files changed, 255 insertions(+), 29 deletions(-) diff --git a/services/repository/gitgraph/graph.go b/services/repository/gitgraph/graph.go index bf15baed2a..1d1cea4225 100644 --- a/services/repository/gitgraph/graph.go +++ b/services/repository/gitgraph/graph.go @@ -16,7 +16,7 @@ import ( // GetCommitGraph return a list of commit (GraphItems) from all branches func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bool, branches, files []string) (*Graph, error) { - format := "DATA:%D|%H|%aD|%h|%s" + format := "DATA:%P|%D|%H|%aD|%h|%s" if page == 0 { page = 1 @@ -112,5 +112,8 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo }); err != nil { return graph, err } + + graph.ComputeGlyphConnectivity() + return graph, nil } diff --git a/services/repository/gitgraph/graph_models.go b/services/repository/gitgraph/graph_models.go index 20107cc646..2c4133e1f2 100644 --- a/services/repository/gitgraph/graph_models.go +++ b/services/repository/gitgraph/graph_models.go @@ -15,6 +15,7 @@ import ( git_model "forgejo.org/models/git" repo_model "forgejo.org/models/repo" user_model "forgejo.org/models/user" + "forgejo.org/modules/container" "forgejo.org/modules/git" "forgejo.org/modules/log" ) @@ -27,18 +28,20 @@ func NewGraph() *Graph { Column: -1, } graph.Flows = map[int64]*Flow{} + graph.continuationAbove = map[[2]int]bool{} return graph } // Graph represents a collection of flows type Graph struct { - Flows map[int64]*Flow - Commits []*Commit - MinRow int - MinColumn int - MaxRow int - MaxColumn int - relationCommit *Commit + Flows map[int64]*Flow + Commits []*Commit + MinRow int + MinColumn int + MaxRow int + MaxColumn int + relationCommit *Commit + continuationAbove map[[2]int]bool } // Width returns the width of the graph @@ -51,6 +54,69 @@ func (graph *Graph) Height() int { return graph.MaxRow - graph.MinRow + 1 } +// ComputeGlyphConnectivity sets ConnectsUp/ConnectsDown for commit glyphs based on parent/child relationships +func (graph *Graph) ComputeGlyphConnectivity() { + revs := make(container.Set[string]) + revByPos := make(map[[2]int]string) + for _, c := range graph.Commits { + if c.Rev != "" { + revs.Add(c.Rev) + revByPos[[2]int{c.Row, c.Column}] = c.Rev + } + } + + connectsDown := make(container.Set[string]) + connectsUp := make(container.Set[string]) + + // Commits with parents connect down (even if parent is on another page) + // Commits with visible children connect up + for _, c := range graph.Commits { + if len(c.ParentHashes) > 0 { + connectsDown.Add(c.Rev) + } + for _, parentHash := range c.ParentHashes { + if revs.Contains(parentHash) { + connectsUp.Add(parentHash) + } + } + } + + // Commits with a non-commit glyph above also connect up + for _, flow := range graph.Flows { + for _, g := range flow.Glyphs { + if g.Glyph != '*' { + pos := [2]int{g.Row + 1, g.Column} + if rev, exists := revByPos[pos]; exists { + connectsUp.Add(rev) + } + } + } + } + + // Commits with continuation from previous page connect up + for pos := range graph.continuationAbove { + if rev, exists := revByPos[pos]; exists { + connectsUp.Add(rev) + } + } + + for _, flow := range graph.Flows { + for i := range flow.Glyphs { + glyph := &flow.Glyphs[i] + if glyph.Glyph == '*' { + pos := [2]int{glyph.Row, glyph.Column} + if rev, exists := revByPos[pos]; exists { + glyph.ConnectsUp = connectsUp.Contains(rev) + glyph.ConnectsDown = connectsDown.Contains(rev) + } + } else { + glyph.ConnectsUp = true + glyph.ConnectsDown = true + } + } + } +} + // AddGlyph adds glyph to flows func (graph *Graph) AddGlyph(row, column int, flowID int64, color int, glyph byte) { flow, ok := graph.Flows[flowID] @@ -175,17 +241,19 @@ func (flow *Flow) AddGlyph(row, column int, glyph byte) { } flow.Glyphs = append(flow.Glyphs, Glyph{ - row, - column, - glyph, + Row: row, + Column: column, + Glyph: glyph, }) } // Glyph represents a coordinate and glyph type Glyph struct { - Row int - Column int - Glyph byte + Row int + Column int + Glyph byte + ConnectsUp bool + ConnectsDown bool } // RelationCommit represents an empty relation commit @@ -195,28 +263,36 @@ var RelationCommit = &Commit{ // NewCommit creates a new commit from a provided line func NewCommit(row, column int, line []byte) (*Commit, error) { - data := bytes.SplitN(line, []byte("|"), 5) - if len(data) < 5 { + data := bytes.SplitN(line, []byte("|"), 6) + if len(data) < 6 { return nil, fmt.Errorf("malformed data section on line %d with commit: %s", row, string(line)) } // Format is a slight modification from RFC1123Z - t, err := time.Parse("Mon, _2 Jan 2006 15:04:05 -0700", string(data[2])) + t, err := time.Parse("Mon, _2 Jan 2006 15:04:05 -0700", string(data[3])) if err != nil { return nil, fmt.Errorf("could not parse date of commit: %w", err) } + + var parents []string + for p := range bytes.FieldsSeq(data[0]) { + parents = append(parents, string(p)) + } + return &Commit{ Row: row, Column: column, - // 0 matches git log --pretty=format:%d => ref names, like the --decorate option of git-log(1) - Refs: newRefsFromRefNames(data[0]), - // 1 matches git log --pretty=format:%H => commit hash - Rev: string(data[1]), - // 2 matches git log --pretty=format:%aD => author date, RFC2822 style + // 0 matches git log --pretty=format:%P => parent hashes + ParentHashes: parents, + // 1 matches git log --pretty=format:%d => ref names, like the --decorate option of git-log(1) + Refs: newRefsFromRefNames(data[1]), + // 2 matches git log --pretty=format:%H => commit hash + Rev: string(data[2]), + // 3 matches git log --pretty=format:%aD => author date, RFC2822 style Date: t, - // 3 matches git log --pretty=format:%h => abbreviated commit hash - ShortRev: string(data[3]), - // 4 matches git log --pretty=format:%s => subject - Subject: string(data[4]), + // 4 matches git log --pretty=format:%h => abbreviated commit hash + ShortRev: string(data[4]), + // 5 matches git log --pretty=format:%s => subject + Subject: string(data[5]), }, nil } @@ -254,6 +330,7 @@ type Commit struct { Date time.Time ShortRev string Subject string + ParentHashes []string } // OnlyRelation returns whether this a relation only commit diff --git a/services/repository/gitgraph/graph_test.go b/services/repository/gitgraph/graph_test.go index 374341b276..6dafaf03fd 100644 --- a/services/repository/gitgraph/graph_test.go +++ b/services/repository/gitgraph/graph_test.go @@ -10,6 +10,9 @@ import ( "testing" "forgejo.org/modules/git" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func BenchmarkGetCommitGraph(b *testing.B) { @@ -32,7 +35,7 @@ func BenchmarkGetCommitGraph(b *testing.B) { } func BenchmarkParseCommitString(b *testing.B) { - testString := "* DATA:|4e61bacab44e9b4730e44a6615d04098dd3a8eaf|2016-12-20 21:10:41 +0100|4e61bac|Add route for graph" + testString := "* DATA:abc123||4e61bacab44e9b4730e44a6615d04098dd3a8eaf|Tue, 20 Dec 2016 21:10:41 +0100|4e61bac|Add route for graph" parser := &Parser{} parser.Reset() @@ -241,7 +244,7 @@ func TestParseGlyphs(t *testing.T) { } func TestCommitStringParsing(t *testing.T) { - dataFirstPart := "* DATA:|4e61bacab44e9b4730e44a6615d04098dd3a8eaf|Tue, 20 Dec 2016 21:10:41 +0100|4e61bac|" + dataFirstPart := "* DATA:abc123||4e61bacab44e9b4730e44a6615d04098dd3a8eaf|Tue, 20 Dec 2016 21:10:41 +0100|4e61bac|" tests := []struct { shouldPass bool testName string @@ -269,6 +272,138 @@ func TestCommitStringParsing(t *testing.T) { } } +func TestNewCommitParentHashes(t *testing.T) { + tests := []struct { + name string + data string + expectedParents []string + }{ + { + name: "no parents (orphan)", + data: "||4e61bacab44e9b4730e44a6615d04098dd3a8eaf|Tue, 20 Dec 2016 21:10:41 +0100|4e61bac|subject", + expectedParents: nil, + }, + { + name: "single parent", + data: "abc123||4e61bacab44e9b4730e44a6615d04098dd3a8eaf|Tue, 20 Dec 2016 21:10:41 +0100|4e61bac|subject", + expectedParents: []string{"abc123"}, + }, + { + name: "multiple parents (merge)", + data: "abc123 def456||4e61bacab44e9b4730e44a6615d04098dd3a8eaf|Tue, 20 Dec 2016 21:10:41 +0100|4e61bac|subject", + expectedParents: []string{"abc123", "def456"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + commit, err := NewCommit(0, 0, []byte(test.data)) + require.NoError(t, err) + assert.Equal(t, test.expectedParents, commit.ParentHashes) + }) + } +} + +func TestComputeGlyphConnectivity(t *testing.T) { + addCommit := func(graph *Graph, row, col int, hash string, parents []string) { + flowID := int64(col + 1) + commit := &Commit{Row: row, Column: col, Rev: hash, ParentHashes: parents, Flow: flowID} + graph.AddGlyph(row, col, flowID, 1, '*') + graph.Commits = append(graph.Commits, commit) + graph.Flows[flowID].Commits = append(graph.Flows[flowID].Commits, commit) + } + + getCommitConnectivity := func(graph *Graph, row, col int) (up, down bool) { + for _, flow := range graph.Flows { + for _, g := range flow.Glyphs { + if g.Row == row && g.Column == col && g.Glyph == '*' { + return g.ConnectsUp, g.ConnectsDown + } + } + } + return false, false + } + + t.Run("ConnectsDown/no parents", func(t *testing.T) { + graph := NewGraph() + addCommit(graph, 0, 0, "orphan", nil) + graph.ComputeGlyphConnectivity() + + _, down := getCommitConnectivity(graph, 0, 0) + assert.False(t, down) + }) + + t.Run("ConnectsDown/has parent in graph", func(t *testing.T) { + graph := NewGraph() + addCommit(graph, 0, 0, "child", []string{"parent"}) + addCommit(graph, 1, 0, "parent", nil) + graph.ComputeGlyphConnectivity() + + _, down := getCommitConnectivity(graph, 0, 0) + assert.True(t, down) + }) + + t.Run("ConnectsDown/has parent outside graph", func(t *testing.T) { + graph := NewGraph() + addCommit(graph, 0, 0, "child", []string{"parent-not-in-graph"}) + graph.ComputeGlyphConnectivity() + + _, down := getCommitConnectivity(graph, 0, 0) + assert.True(t, down) + }) + + t.Run("ConnectsUp/no child, no glyph above, no continuation", func(t *testing.T) { + graph := NewGraph() + addCommit(graph, 0, 0, "orphan", nil) + graph.ComputeGlyphConnectivity() + + up, _ := getCommitConnectivity(graph, 0, 0) + assert.False(t, up) + }) + + t.Run("ConnectsUp/has visible child", func(t *testing.T) { + graph := NewGraph() + addCommit(graph, 0, 0, "child", []string{"parent"}) + addCommit(graph, 1, 0, "parent", nil) + graph.ComputeGlyphConnectivity() + + up, _ := getCommitConnectivity(graph, 1, 0) + assert.True(t, up) + }) + + t.Run("ConnectsUp/has non-commit glyph above", func(t *testing.T) { + graph := NewGraph() + graph.AddGlyph(0, 0, 1, 1, '|') + addCommit(graph, 1, 0, "commit", nil) + graph.ComputeGlyphConnectivity() + + up, _ := getCommitConnectivity(graph, 1, 0) + assert.True(t, up) + }) + + t.Run("ConnectsUp/has continuationAbove", func(t *testing.T) { + graph := NewGraph() + addCommit(graph, 0, 0, "commit", nil) + graph.continuationAbove[[2]int{0, 0}] = true + graph.ComputeGlyphConnectivity() + + up, _ := getCommitConnectivity(graph, 0, 0) + assert.True(t, up) + }) + + t.Run("non-commit glyphs always connect both ways", func(t *testing.T) { + for _, glyph := range []byte{'|', '/', '\\'} { + graph := NewGraph() + graph.AddGlyph(0, 0, 1, 1, glyph) + graph.ComputeGlyphConnectivity() + + g := graph.Flows[1].Glyphs[0] + assert.True(t, g.ConnectsUp, "glyph %q should connect up", glyph) + assert.True(t, g.ConnectsDown, "glyph %q should connect down", glyph) + } + }) +} + var testglyphs = `* * * diff --git a/services/repository/gitgraph/parser.go b/services/repository/gitgraph/parser.go index f6bf9b0b90..3c98e66273 100644 --- a/services/repository/gitgraph/parser.go +++ b/services/repository/gitgraph/parser.go @@ -80,6 +80,9 @@ func (parser *Parser) AddLineToGraph(graph *Graph, row int, line []byte) error { } continue } + if column < len(parser.oldGlyphs) && parser.oldGlyphs[column] == '|' { + graph.continuationAbove[[2]int{row, column}] = true + } err2 := graph.AddCommit(row, column, flowID, line[idx+5:]) if err != nil && err2 != nil { err = fmt.Errorf("%v %w", err2, err) diff --git a/templates/repo/graph/svgcontainer.tmpl b/templates/repo/graph/svgcontainer.tmpl index 99c3c87399..a04b8d310e 100644 --- a/templates/repo/graph/svgcontainer.tmpl +++ b/templates/repo/graph/svgcontainer.tmpl @@ -3,8 +3,16 @@ {{range $flowid, $flow := .Graph.Flows}}