launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #30571
Re: [Merge] ~bowenfan/turnip:SN2052-add_ref_creation_endpoint_to_turnip_api into turnip:master
Thanks William for your review. I'll implement your feedback shortly.
One thing though - I'm not clear about the intended behaviour on passing in arbitrary ref strings, could you please take a look at my diff comment?
Diff comments:
> 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
I did recall you mentioning that clients should send 'refs/', but I tested things out and wasn't sure it made sense:
git (and pygit2) appears to allow clients to set arbitrary paths outside the 'refs' directory, which once set aren't retrievable with `git show-ref` (and pygit2's `repo.references` wrapper).
A payload using {"ref": "random-dir/tags/1701"} will lead to this:
```
$ ls
HEAD config description hooks info objects random-dir refs
$ git show-ref
479304f8826fd8eeaf2e3ffe083f8e1abb42a55d refs/heads/master
$ cat random-dir/tags/1701
479304f8826fd8eeaf2e3ffe083f8e1abb42a55d
```
This doesn't appear to be recognized as a ref by git and isn't fetched by Turnip get_refs.
Is this intentional?
> + 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",
> + )
> + 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)
> +
> + @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.
References