Skip to content

Conversation

@wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 15, 2023

Since #23493 has conflicts with latest commits, this PR is my proposal for fixing #23371

Details are in the comments

And refactor the modules/options module, to make it always use "filepath" to access local files.

Benefits:

  • No need to do util.CleanPath(strings.ReplaceAll(p, "\\", "/"))), "/") any more (not only one before)
  • The function behaviors are clearly defined

@wxiaoguang wxiaoguang force-pushed the fix-clean-path branch 6 times, most recently from cad63c1 to 6f48947 Compare March 15, 2023 13:47
@lunny lunny added the type/bug label Mar 15, 2023
@lunny lunny added this to the 1.20.0 milestone Mar 15, 2023
@techknowlogick
Copy link
Member

CI Fail is related with:

-- FAIL: TestBuildLocalPath (0.00s)
    --- FAIL: TestBuildLocalPath/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14 (0.00s)
panic: FilePathJoinAbs: result is not absolute, do not guess a relative path based on current working directory [recovered]
	panic: FilePathJoinAbs: result is not absolute, do not guess a relative path based on current working directory
...

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 15, 2023
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 16, 2023

CI Fail is related with: TestBuildLocalPath

Fixed by cb9c518 , I think it's better to use absolute paths internally as much as possible, it avoids unclear behaviors.

@wxiaoguang wxiaoguang changed the title Introduce CleanPath helper functions Introduce path Clean/Join helper functions Mar 16, 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 Mar 16, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2023

Codecov Report

Merging #23495 (e94155b) into main (f521e88) will decrease coverage by 0.02%.
The diff coverage is 37.89%.

❗ Current head e94155b differs from pull request most recent head 4191802. Consider uploading reports for the commit 4191802 to get more accurate results

📣 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   #23495      +/-   ##
==========================================
- Coverage   47.14%   47.12%   -0.02%     
==========================================
  Files        1149     1154       +5     
  Lines      151446   152353     +907     
==========================================
+ Hits        71397    71795     +398     
- Misses      71611    72079     +468     
- Partials     8438     8479      +41     
Impacted Files Coverage Δ
cmd/dump.go 0.67% <0.00%> (ø)
cmd/web.go 0.00% <0.00%> (ø)
models/actions/run.go 1.64% <0.00%> (-0.08%) ⬇️
models/actions/runner.go 1.44% <ø> (ø)
models/packages/package.go 45.45% <0.00%> (-1.13%) ⬇️
models/user/search.go 77.50% <0.00%> (-6.29%) ⬇️
modules/actions/github.go 0.00% <0.00%> (ø)
modules/actions/workflows.go 0.00% <0.00%> (ø)
modules/context/context.go 64.54% <0.00%> (-3.53%) ⬇️
modules/doctor/storage.go 31.93% <0.00%> (ø)
... and 33 more

... and 37 files with indirect coverage changes

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

@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 21, 2023
@wxiaoguang
Copy link
Contributor Author

Made some new commits, major changes:

  • Refactor the modules/options module, to make it always use "filepath" to access local files
  • Extract some common code to general functions, like listLocalDirIfExist, readLocalFile, then the dynamic.go and static.go could be simplified a lot.
  • The StaticRootPath was never well-defined, it could be a relative path, so introduce mustLocalPathAbs to tolerate such unstable behavior: make it absolute to current working directory if it was relative, just the same as before.

@delvh

This comment was marked as resolved.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 21, 2023

To address the concern about "misunderstand or misuse the SafePathXxx", in e94155b :

  • Use PathJoinRel instead of SafePathRel, because it's only generally safe across elements, but it doesn't prevent the "/../" inside a single path element, it's caller's duty to define every single path's rule. Add related comments.
  • Add tests suggested by Introduce path Clean/Join helper functions #23495 (comment)

@jolheiser jolheiser enabled auto-merge (squash) March 21, 2023 19:36
@jolheiser
Copy link
Member

🎺 🤖

@jolheiser jolheiser merged commit ce9dee5 into go-gitea:main Mar 21, 2023
@jolheiser jolheiser removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 21, 2023
@wxiaoguang wxiaoguang deleted the fix-clean-path branch March 22, 2023 01:45
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 22, 2023
* upstream/main:
  Use a general approch to improve a11y for all checkboxes and dropdowns. (go-gitea#23542)
  [skip ci] Updated translations via Crowdin
  Update PR documentation (go-gitea#23620)
  Set opaque background on markup and images (go-gitea#23578)
  Decouple the issue-template code from comment_tab.tmpl  (go-gitea#23556)
  Remove `id="comment-form"` dead code, fix tag (go-gitea#23555)
  Introduce path Clean/Join helper functions (go-gitea#23495)
  Remove conflicting CSS rules on notifications, improve notifications table (go-gitea#23565)
  Remove @metalmatze as maintainer (go-gitea#23612)
  Keep (add if not existing) xmlns attribute for generated SVG images (go-gitea#23410)
@wxiaoguang
Copy link
Contributor Author

Backport: Introduce path Clean/Join helper functions, partially backport&refactor #23607

lunny pushed a commit that referenced this pull request Mar 22, 2023
…or (#23495) (#23607)

Backport #23495, partially backport&refactor

The `modules/options` files are just copied from 1.20 to 1.19
techknowlogick pushed a commit that referenced this pull request Mar 31, 2023
@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. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants