diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-08-08 12:05:29 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-08-08 12:34:43 +0300 |
commit | 2db323c12071fc8f1ff16f42ecadac09db0b95fd (patch) | |
tree | c4e05727c90c8deae42e9baa836dc2a526a99fec | |
parent | 1fa1a0565697a732452753923b455e59c02177c6 (diff) |
repository: Fix passing zero OID to WriteRef
In ceb9161b1 (repository: Resolve revisions passed to the WriteRef RPC,
2022-07-29) we have refactored the `WriteRef()` RPC to resolve both old
and new revision before executing git-update-ref(1) in order to ensure
that the objects we want to update to actually exist. This refactoring
broke the case though where the client passes the all-zeroes object ID
either as old or new reference, which both have a well-defined meaning
in this context.
Fix this bug by special-casing the all-zeroes object ID.
Changelog: fixed
-rw-r--r-- | internal/gitaly/service/repository/write_ref.go | 35 | ||||
-rw-r--r-- | internal/gitaly/service/repository/write_ref_test.go | 40 |
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)) + } }) } } |