gitserver: port FirstEverCommit functionality from client to git cli backend

[ggilmore]

Part of #61689

Overview

This PR adapts the existing FirstEverCommit logic from the gitserver client to our new git cli backend. This is the first step as part of a set of stacked PRs to port the functionality to gRPC.

Existing func (c *clientImplementor) FirstEverCommit() implementation for reference:

sourcegraph/internal/gitserver/commands.go

Lines 1734 to 1757 in 412654f

	func (c *clientImplementor) FirstEverCommit(ctx context.Context, repo api.RepoName) (_ *gitdomain.Commit, err error) {
	ctx, _, endObservation := c.operations.firstEverCommit.With(ctx, &err, observation.Args{
	MetricLabelValues: []string{c.scope},
	Attrs: []attribute.KeyValue{
	repo.Attr(),
	},
	})
	defer endObservation(1, observation.Args{})
	args := []string{"rev-list", "--reverse", "--date-order", "--max-parents=0", "HEAD"}
	cmd := c.gitCommand(repo, args...)
	out, err := cmd.Output(ctx)
	if err != nil {
	return nil, errors.WithMessage(err, fmt.Sprintf("git command %v failed (output: %q)", args, out))
	}
	lines := bytes.TrimSpace(out)
	tokens := bytes.SplitN(lines, []byte("\n"), 2)
	if len(tokens) == 0 {
	return nil, errors.New("FirstEverCommit returned no revisions")
	}
	first := tokens[0]
	id := api.CommitID(bytes.TrimSpace(first))
	return c.GetCommit(ctx, repo, id)
	}

Note

One of the existing tests that I ported called out that code insights depended on the formatting of a specific error string in order to detect if the repository was empty.

(Test in question):

sourcegraph/internal/gitserver/commands_test.go

Lines 721 to 729 in 948848b

	// Added for awareness if this error message changes. Insights skip over empty repos and check against error message
	t.Run("empty repo", func(t *testing.T) {
	repo := MakeGitRepository(t)
	_, err := NewClient("test").FirstEverCommit(ctx, repo)
	wantErr := `git command [rev-list --reverse --date-order --max-parents=0 HEAD] failed (output: ""): exit status 128`
	if err.Error() != wantErr {
	t.Errorf("expected :%s, got :%s", wantErr, err)
	}
	})

(String checking logic in code insights):

https://github.com/sourcegraph/sourcegraph/blob/2443801b4ea4a4d48a8badf3c34ed2761af65db4/internal/insights/gitserver/first_commit.go#L18-L38

This is quite brittle, as any unrelated formatting change with our errors could change the semantics here. I decided to create a new sentinel error for this condition, gitdomain.RepoEmptyErr, to 1) better state our intent and 2) make this check more robust against minor formatting changes in our error libraries. When we generate the gRPC bindings, we'll need to create a new protobuf message for this error and write ToProto/FromProto functions, etc.

Test plan

Adapted existing unit tests

[ggilmore]

• searcher: remove unused diff.ParseGitDiffNameStatus package #62363
• gitserver: grpc: remove now unused DiffSymbols func #62360
• worker: embeddings: replace usage of old gitserver.DiffSymbols client method with new gitserver.ChangedFiles method #62359
• searcher: replace usage of gitserver.DiffSymbols with new gitserver.ChangedFiles client method #62358
• symbols: replace usage of gitserver client's DiffSymbols method with new ChangedFiles method #62355
• gitserver: grpc: add new ChangedFiles method to gitserver client #62354
• gitserver: grpc: create changed files RPC implementation for gRPC server #62262
• gitserver: grpc: port DiffSymbols from client as new GitBackend.ChangedFiles() func #62252
• gitserver: grpc: swap client implementation of GetAheadBehind from Exec to bespoke gRPC endpoint #62217
• gitserver: grpc: create server implementation for GetBehindAhead #62216
• gitserver: grpc: port GetBehindAhead from client to gitcli backend #62212
• codeinsights: remove now useless empty repo string error check #62302
• gitserver: grpc: switch clientImplementor to use new dedicated gRPC method for FirstEverCommit #62173
• gitserver: grpc: create FirstEverCommit RPC method and server implementation #62169
• gitserver: port FirstEverCommit functionality from client to git cli backend #62165 👈
• gitserver: grpc: create exhaustive logger for repository service #62158
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ggilmore and the rest of your teammates on Graphite

[ggilmore]

This exit condition comes from https://sourcegraph.com/github.com/git/git@bf995e7a4f94a9388aa8042dc9e338f3fcb75496/-/blob/builtin/rev-list.c?L701-706 in the source I believe.

[eseliger]

strange that it'd print the usage 😬 but thanks for the link, very helpful!

[eseliger]

This is quite brittle, as any unrelated formatting change with our errors could change the semantics here. I decided to create a new sentinel error for this condition, gitdomain.RepoEmptyErr, to 1) better state our intent and 2) make this check more robust against minor formatting changes in our error libraries. When we generate the gRPC bindings, we'll need to create a new protobuf message for this error and write ToProto/FromProto functions, etc.

Strongly agree that no caller should depend on any git error message. We want to fully own the process of potentially switching to go-git, libgit2, or any self-made implementations that will yield different error strings.

One question - in alignment with gRPC saying "there should not be too many different error codes, because callers will most likely not add more than a few checks", should we try to keep the error types in check as well, and repurpose the RevisionNotFoundError here?
I think it's technically a correct error, since FirstEverCommit would try to look at HEAD, HEAD resolves to no branch yet, so HEAD is not found?

[eseliger]

Should we remove this, after the gRPC API PR is merged? Once the client makes use of the gRPC API there's no way this could still be returned.

[eseliger]

Ping :)

[ggilmore]

#62302

[eseliger]

strange that it'd print the usage 😬 but thanks for the link, very helpful!

[eseliger]

Ping :)

[ggilmore]

Merge activity

• May 1, 12:14 AM EDT: @ggilmore merged this pull request with Graphite.