worktree: add method #361
worktree: add method #361
Conversation
worktree_status.go
Outdated
| return plumbing.ZeroHash, err | ||
| } | ||
|
|
||
| if s.File(path).Worktree == Unmodified { |
There was a problem hiding this comment.
I wonder if returning a zero hash when the operation is successful but unmodified is the right behavior. If it is, please, explain it in godoc, it is not that intuitive.
worktree_status.go
Outdated
| return plumbing.ZeroHash, err | ||
| } | ||
|
|
||
| return h.Sum(), nil |
There was a problem hiding this comment.
return h.Sum(), err to get close error?
There was a problem hiding this comment.
Better yet, declare error in the signature and defer a closure on it instead.
worktree_test.go
Outdated
| @@ -5,6 +5,7 @@ import ( | |||
| "os" | |||
| "path/filepath" | |||
|
|
|||
| billy "gopkg.in/src-d/go-billy.v2" | |||
There was a problem hiding this comment.
mixed external/internal imports
worktree_status.go
Outdated
| return plumbing.ZeroHash, err | ||
| } | ||
|
|
||
| return h.Sum(), nil |
There was a problem hiding this comment.
Better yet, declare error in the signature and defer a closure on it instead.
| return w.r.Storer.SetIndex(idx) | ||
| } | ||
|
|
||
| func (w *Worktree) doAddFileToIndex(idx *index.Index, filename string) error { |
There was a problem hiding this comment.
change the name of this method to addFileToIndex and make it a function, instead of a method.
There was a problem hiding this comment.
could be a function, but since update cannot be a function I rather keep it as a method
| return nil | ||
| } | ||
|
|
||
| func (w *Worktree) doUpdateFileToIndex(idx *index.Index, filename string, h plumbing.Hash) error { |
There was a problem hiding this comment.
Change the name of the method to updateFileToIndex.
There was a problem hiding this comment.
it need context, and since is a very deep method I prefer having the do prefix
There was a problem hiding this comment.
What does do mean for you?
There was a problem hiding this comment.
that is performing the action with a function is a name close to this one without the do
| return err | ||
| } | ||
|
|
||
| for i, e := range idx.Entries { |
There was a problem hiding this comment.
Can we use the Entry method here instead of looking for it again? (like we did in line 217)
There was a problem hiding this comment.
nope, I need to know the position for overwritten it.
There was a problem hiding this comment.
That is part of the problem with this PR, it is too C style, everything changing other things that are sometimes several levels of indirection away.
I think you should refactor and modify the index and the entries, adding the necessary methods there util the functionality here is trivial and it is performed by the entities responsible for it instead of doing everything here.
There was a problem hiding this comment.
I don't if worth do this change, and change the other places where the index is read. When he is the only place that an entry is updated.
|
merging this making other PR change the Index implementation |
This is the bare minimum implementation of
git add