Skip to content
This repository has been archived by the owner on Sep 11, 2020. It is now read-only.

worktree, status and reset implementation based on merkletrie #339

Merged
merged 9 commits into from
Apr 12, 2017
52 changes: 52 additions & 0 deletions options.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,3 +177,55 @@ type SubmoduleUpdateOptions struct {
// submodules (and so on). Until the SubmoduleRescursivity is reached.
RecurseSubmodules SubmoduleRescursivity
}

// CheckoutOptions describes how a checkout operation should be performed.
type CheckoutOptions struct {
// Branch to be checked out, if empty uses `master`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

End the sentence with a full stop.

Branch plumbing.ReferenceName
Hash plumbing.Hash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing documentation.

// RemoteName is the name of the remote to be pushed to.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment does not match any code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is godoc for RemoteName and the variable is Force, copypasta?

Force bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing documentation.

}

// Validate validates the fields and sets the default values.
func (o *CheckoutOptions) Validate() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this method return an error?

On top of that, maybe validate is not the best name for this method, when validating you return true or false according to some checks, while this method overwrites some attributes (and only some of them, leaving other invalid values untouched) probably because there is not constructor or setters to this properly. Maybe a better name will be FillInDefaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already discussed on previous PRs, and is not related to this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, it was in #178.

I'll open an issue, as we agreed back then.

if o.Branch == "" {
o.Branch = plumbing.Master
}

return nil
}

type ResetMode int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing documentation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StatusCode below is an int8, but this is an int. Maybe we should use the same criteria in both cases.


const (
// HardReset resets the index and working tree. Any changes to tracked files
// in the working tree are discarded.
HardReset ResetMode = iota
// MixedReset Resets the index but not the working tree (i.e., the changed
// files are preserved but not marked for commit) and reports what has not
// been updated. This is the default action.
MixedReset
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can ResetMode be simplified into a boolean for the hard mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResetMode has 5 posible values

)

// ResetOptions describes how a reset operation should be performed.
type ResetOptions struct {
// Commit, if commit is pressent set the current branch head (HEAD) to it.
Commit plumbing.Hash
// Mode
Mode ResetMode
}

// Validate validates the fields and sets the default values.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as before, maybe FillInDefaults is a better name for this function, or just add a constructor that does all the work.

func (o *ResetOptions) Validate(r *Repository) error {
if o.Commit == plumbing.ZeroHash {
ref, err := r.Head()
if err != nil {
return err
}

o.Commit = ref.Hash()
}

return nil
}
23 changes: 23 additions & 0 deletions plumbing/format/index/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package index

import (
"errors"
"fmt"
"time"

"gopkg.in/src-d/go-git.v4/plumbing"
Expand Down Expand Up @@ -47,6 +48,16 @@ type Index struct {
ResolveUndo *ResolveUndo
}

// String is equivalent to `git ls-files --stage --debug`
func (i *Index) String() string {
var o string
for _, e := range i.Entries {
o += e.String()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though efficiency is not important here, but my spider sense is tingling: with strings of this size, string concatenation is about 2000 times slower than using bytes.Buffer.

}

return o
}

// Entry represents a single file (or stage of a file) in the cache. An entry
// represents exactly one stage of a file. If a file path is unmerged then
// multiple Entry instances may appear for the same path name.
Expand Down Expand Up @@ -78,6 +89,18 @@ type Entry struct {
IntentToAdd bool
}

func (e Entry) String() string {
var o string
o += fmt.Sprintf("%06o %s %d\t%s\n", e.Mode, e.Hash, e.Stage, e.Name)
o += fmt.Sprintf(" ctime: %d:%d\n", e.CreatedAt.Unix(), e.CreatedAt.Nanosecond())
o += fmt.Sprintf(" mtime: %d:%d\n", e.ModifiedAt.Unix(), e.ModifiedAt.Nanosecond())
o += fmt.Sprintf(" dev: %d\tino: %d\n", e.Dev, e.Inode)
o += fmt.Sprintf(" uid: %d\tgid: %d\n", e.UID, e.GID)
o += fmt.Sprintf(" size: %d\tflags: %x\n", e.Size, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same things here with string concatenation.


return o
}

// Tree contains pre-computed hashes for trees that can be derived from the
// index. It helps speed up tree object generation from index for a new commit.
type Tree struct {
Expand Down
4 changes: 2 additions & 2 deletions plumbing/object/difftree.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import (
// DiffTree compares the content and mode of the blobs found via two
// tree objects.
func DiffTree(a, b *Tree) (Changes, error) {
from := newTreeNoder(a)
to := newTreeNoder(b)
from := NewTreeRootNode(a)
to := NewTreeRootNode(b)

hashEqual := func(a, b noder.Hasher) bool {
return bytes.Equal(a.Hash(), b.Hash())
Expand Down
8 changes: 5 additions & 3 deletions plumbing/object/tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ type TreeEntry struct {
// File returns the hash of the file identified by the `path` argument.
// The path is interpreted as relative to the tree receiver.
func (t *Tree) File(path string) (*File, error) {
e, err := t.findEntry(path)
e, err := t.FindEntry(path)
if err != nil {
return nil, ErrFileNotFound
}
Expand All @@ -86,7 +86,7 @@ func (t *Tree) File(path string) (*File, error) {
// Tree returns the tree identified by the `path` argument.
// The path is interpreted as relative to the tree receiver.
func (t *Tree) Tree(path string) (*Tree, error) {
e, err := t.findEntry(path)
e, err := t.FindEntry(path)
if err != nil {
return nil, ErrDirectoryNotFound
}
Expand All @@ -109,7 +109,8 @@ func (t *Tree) TreeEntryFile(e *TreeEntry) (*File, error) {
return NewFile(e.Name, e.Mode, blob), nil
}

func (t *Tree) findEntry(path string) (*TreeEntry, error) {
// FindEntry search a TreeEntry in this tree or any subtree
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a full stop at the end of the sentence.

func (t *Tree) FindEntry(path string) (*TreeEntry, error) {
pathParts := strings.Split(path, "/")

var tree *Tree
Expand Down Expand Up @@ -146,6 +147,7 @@ func (t *Tree) entry(baseName string) (*TreeEntry, error) {
if t.m == nil {
t.buildMap()
}

entry, ok := t.m[baseName]
if !ok {
return nil, errEntryNotFound
Expand Down
6 changes: 6 additions & 0 deletions plumbing/object/tree_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ func (s *TreeSuite) TestFiles(c *C) {
c.Assert(count, Equals, 9)
}

func (s *TreeSuite) TestFindEntry(c *C) {
e, err := s.Tree.FindEntry("vendor/foo.go")
c.Assert(err, IsNil)
c.Assert(e.Name, Equals, "foo.go")
}

// This plumbing.EncodedObject implementation has a reader that only returns 6
// bytes at a time, this should simulate the conditions when a read
// returns less bytes than asked, for example when reading a hash which
Expand Down
7 changes: 4 additions & 3 deletions plumbing/object/treenoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ type treeNoder struct {
name string // empty string for the root node
mode filemode.FileMode
hash plumbing.Hash
children []noder.Noder // memoized
children []noder.Noder // memorized
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memoized was actually the right term ;-)

}

func newTreeNoder(t *Tree) *treeNoder {
// NewTreeRootNode returns the root node of a Tree
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a full stop at the end of the sentence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a full stop at the end of the sentence.

func NewTreeRootNode(t *Tree) *treeNoder {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change of name is weird. For once, the type this is constructing is treeNoder, not treeRootNode. Then, a root is always a node, so RootNode is redundant, also my trees are always roots, so TreeRoot is also redundant.

Maybe if you explain what you want to do here we can find a better name for this ctor (and its associated type).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be just NewTreeNode? The Root part seems redundant?

if t == nil {
return &treeNoder{}
}
Expand Down Expand Up @@ -74,7 +75,7 @@ func (t *treeNoder) Children() ([]noder.Noder, error) {
return noder.NoChildren, nil
}

// children are memoized for efficiency
// children are memorized for efficiency
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before, children are memoized for efficiency.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/memorized/memoized/

if t.children != nil {
return t.children, nil
}
Expand Down
20 changes: 11 additions & 9 deletions repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,11 +340,11 @@ func (r *Repository) clone(o *CloneOptions) error {
return err
}

if _, err := r.updateReferences(c.Fetch, o.ReferenceName, head); err != nil {
if _, err := r.updateReferences(c.Fetch, head); err != nil {
return err
}

if err := r.updateWorktree(); err != nil {
if err := r.updateWorktree(head.Name()); err != nil {
return err
}

Expand Down Expand Up @@ -429,7 +429,7 @@ func (r *Repository) updateRemoteConfig(remote *Remote, o *CloneOptions,
}

func (r *Repository) updateReferences(spec []config.RefSpec,
headName plumbing.ReferenceName, resolvedHead *plumbing.Reference) (updated bool, err error) {
resolvedHead *plumbing.Reference) (updated bool, err error) {

if !resolvedHead.IsBranch() {
// Detached HEAD mode
Expand Down Expand Up @@ -534,7 +534,7 @@ func (r *Repository) Pull(o *PullOptions) error {
return err
}

refsUpdated, err := r.updateReferences(remote.c.Fetch, o.ReferenceName, head)
refsUpdated, err := r.updateReferences(remote.c.Fetch, head)
if err != nil {
return err
}
Expand All @@ -547,7 +547,7 @@ func (r *Repository) Pull(o *PullOptions) error {
return NoErrAlreadyUpToDate
}

if err := r.updateWorktree(); err != nil {
if err := r.updateWorktree(head.Name()); err != nil {
return err
}

Expand All @@ -560,22 +560,24 @@ func (r *Repository) Pull(o *PullOptions) error {
return nil
}

func (r *Repository) updateWorktree() error {
func (r *Repository) updateWorktree(branch plumbing.ReferenceName) error {
if r.wt == nil {
return nil
}

w, err := r.Worktree()
b, err := r.Reference(branch, true)
if err != nil {
return err
}

h, err := r.Head()
w, err := r.Worktree()
if err != nil {
return err
}

return w.Checkout(h.Hash())
return w.reset(&ResetOptions{
Commit: b.Hash(),
})
}

// Fetch fetches changes from a remote repository.
Expand Down
92 changes: 92 additions & 0 deletions status.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package git

import "fmt"

// Status current status of a Worktree
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a verb and a full stop to this sentence.

Can you explain what do you mean by status, the comment right now is useless.

Also explain what do the keys represent in the map.

type Status map[string]*FileStatus

func (s Status) File(filename string) *FileStatus {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document the method.

Does this receive paths of filenames?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were documented moved code. This doesn't bellow to this PR

if _, ok := (s)[filename]; !ok {
s[filename] = &FileStatus{}
}

return s[filename]

}

func (s Status) IsClean() bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document the method.

for _, status := range s {
if status.Worktree != Unmodified || status.Staging != Unmodified {
return false
}
}

return true
}

func (s Status) String() string {
var names []string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe paths and path are better names instead of names and name.

for name := range s {
names = append(names, name)
}

var output string
for _, name := range names {
Copy link
Contributor

@alcortesm alcortesm Apr 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the point of generating the list of keys (names) and then iterating it to go over all values. Why don't you iterate over the map directly?

Were you thinking in sorting by name? (it would make sense for a string method) but it would be better to generate a unsorted slice of results and then sort it by name.

status := s[name]
if status.Staging == 0 && status.Worktree == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use magic numbers.

continue
}

if status.Staging == Renamed {
name = fmt.Sprintf("%s -> %s", name, status.Extra)
}

output += fmt.Sprintf("%s%s %s\n", status.Staging, status.Worktree, name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before, use a bytes.Buffer.

}

return output
}

// FileStatus status of a file in the Worktree
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a verb and a full stop to this comment.

type FileStatus struct {
Staging StatusCode
Worktree StatusCode
Extra string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document this.

}

// StatusCode status code of a file in the Worktree
type StatusCode int8

const (
Unmodified StatusCode = iota
Untracked
Modified
Added
Deleted
Renamed
Copied
UpdatedButUnmerged
)

func (c StatusCode) String() string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be simpler if declare StatusCode as a rune or a string instead of an int8?

switch c {
case Unmodified:
return " "
case Modified:
return "M"
case Added:
return "A"
case Deleted:
return "D"
case Renamed:
return "R"
case Copied:
return "C"
case UpdatedButUnmerged:
return "U"
case Untracked:
return "?"
default:
return "-"
}
}
6 changes: 3 additions & 3 deletions submodule.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,10 @@ func (s *Submodule) Update(o *SubmoduleUpdateOptions) error {
return err
}

return s.doRecrusiveUpdate(r, o)
return s.doRecursiveUpdate(r, o)
}

func (s *Submodule) doRecrusiveUpdate(r *Repository, o *SubmoduleUpdateOptions) error {
func (s *Submodule) doRecursiveUpdate(r *Repository, o *SubmoduleUpdateOptions) error {
if o.RecurseSubmodules == NoRecurseSubmodules {
return nil
}
Expand Down Expand Up @@ -140,7 +140,7 @@ func (s *Submodule) fetchAndCheckout(r *Repository, o *SubmoduleUpdateOptions, h
return err
}

if err := w.Checkout(hash); err != nil {
if err := w.Checkout(&CheckoutOptions{Hash: hash}); err != nil {
return err
}

Expand Down
5 changes: 2 additions & 3 deletions submodule_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ func (s *SubmoduleSuite) SetUpTest(c *C) {
dir, err := ioutil.TempDir("", "submodule")
c.Assert(err, IsNil)

r, err := PlainClone(dir, false, &CloneOptions{
URL: fmt.Sprintf("file://%s", filepath.Join(path)),
r, err := PlainClone(filepath.Join(dir, "worktree"), false, &CloneOptions{
URL: fmt.Sprintf("file://%s", path),
})

c.Assert(err, IsNil)
Expand Down Expand Up @@ -74,7 +74,6 @@ func (s *SubmoduleSuite) TestUpdate(c *C) {
ref, err := r.Reference(plumbing.HEAD, true)
c.Assert(err, IsNil)
c.Assert(ref.Hash().String(), Equals, "6ecf0ef2c2dffb796033e5a02219af86ec6584e5")

}

func (s *SubmoduleSuite) TestUpdateWithoutInit(c *C) {
Expand Down
Loading