gitserver: grpc: port DiffSymbols from client as new GitBackend.ChangedFiles() func

[ggilmore]

Part of #61691

This PR adds a new method, ChangedFiles to the git cli backend that lists the files that between the base and head commits, along with their added/modified/deleted status. Unlike the current workflow that involves multiple callsites with their own implementations for parsing the raw gitserver exec output, all the parsing now happens server-side and is well tested.

This functionality should be able to subsume the other diff-tree / diff use-cases in:

• Provide GQL API to list all files and change mode in a given diff #60654 and
• https://sourcegraph.slack.com/archives/C03K05FCRFH/p1714497813425009?thread_ts=1714480733.543729&cid=C03K05FCRFH

Test plan

Added new unit tests:

• some that test the E2E output of the git diff-tree command
• adapted tests from

  sourcegraph/cmd/symbols/gitserver/client_test.go

  Lines 9 to 61 in 458ce56

  	func TestParseGitDiffOutput(t *testing.T) {
  	testCases := []struct {
  	output []byte
  	expectedChanges Changes
  	shouldError bool
  	}{
  	{
  	output: combineBytes(
  	[]byte("A"), NUL, []byte("added1.json"), NUL,
  	[]byte("M"), NUL, []byte("modified1.json"), NUL,
  	[]byte("D"), NUL, []byte("deleted1.json"), NUL,
  	[]byte("A"), NUL, []byte("added2.json"), NUL,
  	[]byte("M"), NUL, []byte("modified2.json"), NUL,
  	[]byte("D"), NUL, []byte("deleted2.json"), NUL,
  	[]byte("A"), NUL, []byte("added3.json"), NUL,
  	[]byte("M"), NUL, []byte("modified3.json"), NUL,
  	[]byte("D"), NUL, []byte("deleted3.json"), NUL,
  	),
  	expectedChanges: Changes{
  	Added: []string{"added1.json", "added2.json", "added3.json"},
  	Modified: []string{"modified1.json", "modified2.json", "modified3.json"},
  	Deleted: []string{"deleted1.json", "deleted2.json", "deleted3.json"},
  	},
  	},
  	{
  	output: combineBytes(
  	[]byte("A"), NUL, []byte("added1.json"), NUL,
  	[]byte("M"), NUL, []byte("modified1.json"), NUL,
  	[]byte("D"), NUL,
  	),
  	shouldError: true,
  	},
  	{
  	output: []byte{},
  	},
  	}
  	for _, testCase := range testCases {
  	changes, err := parseGitDiffOutput(testCase.output)
  	if err != nil {
  	if !testCase.shouldError {
  	t.Fatalf("unexpected error parsing git diff output: %s", err)
  	}
  	} else if testCase.shouldError {
  	t.Fatalf("expected error, got none")
  	}
  	if diff := cmp.Diff(testCase.expectedChanges, changes); diff != "" {
  	t.Errorf("unexpected changes (-want +got):\n%s", diff)
  	}
  	}
  	}

[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]

@sourcegraph/source Could you please re-review this (and the upstream PRs)? I refactored the approach to use the iterator pattern.

[eseliger]

LGTM, left a question about using an iterator here as well

[eseliger]

The logic in LogReverseEach that we had here: https://github.com/sourcegraph/sourcegraph/pull/62260/files#diff-e7b89c0e3e24a2fda065a7cfa5f06f7b7def4d36189d88e495e5b40f5ca9795fL112-L194 also does some more checks on submodule status and stuff. Not sure if needed here, but might be cool for consolidation. We can do that separately though

[ggilmore]

Merge activity

• May 7, 1:32 PM EDT: @ggilmore merged this pull request with Graphite.