Skip to content

Fix Readme render bug #19992

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
Merged
merged 4 commits into from
Jun 17, 2022
Merged

Fix Readme render bug #19992

merged 4 commits into from
Jun 17, 2022

Conversation

lunny
Copy link
Member

@lunny lunny commented Jun 17, 2022 •
edited
Loading

Fix #19988

The problem is caused sometiems TreePath is not the file path but the file's parent dir path.

This is a temporory bug fix, the same variable should not have different ,meaning.

For README render, it's an index file, so treepath is "" or "/", but we need to render the README.md. That's the problem.

@lunny lunny added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jun 17, 2022
@lunny lunny requested a review from wxiaoguang June 17, 2022 02:13
@lunny lunny added this to the 1.17.0 milestone Jun 17, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jun 17, 2022
zeripath
zeripath reviewed Jun 17, 2022
View reviewed changes
@zeripath
Copy link
Contributor

I'm not sure that there's actually any overloading of the TreePath going on here.

The TreePath is the path that the user has requested. They didn't request the README file explicitly - we're just additionally rendering the README file within the rendering of the directory.

@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 Jun 17, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 17, 2022 •
edited
Loading

I'm not sure that there's actually any overloading of the TreePath going on here.

The TreePath is the path that the user has requested. They didn't request the README file explicitly - we're just additionally rendering the README file within the rendering of the directory.

I think it's correct, the render is independent of the TreePath at the moment.

How about a README in a subdir ?

https://try.gitea.io/wxiaoguang/test/src/branch/master/subdir

https://try.gitea.io/wxiaoguang/test/src/branch/master/subdir/README.md

I have tested that it should work correctly for sub directory, then the ctx.Repo.TreePath is "subdir".

@zeripath zeripath added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Jun 17, 2022
zeripath
zeripath previously requested changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@zeripath zeripath left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This won't work for readmes that are actually within docs/ .gitea/ .github/

Ignore me readmeFile.name has the right path bits.

@zeripath zeripath dismissed their stale review June 17, 2022 05:27

Ignore me readmefile.Name has the local path

@zeripath zeripath removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Jun 17, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 17, 2022 •
edited
Loading

This won't work for readmes that are actually within docs/ .gitea/ .github/

readmeFile.name has the right path bits.

But it works, I have tested.

image

@zeripath
Copy link
Contributor

This won't work for readmes that are actually within docs/ .gitea/ .github/
readmeFile.name has the right path bits.

But it works, I have tested.

The test you were describing was subdir/ readmes. I was actually concerned about the case where TreePath is "" but the readme file is in docs/README.md, .github/README.md or .gitea/README.md.

In those cases, readmeFile.Name actually is as above - in order that this kind of thing worked transparently and I had forgotten that that was the case.

@wxiaoguang
Copy link
Contributor

Sorry for that I didn't say it clearly .....

@zeripath zeripath changed the title Fix render bug Fix Readme render bug Jun 17, 2022
@zeripath zeripath merged commit bdde56c into go-gitea:main Jun 17, 2022
@zeripath zeripath added type/bug issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change labels Jun 17, 2022
@lunny lunny deleted the lunny/fix_render_bug branch June 17, 2022 06:40
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
@lunny @zeripath
Fix Readme render bug (go-gitea#19992)
013632d 
Fix go-gitea#19988 

Co-authored-by: zeripath <[email protected]>
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
@lunny @zeripath @AbdulrhmnGhanem
Fix Readme render bug (go-gitea#19992)
50c5cab 
Fix go-gitea#19988 

Co-authored-by: zeripath <[email protected]>
@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

@zeripath zeripath zeripath approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

Assignees
No one assigned
Labels
issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

README rendering broken
4 participants
@lunny @zeripath @wxiaoguang @GiteaBot