remote: add Push #178

smola · 2016-12-12T09:39:20Z

add Push method to Remote.
add method Push to Repository.
examples: add push example.

codecov-io · 2016-12-12T09:43:36Z

Current coverage is 76.96% (diff: 70.88%)

Merging #178 into master will decrease coverage by 0.88%

@@             master       #178   diff @@
==========================================
  Files            92         92          
  Lines          5793       5940   +147   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4510       4572    +62   
- Misses          810        882    +72   
- Partials        473        486    +13

Powered by Codecov. Last update ff449c6...1bdce69

mcuadros · 2016-12-12T22:18:25Z

examples/push/main.go

+)
+
+func main() {
+	if len(os.Args) != 4 {


I created a helper function for the args parse:

CheckArgs("<directory>", "<remote-url>", "<refspec>") directory := os.Args[1] remoteUrl := os.Args[2]

mcuadros · 2016-12-12T22:19:09Z

examples/push/main.go

+	}
+
+	repo, err := git.NewFilesystemRepository(repoPath)
+	if err != nil {


The err are checked using CheckIfError(err)

mcuadros · 2016-12-12T22:19:46Z

examples/push/main.go

+		panic(err)
+	}
+
+	if err := repo.Push(&git.PushOptions{


And is supposed to illustrate the code with his homologue at git
Info("git clone %s %s", url, directory)

mcuadros · 2016-12-12T22:21:46Z

remote.go


 // Remote represents a connection to a remote repository
 type Remote struct {
 	c *config.RemoteConfig
+	r *Repository


if you add the Repository then remove the Storer

it will not be present in the next version (after rebase).

mcuadros · 2016-12-12T22:22:09Z

remote.go

-// Connect with the endpoint
-func (r *Remote) Connect() error {
+// ConnectFetch connects to the remote and starts a fetch session.
+func (r *Remote) ConnectFetch() error {


make private this methods

they are gone after the next rebase

mcuadros · 2016-12-12T22:23:34Z

remote.go

+		return fmt.Errorf("no refspec given")
+	}
+
+	if err := o.Validate(); err != nil {


the validation should be the first thing, doesn't worth to make any operation if the o are not valid

mcuadros · 2016-12-12T22:24:21Z

remote.go

+			return err
+		}
+
+		if err := iter.ForEach(func(ref *plumbing.Reference) error {


I prefer not having the if and the ForEach with the closure in the same line

I find hard to understand at a glance what operation are we doing on each element in the collection.

Can we define the function separately, give it a good name, and call it here instead?

mcuadros · 2016-12-12T22:24:50Z

remote.go

@@ -318,10 +511,19 @@ func (r *Remote) References() (storer.ReferenceIter, error) {
 	return r.refs.IterReferences()
 }

-// Disconnect from the remote and save the config
+// Disconnect from the remote server. Calling Disconnect on a disconnected
+// remote has no effect.
 func (r *Remote) Disconnect() error {


make it private

Not until we make Fetch atomic, which currently it is not.

alcortesm · 2016-12-13T10:55:48Z

options.go

+}
+
+// Validate validate the fields and set the default values
+func (o *PushOptions) Validate() error {


Do we want to have invalid PushOptions values around in our program?, if not, I would make PushOptions fields private, provide a ctor for it (that checks for validity) and add some getters (if they are needed).

Why is Validate not used in the example?

See the above comment, I'm using the same conventions for Validate as in the rest of the code base. The example doesn't use Validate, the user doesn't need to call it, since it's always called by porcelain methods when they get options.

alcortesm · 2016-12-13T11:37:45Z

examples/push/main.go

+		panic(err)
+	}
+
+	if _, err := repo.CreateRemote(&config.RemoteConfig{


I find this confusing: Why pushing to a remote repository creates a remote in the current repository?

Should this be a different (an optional) operation from pushing?, it is a nasty side effect.

In other words, can we push to a repository that is not a remote? why not?

Yeah, I'm changing the example to do a more standard/intuitive thing.

alcortesm · 2016-12-13T11:39:15Z

options.go

+// PushOptions describe how a push should be performed.
+type PushOptions struct {
+	// RemoteName is the name of the remote to be pushed to.
+	RemoteName string


This should be a URL, not a remote, I think.

git-push supports both, either getting a remote name or getting a URL directly (i.e. push to a repository without adding a remote). However, at the moment, we implent only pull/push by remote name.

Adding support for direct URL would be good (for clone/pull/push/fetch), but it seems work for a separate PR.

alcortesm · 2016-12-13T11:40:56Z

remote.go

 	s Storer
 	p sideband.Progress

 	// cache fields, there during the connection is open
 	endpoint     transport.Endpoint
 	client       transport.Client
 	fetchSession transport.FetchPackSession
+	sendSession  transport.SendPackSession
+	session      transport.Session


can you explain the difference between session and the other two (sendSession and fetchSession)?

session can be either fetchSession / sendSession. It's used for some common operations.

This will go away if we merge #185 first.

alcortesm · 2016-12-13T11:44:12Z

remote.go

+// TODO: Support pushing tags.
+// TODO: Check if force update is given, otherwise reject non-fast forward.
+func (r *Remote) Push(o *PushOptions) (err error) {
+	if o.RemoteName == "" {


I though PushOptions.Validate didn't allow for empty strings in the remote name. I'm confused.

It is allowed to pass empty remote directly to Remote, since the name is already set in the remote itself and it cannot push to a different Remote.

alcortesm · 2016-12-13T12:58:44Z

remote.go

+			return err
+		}
+
+		if err := iter.ForEach(func(ref *plumbing.Reference) error {


I find hard to understand at a glance what operation are we doing on each element in the collection.

Can we define the function separately, give it a good name, and call it here instead?

alcortesm · 2016-12-13T13:00:18Z

remote.go

@@ -294,6 +423,70 @@ func (r *Remote) buildFetchedTags() error {
 	})
 }

+func (r *Remote) getHaves(refs memory.ReferenceStorage) []plumbing.Hash {


why is this a method of Remote instead of a free function?

alcortesm · 2016-12-13T13:03:50Z

repository.go

@@ -275,6 +269,25 @@ func (r *Repository) Pull(o *PullOptions) error {
 	return r.createReferences(head)
 }

+// Push pushes changes to a remote.
+func (r *Repository) Push(o *PushOptions) error {
+	if o.RemoteName == "" {


Again, this was supposed to be part of Validate.

alcortesm · 2016-12-13T13:04:49Z

repository.go

+		return err
+	}
+
+	if err := remote.ConnectPush(); err != nil {


I think Connect should not be needed here, unless you want to do other things over the connection in addition to Push.

I did it this way to keep symmetry with the Fetch implementation. Although both should be changed to have atomic operations without Connect/Disconnect at all.

alcortesm · 2016-12-13T13:07:06Z

options.go

+	RefSpecs []config.RefSpec
+}
+
+// Validate validate the fields and set the default values


Based on its name, I find weird that Validate set anything, I would expect it to have a non-pointer receiver and just return true or false.

I'm following the same convention that we are using for a lot of objects in go-git (check other Validate methods). We can discuss changing them as a separate issue, not related to this PR.

ajnavarro · 2016-12-14T11:43:11Z

examples/push/main.go

+
+	"gopkg.in/src-d/go-git.v4"
+	"gopkg.in/src-d/go-git.v4/config"
+	. "gopkg.in/src-d/go-git.v4/examples"


missing space

why? they are all internal imports.

oops, you're right, I just saw the period and i thought that was go-check or something... 👍

mcuadros · 2016-12-15T11:10:44Z

@smola rebase

mcuadros · 2016-12-19T09:31:44Z

examples/common_test.go

@@ -20,6 +25,16 @@ var args = map[string][]string{
 	"clone":       []string{defaultURL, tempFolder()},
 	"progress":    []string{defaultURL, tempFolder()},
 	"open":        []string{filepath.Join(cloneRepository(defaultURL, tempFolder()), ".git")},
+	"push": []string{


can you create a function?

mcuadros · 2016-12-19T09:34:18Z

examples/common_test.go

+			setRemote(local, fmt.Sprintf("file://%s", remote))
+			return local
+		}(),
+		"origin",


the default RemoteName is "origin" should to omit this arg to simplify the example?

done, reduced to just push origin with default refspec.

mcuadros · 2016-12-19T09:35:04Z

options.go

+	RemoteName string
+	// RefSpecs specify what destination ref to update with what source
+	// object. A refspec with empty src can be used to delete a reference.
+	RefSpecs []config.RefSpec


We can have a default RefSpec?

yes: refs/heads/:refs/heads/. I've just added it. Note that git has different behaviours for default push refspec depending on config.

mcuadros · 2016-12-19T09:36:31Z

remote.go

@@ -221,6 +376,27 @@ func getWants(spec []config.RefSpec, localStorer Storer, remoteRefs storer.Refer
 	return result, nil
 }

+func (r *Remote) referenceNeedsUpdate(wantTags bool, specs []config.RefSpec,


function not tested, or codecov failing

I think it's not in use.

removed, leftover from refactor

mcuadros · 2016-12-19T09:37:01Z

repository.go

@@ -345,6 +345,20 @@ func (r *Repository) Fetch(o *FetchOptions) error {
 	return remote.Fetch(o)
 }

+// Push pushes changes to a remote.
+func (r *Repository) Push(o *PushOptions) error {


function not tested

* add Push method to Remote. * add method Push to Repository. * examples: add push example.

* remote: add Push. * add Push method to Remote. * add method Push to Repository. * examples: add push example. * requested changes * add tests, fixes

1.2

smola requested review from ajnavarro, mcuadros and alcortesm December 12, 2016 09:39

mcuadros suggested changes Dec 12, 2016

View reviewed changes

alcortesm suggested changes Dec 13, 2016

View reviewed changes

ajnavarro reviewed Dec 14, 2016

View reviewed changes

smola added this to the v4 milestone Dec 14, 2016

alcortesm approved these changes Dec 15, 2016

View reviewed changes

ajnavarro approved these changes Dec 15, 2016

View reviewed changes

smola changed the title ~~[WIP][RFC] remote: implement push~~ [WIP][RFC] remote: add Push Dec 15, 2016

mcuadros force-pushed the master branch from b448280 to 60cf153 Compare December 16, 2016 21:23

smola changed the title ~~[WIP][RFC] remote: add Push~~ remote: add Push Dec 19, 2016

mcuadros suggested changes Dec 19, 2016

View reviewed changes

smola mentioned this pull request Dec 19, 2016

server: add git server implementation #190

Merged

smola added 3 commits December 19, 2016 17:34

remote: add Push.

eac054a

* add Push method to Remote. * add method Push to Repository. * examples: add push example.

requested changes

cc54445

add tests, fixes

1bdce69

mcuadros merged commit f599ee3 into src-d:master Dec 19, 2016

mcuadros pushed a commit that referenced this pull request Jan 31, 2017

remote: add Push (#178)

90d67bb

* remote: add Push. * add Push method to Remote. * add method Push to Repository. * examples: add push example. * requested changes * add tests, fixes

This was referenced Apr 12, 2017

Review the rationale and naming of the validate methods in options.go #341

Closed

worktree, status and reset implementation based on merkletrie #339

Merged

gsalingu-ovhus pushed a commit to gsalingu-ovhus/go-git that referenced this pull request Mar 28, 2019

Merge pull request src-d#178 from camathieu/1.2

913f256

1.2

remote: add Push #178

remote: add Push #178

Uh oh!

Conversation

smola commented Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 76.96% (diff: 70.88%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smola Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

smola commented Dec 12, 2016 •

edited

Loading

codecov-io commented Dec 12, 2016 •

edited

Loading

smola Dec 13, 2016 •

edited

Loading