artin/sourcegraph
1
0
Fork 0
mirror of https://github.com/sourcegraph/sourcegraph.git synced 2026-02-06 17:31:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

feat(Source): Properly render multi-line changelist messages from Perforce (#63728)

Browse Source 
Fixes SRC-431

Mutli-line perforce changelist descriptions were not being handle since
the code implicitly assumed they could not exist. This change enables
support for them.


## Screenshots

**Changelist view with both single and multiline commits**

![image](https://github.com/sourcegraph/sourcegraph/assets/304410/8c7ead01-fdec-461c-826b-83013f89ad77)

**Changelist view with expanded commit message**

![image](https://github.com/sourcegraph/sourcegraph/assets/304410/a16c8637-7180-4631-88cb-39b4cc49a74c)

**Individual changelist item**

![image](https://github.com/sourcegraph/sourcegraph/assets/304410/c63f0a49-f6a9-430c-9caf-0480c17bb64e)


## Test plan
1. Update unit tests
2. Validate both P4 and non-p4 commits work
3. Validate on s2

## Changelog

- Properly render multi-line perforce changelist descriptions
This commit is contained in:
Matthew Manela 2024-07-09 17:52:05 -04:00 committed by GitHub
parent 28f797e866
commit 4c3985e16f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 131 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
cmd/frontend/graphqlbackend/git_commit.go
View File

@ -180,9 +180,6 @@ func (r *GitCommitResolver) Subject(ctx context.Context) (string, error) {
}
func (r *GitCommitResolver) Body(ctx context.Context) (*string, error) {
if r.repoResolver.isPerforceDepot(ctx) {
return nil, nil
}
commit, err := r.resolveCommit(ctx)
if err != nil {
@ -190,6 +187,10 @@ func (r *GitCommitResolver) Body(ctx context.Context) (*string, error) {
}
body := commit.Message.Body()
if r.repoResolver.isPerforceDepot(ctx) {
return maybeTransformP4Body(body), nil
}
if body == "" {
return nil, nil
}

58
cmd/frontend/graphqlbackend/git_commit_test.go
View File

@ -180,7 +180,7 @@ func TestGitCommitResolver(t *testing.T) {
}
})
runPerforceTests := func(t *testing.T, commit *gitdomain.Commit) {
runPerforceTests := func(t *testing.T, commit *gitdomain.Commit, multiLine bool) {
repo := &types.Repo{
ID: 1,
Name: "perforce/test-depot",
@ -211,6 +211,14 @@ func TestGitCommitResolver(t *testing.T) {
require.NoError(t, err)
require.Equal(t, "subject: Changes things", subject)
body, err := commitResolver.Body(ctx)
require.NoError(t, err)
if multiLine {
require.Equal(t, "Multi-line message", *body)
} else {
require.Empty(t, *body)
}
f, err := ioutil.TempFile("/tmp", "foo")
require.NoError(t, err)
@ -235,7 +243,7 @@ func TestGitCommitResolver(t *testing.T) {
)
}
t.Run("perforce depot, git-p4 commit", func(t *testing.T) {
t.Run("perforce depot, git-p4 commit, single line", func(t *testing.T) {
commit := &gitdomain.Commit{
ID: "c1",
Message: `subject: Changes things
@ -251,10 +259,30 @@ func TestGitCommitResolver(t *testing.T) {
},
}
runPerforceTests(t, commit)
runPerforceTests(t, commit, false)
})
t.Run("perforce depot, p4-fusion commit", func(t *testing.T) {
t.Run("perforce depot, git-p4 commit, multi line", func(t *testing.T) {
commit := &gitdomain.Commit{
ID: "c1",
Message: `subject: Changes things
Multi-line message
[git-p4: depot-paths = "//test-depot/": change = 123]"`,
Parents: []api.CommitID{"p1", "p2"},
Author: gitdomain.Signature{
Name: "Bob",
Email: "bob@alice.com",
},
Committer: &gitdomain.Signature{
Name: "Alice",
Email: "alice@bob.com",
},
}
runPerforceTests(t, commit, true)
})
t.Run("perforce depot, p4-fusion commit, single line", func(t *testing.T) {
commit := &gitdomain.Commit{
ID: "c1",
Message: `123 - subject: Changes things
@ -270,7 +298,27 @@ func TestGitCommitResolver(t *testing.T) {
},
}
runPerforceTests(t, commit)
runPerforceTests(t, commit, false)
})
t.Run("perforce depot, p4-fusion commit, multi line", func(t *testing.T) {
commit := &gitdomain.Commit{
ID: "c1",
Message: `123 - subject: Changes things
Multi-line message
[p4-fusion: depot-paths = "//test-perms/": change = 123]"`,
Parents: []api.CommitID{"p1", "p2"},
Author: gitdomain.Signature{
Name: "Bob",
Email: "bob@alice.com",
},
Committer: &gitdomain.Signature{
Name: "Alice",
Email: "alice@bob.com",
},
}
runPerforceTests(t, commit, true)
})
}

17
cmd/frontend/graphqlbackend/perforce_changelist.go
View File

@ -8,6 +8,7 @@ import (
"sync"
"github.com/sourcegraph/log"
"github.com/sourcegraph/sourcegraph/cmd/frontend/backend"
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain"
@ -120,13 +121,27 @@ func parseP4FusionCommitSubject(subject string) (string, error) {
return matches[2], nil
}
// maybeTransformP4Body is used for special handling of perforce depots converted to git using
// p4-fusion or git-p4. We want to strip out the generated commit message and use the original
// We handle both p4-fusion and git-p4 so that we stripe the system message from both.
func maybeTransformP4Body(body string) *string {
if idx := strings.Index(body, "[p4-fusion"); idx != -1 {
body = body[:idx]
} else if idx := strings.Index(body, "[git-p4"); idx != -1 {
body = body[:idx]
}
trimmedBody := strings.TrimSpace(body)
return &trimmedBody
}
// maybeTransformP4Subject is used for special handling of perforce depots converted to git using
// p4-fusion. We want to parse and use the subject from the original changelist and not the subject
// that is generated during the conversion.
//
// For depots converted with git-p4, this special handling is NOT required.
func maybeTransformP4Subject(ctx context.Context, repoResolver *RepositoryResolver, commit *gitdomain.Commit) *string {
if repoResolver.isPerforceDepot(ctx) && strings.HasPrefix(commit.Message.Body(), "[p4-fusion") {
if repoResolver.isPerforceDepot(ctx) && strings.Contains(commit.Message.Body(), "[p4-fusion") {
subject, err := parseP4FusionCommitSubject(commit.Message.Subject())
if err == nil {
return &subject

58
cmd/frontend/graphqlbackend/perforce_changelist_test.go
View File

@ -4,6 +4,8 @@ import (
"context"
"testing"
"github.com/google/go-cmp/cmp"
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/database/dbmocks"
"github.com/sourcegraph/sourcegraph/internal/extsvc"
@ -154,3 +156,59 @@ func TestParseP4FusionCommitSubject(t *testing.T) {
}
}
}
func TestMaybeTransformP4Body(t *testing.T) {
tests := []struct {
name string
input string
expected string
}{
{
name: "p4-fusion tag",
input: "This is a commit message\n[p4-fusion: depot-paths = \"//test-perms/\": change = 80972]",
expected: "This is a commit message",
},
{
name: "git-p4 tag",
input: "Another commit message\n[git-p4: depot-paths = \"//test-perms/\": change = 80999]",
expected: "Another commit message",
},
{
name: "git-p4 tag no new line",
input: "Another commit message[git-p4: depot-paths = \"//test-perms/\": change = 80999]",
expected: "Another commit message",
},
{
name: "no tags",
input: "A simple commit message without tags",
expected: "A simple commit message without tags",
},
{
name: "empty input",
input: "",
expected: "",
},
{
name: "only whitespace",
input: " \n \t ",
expected: "",
},
{
name: "multiple lines without tags",
input: "First line\nSecond line\nThird line",
expected: "First line\nSecond line\nThird line",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := maybeTransformP4Body(tt.input)
if result == nil {
t.Fatal("Expected non-nil result")
}
if diff := cmp.Diff(tt.expected, *result); diff != "" {
t.Errorf("maybeTransformP4Body() mismatch (-want +got):\n%s", diff)
}
})
}
}