launchpad-reviewers team mailing list archive

[Gravity Yang <mp+452409@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/turnip/api/store.py b/turnip/api/store.py
> index ae897c2..9475bce 100644
> --- a/turnip/api/store.py
> +++ b/turnip/api/store.py
> @@ -555,6 +557,24 @@ def get_ref(repo_store, repo_name, ref):
>          return ref_obj
>  
>  
> +def write_lightweight_tag(repo_store, repo_name, commit_sha1, tag_name):
> +    ref_base = "refs/tags/"
> +    ref_name = ref_base + tag_name
> +    with open_repo(repo_store, repo_name) as repo:
> +        try:
> +            repo.create_reference(ref_name, commit_sha1)
> +        except (AlreadyExistsError, InvalidSpecError):
> +            raise
> +
> +
> +def create_branch(repo_store, repo_name, commit, branch_name):
> +    with open_repo(repo_store, repo_name) as repo:
> +        try:
> +            repo.create_branch(branch_name, commit)

likely a question and nitpicking: 

1. what if the commit does not exist in the case? InvalidSpecError?

2. would it be good to pass-in commit_sha1 to the logic layer and try get commit from here. Afaics, the benefit of doing this are 1. create_branch takes commit_sha1 and explictly validate it, so the caller can just pass commit_hash and leave it to logic. 2. the signature become more unified in this file, which seems mostly taking commit_sha1 rather than commit object

I think the current code would just work without doing 2. And either way, think it'd be good to add a test for non-exisinting commit.

> +        except (AlreadyExistsError, InvalidSpecError):
> +            raise
> +
> +
>  def get_common_ancestor_diff(
>      repo_store, repo_name, sha1_target, sha1_source, context_lines=3
>  ):
> diff --git a/turnip/api/tests/test_api.py b/turnip/api/tests/test_api.py
> index 1513618..e30e4a3 100644
> --- a/turnip/api/tests/test_api.py
> +++ b/turnip/api/tests/test_api.py
> @@ -241,6 +241,81 @@ class ApiTestCase(TestCase, ApiRepoStoreMixin):
>          resp = self.app.get("/repo/1/refs", expect_errors=True)
>          self.assertEqual(404, resp.status_code)
>  
> +    def test_repo_write_tag(self):
> +        factory = RepoFactory(self.repo_store)
> +        commit_oid = factory.add_commit("foo", "foobar.txt")
> +        body = {
> +            "commit_sha1": commit_oid.hex,
> +            "ref_type": "lightweight_tag",
> +            "ref_name": "1701",
> +        }
> +        resp = self.app.post_json(f"/repo/{self.repo_path}/refs", body)
> +
> +        self.assertEqual(201, resp.status_code)
> +
> +    def test_repo_create_branch(self):
> +        factory = RepoFactory(self.repo_store)
> +        commit_oid = factory.add_commit("foo", "foobar.txt")
> +        body = {
> +            "commit_sha1": commit_oid.hex,
> +            "ref_type": "branch",
> +            "ref_name": "latest/stable",
> +        }
> +        resp = self.app.post_json(f"/repo/{self.repo_path}/refs", body)
> +
> +        self.assertEqual(201, resp.status_code)
> +
> +    def test_repo_write_invalid_ref_name(self):
> +        factory = RepoFactory(self.repo_store)
> +        commit_oid = factory.add_commit("foo", "foobar.txt")
> +
> +        tag_body = {
> +            "commit_sha1": commit_oid.hex,
> +            "ref_type": "lightweight_tag",
> +            "ref_name": "?*",
> +        }
> +        resp = self.app.post_json(
> +            f"/repo/{self.repo_path}/refs", tag_body, expect_errors=True
> +        )
> +        self.assertEqual(400, resp.status_code)
> +
> +        branch_body = {
> +            "commit_sha1": commit_oid.hex,
> +            "ref_type": "branch",
> +            "ref_name": "?*",
> +        }
> +        resp = self.app.post_json(
> +            f"/repo/{self.repo_path}/refs", branch_body, expect_errors=True
> +        )
> +        self.assertEqual(400, resp.status_code)
> +
> +    def test_repo_ref_already_exists(self):

think It'd be good to also add tests for invalid ref_type and commit_sha1 etc. so save some manual test effort :)

> +        factory = RepoFactory(self.repo_store)
> +        commit_oid = factory.add_commit("foo", "foobar.txt")
> +        factory.add_tag("1701", "", commit_oid)
> +        factory.add_branch("latest/stable", commit_oid)
> +
> +        tag_body = {
> +            "commit_sha1": commit_oid.hex,
> +            "ref_type": "lightweight_tag",
> +            "ref_name": "1701",
> +        }
> +        tag_resp = self.app.post_json(
> +            f"/repo/{self.repo_path}/refs", tag_body, expect_errors=True
> +        )
> +        self.assertEqual(409, tag_resp.status_code)
> +
> +        branch_body = {
> +            "commit_sha1": commit_oid.hex,
> +            "ref_type": "branch",
> +            # factory.add_branch prefixes 'branch-'
> +            "ref_name": "branch-latest/stable",
> +        }
> +        branch_resp = self.app.post_json(
> +            f"/repo/{self.repo_path}/refs", branch_body, expect_errors=True
> +        )
> +        self.assertEqual(409, branch_resp.status_code)
> +
>      def test_ignore_non_unicode_refs(self):
>          """Ensure non-unicode refs are dropped from ref collection."""
>          factory = RepoFactory(self.repo_store)
> diff --git a/turnip/api/views.py b/turnip/api/views.py
> index 5014634..6ee9833 100644
> --- a/turnip/api/views.py
> +++ b/turnip/api/views.py
> @@ -265,6 +265,76 @@ class RefAPI(BaseAPI):
>              return exc.HTTPNotFound()  # 404
>  
>      @validate_path
> +    def collection_post(self, repo_store, repo_name):
> +        """Create a lightweight tag or a branch against a commit.
> +
> +        The JSON request dictionary should contain the following keys:
> +        {commit_sha1} must be the full 40-char SHA1 hash of the commit to tag
> +        or set as a branch head.
> +        """
> +        json_data = extract_cstruct(self.request)["body"]
> +        commit_sha1 = json_data.get("commit_sha1")
> +        ref_type = json_data.get("ref_type")
> +        ref_name = json_data.get("ref_name")
> +

potentially we can group & place simple input validation (commit_sha1, ref_type, ref_name) here before any logic check, think it'd aid in readibility also do earily return on invalid payload.

> +        repo_path = os.path.join(repo_store, repo_name)
> +        if not os.path.exists(repo_path):
> +            self.request.errors.add(
> +                "body", "name", "repository does not exist"
> +            )
> +            raise exc.HTTPNotFound()
> +
> +        if not commit_sha1:
> +            self.request.errors.add(
> +                "body", "commit_sha1", "commit_sha1 for ref target is missing"
> +            )
> +            return
> +        try:
> +            commit = store.get_commit(repo_store, repo_name, commit_sha1)
> +        except GitError:
> +            self.request.errors.add(
> +                "body", "commit_sha1", "no commit found for commit_sha1 given"
> +            )
> +            raise exc.HTTPNotFound()
> +        if not ref_type or ref_type not in {"lightweight_tag", "branch"}:
> +            self.request.errors.add(
> +                "body",
> +                "ref_type",
> +                "must specify ref type (lightweight_tag or branch)",
> +            )
> +            return
> +        if not ref_name:
> +            self.request.errors.add(
> +                "body", "ref_name", "must specify ref name"
> +            )
> +            return
> +
> +        try:
> +            if ref_type == "lightweight_tag":
> +                store.write_lightweight_tag(
> +                    repo_store, repo_name, commit_sha1, ref_name
> +                )
> +            elif ref_type == "branch":
> +                store.create_branch(repo_store, repo_name, commit, ref_name)
> +        except AlreadyExistsError:
> +            self.request.errors.add(
> +                "body",
> +                "ref_name",
> +                "tag or branch name already exists",
> +            )
> +            self.request.errors.status = 409
> +            return
> +        except InvalidSpecError:
> +            self.request.errors.add(
> +                "body",
> +                "ref_name",
> +                "ensure tag or branch name is valid per git-check-ref-format",
> +            )
> +            return
> +        else:
> +            return Response(status=201)
> +
> +    @validate_path
>      def get(self, repo_store, repo_name):
>          ref = "refs/" + self.request.matchdict["ref"]
>          try:


-- 
https://code.launchpad.net/~bowenfan/turnip/+git/turnip/+merge/452409
Your team Launchpad code reviewers is requested to review the proposed merge of ~bowenfan/turnip:SN2052-add_ref_creation_to_turnip_api into turnip:master.