Merge branch 'pks-write-ref-fix-resolve-revision' into 'master'

repository: Fix passing zero OID to WriteRef

See merge request gitlab-org/gitaly!4794

2 files changed, 61 insertions, 14 deletions

diff --git a/internal/gitaly/service/repository/write_ref.go b/internal/gitaly/service/repository/write_ref.go
index 32271acfa..e0a83f561 100644
--- a/internal/gitaly/service/repository/write_ref.go
+++ b/internal/gitaly/service/repository/write_ref.go
@@ -38,20 +38,35 @@ func (s *server) writeRef(ctx context.Context, req *gitalypb.WriteRefRequest) er
 }
 
 func updateRef(ctx context.Context, repo *localrepo.Repo, req *gitalypb.WriteRefRequest) error {
-	// We need to resolve the new revision in order to make sure that we're actually passing an
-	// object ID to git-update-ref(1), but more importantly this will also ensure that the
-	// object ID we're updating to actually exists. Note that we also verify that the object
-	// actually exists in the repository by adding "^{object}".
-	newObjectID, err := repo.ResolveRevision(ctx, git.Revision(req.GetRevision())+"^{object}")
-	if err != nil {
-		return fmt.Errorf("resolving new revision: %w", err)
+	var newObjectID git.ObjectID
+	if git.ObjectHashSHA1.IsZeroOID(git.ObjectID(req.GetRevision())) {
+		// Passing the all-zeroes object ID as new value means that we should delete the
+		// reference.
+		newObjectID = git.ObjectHashSHA1.ZeroOID
+	} else {
+		// We need to resolve the new revision in order to make sure that we're actually
+		// passing an object ID to git-update-ref(1), but more importantly this will also
+		// ensure that the object ID we're updating to actually exists. Note that we also
+		// verify that the object actually exists in the repository by adding "^{object}".
+		var err error
+		newObjectID, err = repo.ResolveRevision(ctx, git.Revision(req.GetRevision())+"^{object}")
+		if err != nil {
+			return fmt.Errorf("resolving new revision: %w", err)
+		}
 	}
 
 	var oldObjectID git.ObjectID
 	if len(req.GetOldRevision()) > 0 {
-		oldObjectID, err = repo.ResolveRevision(ctx, git.Revision(req.GetOldRevision())+"^{object}")
-		if err != nil {
-			return fmt.Errorf("resolving old revision: %w", err)
+		if git.ObjectHashSHA1.IsZeroOID(git.ObjectID(req.GetOldRevision())) {
+			// Passing an all-zeroes object ID indicates that we should only update the
+			// reference if it didn't previously exist.
+			oldObjectID = git.ObjectHashSHA1.ZeroOID
+		} else {
+			var err error
+			oldObjectID, err = repo.ResolveRevision(ctx, git.Revision(req.GetOldRevision())+"^{object}")
+			if err != nil {
+				return fmt.Errorf("resolving old revision: %w", err)
+			}
 		}
 	}
 
diff --git a/internal/gitaly/service/repository/write_ref_test.go b/internal/gitaly/service/repository/write_ref_test.go
index cb6262c27..4434bf0f6 100644
--- a/internal/gitaly/service/repository/write_ref_test.go
+++ b/internal/gitaly/service/repository/write_ref_test.go
@@ -8,9 +8,11 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
@@ -58,6 +60,26 @@ func TestWriteRef_successful(t *testing.T) {
 			},
 			expectedVotes: 2,
 		},
+		{
+			desc: "race-free creation of reference",
+			req: &gitalypb.WriteRefRequest{
+				Repository:  repo,
+				Ref:         []byte("refs/heads/branch"),
+				Revision:    []byte("498214de67004b1da3d820901307bed2a68a8ef6"),
+				OldRevision: []byte("0000000000000000000000000000000000000000"),
+			},
+			expectedVotes: 2,
+		},
+		{
+			desc: "race-free delete of reference",
+			req: &gitalypb.WriteRefRequest{
+				Repository:  repo,
+				Ref:         []byte("refs/heads/branch"),
+				Revision:    []byte("0000000000000000000000000000000000000000"),
+				OldRevision: []byte("498214de67004b1da3d820901307bed2a68a8ef6"),
+			},
+			expectedVotes: 2,
+		},
 	}
 
 	ctx, err := txinfo.InjectTransaction(testhelper.Context(t), 1, "node", true)
@@ -80,11 +102,21 @@ func TestWriteRef_successful(t *testing.T) {
 				require.EqualValues(t, refRevision, content)
 				return
 			}
-			rev := gittest.Exec(t, cfg, "--git-dir", repoPath, "log", "--pretty=%H", "-1", string(tc.req.Ref))
-
-			rev = bytes.Replace(rev, []byte("\n"), nil, 1)
 
-			require.Equal(t, string(tc.req.Revision), string(rev))
+			revParseCmd := gittest.NewCommand(t, cfg, "-C", repoPath, "rev-parse", "--verify", string(tc.req.Ref))
+			output, err := revParseCmd.CombinedOutput()
+
+			if git.ObjectHashSHA1.IsZeroOID(git.ObjectID(tc.req.GetRevision())) {
+				// If the new OID is the all-zeroes object ID then it indicates we
+				// should delete the branch. It's thus expected to get an error when
+				// we try to resolve the reference here given that it should not
+				// exist anymore.
+				require.Error(t, err)
+				require.Equal(t, "fatal: Needed a single revision\n", string(output))
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, string(tc.req.Revision), text.ChompBytes(output))
+			}
 		})
 	}
 }