gitserver: Add option to allow setting custom context line count (#63840)

This PR adds support for two new arguments in the gitserver API:
InterHunkContext and ContextLines.
See the inline docstring for the two, but basically this allows to
request diffs with a custom amount of context lines and hunk merging.

The defaults should be unchanged at `3` for both, according to
https://sourcegraph.com/github.com/git/git/-/blob/diff.c?L60.

Closes SRC-467

Test plan:

Added unit tests.

Author: eseliger

cmd/gitserver/internal/git/gitcli/diff.go

@@ -3,6 +3,7 @@ package gitcli
 import (
 	"bufio"
 	"context"
+	"fmt"
 	"io"
 	"sync"
 
@@ -13,7 +14,7 @@ import (
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 )
 
-func (g *gitCLIBackend) RawDiff(ctx context.Context, base string, head string, typ git.GitDiffComparisonType, paths ...string) (io.ReadCloser, error) {
+func (g *gitCLIBackend) RawDiff(ctx context.Context, base string, head string, typ git.GitDiffComparisonType, opts git.RawDiffOpts, paths ...string) (io.ReadCloser, error) {
 	baseOID, err := g.revParse(ctx, base)
 	if err != nil {
 		return nil, err
@@ -58,12 +59,12 @@ func (g *gitCLIBackend) RawDiff(ctx context.Context, base string, head string, t
 		// we want.
 	}
 
-	args := buildRawDiffArgs(baseOID, headOID, paths)
+	args := buildRawDiffArgs(opts, baseOID, headOID, paths)
 
 	return g.NewCommand(ctx, WithArguments(args...))
 }
 
-func buildRawDiffArgs(base, head api.CommitID, paths []string) []string {
+func buildRawDiffArgs(opts git.RawDiffOpts, base, head api.CommitID, paths []string) []string {
 	return append([]string{
 		// Note: We use git diff-tree instead of git diff because git diff lets
 		// you diff any arbitrary files on disk, which is a security risk, diffing
@@ -72,7 +73,8 @@ func buildRawDiffArgs(base, head api.CommitID, paths []string) []string {
 		"--patch",
 		"--find-renames",
 		"--full-index",
-		"--inter-hunk-context=3",
+		fmt.Sprintf("--inter-hunk-context=%d", opts.InterHunkContext),
+		fmt.Sprintf("--unified=%d", opts.ContextLines),
 		"--no-prefix",
 		string(base),
 		string(head),

cmd/gitserver/internal/git/gitcli/diff_test.go

@@ -18,6 +18,10 @@ import (
 )
 
 func TestGitCLIBackend_RawDiff(t *testing.T) {
+	defaultOpts := git.RawDiffOpts{
+		InterHunkContext: 3,
+		ContextLines:     3,
+	}
 	var f1Diff = []byte(`diff --git f f
 index a29bdeb434d874c9b1d8969c40c42161b03fafdc..c0d0fb45c382919737f8d0c20aaf57cf89b74af8 100644
 --- f
@@ -48,15 +52,15 @@ index 0000000000000000000000000000000000000000..8a6a2d098ecaf90105f1cf2fa90fc460
 	)
 
 	t.Run("streams diff", func(t *testing.T) {
-		r, err := backend.RawDiff(ctx, "testbase", "HEAD", git.GitDiffComparisonTypeOnlyInHead)
+		r, err := backend.RawDiff(ctx, "testbase", "HEAD", git.GitDiffComparisonTypeOnlyInHead, defaultOpts)
 		require.NoError(t, err)
 		diff, err := io.ReadAll(r)
 		require.NoError(t, err)
 		require.NoError(t, r.Close())
 		require.Equal(t, string(f1Diff), string(diff))
 	})
 	t.Run("streams diff intersection", func(t *testing.T) {
-		r, err := backend.RawDiff(ctx, "testbase", "HEAD", git.GitDiffComparisonTypeIntersection)
+		r, err := backend.RawDiff(ctx, "testbase", "HEAD", git.GitDiffComparisonTypeIntersection, defaultOpts)
 		require.NoError(t, err)
 		diff, err := io.ReadAll(r)
 		require.NoError(t, err)
@@ -75,14 +79,49 @@ index 0000000000000000000000000000000000000000..8a6a2d098ecaf90105f1cf2fa90fc460
 			"git commit -m foo --author='Foo Author <foo@sourcegraph.com>'",
 		)
 
-		r, err := backend.RawDiff(ctx, "testbase", "HEAD", git.GitDiffComparisonTypeOnlyInHead, "f2")
+		r, err := backend.RawDiff(ctx, "testbase", "HEAD", git.GitDiffComparisonTypeOnlyInHead, defaultOpts, "f2")
 		require.NoError(t, err)
 		diff, err := io.ReadAll(r)
 		require.NoError(t, err)
 		require.NoError(t, r.Close())
 		// We expect only a diff for f2, not for f.
 		require.Equal(t, string(f2Diff), string(diff))
 	})
+	t.Run("custom context", func(t *testing.T) {
+		// Prepare repo state:
+		backend := BackendWithRepoCommands(t,
+			"echo 'line1\nline2\nline3\nlin4\nline5\nline6\nline7\nline8\n' > f",
+			"git add f",
+			"git commit -m foo --author='Foo Author <foo@sourcegraph.com>'",
+			"git tag testbase",
+			"echo 'line1.1\nline2\nline3\nlin4\nline5.5\nline6\nline7\nline8\n' > f",
+			"git add f",
+			"git commit -m foo --author='Foo Author <foo@sourcegraph.com>'",
+		)
+
+		var expectedDiff = []byte(`diff --git f f
+index 0ef51c52043997fdd257a0b77d761e9ca58bcc1f..58692a00a73d1f78df00014edf4ef39ef4ba0019 100644
+--- f
++++ f
+@@ -1 +1 @@
+-line1
++line1.1
+@@ -5 +5 @@ lin4
+-line5
++line5.5
+`)
+
+		r, err := backend.RawDiff(ctx, "testbase", "HEAD", git.GitDiffComparisonTypeOnlyInHead, git.RawDiffOpts{
+			InterHunkContext: 0,
+			ContextLines:     0,
+		})
+		require.NoError(t, err)
+		diff, err := io.ReadAll(r)
+		require.NoError(t, err)
+		require.NoError(t, r.Close())
+		t.Log(string(diff))
+		require.Equal(t, string(expectedDiff), string(diff))
+	})
 	t.Run("not found revspec", func(t *testing.T) {
 		// Prepare repo state:
 		backend := BackendWithRepoCommands(t,
@@ -92,19 +131,19 @@ index 0000000000000000000000000000000000000000..8a6a2d098ecaf90105f1cf2fa90fc460
 			"git tag test",
 		)
 
-		_, err := backend.RawDiff(ctx, "unknown", "test", git.GitDiffComparisonTypeOnlyInHead)
+		_, err := backend.RawDiff(ctx, "unknown", "test", git.GitDiffComparisonTypeOnlyInHead, defaultOpts)
 		require.Error(t, err)
 		require.True(t, errors.HasType[*gitdomain.RevisionNotFoundError](err))
 
-		_, err = backend.RawDiff(ctx, "test", "unknown", git.GitDiffComparisonTypeOnlyInHead)
+		_, err = backend.RawDiff(ctx, "test", "unknown", git.GitDiffComparisonTypeOnlyInHead, defaultOpts)
 		require.Error(t, err)
 		require.True(t, errors.HasType[*gitdomain.RevisionNotFoundError](err))
 	})
 	t.Run("files outside repository", func(t *testing.T) {
 		// We use git-diff-tree, but with git-diff you can diff any files on disk
 		// which is dangerous. So we have this safeguard test here in place to
 		// make sure we don't regress on that.
-		r, err := backend.RawDiff(ctx, "testbase", "HEAD", git.GitDiffComparisonTypeOnlyInHead, "/dev/null", "/etc/hosts")
+		r, err := backend.RawDiff(ctx, "testbase", "HEAD", git.GitDiffComparisonTypeOnlyInHead, defaultOpts, "/dev/null", "/etc/hosts")
 		require.NoError(t, err)
 		_, err = io.ReadAll(r)
 		require.Error(t, err)
@@ -115,7 +154,7 @@ index 0000000000000000000000000000000000000000..8a6a2d098ecaf90105f1cf2fa90fc460
 		ctx, cancel := context.WithCancel(ctx)
 		t.Cleanup(cancel)
 
-		r, err := backend.RawDiff(ctx, "testbase", "HEAD", git.GitDiffComparisonTypeOnlyInHead)
+		r, err := backend.RawDiff(ctx, "testbase", "HEAD", git.GitDiffComparisonTypeOnlyInHead, defaultOpts)
 		require.NoError(t, err)
 
 		cancel()

cmd/gitserver/internal/git/iface.go

@@ -87,11 +87,11 @@ type GitBackend interface {
 	RevAtTime(ctx context.Context, revspec string, time time.Time) (api.CommitID, error)
 	// RawDiff returns the raw git diff for the given range.
 	// Diffs returned from this function will have the following settings applied:
-	// - 3 lines of context
+	// - N lines of context according to opts
 	// - No a/ b/ prefixes
 	// - Rename detection
 	// If either base or head don't exist, a RevisionNotFoundError is returned.
-	RawDiff(ctx context.Context, base string, head string, typ GitDiffComparisonType, paths ...string) (io.ReadCloser, error)
+	RawDiff(ctx context.Context, base string, head string, typ GitDiffComparisonType, opts RawDiffOpts, paths ...string) (io.ReadCloser, error)
 	// ContributorCounts returns the number of commits per contributor in the
 	// set of commits specified by the options.
 	// Aggregations are done by email address.
@@ -355,3 +355,17 @@ type ReadDirIterator interface {
 	// Close closes the iterator.
 	Close() error
 }
+
+// RawDiffOpts contaions extra options for the RawDiff method.
+type RawDiffOpts struct {
+	// InterHunkContext specifies the number of lines to consider for fusing hunks
+	// together. I.e., when set to 5 and between 2 hunks there are at most 5 lines,
+	// the 2 hunks will be fused together into a single chunk.
+	InterHunkContext int
+	// ContextLines specifies the number of lines of context to show around added/removed
+	// lines.
+	// This is the number of lines that will be shown before and after each line that
+	// has been added/removed. If InterHunkContext is not zero, the context will still
+	// be fused together with other hunks if they meet the threshold.
+	ContextLines int
+}

cmd/gitserver/internal/git/mock.go

@@ -316,14 +316,14 @@ func (hr *observableRefIterator) Close() error {
 	return err
 }
 
-func (b *observableBackend) RawDiff(ctx context.Context, base string, head string, typ GitDiffComparisonType, paths ...string) (_ io.ReadCloser, err error) {
+func (b *observableBackend) RawDiff(ctx context.Context, base string, head string, typ GitDiffComparisonType, opts RawDiffOpts, paths ...string) (_ io.ReadCloser, err error) {
 	ctx, errCollector, endObservation := b.operations.rawDiff.WithErrors(ctx, &err, observation.Args{})
 	ctx, cancel := context.WithCancel(ctx)
 	endObservation.OnCancel(ctx, 1, observation.Args{})
 
 	concurrentOps.WithLabelValues("RawDiff").Inc()
 
-	r, err := b.backend.RawDiff(ctx, base, head, typ, paths...)
+	r, err := b.backend.RawDiff(ctx, base, head, typ, opts, paths...)
 	if err != nil {
 		concurrentOps.WithLabelValues("RawDiff").Dec()
 		cancel()

cmd/gitserver/internal/git/observability.go

@@ -1175,7 +1175,20 @@ func (gs *grpcServer) RawDiff(req *proto.RawDiffRequest, ss proto.GitserverServi
 		typ = git.GitDiffComparisonTypeOnlyInHead
 	}
 
-	r, err := backend.RawDiff(ctx, string(req.GetBaseRevSpec()), string(req.GetHeadRevSpec()), typ, paths...)
+	opts := git.RawDiffOpts{
+		InterHunkContext: 3,
+		ContextLines:     3,
+	}
+
+	if req.InterHunkContext != nil {
+		opts.InterHunkContext = int(*req.InterHunkContext)
+	}
+
+	if req.ContextLines != nil {
+		opts.ContextLines = int(*req.ContextLines)
+	}
+
+	r, err := backend.RawDiff(ctx, string(req.GetBaseRevSpec()), string(req.GetHeadRevSpec()), typ, opts, paths...)
 	if err != nil {
 		var e *gitdomain.RevisionNotFoundError
 		if errors.As(err, &e) {

cmd/gitserver/internal/server_grpc.go

@@ -887,6 +887,8 @@ func rawDiffRequestToLogFields(req *proto.RawDiffRequest) []log.Field {
 		log.String("headRevSpec", string(req.GetHeadRevSpec())),
 		log.String("comparisonType", req.GetComparisonType().String()),
 		log.Strings("paths", byteSlicesToStrings(req.GetPaths())),
+		log.Int("interHunkContext", int(req.GetInterHunkContext())),
+		log.Int("contextLines", int(req.GetContextLines())),
 	}
 }
 

cmd/gitserver/internal/server_grpc_logger.go

@@ -77,6 +77,7 @@ go_test(
         "//internal/gitserver/v1:gitserver",
         "//internal/grpc",
         "//lib/errors",
+        "//lib/pointers",
         "//schema",
         "@com_github_derision_test_go_mockgen_v2//testutil/require",
         "@com_github_google_go_cmp//cmp",

internal/gitserver/BUILD.bazel

@@ -323,6 +323,22 @@ type DiffOptions struct {
 	// NOTE: Rename detection does not work if only the old path or the new path
 	// is specified in this slice.
 	Paths []string
+
+	// InterHunkContext specifies the number of lines to consider for fusing hunks
+	// together. I.e., when set to 5 and between 2 hunks there are at most 5 lines,
+	// the 2 hunks will be fused together into a single chunk.
+	//
+	// The default for this is 3.
+	InterHunkContext *int
+
+	// ContextLines specifies the number of lines of context to show around added/removed
+	// lines.
+	// This is the number of lines that will be shown before and after each line that
+	// has been added/removed. If InterHunkContext is not zero, the context will still
+	// be fused together with other hunks if they meet the threshold.
+	//
+	// The default for this is 3.
+	ContextLines *int
 }
 
 type Client interface {

internal/gitserver/client.go

@@ -45,6 +45,14 @@ func (c *clientImplementor) Diff(ctx context.Context, repo api.RepoName, opts Di
 		Paths:       stringsToByteSlices(opts.Paths),
 	}
 
+	if opts.InterHunkContext != nil {
+		req.InterHunkContext = pointers.Ptr(uint32(*opts.InterHunkContext))
+	}
+
+	if opts.ContextLines != nil {
+		req.ContextLines = pointers.Ptr(uint32(*opts.ContextLines))
+	}
+
 	// Rare case: the base is the empty tree, in which case we must use ..
 	// instead of ... as the latter only works for commits.
 	if opts.Base == DevNullSHA {