Add sparse reset directory validation and remove ResetSparsely. #1531
Conversation
| } | ||
|
|
||
| t, err := w.r.getTreeFromCommitHash(opts.Commit) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if len(opts.SparseDirs) > 0 && !opts.SkipSparseDirValidation { |
There was a problem hiding this comment.
| if len(opts.SparseDirs) > 0 && !opts.SkipSparseDirValidation { | |
| if !opts.SkipSparseDirValidation { |
| @@ -324,6 +337,20 @@ func (w *Worktree) ResetSparsely(opts *ResetOptions, dirs []string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func treeContainsDirs(tree *object.Tree, dirs []string) bool { | |||
| for _, dir := range dirs { | |||
There was a problem hiding this comment.
| for _, dir := range dirs { | |
| if len(dirs) == 0 { | |
| return false | |
| } | |
| for _, dir := range dirs { |
There was a problem hiding this comment.
Are we sure that we should validate dirs of zero length on treeContainsDirs?
I think it will break the applications that migrate from v5 to v6, given that
opts.SkipSparseDirValidation == false and len(opts.SparseDirs) == 0 would be the default for options.
If that's affordable, I think something like ErrSparseResetWithNodirs would be more appropriate than ErrSparseResetDirectoryNotFound. So I suggest we check len(dirs) == 0 && !opts.SkipSparseDirValidation on ResetOptions.Validate().
| if err != nil { | ||
| return false | ||
| } | ||
| if entry.Mode != filemode.Dir { |
There was a problem hiding this comment.
Do we always get an exclusive entry for each dir in the tree? I was under the impression that this was not necessarily always the case.
There was a problem hiding this comment.
AFAIK, yeah, git creates tree object for each directory that is not empty. Do you see any edge cases?
| @@ -1344,7 +1344,7 @@ func (s *WorktreeSuite) TestResetSparsely() { | |||
|
|
|||
| sparseResetDirs := []string{"php"} | |||
|
|
|||
| err := w.ResetSparsely(&ResetOptions{Mode: HardReset}, sparseResetDirs) | |||
| err := w.Reset(&ResetOptions{Mode: HardReset, SparseDirs: sparseResetDirs}) | |||
There was a problem hiding this comment.
Agree with the change, but let's do that straight into the v6 instead. So please rebase and change the PR target to the v6-transport branch.
| {"non-existent"}, | ||
| {"php/crappy.php"}, // non-dir |
There was a problem hiding this comment.
Let's expand this to be more like table driven tests, so that we can have different input/output checks. With that, please add a test case ensuring the behaviour of SkipSparseDirValidation.
| Filesystem: fs, | ||
| } | ||
|
|
||
| testcases := [][]string{ |
There was a problem hiding this comment.
| testcases := [][]string{ | |
| tests := [][]string{ |
Fixes #1500
ResetSparselyand moveddirsintoResetOptions.