Skip to content

Link to tree views of submodules if possible #33424

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

Merged
merged 9 commits into from
Jan 30, 2025

Conversation

bohde
Copy link
Contributor

@bohde bohde commented Jan 28, 2025

This is a follow-up to #33097.

When linking a submodule at a commit in either the repo view, or a diff when adding a new submodule, link to the tree view of that submodules intead of the individual commit. This shows the user the full tree, instead of the diff of the commit.

This makes the assumption that the tree for a given SHA is at <repo_url>/tree/<sha>. This URL format is supported by both Github & Gitlab, but not Gitea. To fix this, add a redirect from <username>/<repo>/tree/<ref> to <username>/<repo>/src/<ref>, so that Gitea can support this URL structure.

When linking a submodule at a commit in either the repo view, or a
diff when adding a new submodule, link to the tree view of that
submodules intead of the individual commit. This shows the user the
full tree, instead of the diff of the commit.

This makes the assumption that the tree for a given SHA is at
`<repo_url>/tree/<sha>`. This URL format is supported by both Github &
Gitlab, but not Gitea. To fix this, add a redirect from
`<username>/<repo>/tree/<ref>` to `<username>/<repo>/src/<ref>`.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 28, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jan 28, 2025
@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 Jan 28, 2025
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jan 29, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 29, 2025

Thank you for the update, but I think we should avoid doing that .....

Sprintf("%s/%s/%s/src/%s", AppSubURL, PathEscape(username), PathEscape(reponame), PathEscapeSegments(*)) should be good enough and it's not worth to test it because it has been widely used everywhere. We can just keep code simple.

If you don't mind, I can help to make some improvements.

@bohde
Copy link
Contributor Author

bohde commented Jan 29, 2025

Thank you for the update, but I think we should avoid doing that .....

Sprintf("%s/%s/%s/src/%s", AppSubURL, PathEscape(username), PathEscape(reponame), PathEscapeSegments(*)) should be good enough and it's not worth to test it because it has been widely used everywhere. We can just keep code simple.

If you don't mind, I can help to make some improvements.

Was in the process of responding to that one. I'm happy to make the change if you want, but before using paths.Join I looked through the existing code used to construct redirects to ensure the style was similar. It looked like there was a preference for using path.Join instead of fmt.Sprintf. (example).

If you still want me to replace it with fmt.Sprintf, I can.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 29, 2025

It looked like there was a preference for using path.Join instead of fmt.Sprintf. (example).

That's the only (maybe a few) legacy usages. Most other code doesn't use path.Join.

Actually, you could also just use RepoLink here since you are in a RepoAssigment context.

image

image

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 29, 2025

Hmm, one more thing, I think the use of /tree/{CommitID} is wrong.

Because it redirects and routes to the deprecated handler /src/{CommitID} if I understand correctly.

image

@pull-request-size pull-request-size bot added size/S and removed size/M labels Jan 29, 2025
@bohde
Copy link
Contributor Author

bohde commented Jan 29, 2025

Actually, you could also just use RepoLink here since you are in a RepoAssigment context.

Used this method in 92cb2a8

Because it redirects and routes to the deprecated handler /src/{CommitID} if I understand correctly.

This is correct, but intentional. Github's URL structure is <repo>/tree/<ref> not <repo>/tree/<sha>. A couple of examples:

When hitting this in Gitea, the user will have two separate redirects to get to the canonical URL. The flow would look like this:

@wxiaoguang
Copy link
Contributor

Then it comes to this question .... #33424 (comment)

Why we should support such /tree/ path? What's the benefit?

@bohde
Copy link
Contributor Author

bohde commented Jan 29, 2025

Then it comes to this question .... #33424 (comment)

Why we should support such /tree/ path? What's the benefit?

The intent of this PR is so that when a user clicks on a submodule commit, either in the repo home view, or when adding a new submodule in a diff, that shows them the tree instead of the commit.

Right now if I add a submodule of gitea.com/gitea/gitea-mirror@a89c73530327784ce99e1e07c269ea8ff1861fdc, it links to https://gitea.com/gitea/gitea-mirror/commit/a89c73530327784ce99e1e07c269ea8ff1861fdc. This updates that to show (after redirects) https://gitea.com/gitea/gitea-mirror/src/commit/a89c73530327784ce99e1e07c269ea8ff1861fdc, allowing them to browse the repo at that commit.

The issue of adding a /tree/ path comes in for submodules that are not hosted on Gitea. If I were to link out to https://github.com/go-gitea/gitea/src/commit/a89c73530327784ce99e1e07c269ea8ff1861fdc, that results in a 404 for the user, who would need to navigate to the repo, and then search for the commit. That content is instead available at https://github.com/go-gitea/gitea/tree/a89c73530327784ce99e1e07c269ea8ff1861fdc.

We could try to guess what type of forge a given submodule links out to, and then either use src/commit for Gitea or tree for others, but self-hosted deployments would make that error-prone. Adding a redirect for these tree paths allows using the same link across Gitea and other forges.

@wxiaoguang
Copy link
Contributor

Thank you for the explanation, it's clear to me now.

I think we need to add the background information to comments, otherwise the future readers would still have the same question: why /tree/ is used ....

@bohde
Copy link
Contributor Author

bohde commented Jan 29, 2025

Thank you for the explanation, it's clear to me now.

I think we need to add the background information to comments, otherwise the future readers would still have the same question: why /tree/ is used ....

I updated the comment in 7da9b01. Is that inline with what you are thinking?

@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 Jan 29, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 29, 2025
@lunny lunny enabled auto-merge (squash) January 29, 2025 23:52
@lunny lunny added this to the 1.24.0 milestone Jan 29, 2025
@lunny lunny merged commit ac2d97c into go-gitea:main Jan 30, 2025
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 30, 2025
@wxiaoguang
Copy link
Contributor

-> Fix "redirect link" handling #33440

Improved the path handling logic and added some tests in links_test.go

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Apr 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants