add support for .git as file, fixes #348 #363
Conversation
|
Given current |
Codecov Report
@@ Coverage Diff @@
## master #363 +/- ##
==========================================
- Coverage 77.28% 76.62% -0.66%
==========================================
Files 122 122
Lines 8518 8483 -35
==========================================
- Hits 6583 6500 -83
- Misses 1198 1267 +69
+ Partials 737 716 -21
Continue to review full report at Codecov.
|
repository.go
Outdated
| if !os.IsNotExist(err) { | ||
| return nil, err | ||
| } | ||
|
|
||
| dot = fs | ||
| } else { | ||
| wt = fs | ||
| dot = fs.Dir(".git") | ||
| if fi.IsDir() { |
There was a problem hiding this comment.
a very nested logic, consider move this to a function
repository.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| fmt.Println("BASE", dot.Base()) |
repository.go
Outdated
| @@ -196,15 +199,25 @@ func PlainOpen(path string) (*Repository, error) { | |||
| var wt, dot billy.Filesystem | |||
|
|
|||
| fs := osfs.New(path) | |||
| if _, err := fs.Stat(".git"); err != nil { | |||
| fi, err := fs.Stat(".git") | |||
| if err != nil { | |||
There was a problem hiding this comment.
I think the logic here is correct, but we can improve its readability by hiding the low-level details: add a switch with few functions, like isWorkingDirectory, isSubModule, isBare... that reveal our intentions to the reader, hiding the details, that are sometimes unkown to the reader and much more difficult to get right.
This will also fix @mcuadros comment as a bonus.
This will also reveal pretty simply what is supported and what is not, from the very beginning, instead of trying to go over the inverted logic checks and details.
I would also move all this logic to a new function that gets a path and returns the filesystem of the git repository it is referencing, whether it is a submodule, a bare or a working directory, something like resolveRepoPath.
There was a problem hiding this comment.
Separating it into such functions would require doing redundant stats operations, or passing quite different arguments to each function. Could you review the new version and check if it's more readable now?
The To learn if absolute paths are common I propose to run a query in our database over all our cloned repos. |
smola
changed the title
|
@alcortesm Finally, it does support both absolute and relative paths, I did not realized that we're not working with arbitrary billy filesystems here, only OS. |
No description provided.