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

error 500 when creating new PR: git read-tree Develop: exit status 128 - error: invalid path #17092

Closed
zuhairamahdi opened this issue Sep 19, 2021 · 22 comments · Fixed by #17300

Comments

@zuhairamahdi
Copy link

Gitea Version

1.15.2

Git Version

No response

Operating System

Windows

How are you running Gitea?

In windows Server as windows process

Database

MSSQL

Can you reproduce the bug on the Gitea demo site?

No

Log Gist

2021/09/19 14:41:51 ...ers/web/repo/pull.go:1113:CompareAndPullRequestPost() [E] NewPullRequest: git read-tree Develop: exit status 128 - error: invalid path 'UCFS TAS'HEEL/Support Files/Assets.xcassets/Icon_NewWordAr.imageset/#New# Icon\Gold\new icons\new arabichdpi.png'

Description

for some projects that have weird path 'created on linux and/or macOS' we keep getting error 500

Screenshots

No response

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 19, 2021

It seems that the some filenames contain strange characters which make Windows filesystem unhappy.

Maybe you can try this first:

https://stackoverflow.com/questions/63727594/github-git-checkout-returns-error-invalid-path-on-windows

And it's highly recommended to use "valid" filenames for files in git in all platforms. Or you can run gitea in Linux, whose filesystem can allow almost every character in filename.

@delvh
Copy link
Member

delvh commented Sep 19, 2021

And it's highly recommended to use "valid" filenames for files in git on all platforms. Or you can run gitea in Linux, whose filesystem can allow almost every character in the filename.

Rule of thumb:
Use only limited ASCII for filenames (0-9 A-Z a-z and -._ as special signs). And especially no spaces, as those can lead to a myriad of other problems.
That prevents exactly those kinds of problems.

@wxiaoguang
Copy link
Contributor

Use only limited ASCII for filenames (0-9 A-Z a-z and -._ as special signs). And especially no spaces, as those can lead to a myriad of other problems.

Spaces are OK in modern days. If you have developed some macOS or iOS application, you will find that Apple recommends to use spaces in filenames.

@zuhairamahdi
Copy link
Author

the issue is that I can see old pull requests worked just fine but once we upgraded to 1.15 we are getting this issue.

@zeripath
Copy link
Contributor

zeripath commented Sep 20, 2021

it's interesting because I can't think of any changes to this area in 1.15.

(In particular this call to read-tree has been present since #9302 in 1.11)

@zeripath
Copy link
Contributor

and the read-tree should not be looking at the filesystem at all.

@zeripath
Copy link
Contributor

We could specifically add a -i to the command-line but there shouldn't be a working tree at all.

@zuhairamahdi
Copy link
Author

it's interesting because I can't think of any changes to this area in 1.15.

(In particular this call to read-tree has been present since #9302 in 1.11)

I think this might explain it. we upgraded from 1.10 to 1.14 and now to 1.15 where all the pull requests we made when we were using 1.10

@zeripath
Copy link
Contributor

zeripath commented Sep 20, 2021

If you could would it be possible change this line:

_, err = git.NewCommand("read-tree", "base").RunInDir(tmpBasePath)

to:

	_, err = git.NewCommand("read-tree", "-i", "base").RunInDir(tmpBasePath)

OK this doesn't work.


which means I need to look at bit closer at why read-tree is even moaning about this - there shouldn't be any interaction of the work-tree.

@zeripath zeripath changed the title error 500 when creating new pull request error 500 when creating new PR: git read-tree Develop: exit status 128 - error: invalid path Sep 20, 2021
zeripath added a commit to zeripath/gitea that referenced this issue Sep 20, 2021
None of our uses of read-tree are supposed to affect the working-tree therefore
explicity ignore it.

Fix go-gitea#17092

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit to zeripath/gitea that referenced this issue Sep 20, 2021
Backport go-gitea#17102

None of our uses of read-tree are supposed to affect the working-tree therefore
explicity ignore it.

Fix go-gitea#17092

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zuhairamahdi
Copy link
Author

zuhairamahdi commented Sep 20, 2021

@zeripath is this specific to gogit version? if we used the default package you think it might help?

@zeripath
Copy link
Contributor

nope. It shouldn't be related to gogit.

I don't understand why the paths are being checked here.

What version of git are you using?

@zuhairamahdi
Copy link
Author

@zeripath
git version 2.31.1.windows.1

I think you were able to replicate this issue earlier when you added "-i". I doubt its the git version that's causing this issue.

@zeripath
Copy link
Contributor

It is related to the git version. (Well partially)

The issue is that you have core.protectNTFS on by default in these later versions, and is not allowing bad names to be even in the index. Setting:

git config core.protectNTFS false

Will stop the problem - but I am not certain how things would respond if a merge was required with this bad file as I know we may attempt to sparse check these out to do merges.

@zuhairamahdi
Copy link
Author

@zeripath this still did not fix it. I run:

git config --global core.protectNTFS false

in the server but it is still no use.

@zeripath
Copy link
Contributor

zeripath commented Sep 22, 2021

I suspect you've not got the right config file and so this setting is not taking effect.


hmm looking at the source code for git the config option is:

core.protectntfs

all lowercase so that could be helpful

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 22, 2021

The git config keys seem case-insensitive : https://git-scm.com/docs/git-config/2.14.6#_configuration_file

The variables are divided into sections ... The variable names are case-insensitive, ...

I also suspect the config are not set correctly on target git repository 😊

@zuhairamahdi
Copy link
Author

@zeripath @wxiaoguang I tried to change the config file for the targeted repo and it still not having any impact, this is the config file:

[core]
	repositoryformatversion = 0
	filemode = false
	bare = true
	symlinks = false
	ignorecase = true
	protectNTFS = false

@zeripath
Copy link
Contributor

@zuhairamahdi sorry I've left this one to stagnate. You would need to set this at the global and user gitconfig level - not the repo level. (Especially as we create temporary repositories.)

Are you able to compile Gitea? Applying the following patch should forcibly switch off core.protectntfs

diff --git a/modules/git/git.go b/modules/git/git.go
index 7ab11736e..c5a45a1ce 100644
--- a/modules/git/git.go
+++ b/modules/git/git.go
@@ -203,6 +203,11 @@ func Init(ctx context.Context) error {
 		if err := checkAndSetConfig("core.longpaths", "true", true); err != nil {
 			return err
 		}
+		if err := checkAndSetConfig("core.protectntfs", "false", true); err != nil {
+			return err
+		}
+		GlobalCommandArgs = append(GlobalCommandArgs, "-c", "core.protectntfs=false")
+
 	}
 	return nil
 }

@zuhairamahdi
Copy link
Author

@zeripath now worries 🙂. I already set protectntfs to false on a global level. but I think that gitea is using another use or something.

I will try the solution you are providing on my local environment and see if it fixes the issue.

I think there should be a patch as this can be a deal breaker for anyone hosting gitea on Windows.

@zuhairamahdi
Copy link
Author

@zeripath it is working now.

@zeripath
Copy link
Contributor

You mean, after applying the above patch the problem is fixed?

@zuhairamahdi
Copy link
Author

@zeripath . yes.
I created a new instance of gitea with the patch and made sure that the gitconfig had protectNTFS = false

I didn't try it without the patch. but I guess it should work as well.

zeripath added a commit to zeripath/gitea that referenced this issue Oct 13, 2021
core.protectNTFS protects NTFS from files which may be difficult to remove or interact
with using the win32 api, however, it also appears to prevent such files from
being entered into the git indexes - fundamentally causing breakages with PRs that
affect these files. However, deliberately setting this to false may cause security
issues due to the remain sparse checkout of files in the merge pipeline.

The only sensible option therefore is to provide an optional setting which admins
could set which would forcibly switch this off if they are affected by this issue.

Fix go-gitea#17092

Signed-off-by: Andrew Thornton <art27@cantab.net>
techknowlogick pushed a commit that referenced this issue Oct 13, 2021
core.protectNTFS protects NTFS from files which may be difficult to remove or interact
with using the win32 api, however, it also appears to prevent such files from
being entered into the git indexes - fundamentally causing breakages with PRs that
affect these files. However, deliberately setting this to false may cause security
issues due to the remain sparse checkout of files in the merge pipeline.

The only sensible option therefore is to provide an optional setting which admins
could set which would forcibly switch this off if they are affected by this issue.

Fix #17092

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit to zeripath/gitea that referenced this issue Oct 13, 2021
Backport go-gitea#17300

core.protectNTFS protects NTFS from files which may be difficult to remove or interact
with using the win32 api, however, it also appears to prevent such files from
being entered into the git indexes - fundamentally causing breakages with PRs that
affect these files. However, deliberately setting this to false may cause security
issues due to the remain sparse checkout of files in the merge pipeline.

The only sensible option therefore is to provide an optional setting which admins
could set which would forcibly switch this off if they are affected by this issue.

Fix go-gitea#17092

Signed-off-by: Andrew Thornton <art27@cantab.net>
lafriks pushed a commit that referenced this issue Oct 13, 2021
Backport #17300

core.protectNTFS protects NTFS from files which may be difficult to remove or interact
with using the win32 api, however, it also appears to prevent such files from
being entered into the git indexes - fundamentally causing breakages with PRs that
affect these files. However, deliberately setting this to false may cause security
issues due to the remain sparse checkout of files in the merge pipeline.

The only sensible option therefore is to provide an optional setting which admins
could set which would forcibly switch this off if they are affected by this issue.

Fix #17092

Signed-off-by: Andrew Thornton <art27@cantab.net>
Chianina pushed a commit to Chianina/gitea that referenced this issue Oct 15, 2021
core.protectNTFS protects NTFS from files which may be difficult to remove or interact
with using the win32 api, however, it also appears to prevent such files from
being entered into the git indexes - fundamentally causing breakages with PRs that
affect these files. However, deliberately setting this to false may cause security
issues due to the remain sparse checkout of files in the merge pipeline.

The only sensible option therefore is to provide an optional setting which admins
could set which would forcibly switch this off if they are affected by this issue.

Fix go-gitea#17092

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants