gitserver: add proto round trip test for endpoints that use grpc (#53125)

Create a proto round trip test for all endpoints in gitserver to insure
that use of ToProto() and FromProto() in the protocol package is
converting structs correctly

## Test plan
unit test
<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->

internal/gitserver/BUILD.bazel

@@ -84,6 +84,7 @@ go_test(
         "//internal/database",
         "//internal/errcode",
         "//internal/extsvc",
+        "//internal/extsvc/gitolite",
         "//internal/gitserver/gitdomain",
         "//internal/gitserver/protocol",
         "//internal/gitserver/v1:gitserver",
@@ -91,7 +92,6 @@ go_test(
         "//internal/grpc/defaults",
         "//internal/httpcli",
         "//internal/observation",
-        "//internal/randstring",
         "//internal/types",
         "//lib/errors",
         "//schema",

internal/gitserver/client_test.go

@@ -38,6 +38,7 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/conf"
 	"github.com/sourcegraph/sourcegraph/internal/database"
 	"github.com/sourcegraph/sourcegraph/internal/extsvc"
+	"github.com/sourcegraph/sourcegraph/internal/extsvc/gitolite"
 	"github.com/sourcegraph/sourcegraph/internal/gitserver"
 	"github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain"
 	"github.com/sourcegraph/sourcegraph/internal/gitserver/protocol"
@@ -46,7 +47,6 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/grpc/defaults"
 	"github.com/sourcegraph/sourcegraph/internal/httpcli"
 	"github.com/sourcegraph/sourcegraph/internal/observation"
-	"github.com/sourcegraph/sourcegraph/internal/randstring"
 	"github.com/sourcegraph/sourcegraph/internal/types"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 	"github.com/sourcegraph/sourcegraph/schema"
@@ -71,10 +71,10 @@ func newMockDB() database.DB {
 	return db
 }
 
-func TestProtoRoundTrip(t *testing.T) {
+func TestClient_Archive_ProtoRoundTrip(t *testing.T) {
 	var diff string
 
-	a := func(original gitserver.ArchiveOptions) bool {
+	fn := func(original gitserver.ArchiveOptions) bool {
 
 		var converted gitserver.ArchiveOptions
 		converted.FromProto(original.ToProto("test"))
@@ -86,11 +86,15 @@ func TestProtoRoundTrip(t *testing.T) {
 		return true
 	}
 
-	if err := quick.Check(a, nil); err != nil {
+	if err := quick.Check(fn, nil); err != nil {
 		t.Errorf("ArchiveOptions proto roundtrip failed (-want +got):\n%s", diff)
 	}
+}
 
-	c := func(original protocol.IsRepoCloneableResponse) bool {
+func TestClient_IsRepoCloneale_ProtoRoundTrip(t *testing.T) {
+	var diff string
+
+	fn := func(original protocol.IsRepoCloneableResponse) bool {
 		var converted protocol.IsRepoCloneableResponse
 		converted.FromProto(original.ToProto())
 
@@ -101,13 +105,17 @@ func TestProtoRoundTrip(t *testing.T) {
 		return true
 	}
 
-	if err := quick.Check(c, nil); err != nil {
+	if err := quick.Check(fn, nil); err != nil {
 		t.Errorf("IsRepoCloneableResponse proto roundtrip failed (-want +got):\n%s", diff)
 	}
+}
 
-	rs := func(updatedAt time.Time, gitDirBytes int64) bool {
+func TestClient_RepoStats_ProtoRoundTrip(t *testing.T) {
+	var diff string
+
+	fn := func(updatedAt fuzzTime, gitDirBytes int64) bool {
 		original := protocol.ReposStats{
-			UpdatedAt:   updatedAt,
+			UpdatedAt:   time.Time(updatedAt),
 			GitDirBytes: gitDirBytes,
 		}
 
@@ -121,29 +129,177 @@ func TestProtoRoundTrip(t *testing.T) {
 		return true
 	}
 
-	// Define the generator for time.Time values
-	timeGenerator := func(rand *rand.Rand, size int) reflect.Value {
-		min := time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC)
-		max := time.Now()
-		delta := max.Unix() - min.Unix()
-		sec := rand.Int63n(delta) + min.Unix()
-		return reflect.ValueOf(time.Unix(sec, 0))
-	}
-
-	if err := quick.Check(rs, &quick.Config{
-		Values: func(args []reflect.Value, rand *rand.Rand) {
-			args[0] = timeGenerator(rand, 0)
-			args[1] = reflect.ValueOf(rand.Int63())
-		},
-	}); err != nil {
+	if err := quick.Check(fn, nil); err != nil {
 		t.Errorf("ReposStats proto roundtrip failed (-want +got):\n%s", diff)
 	}
-	durationGenerator := func(rand *rand.Rand, size int) reflect.Value {
-		return reflect.ValueOf(time.Duration(rand.Int63()))
-	}
+}
 
-	ruReq := func(original protocol.RepoUpdateRequest) bool {
-		var converted protocol.RepoUpdateRequest
+func TestClient_RepoUpdateRequest_ProtoRoundTrip(t *testing.T) {
+	var diff string
+	t.Run("request", func(t *testing.T) {
+		fn := func(repo api.RepoName, since int64, cloneFromShard string) bool {
+			original := protocol.RepoUpdateRequest{
+				Repo:           repo,
+				Since:          time.Duration(since),
+				CloneFromShard: cloneFromShard,
+			}
+
+			var converted protocol.RepoUpdateRequest
+			converted.FromProto(original.ToProto())
+
+			if diff = cmp.Diff(original, converted); diff != "" {
+				return false
+			}
+
+			return true
+		}
+
+		if err := quick.Check(fn, nil); err != nil {
+			t.Errorf("RepoUpdateRequest proto roundtrip failed (-want +got):\n%s", diff)
+		}
+	})
+
+	t.Run("response", func(t *testing.T) {
+		fn := func(lastFetched fuzzTime, lastChanged fuzzTime, err string) bool {
+			lastFetchedPtr := time.Time(lastFetched)
+			lastChangedPtr := time.Time(lastChanged)
+
+			original := protocol.RepoUpdateResponse{
+				LastFetched: &lastFetchedPtr,
+				LastChanged: &lastChangedPtr,
+				Error:       err,
+			}
+			var converted protocol.RepoUpdateResponse
+			converted.FromProto(original.ToProto())
+
+			if diff = cmp.Diff(original, converted); diff != "" {
+				return false
+			}
+
+			return true
+		}
+
+		if err := quick.Check(fn, nil); err != nil {
+			t.Errorf("RepoUpdateResponse proto roundtrip failed (-want +got):\n%s", diff)
+		}
+	})
+}
+
+func TestClient_CreateCommitFromPatchRequest_ProtoRoundTrip(t *testing.T) {
+	var diff string
+
+	t.Run("request", func(t *testing.T) {
+		fn := func(
+			repo string,
+			baseCommit string,
+			patch []byte,
+			targetRef string,
+			uniqueRef bool,
+			pushRef *string,
+
+			commitInfo struct {
+				Messages    []string
+				AuthorName  string
+				AuthorEmail string
+				Date        fuzzTime
+			},
+
+			pushConfig *protocol.PushConfig,
+			gitApplyArgs []string,
+		) bool {
+			original := protocol.CreateCommitFromPatchRequest{
+				Repo:       api.RepoName(repo),
+				BaseCommit: api.CommitID(baseCommit),
+				Patch:      patch,
+				TargetRef:  targetRef,
+				UniqueRef:  uniqueRef,
+				CommitInfo: protocol.PatchCommitInfo{
+					Messages:    commitInfo.Messages,
+					AuthorName:  commitInfo.AuthorName,
+					AuthorEmail: commitInfo.AuthorEmail,
+					Date:        time.Time(commitInfo.Date),
+				},
+				Push:         pushConfig,
+				PushRef:      pushRef,
+				GitApplyArgs: gitApplyArgs,
+			}
+			var converted protocol.CreateCommitFromPatchRequest
+			converted.FromProto(original.ToProto())
+
+			if diff = cmp.Diff(original, converted); diff != "" {
+				return false
+			}
+
+			return true
+		}
+
+		if err := quick.Check(fn, nil); err != nil {
+			t.Errorf("CreateCommitFromPatchRequest proto roundtrip failed (-want +got):\n%s", diff)
+		}
+	})
+
+	t.Run("response", func(t *testing.T) {
+		fn := func(original protocol.CreateCommitFromPatchResponse) bool {
+			var converted protocol.CreateCommitFromPatchResponse
+			converted.FromProto(original.ToProto())
+
+			if diff = cmp.Diff(original, converted); diff != "" {
+				return false
+			}
+
+			return true
+		}
+
+		if err := quick.Check(fn, nil); err != nil {
+			t.Errorf("CreateCommitFromPatchResponse proto roundtrip failed (-want +got):\n%s", diff)
+		}
+	})
+}
+
+func TestClient_BatchLog_ProtoRoundTrip(t *testing.T) {
+	var diff string
+
+	t.Run("request", func(t *testing.T) {
+		fn := func(original protocol.BatchLogRequest) bool {
+			var converted protocol.BatchLogRequest
+			converted.FromProto(original.ToProto())
+
+			if diff = cmp.Diff(original, converted); diff != "" {
+				return false
+			}
+
+			return true
+		}
+
+		if err := quick.Check(fn, nil); err != nil {
+			t.Errorf("BatchChangesLogResponse proto roundtrip failed (-want +got):\n%s", diff)
+		}
+	})
+
+	t.Run("response", func(t *testing.T) {
+		fn := func(original protocol.BatchLogResponse) bool {
+			var converted protocol.BatchLogResponse
+			converted.FromProto(original.ToProto())
+
+			if diff = cmp.Diff(original, converted); diff != "" {
+				return false
+			}
+
+			return true
+		}
+
+		if err := quick.Check(fn, nil); err != nil {
+			t.Errorf("BatchChangesLogResponse proto roundtrip failed (-want +got):\n%s", diff)
+		}
+	})
+
+}
+
+func TestClient_RepoCloneProgress_ProtoRoundTrip(t *testing.T) {
+	var diff string
+
+	fn := func(original protocol.RepoCloneProgress) bool {
+		var converted protocol.RepoCloneProgress
 		converted.FromProto(original.ToProto())
 
 		if diff = cmp.Diff(original, converted); diff != "" {
@@ -153,20 +309,16 @@ func TestProtoRoundTrip(t *testing.T) {
 		return true
 	}
 
-	if err := quick.Check(ruReq, &quick.Config{
-		Values: func(args []reflect.Value, rand *rand.Rand) {
-			args[0] = reflect.ValueOf(protocol.RepoUpdateRequest{
-				Repo:           api.RepoName(fmt.Sprintf("repo-%d", rand.Int())),
-				Since:          durationGenerator(rand, 0).Interface().(time.Duration),
-				CloneFromShard: randstring.NewLen(10),
-			})
-		},
-	}); err != nil {
-		t.Errorf("RepoUpdateRequest proto roundtrip failed (-want +got):\n%s", diff)
+	if err := quick.Check(fn, nil); err != nil {
+		t.Errorf("RepoCloneProgress proto roundtrip failed (-want +got):\n%s", diff)
 	}
+}
 
-	ruRes := func(original protocol.RepoUpdateResponse) bool {
-		var converted protocol.RepoUpdateResponse
+func TestClient_P4ExecRequest_ProtoRoundTrip(t *testing.T) {
+	var diff string
+
+	fn := func(original protocol.P4ExecRequest) bool {
+		var converted protocol.P4ExecRequest
 		converted.FromProto(original.ToProto())
 
 		if diff = cmp.Diff(original, converted); diff != "" {
@@ -176,33 +328,47 @@ func TestProtoRoundTrip(t *testing.T) {
 		return true
 	}
 
-	if err := quick.Check(ruRes, &quick.Config{
-		Values: func(args []reflect.Value, rand *rand.Rand) {
-
-			lastFetched := timeGenerator(rand, 0).Interface().(time.Time)
-			lastChangedAt := timeGenerator(rand, 0).Interface().(time.Time)
-
-			args[0] = reflect.ValueOf(protocol.RepoUpdateResponse{
-				LastFetched: &lastFetched,
-				LastChanged: &lastChangedAt,
-				Error:       randstring.NewLen(10),
-			})
-		},
-	}); err != nil {
-		t.Errorf("RepoUpdateResponse proto roundtrip failed (-want +got):\n%s", diff)
+	if err := quick.Check(fn, nil); err != nil {
+		t.Errorf("P4ExecRequest proto roundtrip failed (-want +got):\n%s", diff)
 	}
+}
 
-	createCommit := func(original protocol.CreateCommitFromPatchResponse) bool {
-		var converted protocol.CreateCommitFromPatchResponse
+func TestClient_RepoClone_ProtoRoundTrip(t *testing.T) {
+	var diff string
+
+	fn := func(original protocol.RepoCloneResponse) bool {
+		var converted protocol.RepoCloneResponse
 		converted.FromProto(original.ToProto())
 
-		return cmp.Diff(original, converted) == ""
+		if diff = cmp.Diff(original, converted); diff != "" {
+			return false
+		}
+
+		return true
 	}
 
-	if err := quick.Check(createCommit, nil); err != nil {
-		t.Errorf("CreateCommitFromPatchRequest proto roundtrip failed (-want +got):\n%s", diff)
+	if err := quick.Check(fn, nil); err != nil {
+		t.Errorf("RepoCloneResponse proto roundtrip failed (-want +got):\n%s", diff)
+	}
+}
+
+func TestClient_ListGitolite_ProtoRoundTrip(t *testing.T) {
+	var diff string
+
+	fn := func(original gitolite.Repo) bool {
+		var converted gitolite.Repo
+		converted.FromProto(original.ToProto())
+
+		if diff = cmp.Diff(original, converted); diff != "" {
+			return false
+		}
+
+		return true
 	}
 
+	if err := quick.Check(fn, nil); err != nil {
+		t.Errorf("ListGitoliteRepo proto roundtrip failed (-want +got):\n%s", diff)
+	}
 }
 
 func TestClient_Remove(t *testing.T) {
@@ -1527,3 +1693,15 @@ func (mc *mockClient) Archive(ctx context.Context, in *proto.ArchiveRequest, opt
 var _ proto.GitserverServiceClient = &mockClient{}
 
 var _ proto.GitserverService_P4ExecClient = &mockP4ExecClient{}
+
+type fuzzTime time.Time
+
+func (fuzzTime) Generate(rand *rand.Rand, _ int) reflect.Value {
+	// The maximum representable year in RFC 3339 is 9999, so we'll use that as our upper bound.
+	maxDate := time.Date(9999, 1, 1, 0, 0, 0, 0, time.UTC)
+
+	ts := time.Unix(rand.Int63n(maxDate.Unix()), rand.Int63n(int64(time.Second)))
+	return reflect.ValueOf(fuzzTime(ts))
+}
+
+var _ quick.Generator = fuzzTime{}

internal/gitserver/protocol/gitserver.go

@@ -339,11 +339,20 @@ type BatchLogResult struct {
 }
 
 func (bl *BatchLogResult) ToProto() *proto.BatchLogResult {
-	return &proto.BatchLogResult{
+	result := &proto.BatchLogResult{
 		RepoCommit:    bl.RepoCommit.ToProto(),
 		CommandOutput: bl.CommandOutput,
-		CommandError:  &bl.CommandError,
 	}
+
+	var cmdErr string
+
+	if bl.CommandError != "" {
+		cmdErr = bl.CommandError
+		result.CommandError = &cmdErr
+	}
+
+	return result
+
 }
 
 func (bl *BatchLogResult) FromProto(p *proto.BatchLogResult) {
@@ -568,6 +577,14 @@ type RepoCloneProgress struct {
 	Cloned          bool   // whether the repository has been cloned successfully
 }
 
+func (r *RepoCloneProgress) ToProto() *proto.RepoCloneProgress {
+	return &proto.RepoCloneProgress{
+		CloneInProgress: r.CloneInProgress,
+		CloneProgress:   r.CloneProgress,
+		Cloned:          r.Cloned,
+	}
+}
+
 func (r *RepoCloneProgress) FromProto(p *proto.RepoCloneProgress) {
 	*r = RepoCloneProgress{
 		CloneInProgress: p.GetCloneInProgress(),
@@ -636,20 +653,32 @@ type CreateCommitFromPatchRequest struct {
 }
 
 func (c *CreateCommitFromPatchRequest) ToProto() *proto.CreateCommitFromPatchBinaryRequest {
-	return &proto.CreateCommitFromPatchBinaryRequest{
+	cc := &proto.CreateCommitFromPatchBinaryRequest{
 		Repo:         string(c.Repo),
 		BaseCommit:   string(c.BaseCommit),
 		Patch:        c.Patch,
 		TargetRef:    c.TargetRef,
 		UniqueRef:    c.UniqueRef,
 		CommitInfo:   c.CommitInfo.ToProto(),
-		Push:         c.Push.ToProto(),
 		GitApplyArgs: c.GitApplyArgs,
 		PushRef:      c.PushRef,
 	}
+
+	if c.Push != nil {
+		cc.Push = c.Push.ToProto()
+	}
+
+	return cc
 }
 
 func (c *CreateCommitFromPatchRequest) FromProto(p *proto.CreateCommitFromPatchBinaryRequest) {
+	gp := p.GetPush()
+	var pushConfig *PushConfig
+	if gp != nil {
+		pushConfig = &PushConfig{}
+		pushConfig.FromProto(gp)
+	}
+
 	*c = CreateCommitFromPatchRequest{
 		Repo:         api.RepoName(p.GetRepo()),
 		BaseCommit:   api.CommitID(p.GetBaseCommit()),
@@ -657,9 +686,12 @@ func (c *CreateCommitFromPatchRequest) FromProto(p *proto.CreateCommitFromPatchB
 		TargetRef:    p.GetTargetRef(),
 		UniqueRef:    p.GetUniqueRef(),
 		CommitInfo:   PatchCommitInfoFromProto(p.GetCommitInfo()),
-		Push:         PushConfigFromProto(p.GetPush()),
+		Push:         pushConfig,
 		GitApplyArgs: p.GetGitApplyArgs(),
-		PushRef:      p.PushRef,
+	}
+
+	if p != nil {
+		c.PushRef = p.PushRef
 	}
 }
 
@@ -714,6 +746,10 @@ type PushConfig struct {
 }
 
 func (p *PushConfig) ToProto() *proto.PushConfig {
+	if p == nil {
+		return nil
+	}
+
 	return &proto.PushConfig{
 		RemoteUrl:  p.RemoteURL,
 		PrivateKey: p.PrivateKey,
@@ -721,11 +757,8 @@ func (p *PushConfig) ToProto() *proto.PushConfig {
 	}
 }
 
-func PushConfigFromProto(p *proto.PushConfig) *PushConfig {
-	if p == nil {
-		return nil
-	}
-	return &PushConfig{
+func (pc *PushConfig) FromProto(p *proto.PushConfig) {
+	*pc = PushConfig{
 		RemoteURL:  p.GetRemoteUrl(),
 		PrivateKey: p.GetPrivateKey(),
 		Passphrase: p.GetPassphrase(),

internal/gitserver/v1/gitserver.pb.go

@@ -545,8 +545,9 @@ type CreateCommitFromPatchBinaryRequest struct {
 	// commit_info is the information to be used for the commit
 	CommitInfo *PatchCommitInfo `protobuf:"bytes,6,opt,name=commit_info,json=commitInfo,proto3" json:"commit_info,omitempty"`
 	// push_config is the configuration to be used for pushing the commit
-	Push         *PushConfig `protobuf:"bytes,7,opt,name=push,proto3" json:"push,omitempty"`
-	GitApplyArgs []string    `protobuf:"bytes,8,rep,name=git_apply_args,json=gitApplyArgs,proto3" json:"git_apply_args,omitempty"`
+	Push *PushConfig `protobuf:"bytes,7,opt,name=push,proto3" json:"push,omitempty"`
+	// git_apply_args are the arguments to be passed to git apply
+	GitApplyArgs []string `protobuf:"bytes,8,rep,name=git_apply_args,json=gitApplyArgs,proto3" json:"git_apply_args,omitempty"`
 	// push_ref is the optional override for the ref that is pushed to
 	PushRef *string `protobuf:"bytes,9,opt,name=push_ref,json=pushRef,proto3,oneof" json:"push_ref,omitempty"`
 }

internal/gitserver/v1/gitserver.proto

@@ -102,6 +102,7 @@ message CreateCommitFromPatchBinaryRequest {
   PatchCommitInfo commit_info = 6;
   // push_config is the configuration to be used for pushing the commit
   PushConfig push = 7;
+  // git_apply_args are the arguments to be passed to git apply
   repeated string git_apply_args = 8;
   // push_ref is the optional override for the ref that is pushed to
   optional string push_ref = 9;