launchpad-reviewers team mailing list archive

[William Grant <mp+452540@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/turnip/api/store.py b/turnip/api/store.py
> index ae897c2..b4e8a33 100644
> --- a/turnip/api/store.py
> +++ b/turnip/api/store.py
> @@ -555,6 +563,23 @@ def get_ref(repo_store, repo_name, ref):
>          return ref_obj
>  
>  
> +def create_reference(repo_store, repo_name, ref, commit_sha1, force=False):
> +    """Create a git reference against a given commit sha1.
> +
> +    :param ref: Name of the git ref to write. The client should send
> +        'tags/<tag_name>' for tags and 'heads/<branch_name>'
> +        for branches. The client is free to send other arbitrary paths.
> +    :param commit_sha1: SHA1 hash of the ref commit target.
> +    :param force: If True, force overwrite the original ref (if any)
> +        to point to the new commit.
> +    """
> +    with open_repo(repo_store, repo_name) as repo:
> +        try:
> +            repo.create_reference(ref, commit_sha1, force)
> +        except (ObjectAlreadyExistsError, InvalidSpecError, GitError):
> +            raise

Is it useful to catch three specific exceptions only to raise them again?

> +
> +
>  def get_common_ancestor_diff(
>      repo_store, repo_name, sha1_target, sha1_source, context_lines=3
>  ):
> diff --git a/turnip/api/views.py b/turnip/api/views.py
> index 5014634..4d64b62 100644
> --- a/turnip/api/views.py
> +++ b/turnip/api/views.py
> @@ -265,6 +265,61 @@ class RefAPI(BaseAPI):
>              return exc.HTTPNotFound()  # 404
>  
>      @validate_path
> +    def collection_post(self, repo_store, repo_name):
> +        """Create a git reference against a commit."""
> +        json_data = extract_cstruct(self.request)["body"]
> +        commit_sha1 = json_data.get("commit_sha1")
> +        ref = json_data.get("ref")
> +        force = json_data.get("force", False)
> +
> +        if not commit_sha1:
> +            self.request.errors.add(
> +                "body", "commit_sha1", "commit_sha1 for ref target is missing"
> +            )
> +            return
> +        if not ref:
> +            self.request.errors.add("body", "ref", "must specify ref name")
> +            return
> +        # Do not accept truthy values (e.g. {"force": "no"}) to avoid confusion
> +        if force and not isinstance(force, bool):
> +            self.request.errors.add(
> +                "body", "force", "'force' must be a json boolean"
> +            )
> +            return
> +
> +        ref = "refs/" + ref

Refs in Git are named `refs/SOME/PATH`; the `refs/` prefix isn't ever implicit. turnip's get_refs returns the `refs/` prefix, so this probably shouldn't prepend it.

> +        try:
> +            store.create_reference(
> +                repo_store, repo_name, ref, commit_sha1, force
> +            )
> +        except AlreadyExistsError:
> +            self.request.errors.add(
> +                "body",
> +                "ref",
> +                "ref already exists",
> +            )
> +            self.request.errors.status = 409
> +            return
> +        except InvalidSpecError:
> +            self.request.errors.add(
> +                "body",
> +                "ref",
> +                "ensure ref name is valid per git-check-ref-format",

Should this directly state the error -- that the ref name is invalid?

> +            )
> +            return
> +        except GitError:
> +            # This diverges from the generic Resource Not Found for get_ref,
> +            # but as we are dealing with multiple object types here it would
> +            # be helpful to raise a more specific error.
> +            self.request.errors.add(
> +                "body", "commit_sha1", "no commit found for commit_sha1 given"
> +            )
> +            self.request.errors.status = 404
> +            return
> +        else:
> +            return Response(status=201)

Could this be outside the else block?

> +
> +    @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/452540
Your team Launchpad code reviewers is requested to review the proposed merge of ~bowenfan/turnip:SN2052-add_ref_creation_endpoint_to_turnip_api into turnip:master.