Handle files starting with colons in WalkGitLog

[kbolashev]

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

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.

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]

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"

[wxiaoguang]

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]

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

[kbolashev]

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]

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]

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]

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

[delvh]

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.

[wxiaoguang]

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.

[codecov-commenter]

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.