# HG changeset patch # User Raphaël Gomès # Date 1617270545 -7200 # Thu Apr 01 11:49:05 2021 +0200 # Node ID 3e4fc8ae2aa97c641489addf1bbc4ac40f978783 # Parent 901e292c6365edfe3e54b8d2e2f10b39f2038dcf # EXP-Topic write-ref.old-revision Explicitly handle `None` refs in some utils This potentially simplifies the logic and removes a bug (thankfully only theoretical) from `gitlab_special_ref_target` diff --git a/hgitaly/branch.py b/hgitaly/branch.py --- a/hgitaly/branch.py +++ b/hgitaly/branch.py @@ -78,6 +78,9 @@ :return: a :class:`changectx` or ``None`` if there's no changeset for the given ``gl_branch``. """ + if gl_branch is None: + return None + gl_branches = gitlab_branches(repo) if gl_branches is not GITLAB_BRANCHES_MISSING: sha = gl_branches.get(gl_branch) diff --git a/hgitaly/gitlab_ref.py b/hgitaly/gitlab_ref.py --- a/hgitaly/gitlab_ref.py +++ b/hgitaly/gitlab_ref.py @@ -28,6 +28,8 @@ :returns: a :class:`changectx` instance, for the unfiltered version of ``repo``. The changeset itself may be obsolete. """ + if ref_path is None: + return None name = parse_special_ref(ref_path) if name is None: return None # HG changeset patch # User Raphaël Gomès # Date 1617270622 -7200 # Thu Apr 01 11:50:22 2021 +0200 # Node ID 2a1f333052319f07b1faacaf1f37836214d89687 # Parent 3e4fc8ae2aa97c641489addf1bbc4ac40f978783 # EXP-Topic write-ref.old-revision Add TODOs explaining why we're using `try... except Exception` Both where it's defined and in the test that uses this path. diff --git a/hgitaly/service/repository_service.py b/hgitaly/service/repository_service.py --- a/hgitaly/service/repository_service.py +++ b/hgitaly/service/repository_service.py @@ -185,6 +185,10 @@ create_keep_around(repo, target) return WriteRefResponse() except Exception: + # TODO this is a stop-gap measure to prevent repository breakage + # until we get confident enough with `ensure_special_refs` for all + # existing repositories. This goes against all of our principles, + # but it's more prudent this way. logger.exception( "WriteRef failed for Repository %r on storage %r", request.repository.relative_path, diff --git a/hgitaly/service/tests/test_repository_service.py b/hgitaly/service/tests/test_repository_service.py --- a/hgitaly/service/tests/test_repository_service.py +++ b/hgitaly/service/tests/test_repository_service.py @@ -122,6 +122,8 @@ (wrapper.path / '.hg' / 'store' / 'gitlab.special-refs').write_text( 'invalid') + # TODO handle exception once we've started raising them. See big + # `try ... except` block in `repository_service`. repo_stub.WriteRef(WriteRefRequest( repository=grpc_repo, ref=b'refs/merge-requests/12/head', # HG changeset patch # User Raphaël Gomès # Date 1617270818 -7200 # Thu Apr 01 11:53:38 2021 +0200 # Node ID 85ed3117f2e6531518b66d140bb6ab86f0319cc9 # Parent 2a1f333052319f07b1faacaf1f37836214d89687 # EXP-Topic write-ref.old-revision Implement `old_revision` parameter for `WriteRef` (#53) WIP diff --git a/hgitaly/service/repository_service.py b/hgitaly/service/repository_service.py --- a/hgitaly/service/repository_service.py +++ b/hgitaly/service/repository_service.py @@ -8,6 +8,7 @@ import logging import tempfile +from hgitaly.git import NULL_COMMIT_ID from mercurial import ( archival, scmutil, @@ -23,7 +24,7 @@ from ..errors import ( invalid_argument, - not_implemented, + not_implemented, internal_error, ) from ..branch import ( iter_gitlab_branches, @@ -164,6 +165,27 @@ repo = self.load_repo(request.repository, context) try: + expected_old_rev = request.old_revision + old_rev = gitlab_revision_changeset(repo, ref) + if old_rev is not None: + old_rev = old_rev.hex() + + NULL_COMMIT_ID_BYTES = NULL_COMMIT_ID.encode('ascii') + is_null_rev = expected_old_rev == NULL_COMMIT_ID_BYTES + # FIXME probably not correct + expected_no_old_rev = (is_null_rev or len(expected_old_rev) == 0) + + if expected_no_old_rev: + if old_rev is not None and old_rev != NULL_COMMIT_ID_BYTES: + msg = "Expected old rev to not exist, got %r" % old_rev + return invalid_argument(context, WriteRefResponse, msg) + elif expected_old_rev != old_rev: + return invalid_argument( + context, + WriteRefResponse, + "Expected old rev %r, got %r" % (expected_old_rev, old_rev) + ) + special_ref_name = parse_special_ref(ref) if special_ref_name is not None: target_sha = gitlab_revision_changeset(repo, target) diff --git a/hgitaly/service/tests/test_repository_service.py b/hgitaly/service/tests/test_repository_service.py --- a/hgitaly/service/tests/test_repository_service.py +++ b/hgitaly/service/tests/test_repository_service.py @@ -28,6 +28,8 @@ ) from hgitaly.stub.repository_service_pb2_grpc import RepositoryServiceStub +from hgitaly.git import NULL_COMMIT_ID + def test_repository_exists(grpc_channel, server_repos_root): repo_stub = RepositoryServiceStub(grpc_channel) @@ -130,6 +132,54 @@ revision=b'76ac23fe' * 5)) +def test_write_ref_with_old_value(grpc_channel, server_repos_root): + repo_stub = RepositoryServiceStub(grpc_channel) + wrapper, grpc_repo = make_empty_repo(server_repos_root) + change = wrapper.commit_file('foo') + change2 = wrapper.commit_file('foo2') + + repo_stub.WriteRef(WriteRefRequest( + repository=grpc_repo, + ref=b'refs/merge-requests/12/head', + revision=change.hex()) + ) + # Pass the right old_revision + repo_stub.WriteRef(WriteRefRequest( + repository=grpc_repo, + ref=b'refs/merge-requests/12/head', + revision=change2.hex(), + old_revision=change.hex()) + ) + # Check that the old rev does not exist + repo_stub.WriteRef(WriteRefRequest( + repository=grpc_repo, + ref=b'refs/merge-requests/10/head', + revision=change2.hex(), + old_revision=NULL_COMMIT_ID.encode('ascii')) + ) + with pytest.raises(grpc.RpcError) as exc_info: + # Pass the wrong old_revision + repo_stub.WriteRef(WriteRefRequest( + repository=grpc_repo, + ref=b'refs/merge-requests/42/head', + revision=change.hex(), + old_revision=change.hex()) + ) + assert exc_info.value.code() == grpc.StatusCode.INVALID_ARGUMENT + assert "Expected old rev %r" % change.hex() in exc_info.value.details() + + with pytest.raises(grpc.RpcError) as exc_info: + # Wrongly expect old revision to not exist + repo_stub.WriteRef(WriteRefRequest( + repository=grpc_repo, + ref=b'refs/merge-requests/10/head', + revision=change.hex(), + old_revision=NULL_COMMIT_ID.encode('ascii')) + ) + assert exc_info.value.code() == grpc.StatusCode.INVALID_ARGUMENT + assert "Expected old rev to not exist" in exc_info.value.details() + + @contextmanager def get_archive_tarfile(stub, grpc_repo, commit_id, path=b''): with BytesIO() as fobj: