worktree: Commit method implementation #375

mcuadros · 2017-05-04T00:07:51Z

This is the implementation of a basic git commit command.

With the objective of create Tree objects the tree and parents were make public, the names are not the best ones, but I will create another PR improving this. (I didn't wanted to become this PR in a monster)

Fixes #256

alcortesm · 2017-05-04T08:03:18Z

Please, fix CI.

codecov · 2017-05-04T08:17:37Z

Codecov Report

Merging #375 into master will decrease coverage by 0.69%.
The diff coverage is 74.07%.

@@            Coverage Diff            @@
##           master     #375     +/-   ##
=========================================
- Coverage   77.37%   76.67%   -0.7%     
=========================================
  Files         122      123      +1     
  Lines        8614     8751    +137     
=========================================
+ Hits         6665     6710     +45     
- Misses       1207     1295     +88     
- Partials      742      746      +4

Impacted Files	Coverage Δ
plumbing/object/tree.go	`76.96% <0%> (-0.93%)`	⬇️
plumbing/object/blob.go	`73.58% <100%> (ø)`	⬆️
status.go	`50% <100%> (ø)`	⬆️
plumbing/memory.go	`100% <100%> (ø)`	⬆️
worktree_status.go	`67.56% <100%> (+2.35%)`	⬆️
worktree_commit.go	`70.49% <70.49%> (ø)`
options.go	`79.62% <83.33%> (+1.05%)`	⬆️
plumbing/object/commit.go	`71.3% <85.71%> (ø)`	⬆️
plumbing/transport/ssh/common.go	`0% <0%> (-50.91%)`	⬇️
plumbing/transport/ssh/auth_method.go	`32.4% <0%> (-24.08%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e80cdba...3713157. Read the comment docs.

mcuadros · 2017-05-04T08:18:24Z

Solved, the problem was on master.

ajnavarro · 2017-05-04T07:58:12Z

options.go

@@ -42,7 +43,7 @@ type CloneOptions struct {
 	// Limit fetching to the specified number of commits.
 	Depth int
 	// RecurseSubmodules after the clone is created, initialize all submodules
-	// within, using their default settings. This option is ignored if the
+	// within, using their defaut settings. This option is ignored if the


ajnavarro · 2017-05-04T08:02:46Z

options.go

+)
+
+// CommitOptions describes how a commit operation should be performed.
+type CommitOptions struct {


I'm missing the commit message.

the commit message is at the method, the message is not a option

I think should be an option (https://git-scm.com/docs/git-commit#git-commit--mltmsggt), but it's depending of how we want to manage the public API.

ajnavarro · 2017-05-04T08:03:20Z

options.go

+		}
+
+		if head != nil {
+			o.Parents = []plumbing.Hash{head.Hash()}


If head is nil what happens?

that is the initial commit, and doesn't have any parents?

ajnavarro · 2017-05-04T08:10:55Z

plumbing/object/tag_test.go

@@ -234,41 +234,40 @@ func (s *TagSuite) TestString(c *C) {
 	)
 }

-func (s *TagSuite) TestTagToTagString(c *C) {
+func (s *TagSuite) TestStringNonCommit(c *C) {


I don't understand why this is related to this PR, and the name changing.

I was fixing the test because after changing the Write to auto-count the size, I figure out that the test was wrong.

The test functions are called "Test[variant]"

The test was wrong

Didn't worked after autocalculate the size, because you can't use a custom hash in the objects. But since size was 0, the hash was not being calculated.

ajnavarro · 2017-05-04T08:30:22Z

plumbing/object/tree.go

@@ -240,29 +240,21 @@ func (t *Tree) Encode(o plumbing.EncodedObject) error {
 		return err
 	}

-	var size int


This change is related to this commit, or is just a separated code improvement?

ajnavarro · 2017-05-04T08:32:36Z

worktree_commit.go

+		return plumbing.ZeroHash, err
+	}
+
+	if opts.All == true {


better do:

if opts.All { ...

ajnavarro · 2017-05-04T08:33:26Z

worktree_commit.go

+
+// Commit stores the current contents of the index in a new commit along with
+// a log message from the user describing the changes.
+func (w *Worktree) Commit(msg string, opts *CommitOptions) (plumbing.Hash, error) {


Why the message is not part of the CommitOptions?

I followed the pattern that we have in some other options. I can imaging someone reusing opts, but never the message.

I find the design of the commit operation very surprising:

I would expect this method to receive what you want to commit (an index or whatever), along with the msg and the options.

I would expect this to be a method of Repository or Storage or any other entity, except for worktree.

Maybe you should document the overall design of this somewhere.

What you commit is what is in staging, is a very basic concept of git. If you want to do dirty things like storing an espontaneous commit just push is to the Storage.

Since you are committing the staging and this is a concept of the worktree, I don't see why this should be at Repository.

I think that this is very natural way of doing a commit:

r, err := Init(storage, fs) w, err := r.Worktree() billy.WriteFile(fs, "foo", []byte("foo"), 0644) w.Add("foo") w.Commit("foo\n", &CommitOptions{Author: defaultSignature()})

You can have that if you want, with wrapper functions. But you don't actually need to write the core logic like that to get that usage pattern. The whole purpose of the index is to isolate the worktree from the repository:

there is a set of conceptual operations between the worktree and the index (like git add).

there is a different set of conceptual operations between the index and the repository (like git commit).

(except with a few concessions here and there due to pure usability that are always implemented at a very high level)

See, passing the index to the commit function avoids relaying on internal state and make it much easier to test and to mock. Then moving the commit function to a class that is not the worktree will be help to distribute responsibilities where they originally belong in the real git.

Sorry but I am complete missing. Why I will want to make to the user pass the index, the index is a plumbing concept, not exposed on the git package.

Mock and test is very easy because you can replace the storage where all is contained.

This is a small example of the jgit doing a commit with the porcelain API:

Git git = new Git(db); CommitCommand commit = git.commit(); commit.setMessage("initial commit").call();

Never mind, it is just a different point of view about the design.

These examples are very nice, maybe we can reuse them as actual real examples. They will help with the review of the PR and with the general documentation of the library.

What I pasted are not examples are the tests. As I said once that is merge I will do some examples

It would love to have these examples before the actual code, as a PR, so I can review them and understand the purposes of a change before reading the code.

ajnavarro · 2017-05-04T08:41:39Z

options.go

+// Validate validates the fields and sets the default values.
+func (o *CommitOptions) Validate(r *Repository) error {
+	if o.Author == nil {
+		return ErrMissingAuthor


You can test it easily.

ajnavarro · 2017-05-04T08:44:01Z

worktree_commit.go

+	}
+
+	if _, ok := h.entries[path]; ok {
+		return false


You can test it easily.

ajnavarro · 2017-05-04T08:47:03Z

worktree_commit.go

+
+	for path, fs := range s {
+		if fs.Worktree != Modified && fs.Worktree != Deleted {
+			continue


You can test it easily.

ajnavarro · 2017-05-04T08:48:06Z

worktree_commit.go

+// a log message from the user describing the changes.
+func (w *Worktree) Commit(msg string, opts *CommitOptions) (plumbing.Hash, error) {
+	if err := opts.Validate(w.r); err != nil {
+		return plumbing.ZeroHash, err


You can test it easily.

alcortesm

I don't understand fully how a worktree works so I didn't understand this commit fully; not sure if when committing copies of files, duplicates appear in the repository objects. Is this kind of things handled by this code or tests anywhere?

alcortesm · 2017-05-04T11:04:53Z

options.go

+	// Author is the author's signature of the commit.
+	Author *object.Signature
+	// Committer is the committer's signature of the commit. If Committer is
+	// equal to nil the Author signature is used.


remove equal to.

alcortesm · 2017-05-04T11:08:16Z

options.go

+	// Committer is the committer's signature of the commit. If Committer is
+	// equal to nil the Author signature is used.
+	Committer *object.Signature
+	// Parents parents commits for the new commit, by default is the hash of


add are the between the repeated [P|p]arents word.

Explain the meaning of by default, is it what happens when Parents is nil or the empty slice or a slice with ZeroHashes inside?

(Personally, I recommend len(Parents) is zero)

alcortesm · 2017-05-04T12:13:45Z

options.go

+			return err
+		}
+
+		if head != nil {


clean the code with something like this:

if initialCommit := head != nil; initialCommit { return nil } o.Parents = []plumbing.Hash{head.Hash()}

alcortesm · 2017-05-04T12:52:28Z

plumbing/object/commit.go

@@ -30,10 +30,12 @@ type Commit struct {
 	Committer Signature
 	// Message is the commit message, contains arbitrary text.
 	Message string
+	// TreeHash hash of the tree pointed by the commit.


this sentence is missing a verb, I suggest you add 'is the ' between the first and the second word.

pointed is probably not the best word here, as it has a very specific meaning in programming, maybe just rewrite the full sentence:

TreeHash is the hash of the root tree of the commit.

alcortesm · 2017-05-04T12:54:33Z

plumbing/object/commit.go

@@ -30,10 +30,12 @@ type Commit struct {
 	Committer Signature
 	// Message is the commit message, contains arbitrary text.
 	Message string
+	// TreeHash hash of the tree pointed by the commit.
+	TreeHash plumbing.Hash
+	// ParentHashes hashes of the parent commits of the commit.


This sentence is missing a verb, insert 'are the ' between the first two words.

I find this too restrictive, I think commit should be an interface, but that's probably something to fix in another commit.

alcortesm · 2017-05-04T13:09:28Z

worktree_commit.go

+
+// Commit stores the current contents of the index in a new commit along with
+// a log message from the user describing the changes.
+func (w *Worktree) Commit(msg string, opts *CommitOptions) (plumbing.Hash, error) {


I find the design of the commit operation very surprising:

I would expect this method to receive what you want to commit (an index or whatever), along with the msg and the options.

I would expect this to be a method of Repository or Storage or any other entity, except for worktree.

Maybe you should document the overall design of this somewhere.

alcortesm · 2017-05-04T13:14:25Z

worktree_commit.go

+		return plumbing.ZeroHash, err
+	}
+
+	return commit, w.updateHEAD(commit)


Please, explain that this method updates the head in the method signature, it is very important information about what the method does.

When you are going a commit? I don't know if we should explain the basic of git, here.
I can add an extra documentation. But could be an endless job.

You are right, explaining all the things that a commit does is quite a lot.

Maybe the piece of information that is missing is that this method is supposed to emulate the porcelain git-commit command. It was not clear to me if this was a low-level operation or a high-level one.

almost all the operations in git package are porceleain ones.

And yet we don't explain that important fact anywhere in the package documentation.

Why should we not document that important feature of the package? why is it better to hide that information from the user?, why not clearly mention that on the functions that are meant to emulate git commands (or the opposite on the ones that are not, if they are minority)?

Our design decisions will be much easier to use and understand if we explain them, up front.

Explain what? We are not emulating the git command we are offering a easy way to manipulate a repository in the easy way. I invite you to document this if you want.

Is the main package and expose an API that is enough to manipulate a repository, the goal is not replicate a command, we are not doing this.

Doing a commit implies add to a repository the latest changes, explaining that this change the HEAD is a detail of implementation. You don't need to know how works internally to use it.

https://git-scm.com/docs/git-commit

Just a clarification, IMHO, we're not replicating git commands because translating commands to Go functions as they are would lead to a weird API. However, we do get a lot of inspiration from Git commands, both plumbing and porcelain, so that we work with the same concepts (even if it's with a different API) and they look familiar to advance Git users.

This is reflected on the fact that in some functions we documented what command they are equivalent or similar to.

alcortesm · 2017-05-04T13:16:29Z

worktree_commit.go

+}
+
+// commitIndexHelper converts a given index.Index file into multiple git objects
+// reading the blogs from the given filesystem and creating the trees from the


s/blogs/blobs/

dreampuf · 2017-05-11T09:01:29Z

When to release this?

mcuadros added 5 commits May 1, 2017 13:09

plumbing: object.Tree making public tree and parents

9a807f4

worktree: Status return empty status instead of nil

2e092f9

worktree: Status return untracked for untracked files

5592dab

plumbing: MemoryObject.Writer tracks his own size

ff18ce3

worktree: Commit method implementation

40fa588

mcuadros changed the title ~~Commit~~ worktree: Commit method implementation May 4, 2017

mcuadros requested review from alcortesm, smola and ajnavarro May 4, 2017 00:08

Merge branch 'master' of github.com:src-d/go-git into commit

b8b61e7

ajnavarro suggested changes May 4, 2017

View reviewed changes

alcortesm suggested changes May 4, 2017

View reviewed changes

worktree: Commit, tests improvements

3713157

mcuadros merged commit ced875a into src-d:master May 5, 2017

worktree: Commit method implementation #375

worktree: Commit method implementation #375

Conversation

mcuadros commented May 4, 2017

alcortesm commented May 4, 2017

codecov bot commented May 4, 2017 • edited Loading

Codecov Report

mcuadros commented May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcuadros May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcuadros May 4, 2017 • edited Loading

Choose a reason for hiding this comment

alcortesm May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcortesm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcortesm May 4, 2017 • edited Loading

Choose a reason for hiding this comment

mcuadros May 4, 2017 • edited Loading

Choose a reason for hiding this comment

alcortesm May 4, 2017 • edited Loading

Choose a reason for hiding this comment

mcuadros May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smola May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreampuf commented May 11, 2017

codecov bot commented May 4, 2017 •

edited

Loading

mcuadros commented May 4, 2017 •

edited

Loading

mcuadros May 4, 2017 •

edited

Loading

mcuadros May 4, 2017 •

edited

Loading

alcortesm May 5, 2017 •

edited

Loading

alcortesm May 4, 2017 •

edited

Loading

mcuadros May 4, 2017 •

edited

Loading

alcortesm May 4, 2017 •

edited

Loading

mcuadros May 4, 2017 •

edited

Loading

smola May 5, 2017 •

edited

Loading