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

Allow branch names longer than 7 characters #3750

Closed
wants to merge 2 commits into from

Conversation

samuelson
Copy link

@samuelson samuelson commented Oct 10, 2016

Resolves #3743

This fixed the issue for me, but I don't know if it will have some other unintended consequences.

@samuelson
Copy link
Author

@unknwon please let me know what I need to change for this to be merged.

The regex was matching any string that was between 7 and 40 characters
and treating them as SHAs.

For branch names that were 7 characters or longer this causes strange
behavior and makes PRs impossible.

This update makes it so that only 40 character strings are treated as
SHAs.
@binford2k
Copy link

@unknwon ping, this would be really nice to get corrected.

Copy link
Contributor

@thibaultmeyer thibaultmeyer left a comment

Choose a reason for hiding this comment

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

Hi @samuelson

I have pull your branch for testing. It seems you have modified too many lines as needed and the SHA1 regexp is not correct.

Thanks.


m.Get("/compare/:before([a-z0-9]{7,40})\\.\\.\\.:after([a-z0-9]{7,40})", repo.CompareDiff)
m.Get("/compare/:before([a-z0-9]{40})\\.\\.\\.:after([a-z0-9]{40})", repo.CompareDiff)
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. It's the root of your problem.

m.Get("/forks", repo.Forks)
}, context.RepoRef())
m.Get("/commit/:sha([a-z0-9]{7,40})\\.:ext(patch|diff)", repo.RawDiff)
m.Get("/commit/:sha([a-z0-9]{40})\\.:ext(patch|diff)", repo.RawDiff)
Copy link
Contributor

Choose a reason for hiding this comment

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

This modification don't fix your issue. But the regexp is not correct : valid SHA1 only contains a b c d e f 0 1 2 3 4 5 6 7 8 9 characters. Could you replace :sha([a-z0-9]{7,40}) with :sha([a-f0-9]{7,40}) please.

@@ -585,12 +585,12 @@ func runWeb(ctx *cli.Context) error {
m.Get("/src/*", repo.Home)
m.Get("/raw/*", repo.SingleDownload)
m.Get("/commits/*", repo.RefCommits)
m.Get("/commit/:sha([a-z0-9]{7,40})$", repo.Diff)
m.Get("/commit/:sha([a-z0-9]{40})$", repo.Diff)
Copy link
Contributor

Choose a reason for hiding this comment

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

This modification don't fix your issue. In fact this route is not used when you call URL like http://domain.local/zero-x-baadf00d/test/compare/production...production

But the regexp is not correct : valid SHA1 only contains a b c d e f 0 1 2 3 4 5 6 7 8 9 characters. Could you replace :sha([a-z0-9]{7,40}) with :sha([a-f0-9]{7,40}) please.

@thibaultmeyer
Copy link
Contributor

Ping @samuelson

@samuelson
Copy link
Author

@0xBAADF00D sorry for the delay, it turns out I had notifications disabled. I updated with the changes you suggested.

@strk
Copy link
Contributor

strk commented Nov 7, 2016

@samuelson please consider sending this PR to https://github.com/go-gitea/gitea

@samuelson
Copy link
Author

@strk looks like it's already fixed: https://github.com/go-gitea/gitea/blob/master/cmd/web.go#L592

@samuelson
Copy link
Author

This is the same, go ahead an merge that one: #3851

@samuelson samuelson closed this Nov 7, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants