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

Fix joined urls #26774

Closed
wants to merge 4 commits into from
Closed

Fix joined urls #26774

wants to merge 4 commits into from

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Aug 28, 2023 •
edited
Loading

Fixes #25632

Depends on #26745

@KN4CK3R KN4CK3R added type/bug backport/v1.20 This PR should be backported to Gitea 1.20 labels Aug 28, 2023
@KN4CK3R KN4CK3R added this to the 1.21.0 milestone Aug 28, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 28, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 28, 2023
KN4CK3R
KN4CK3R commented Aug 28, 2023
View reviewed changes
modules/util/util_test.go
Comment on lines +149 to +153
{ // https://github.com/go-gitea/gitea/issues/25632
Base: "/owner/repo/media/branch/main",
Elems: []string{"/../other/image.png"},
Expected: "/owner/repo/media/branch/other/image.png",
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Old code: "/other/image.png"
New code: "/owner/repo/media/branch/other/image.png"

modules/util/util_test.go
Comment on lines -25 to -42
newTest("https://try.gitea.io/a/b/c",
"https://try.gitea.io", "a/b", "c"),
newTest("https://try.gitea.io/a/b/c",
"https://try.gitea.io/", "/a/b/", "/c/"),
newTest("https://try.gitea.io/a/c",
"https://try.gitea.io/", "/a/./b/", "../c/"),
newTest("a/b/c",
"a", "b/c/"),
newTest("a/b/d",
"a/", "b/c/", "/../d/"),
newTest("https://try.gitea.io/a/b/c#d",
"https://try.gitea.io", "a/b", "c#d"),
newTest("/a/b/d",
"/a/", "b/c/", "/../d/"),
newTest("/a/b/c",
"/a", "b/c/"),
newTest("/a/b/c#hash",
"/a", "b/c#hash"),
Copy link
Member Author

Choose a reason for hiding this comment

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

Converted these test cases into a (for me) more readable format.

@KN4CK3R KN4CK3R added status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR and removed backport/v1.20 This PR should be backported to Gitea 1.20 labels Aug 28, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 29, 2023
@wxiaoguang
Copy link
Contributor

Overall LGTM (although CI failures seem related)

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Aug 29, 2023

Overall LGTM (although CI failures seem related)

Fixed with e823e33

Our URLJoin method may not be well designed. We usualy use this method just to join parts of a url (what the name suggests). The MakeAbsoluteAssetURL method uses URLJoin because it had the ability to resolve a path from a base. Both in one method does not really work and was the reason for #25632.
But to be honest, currently I don't know how a better "url builder" could be designed.

puni9869
puni9869 reviewed Aug 30, 2023
View reviewed changes
modules/util/url.go
Comment on lines +34 to +35
if strings.Contains(elems[last], "#") {
elems[last], fragment, _ = strings.Cut(elems[last], "#")
Copy link
Member

@puni9869 puni9869 Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

banch name can also have # like BranchName#123

"/a/b/c#hash", "/a#123", "b/c#1212"

Copy link
Member Author

@KN4CK3R KN4CK3R Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's correct but does not affect the method because it is called with already escaped parameters.

grafik
The screenshot shows a branch with # with a readme which includes the picture from the repo.

Copy link
Member

Choose a reason for hiding this comment

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

cool, works for me then.

Copy link
Member

Choose a reason for hiding this comment

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

Some test cases are failing
image

Copy link
Member Author

Choose a reason for hiding this comment

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

That's why this PR is marked as depending on the other PR. If #26745 is merged, the test will work.

Copy link
Member

Choose a reason for hiding this comment

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

Ohk, got it. my bad.

puni9869
puni9869 approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@puni9869 puni9869 left a comment

Choose a reason for hiding this comment

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

🚀

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 31, 2023
@lunny lunny modified the milestones: 1.21.0, 1.22.0 Sep 21, 2023
@CaiCandong CaiCandong mentioned this pull request Sep 26, 2023
Open
@lunny
Copy link
Member

lunny commented Nov 8, 2023

CI still failed.

@KN4CK3R KN4CK3R closed this Jan 14, 2024
@KN4CK3R KN4CK3R deleted the fix-25632 branch January 14, 2024 21:23
@GiteaBot GiteaBot removed this from the 1.22.0 milestone Jan 14, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@puni9869 puni9869 puni9869 approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR type/bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Relative image link on different branch broken for github migrations / mirrors
5 participants
@KN4CK3R @wxiaoguang @lunny @puni9869 @GiteaBot