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

Deduplicate findReadmeFile() #22177

Merged
merged 1 commit into from
Feb 12, 2023
Merged

Conversation

kousu
Copy link
Contributor

@kousu kousu commented Dec 20, 2022

This code was copy-pasted at some point. Revisit it to reunify it.

Doing that then encouraged simplifying the types of a couple of related functions.

As a follow-up, move two helper functions, isReadmeFile() and isReadmeFileExtension(), intimately tied to findReadmeFile(), in as package-private.

@kousu kousu force-pushed the dedupe-findReadmeFile branch 4 times, most recently from c424ae3 to 981f19b Compare December 20, 2022 07:11
Comment on lines 345 to 354
URLPrefix: readmeTreelink,
URLPrefix: path.Dir(treeLink),
Copy link
Contributor Author

@kousu kousu Dec 20, 2022

Choose a reason for hiding this comment

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

I need some advice on this. readmeTreelink was an annoying appendix that needed to be carried around like an albatross on our shoulders. I believe the only reason it existed was for the docs/README.md cases, where you need to append e.g. "docs/" to treeLink to maintain the invariant that URLPrefix is the full URL path, '/src/', the branch name, and any subfolder, for the file being rendered.

But as far as I can tell, in this subroutine the contents of URLPrefix are never served to the user, and nothing seems broken with it switched out like this.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 20, 2022
@kousu kousu marked this pull request as ready for review December 20, 2022 07:21
@kousu kousu force-pushed the dedupe-findReadmeFile branch 2 times, most recently from c66a1f0 to 0b0f2bb Compare December 20, 2022 16:09
@silverwind silverwind added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Dec 20, 2022
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Seems like a good refactor. Afraid I can not answer about readmeTreelink but if nothing breaks, I guess we just go for it.

@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 Dec 20, 2022
@kousu
Copy link
Contributor Author

kousu commented Dec 20, 2022

Thanks!

Hm looks like CI broke between when I submitted and now. I'll fix that up.

@silverwind
Copy link
Member

CI failure is likely unrelated, but you can restart by pushing an empty commit just to be sure.

@kousu
Copy link
Contributor Author

kousu commented Dec 20, 2022

Good call, so it was :)

@kousu kousu mentioned this pull request Dec 20, 2022
routers/web/repo/view.go Outdated Show resolved Hide resolved
routers/web/repo/view.go Outdated Show resolved Hide resolved
@kousu kousu force-pushed the dedupe-findReadmeFile branch 4 times, most recently from b907a0e to 4f31708 Compare December 21, 2022 20:18
@kousu kousu changed the title Deduplicate findReadmeFile() code. Deduplicate findReadmeFile() Dec 22, 2022
@kousu kousu force-pushed the dedupe-findReadmeFile branch 2 times, most recently from 067bf63 to 4138aa2 Compare December 24, 2022 21:06
@lunny
Copy link
Member

lunny commented Jan 24, 2023

I have cut this down to just the primary commit and merged main again (I'll submit the second cleanup in a second PR, since it seems to be slowing down the review of this one). Would anyone be willing to take another look? Thanks! 💮

I will have a look again after the CI PASS.

@yardenshoham yardenshoham added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jan 25, 2023
@kousu
Copy link
Contributor Author

kousu commented Jan 25, 2023

Hiya, this is passing and up to date again with main. :)

@codecov-commenter
Copy link

Codecov Report

Merging #22177 (c2304e6) into main (ff18d17) will increase coverage by 47.32%.
The diff coverage is 28.81%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff            @@
##           main   #22177       +/-   ##
=========================================
+ Coverage      0   47.32%   +47.32%     
=========================================
  Files         0     1111     +1111     
  Lines         0   149389   +149389     
=========================================
+ Hits          0    70701    +70701     
- Misses        0    70298    +70298     
- Partials      0     8390     +8390     
Impacted Files Coverage Δ
modules/git/tree_entry.go 45.00% <0.00%> (ø)
routers/web/repo/view.go 50.43% <32.07%> (ø)
routers/web/repo/find.go 0.00% <0.00%> (ø)
modules/log/groutinelabel.go 100.00% <0.00%> (ø)
models/repo/pushmirror.go 75.00% <0.00%> (ø)
routers/api/v1/repo/branch.go 50.39% <0.00%> (ø)
models/auth/token_scope.go 78.94% <0.00%> (ø)
modules/recaptcha/recaptcha.go 0.00% <0.00%> (ø)
routers/web/repo/middlewares.go 58.62% <0.00%> (ø)
modules/log/flags.go 72.72% <0.00%> (ø)
... and 1103 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@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 Feb 12, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 12, 2023
@lunny
Copy link
Member

lunny commented Feb 12, 2023

Please update branch

This code was copy-pasted at some point. Reunify it.

But to do this I found I had to expose git.TreeEntry.Tree()

Signed-off-by: Nick Guenther <nick.guenther@polymtl.ca>
@kousu
Copy link
Contributor Author

kousu commented Feb 12, 2023

Please update branch

Great! On its way now. Thanks for your time :)

@lunny lunny merged commit e1aca7c into go-gitea:main Feb 12, 2023
@lunny
Copy link
Member

lunny commented Feb 12, 2023

Please update branch

Great! On its way now. Thanks for your time :)

Thank you for your work and patience!!!

@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 12, 2023
@kousu kousu deleted the dedupe-findReadmeFile branch February 12, 2023 07:49
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 13, 2023
* upstream/main:
  Add some headings to repo views (go-gitea#22869)
  Fix style of actions rerun button (go-gitea#22835)
  Make issue and code search support camel case (go-gitea#22829)
  Revert "Fix notification and stopwatch empty states" (go-gitea#22876)
  Deduplicate findReadmeFile() (go-gitea#22177)
  Fix milestone title font problem (go-gitea#22863)
  Fix PR file tree folders no longer collapsing (go-gitea#22864)
  escape filename when assemble URL (go-gitea#22850)
  Fix notification and stopwatch empty states (go-gitea#22845)
  Fix .golangci.yml (go-gitea#22868)
  Fix migration issue. (go-gitea#22867)
  Add `/$count` endpoints for NuGet v2 (go-gitea#22855)
  Preview images for Issue cards in Project Board view (go-gitea#22112)
  Fix improper HTMLURL usages in Go code (go-gitea#22839)
  Use proxy for pull mirror (go-gitea#22771)
techknowlogick pushed a commit that referenced this pull request Feb 13, 2023
These functions don't examine contents, just filenames, so they don't
fit in well in a markup module.

This was originally part of
#22177.

Signed-off-by: Nick Guenther <nick.guenther@polymtl.ca>
kousu added a commit to neuropoly/gitea that referenced this pull request Feb 14, 2023
Fixes a bug introduced in go-gitea#22177
which allows finding READMEs like docs/docs/docs/.gitea/.github/docs/README.md
kousu added a commit to neuropoly/gitea that referenced this pull request Feb 14, 2023
!fixup go-gitea#22177

The only place this function is used so far is in
findReadmeFileInEntries(), so the only visible effect
of this oversight was in an obscure corner: if the
README was in a subfolder and was a symlink that pointed up,
as in .github/README.md -> ../docs/old/setup.md, the
README would fail to render when because FollowLinks()
hit the nil ptree.
zeripath pushed a commit that referenced this pull request Feb 14, 2023
…e() (#22902)

!fixup #22177

The only place this function is used so far is in
findReadmeFileInEntries(), so the only visible effect of this oversight
was in an obscure README-related corner: if the README was in a
subfolder and was a symlink that pointed up, as in .github/README.md ->
../docs/old/setup.md, the README would fail to render when FollowLinks()
hit the nil ptree. This makes the ptree non-nil and thus repairs it.
kousu added a commit to neuropoly/gitea that referenced this pull request Mar 25, 2023
Fixes a bug introduced in go-gitea#22177
which allows finding READMEs like docs/docs/docs/.gitea/.github/docs/README.md

Fixes go-gitea#23694
jolheiser pushed a commit that referenced this pull request Apr 11, 2023
…s. (#23695)

Fixes a bug introduced in #22177
which allows finding READMEs like
docs/docs/docs/.gitea/.github/docs/README.md

Fixes #23694
@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.
Labels
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/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants