From ac60779abf06ec7173f34829641a50b1b53e4f7b Mon Sep 17 00:00:00 2001 From: Quinn Slack Date: Fri, 23 Dec 2022 22:19:22 -1000 Subject: [PATCH] Support following single-file history across renames in GraphQL API (#45882) follow single-file history across renames fix #4383 --- CHANGELOG.md | 1 + cmd/frontend/graphqlbackend/git_commit.go | 2 + cmd/frontend/graphqlbackend/git_commits.go | 2 + cmd/frontend/graphqlbackend/schema.graphql | 4 ++ internal/gitserver/commands.go | 47 +++++++++------------- internal/gitserver/commands_test.go | 19 ++------- 6 files changed, 32 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 91d5f8f13e4..c930fa44384 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ All notable changes to Sourcegraph are documented in this file. - Keyboard navigation for search results is now enabled by default. Use Arrow Up/Down keys to navigate between search results, Arrow Left/Right to collapse and expand file matches, Enter to open the search result in the current tab, Ctrl/Cmd+Enter to open the result in a separate tab, / to refocus the search input, and Ctrl/Cmd+Arrow Down to jump from the search input to the first result. Arrow Left/Down/Up/Right in previous examples can be substituted with h/j/k/l for Vim-style bindings. Keyboard navigation can be disabled by creating the `search-results-keyboard-navigation` feature flag and setting it to false. [#45890](https://github.com/sourcegraph/sourcegraph/pull/45890) - Added support for receiving GitLab webhook `push` events. [#45856](https://github.com/sourcegraph/sourcegraph/pull/45856) - Added support for receiving Bitbucket Server / Datacenter webhook `push` events. [#45909](https://github.com/sourcegraph/sourcegraph/pull/45909) +- The GraphQL API now supports listing single-file commit history across renames (with `GitCommit.ancestors(follow: true, path: "")`). [#45882](https://github.com/sourcegraph/sourcegraph/pull/45882) ### Changed diff --git a/cmd/frontend/graphqlbackend/git_commit.go b/cmd/frontend/graphqlbackend/git_commit.go index ff5fffbdf4e..9a04effe691 100644 --- a/cmd/frontend/graphqlbackend/git_commit.go +++ b/cmd/frontend/graphqlbackend/git_commit.go @@ -323,6 +323,7 @@ func (r *GitCommitResolver) Ancestors(ctx context.Context, args *struct { graphqlutil.ConnectionArgs Query *string Path *string + Follow bool After *string AfterCursor *string }) (*gitCommitConnectionResolver, error) { @@ -333,6 +334,7 @@ func (r *GitCommitResolver) Ancestors(ctx context.Context, args *struct { first: args.ConnectionArgs.First, query: args.Query, path: args.Path, + follow: args.Follow, after: args.After, afterCursor: args.AfterCursor, repo: r.repoResolver, diff --git a/cmd/frontend/graphqlbackend/git_commits.go b/cmd/frontend/graphqlbackend/git_commits.go index 633947609df..7efb811a9ca 100644 --- a/cmd/frontend/graphqlbackend/git_commits.go +++ b/cmd/frontend/graphqlbackend/git_commits.go @@ -21,6 +21,7 @@ type gitCommitConnectionResolver struct { first *int32 query *string path *string + follow bool author *string // after corresponds to --after in the git log / git rev-spec commands. Not to be confused with @@ -83,6 +84,7 @@ func (r *gitCommitConnectionResolver) compute(ctx context.Context) ([]*gitdomain After: toValue(r.after).(string), Skip: uint(afterCursor), Path: toValue(r.path).(string), + Follow: r.follow, }) } diff --git a/cmd/frontend/graphqlbackend/schema.graphql b/cmd/frontend/graphqlbackend/schema.graphql index 71d2103cc1b..451721274ca 100755 --- a/cmd/frontend/graphqlbackend/schema.graphql +++ b/cmd/frontend/graphqlbackend/schema.graphql @@ -4615,6 +4615,10 @@ type GitCommit implements Node { """ path: String """ + Follow history beyond renames (only works for a single file). + """ + follow: Boolean = false + """ Return commits more recent than the specified date. """ after: String diff --git a/internal/gitserver/commands.go b/internal/gitserver/commands.go index 65b71d97fb5..8ec36cfbddb 100644 --- a/internal/gitserver/commands.go +++ b/internal/gitserver/commands.go @@ -1478,7 +1478,7 @@ func (c *clientImplementor) Stat(ctx context.Context, checker authz.SubRepoPermi return fi, nil } -// CommitsOptions specifies options for (Repository).Commits (Repository).CommitCount. +// CommitsOptions specifies options for Commits. type CommitsOptions struct { Range string // commit range (revspec, "A..B", "A...B", etc.) @@ -1496,6 +1496,8 @@ type CommitsOptions struct { Path string // only commits modifying the given path are selected (optional) + Follow bool // follow the history of the path beyond renames (works only for a single path) + // When true we opt out of attempting to fetch missing revisions NoEnsureRevision bool @@ -1703,11 +1705,23 @@ func (c *clientImplementor) HasCommitAfter(ctx context.Context, checker authz.Su return false, err } - n, err := c.commitCount(ctx, repo, CommitsOptions{ + args, err := commitLogArgs([]string{"rev-list", "--count"}, CommitsOptions{ N: 1, After: date, Range: string(commitid), }) + if err != nil { + return false, err + } + + cmd := c.gitCommand(repo, args...) + out, err := cmd.Output(ctx) + if err != nil { + return false, errors.WithMessage(err, fmt.Sprintf("git command %v failed (output: %q)", cmd.Args(), out)) + } + + out = bytes.TrimSpace(out) + n, err := strconv.Atoi(string(out)) return n > 0, err } @@ -1903,38 +1917,15 @@ func commitLogArgs(initialArgs []string, opt CommitsOptions) (args []string, err if opt.NameOnly { args = append(args, "--name-only") } + if opt.Follow { + args = append(args, "--follow") + } if opt.Path != "" { args = append(args, "--", opt.Path) } return args, nil } -// commitCount returns the number of commits that would be returned by Commits. -func (c *clientImplementor) commitCount(ctx context.Context, repo api.RepoName, opt CommitsOptions) (uint, error) { - span, ctx := ot.StartSpanFromContext(ctx, "Git: CommitCount") //nolint:staticcheck // OT is deprecated - span.SetTag("Opt", opt) - defer span.Finish() - - args, err := commitLogArgs([]string{"rev-list", "--count"}, opt) - if err != nil { - return 0, err - } - - if opt.Path != "" { - // This doesn't include --follow flag because rev-list doesn't support it, so the number may be slightly off. - args = append(args, "--", opt.Path) - } - cmd := c.gitCommand(repo, args...) - out, err := cmd.Output(ctx) - if err != nil { - return 0, errors.WithMessage(err, fmt.Sprintf("git command %v failed (output: %q)", cmd.Args(), out)) - } - - out = bytes.TrimSpace(out) - n, err := strconv.ParseUint(string(out), 10, 64) - return uint(n), err -} - // FirstEverCommit returns the first commit ever made to the repository. func (c *clientImplementor) FirstEverCommit(ctx context.Context, checker authz.SubRepoPermissionChecker, repo api.RepoName) (*gitdomain.Commit, error) { span, ctx := ot.StartSpanFromContext(ctx, "Git: FirstEverCommit") //nolint:staticcheck // OT is deprecated diff --git a/internal/gitserver/commands_test.go b/internal/gitserver/commands_test.go index c0d02640481..05ecfc25e3e 100644 --- a/internal/gitserver/commands_test.go +++ b/internal/gitserver/commands_test.go @@ -1617,7 +1617,7 @@ func TestRepository_Commits(t *testing.T) { runCommitsTests := func(checker authz.SubRepoPermissionChecker) { for label, test := range tests { t.Run(label, func(t *testing.T) { - testCommits(ctx, label, test.repo, CommitsOptions{Range: string(test.id)}, checker, test.wantTotal, test.wantCommits, t) + testCommits(ctx, label, test.repo, CommitsOptions{Range: string(test.id)}, checker, test.wantCommits, t) // Test that trying to get a nonexistent commit returns RevisionNotFoundError. if _, err := client.Commits(ctx, nil, test.repo, CommitsOptions{Range: string(NonExistentCommitID)}); !errors.HasType(err, &gitdomain.RevisionNotFoundError{}) { @@ -1881,7 +1881,7 @@ func TestRepository_Commits_options(t *testing.T) { for label, test := range tests { t.Run(label, func(t *testing.T) { repo := MakeGitRepository(t, gitCommands...) - testCommits(ctx, label, repo, test.opt, checker, test.wantTotal, test.wantCommits, t) + testCommits(ctx, label, repo, test.opt, checker, test.wantCommits, t) }) } // Added for awareness if this error message changes. Insights record last repo indexing and consider empty @@ -1940,7 +1940,6 @@ func TestRepository_Commits_options_path(t *testing.T) { tests := map[string]struct { opt CommitsOptions wantCommits []*gitdomain.Commit - wantTotal uint }{ "git cmd Path 0": { opt: CommitsOptions{ @@ -1948,7 +1947,6 @@ func TestRepository_Commits_options_path(t *testing.T) { Path: "doesnt-exist", }, wantCommits: nil, - wantTotal: 0, }, "git cmd Path 1": { opt: CommitsOptions{ @@ -1956,7 +1954,6 @@ func TestRepository_Commits_options_path(t *testing.T) { Path: "file1", }, wantCommits: wantGitCommits, - wantTotal: 1, }, } @@ -1964,7 +1961,7 @@ func TestRepository_Commits_options_path(t *testing.T) { for label, test := range tests { t.Run(label, func(t *testing.T) { repo := MakeGitRepository(t, gitCommands...) - testCommits(ctx, label, repo, test.opt, checker, test.wantTotal, test.wantCommits, t) + testCommits(ctx, label, repo, test.opt, checker, test.wantCommits, t) }) } } @@ -2337,7 +2334,7 @@ func TestCommitDate(t *testing.T) { }) } -func testCommits(ctx context.Context, label string, repo api.RepoName, opt CommitsOptions, checker authz.SubRepoPermissionChecker, wantTotal uint, wantCommits []*gitdomain.Commit, t *testing.T) { +func testCommits(ctx context.Context, label string, repo api.RepoName, opt CommitsOptions, checker authz.SubRepoPermissionChecker, wantCommits []*gitdomain.Commit, t *testing.T) { t.Helper() db := database.NewMockDB() client := NewClient(db).(*clientImplementor) @@ -2347,14 +2344,6 @@ func testCommits(ctx context.Context, label string, repo api.RepoName, opt Commi return } - total, err := client.commitCount(ctx, repo, opt) - if err != nil { - t.Errorf("%s: commitCount(): %s", label, err) - return - } - if total != wantTotal { - t.Errorf("%s: got %d total commits, want %d", label, total, wantTotal) - } if len(commits) != len(wantCommits) { t.Errorf("%s: got %d commits, want %d", label, len(commits), len(wantCommits)) }