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.

Sign up for GitHub

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

Jump to bottom

Double slash in URL makes git clone fail #23163

Closed
wants to merge 3 commits into from
Closed

Double slash in URL makes git clone fail #23163

wants to merge 3 commits into from

Conversation

stuzer05
Copy link

@stuzer05 stuzer05 commented Feb 26, 2023 •
edited
Loading

Allow urls prefixes with double slashes be valid

e.g. http://localhost:3000//stuzer05/test_rights.git

Closes #20462

lunny
lunny reviewed Feb 26, 2023
View reviewed changes
routers/common/middleware.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 26, 2023
lunny
lunny reviewed Feb 26, 2023
View reviewed changes
routers/common/middleware.go Outdated Show resolved Hide resolved
@stuzer05 stuzer05 changed the title Extra slash in URL makes git clone fail Double slash in URL makes git clone fail Feb 26, 2023
@wxiaoguang wxiaoguang mentioned this pull request Feb 27, 2023
Closed
@wolfogre
Copy link
Member

wolfogre commented Feb 27, 2023 •
edited
Loading

Hi @stuzer05, there's an oversight at work, we didn't connect PR #21333 to issue #20462, and this led to you overlooking the fact that there's already a PR addressing the same issue. Sorry for that.

That's not your fault, but I have to say this PR is duplicated and I have to close it. If you have different ideas, please feel free to reopen it.

@wolfogre wolfogre closed this Feb 27, 2023
@wolfogre wolfogre added the issue/duplicate The issue has already been reported. label Feb 27, 2023
@stuzer05
Copy link
Author

Hi @stuzer05, there's an oversight at work, we didn't connect PR #21333 to issue #20462, and this led to you overlooking the fact that there's already a PR addressing the same issue. Sorry for that.

That's not your fault, but I have to say this PR is duplicated and I have to close it. If you have different ideas, please feel free to reopen it.

My code looks cleaner for the same job, I think. It's mainly taken from middleware.StripSlashes. I can adjust it to strip all double slashes pretty easily

@wolfogre
Copy link
Member

Hi @stuzer05, there's an oversight at work, we didn't connect PR #21333 to issue #20462, and this led to you overlooking the fact that there's already a PR addressing the same issue. Sorry for that.
That's not your fault, but I have to say this PR is duplicated and I have to close it. If you have different ideas, please feel free to reopen it.

My code looks cleaner for the same job, I think. It's mainly taken from middleware.StripSlashes. I can adjust it to strip all double slashes pretty easily

I understand, however, you know it's not a competition. We can also adjust the code in #21333. If you have better ideas, you are welcome to help review #21333.

@stuzer05
Copy link
Author

Hi @stuzer05, there's an oversight at work, we didn't connect PR #21333 to issue #20462, and this led to you overlooking the fact that there's already a PR addressing the same issue. Sorry for that.
That's not your fault, but I have to say this PR is duplicated and I have to close it. If you have different ideas, please feel free to reopen it.

My code looks cleaner for the same job, I think. It's mainly taken from middleware.StripSlashes. I can adjust it to strip all double slashes pretty easily

I understand, however, you know it's not a competition. We can also adjust the code in #21333. If you have better ideas, you are welcome to help review #21333.

Sure! Don't get me wrong, I just want the code to be good, that's all)

@stuzer05 stuzer05 deleted the allow-extra-slash-in-clone-urls branch February 27, 2023 09:56
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny left review comments

Assignees
No one assigned
Labels
issue/duplicate The issue has already been reported. lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extra slash in URL makes git clone fail
4 participants
@stuzer05 @wolfogre @lunny @GiteaBot