- Notifications
You must be signed in to change notification settings - Fork868
transport: fix opening relative paths#1782
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
This PR fixes an issue with opening or cloning git repositories using relative paths. The fix addresses a conflict with go-billy's chroot filesystem, which rejects paths starting with../. The solution converts relative paths to absolute paths in theparseFile function usingfilepath.Abs(), ensuring compatibility with the chroot filesystem while maintaining backward compatibility with v5 behavior.
Key Changes
- Modified
parseFile()inplumbing/transport/transport.goto convert endpoints to absolute paths usingfilepath.Abs() - Added
TestCloneRelative()test to verify that repositories can be cloned using relative paths
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| plumbing/transport/transport.go | UpdatedparseFile() to convert relative paths to absolute paths usingfilepath.Abs() before creating the file endpoint |
| repository_test.go | AddedTestCloneRelative() test case to verify relative path cloning functionality |
💡Add Copilot custom instructions for smarter, more guided reviews.Learn how to get started.
Uh oh!
There was an error while loading.Please reload this page.
plumbing/transport/transport.go Outdated
| path,err:=filepath.Abs(endpoint) | ||
| iferr!=nil { | ||
| returnnil,false | ||
| } |
CopilotAIDec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The change to usefilepath.Abs() will break the existing test inplumbing/transport/transport_test.go at lines 109-110. That test expects the relative path "foo.git" to remain as "file://foo.git", butfilepath.Abs() will convert it to an absolute path like "file:///current/working/dir/foo.git".
The test inTestNewEndpointFile should be updated to reflect the new behavior, or the implementation should handle this case differently.
repository_test.go Outdated
| } | ||
| fs:=memfs.New() | ||
| r,_:=Init(memory.NewStorage(),WithWorkTree(fs)) |
CopilotAIDec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The error return value fromInit() is being ignored. While this may work in tests, it's better practice to check the error to ensure the test setup is correct:
r,err:=Init(memory.NewStorage(),WithWorkTree(fs))s.NoError(err)
| r,_:=Init(memory.NewStorage(),WithWorkTree(fs)) | |
| r,err:=Init(memory.NewStorage(),WithWorkTree(fs)) | |
| s.NoError(err) |
repository_test.go Outdated
| path:=filepath.Join(root,origPath) | ||
| err=r.clone(context.Background(),&CloneOptions{ | ||
| URL:path, |
CopilotAIDec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The test logic has a fundamental flaw.GetBasicLocalRepositoryURL() returns an absolute path (e.g.,/tmp/xyz123/repo/.git), and when you usefilepath.Join(root, origPath) whereorigPath is absolute,filepath.Join will discard theroot prefix and just returnorigPath. This means the test isn't actually testing a relative path scenario.
To properly test relative paths, you need to either:
- Use
filepath.Rel()to convertorigPathto a relative path from the current directory, or - Use a path that's already relative (not starting with
/or a drive letter)
For example:
relPath,err:=filepath.Rel(".",origPath)s.NoError(err)err=r.clone(context.Background(),&CloneOptions{URL:relPath,})
| path:=filepath.Join(root,origPath) | |
| err=r.clone(context.Background(),&CloneOptions{ | |
| URL:path, | |
| relPath,err:=filepath.Rel(".",origPath) | |
| s.NoError(err) | |
| err=r.clone(context.Background(),&CloneOptions{ | |
| URL:relPath, |
go-billy checks that in a chroot filesystem the paths do not start by`../`. This causes opening or cloning relative paths to fail. This checkis good to find repository links that point to files outside therepository but for opening path is not that useful as the defaultfilesystem is based at `/`.The change to `parseFile` does the same as v5. The test and the embeddedfunction to generate the relative path is quite naive but checks thatthe change fixes the issue.Fixesgo-git#1723Signed-off-by: Javi Fontan <jfontan@gmail.com>
jfontan commentedDec 4, 2025
I'll go back to the drawing board as I'm not happy with the test or the solution. |
Default filesystem was creating a ChrootOS that does not make much senseas the base dir is "". This filesystem does not have that problem withrelative directories.Signed-off-by: Javi Fontan <jfontan@gmail.com>
jfontan commentedDec 16, 2025
I'm still checking how I can test it and make it work with Windows but I believe BoundOS could be better fitted for the default FS for loader. |
jfontan commentedDec 16, 2025
The error in Windows tests is given by |
go-billy checks that in a chroot filesystem the paths do not start by
../. This causes opening or cloning relative paths to fail. This check is good to find repository links that point to files outside the repository but for opening path is not that useful as the default filesystem is based at/.The change to
parseFiledoes the same as v5. The test and the embedded function to generate the relative path is quite naive but checks that the change fixes the issue.Fixes#1723