Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug(api): handle branch names with slashes in them #21

Merged
merged 3 commits into from
Nov 1, 2023
Merged

bug(api): handle branch names with slashes in them #21

merged 3 commits into from
Nov 1, 2023

Conversation

algernon
Copy link
Collaborator

When dealing with branches, don't capture just one slice of the path as the branch name, but every part of the path from that point on. This lets us easily deal with branches that have slashes in their names.

Since the branch is always the last part of the URI, and the /b/ or /branch/ part routes things here anyway, capturing the rest of the path is safe. Only the path is captured, query strings are not, those aren't part of the routing.

Fixes #19.

@algernon algernon requested a review from cafkafk as a code owner October 30, 2023 20:11
@cafkafk
Copy link
Owner

cafkafk commented Oct 31, 2023

(love the name of this branch)

Copy link
Owner

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to work with gitlab for some reason (or I might have messed up the test)

@algernon
Copy link
Collaborator Author

This doesn't seem to work with gitlab for some reason (or I might have messed up the test)

If you're testing with this monstrosity, then I think the @ confuses Nix. Rime's redirect is correct, it is the same URL as the one on the GitLab UI - Nix fails with both. The @ is probably treated as a HTTP basic auth separator.

@cafkafk
Copy link
Owner

cafkafk commented Oct 31, 2023

This doesn't seem to work with gitlab for some reason (or I might have messed up the test)

If you're testing with this monstrosity, then I think the @ confuses Nix. Rime's redirect is correct, it is the same URL as the one on the GitLab UI - Nix fails with both. The @ is probably treated as a HTTP basic auth separator.

I mean it works if I comment out gitlab

diff --git a/justfile b/justfile
index 0f736ad..9ea0258 100644
--- a/justfile
+++ b/justfile
@@ -204,7 +204,7 @@ itest:
 
     just run_test "http://localhost:3000/v1/codeberg/cafkafk/hello/b/a-/t/e/s/t/i/n/g/b/r/a/n/c/h-th@t-should-be-/ha/rd/to/d/e/a/l/wi/th.tar.gz"
     just run_test "http://localhost:3000/v1/github/cafkafk/hello/b/a-/t/e/s/t/i/n/g/b/r/a/n/c/h-th@t-should-be-/ha/rd/to/d/e/a/l/wi/th.tar.gz"
-    just run_test "http://localhost:3000/v1/gitlab/gitlab.com/cafkafk/hello/b/a-/t/e/s/t/i/n/g/b/r/a/n/c/h-th@t-should-be-/ha/rd/to/d/e/a/l/wi/th.tar.gz"
+    #just run_test "http://localhost:3000/v1/gitlab/gitlab.com/cafkafk/hello/b/a-/t/e/s/t/i/n/g/b/r/a/n/c/h-th@t-should-be-/ha/rd/to/d/e/a/l/wi/th.tar.gz"
     just run_test "http://localhost:3000/v1/forgejo/next.forgejo.org/cafkafk/hello/b/a-/t/e/s/t/i/n/g/b/r/a/n/c/h-th@t-should-be-/ha/rd/to/d/e/a/l/wi/th.tar.gz"
     just run_test "http://localhost:3000/v1/git.madhouse-project.org/cafkafk/hello/b/a-/t/e/s/t/i/n/g/b/r/a/n/c/h-th@t-should-be-/ha/rd/to/d/e/a/l/wi/th.tar.gz"

This might be a gitlab'ism?

@algernon
Copy link
Collaborator Author

Possibly, yeah. Can you maybe try a branch name with @, but without any / (I don't have an account on any gitlab server)?

@algernon
Copy link
Collaborator Author

algernon commented Oct 31, 2023

Oh. This is a bug in Rime! I'm just blind.

Compare:

  • URL from GitLab web UI: https://gitlab.com/cafkafk/hello/-/archive/a-/t/e/s/t/i/n/g/b/r/a/n/c/h-th@t-should-be-/ha/rd/to/d/e/a/l/wi/th/hello-a--t-e-s-t-i-n-g-b-r-a-n-c-h-th@t-should-be--ha-rd-to-d-e-a-l-wi-th.tar.gz
  • URL from Rime: https://gitlab.com/cafkafk/hello/-/archive/a-/t/e/s/t/i/n/g/b/r/a/n/c/h-th@t-should-be-/ha/rd/to/d/e/a/l/wi/th/hello-a-/t/e/s/t/i/n/g/b/r/a/n/c/h-th@t-should-be-/ha/rd/to/d/e/a/l/wi/th.tar.gz

We need to turn some of the /s into -s. As in, this is the format string we use:

        let uri = format!(
            "https://{}/{}/{}/-/archive/{}/{}-{}.tar.gz",
            host, user, repo, git_ref_name, repo, git_ref_name,
        );

The second git_ref_name needs a /->- treatment. This is not a new bug in this PR, this just brings it to the surface.

@algernon
Copy link
Collaborator Author

I'll update this PR to fix the bug too. Might take a couple of hours to find some uninterrupted screentime.

@cafkafk
Copy link
Owner

cafkafk commented Oct 31, 2023

I'll update this PR to fix the bug too. Might take a couple of hours to find some uninterrupted screentime.

nw, take whatever time you need!

@algernon
Copy link
Collaborator Author

Turns out this was easier than I imagined, so fixed & rebased branch force pushed. While I was there, applied the same change to the sourcehut routes too.

algernon and others added 3 commits November 1, 2023 15:32
GitLab enforces that the tarball's *filename* does not contain any
slashes: it replaces them with dashes. We need to do the same, so our
redirects will point to something that exists.

Signed-off-by: Gergely Nagy <me@gergo.csillger.hu>
When dealing with branches, don't capture just one slice of the path as
the branch name, but every part of the path from that point on. This
lets us easily deal with branches that have slashes in their names.

Since the branch is always the last part of the URI, and the `/b/` or
`/branch/` part routes things here anyway, capturing the rest of the
path is safe. Only the path is captured, query strings are not, those
aren't part of the routing.

Fixes #19.

Signed-off-by: Gergely Nagy <me@gergo.csillger.hu>
Copy link
Owner

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, that fixed it! nix flake check and just itests also work, looks good!

@cafkafk cafkafk merged commit 7367186 into cafkafk:main Nov 1, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: we can't deal with branches with slashes /
2 participants