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

Handle files starting with colons in WalkGitLog #22935

Merged
merged 4 commits into from
Mar 16, 2023

Conversation

kbolashev
Copy link
Contributor

Currently gitea shows no commit information for files starting with a colon.

I set up a minimal repro repository that reproduces this error once it's migrated on gitea

image

This is happening because the filenames piped to the git log command are written as is, and it doesn't work when you have a colon at the start of the filename, and you need to escape it.

You can test it locally, if you do

mkdir repo
git init
touch :file 
git add . && git commit -m "Add file with colon"
git log -- :file 

git log returns nothing. However, if you do git log -- "\:file", it will show the commit with the file change.

This PR escapes the starting colons in paths in the LogNameStatusRepo function, making gitea return commit info about the file with the bad filename.

image

This error shows up only with files starting with colon, anywhere else in filename is ok. Dashes at the beginning also seem to be working.
I don't know gitea internals well enough to know where else this error can pop up, so I'm keeping this PR small as suggested by your contributor guide

@wxiaoguang
Copy link
Contributor

Surprise .... According to git document: https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefpathspecapathspec :

A pathspec that begins with a colon : has special meaning. In the short form, .... In the long form, ...

If I understand correctly, the :(literal) prefix should be used: git log -- ":(literal):test"

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

And one more thing, I think we should process the file list from framework level (the git module), there could be a lot of functions need to pass the file list (path list)

@delvh
Copy link
Member

delvh commented Feb 16, 2023

Yes, that also handles the other edge cases much better (i.e. !file)

@kbolashev
Copy link
Contributor Author

kbolashev commented Feb 16, 2023

If I understand correctly, the :(literal) prefix should be used: git log -- ":(literal):test"

Thanks for the suggestion!! I checked it and it does work, would you suggest adding the :(literal) magic in front of every path then?

And one more thing, I think we should process the file list from framework level (the git module), there could be a lot of functions need to pass the file list (path list)

Sure, I'll try to touch only the places where paths are being passed, and not oid's/etc

I have a concern with setting this on the git-module level, because now if some command say specifically gets only jpegs by doing git log -- *.jpg, with the literal magic it will only look for the literal '*.jpg' file.
So I guess the way to go would probably be a function on the command struct that adds specifically literal files. Or a "literal" flag on the AddDashesAndList function

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 16, 2023

I have a concern with setting this on the git-module level, because now if some command say specifically gets only jpegs by doing git log -- *.jpg, with the literal magic it will only look for the literal '*.jpg' file. So I guess the way to go would probably be a function on the command struct that adds specifically literal files. Or a "literal" flag on the AddDashesAndList function

Actually I am not sure whether AddDashesAndList needs to process wildcard or other patterns at the moment, either. If I remember correctly, there might be no such usage before, so maybe it's safe to make all paths literal. Just for reference only, this problem seems more complicated that it looks. 🤔


ps: as a bug fix, only process the paths for the git log seems enough

@kbolashev
Copy link
Contributor Author

I tried changing AddDashesAndList and it ended breaking a bunch of tests, mostly related to tags/release creation, where it tried to do :(literal)<sha1>, so sadly it doesn't seem like a super easy to thing to change and requires to actually know which command handles only hashes, and which command handle files.
So I just changed the git log arguments for now, but using the :(literal) now.

@kbolashev
Copy link
Contributor Author

kbolashev commented Feb 18, 2023

Since the :(literal) has to be only in front of non-alphanumeric pathspecs, maybe we can check for a non-alphanumeric (or even straight up just non-sha1) and place the magic only there 🤔
I'll try to do that

EDIT: Tried doing it, and it still breaks the release creation tests, because the tag name is passed to AddDashesAndList, so it ends creating a :(literal)v0.1 tag and failing the test

@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
@lunny lunny added the type/enhancement An improvement of existing functionality label Mar 16, 2023
@lunny lunny added this to the 1.20.0 milestone Mar 16, 2023
Comment on lines +59 to 63
// Use the :(literal) pathspec magic to handle edge cases with files named like ":file.txt" or "*.jpg"
for i, file := range files {
files[i] = ":(literal)" + file
}
cmd.AddDashesAndList(files...)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, wouldn't it be better to add it to AddDashesAndList directly?
That way, it will already be applied to every possible instance, I don't know of any usage that expects wildcards to work, and it is in the other cases not intended to be used as a wildcard.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, wouldn't it be better to add it to AddDashesAndList directly?

This approach has been discussed before, it might break something.

Maybe next time we can introduce AddDashesAndLiterals, but not change the behavior of AddDashesAndList.

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

Codecov Report

Merging #22935 (db75049) into main (f521e88) will increase coverage by 0.02%.
The diff coverage is 44.22%.

📣 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   #22935      +/-   ##
==========================================
+ Coverage   47.14%   47.16%   +0.02%     
==========================================
  Files        1149     1154       +5     
  Lines      151446   152196     +750     
==========================================
+ Hits        71397    71790     +393     
- Misses      71611    71938     +327     
- Partials     8438     8468      +30     
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/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/doctor/storage.go 31.93% <0.00%> (ø)
modules/setting/git.go 45.45% <ø> (ø)
modules/storage/minio.go 1.51% <0.00%> (-0.06%) ⬇️
... and 39 more

... and 16 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.

@jolheiser jolheiser merged commit 4938945 into go-gitea:main Mar 16, 2023
@jolheiser jolheiser removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 16, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 17, 2023
* giteaofficial/main:
  Use `<nav>` instead of `<div>` in the global navbar (go-gitea#23125)
  Fix aria.js bugs: incorrect role element problem, mobile focus problem, tippy problem (go-gitea#23450)
  [skip ci] Updated translations via Crowdin
  Make time tooltips interactive (go-gitea#23526)
  Update mini-css-extract-plugin, remove postcss (go-gitea#23520)
  Fix review comment context menu clipped bug (go-gitea#23523)
  Add absent repounits to create/edit repo API (go-gitea#23500)
  Fix tags sort by creation time (descending) on branch/tag dropdowns  (go-gitea#23491)
  Allow both fullname and username search when `DEFAULT_SHOW_FULL_NAME` is true (go-gitea#23463)
  Handle files starting with colons in WalkGitLog (go-gitea#22935)
  Change `Close` to either `Close issue` or `Close pull request` (go-gitea#23506)
  Update act (go-gitea#23512)
  Move pidfile creation from setting to web cmd package (go-gitea#23285)
@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/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants