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

Support sticky redirects in git2-curl. #360

Merged
merged 1 commit into from
Oct 11, 2018

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Oct 11, 2018

If you use a gitlab url such as "https://gitlab.com/user/repo", then gitlab will
redirect the first GET request to "https://gitlab.com/user/repo.git" (with the
.git at the end). However, gitlab does not redirect the next POST, causing a
404. This change keeps track of the original redirect so that subsequent actions
use the new base url. This roughly mirrors what is done in libgit2's transport.

Fixes rust-lang/cargo#6114

@ehuss
Copy link
Contributor Author

ehuss commented Oct 11, 2018

This seems a little hacky, but there needs to be some way to bubble up the base path to the CurlTransport. Please let me know if you see ways it could be cleaner.

I tested against gitlab and it seems to solve the issue, otherwise I'm not sure what other testing would be appropriate.

If you use a gitlab url such as "https://gitlab.com/user/repo", then gitlab will
redirect the first GET request to "https://gitlab.com/user/repo.git" (with the
`.git` at the end). However, gitlab does not redirect the next POST, causing a
404. This change keeps track of the original redirect so that subsequent actions
use the new base url. This roughly mirrors what is done in libgit2's transport.

Fixes rust-lang/cargo#6114
@alexcrichton
Copy link
Member

Oh jeez, that really is pretty icky! I think this is good enough for now though. I may have been a bit too conservative in requiring things to be threadsafe, and that'd make this much nicer with interior mutability via RefCell rather than mutexes. In any case, thanks so much for investigating this!

@alexcrichton alexcrichton merged commit 24a9072 into rust-lang:master Oct 11, 2018
@alexcrichton
Copy link
Member

I've included this in 0.8.2 and folded the version bump into rust-lang/rust#54919

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.

2 participants