Allow fetch operation to receive a `ref` param #362

taquitos · 2018-04-19T17:03:30Z

addresses #361

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the
    best of my knowledge, is covered under an appropriate open
    source license and I have the right under that license to   
    submit that work with modifications, whether created in whole
    or in part by me, under the same open source license (unless
    I am permitted to submit under a different license), as
    Indicated in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including
    all personal information I submit with it, including my
    sign-off) is maintained indefinitely and may be redistributed
    consistent with this project or the open source license(s)
    involved.

addresses #361

kylemacey

Can we add a test as well to show that the ref is properly added to the argument string?

kylemacey · 2018-04-26T20:40:11Z

lib/git/base.rb

@@ -313,8 +313,8 @@ def checkout_file(version, file)

    # fetches changes from a remote branch - this does not modify the working directory,
    # it just gets the changes from the remote if there are any
-    def fetch(remote = 'origin', opts={})
-      self.lib.fetch(remote, opts)
+    def fetch(remote = 'origin', ref = nil, opts={})


This could potentially break implementations currently using #fetch. For example,

g.fetch(origin, option: "something")

Will set remote to origin and ref to { option: "something"}

Maybe we can utilize it inside of opts?

Good point, will look into this.

taquitos · 2018-06-19T20:45:56Z

tests/units/test_remotes.rb

+        second_commit = loc.log.first.sha
+
+        # Make sure fetch message only has the first commit when we fetch the first commit
+        assert(loc.fetch('origin', {:ref => first_commit}).include?(first_commit))


Any thought on how to make this a little more robust?

I think it might be more pertinent to test that the #command method receives the expected argument strings, rather than testing the functionality of git from this high up.

Perhaps something like

assert_send(loc, :command, ["fetch", "origin", first_commit])

Some other considerations:

Can these refs be an array? What if I want to do this: (from git fetch's docs)

git fetch origin +pu:pu maint:tmp

This could be a future improvement.

I'm a little skeptical about using the name first_commit, when it's actually just the sha. It might be more clear to the reader if you call it first_commit_sha.

Yeah, that's a bad name, I thought I'd use more than the sha, then I forgot to update the variable name.
I'll look into testing the #command side of things, I wasn't super familiar with how best to accomplish that, so your example is really helpful
Also, refs could be an array, but I didn't take that into account. I'll see if that's easy to support, which I bet it is. If it's too annoying to add, I'll at least add a comment about the limitation.

Thanks for reviewing!!!

Ok, so adding assert_send([loc.lib, :command, "fetch", ["origin", first_commit_sha]]) doesn't actually help test, as it passes as long as there isn't an error raised.
I think testing it from a high-level is probably #goodEnough

Normally I would just use expect(loc.lib).to receive(...) if I were using rspec, but it seems like UnitTest is just not very featured when it comes to mocking.

taquitos · 2018-06-19T23:15:23Z

Last commit folded my last comment:

Ok, so adding assert_send([loc.lib, :command, "fetch", ["origin", first_commit_sha]]) doesn't actually help test, as it passes as long as there isn't an error raised.
I think testing it from a high-level is probably #goodEnough

Normally I would just use expect(loc.lib).to receive(...) if I were using rspec, but it seems like UnitTest is just not very featured when it comes to mocking.

kylemacey · 2018-06-20T20:37:59Z

Ok, so adding assert_send([loc.lib, :command, "fetch", ["origin", first_commit_sha]]) doesn't actually help test, as it passes as long as there isn't an error raised.

@taquitos sorry that was 100% my bad. I actually didn't know the actual method to use (I'm also a bit reliant on rspec for mocking) and spit out the closest thing I could find while Googling. Maybe it's something we can improve later.

kylemacey · 2018-06-20T20:39:11Z

@perlun @tarcinil want to give this a final 👍?

perlun

Looks good, thanks! If you fix the stylistic issue I'll be happy to merge this.

perlun · 2018-06-21T11:58:58Z

lib/git/lib.rb

@@ -738,6 +738,7 @@ def tag(name, *opts)

    def fetch(remote, opts)
      arr_opts = [remote]
+      arr_opts << opts[:ref] unless opts[:ref].nil?
      arr_opts << '--tags' if opts[:t] || opts[:tags]
      arr_opts << '--prune' if opts[:p] || opts[:prune]


I don't like us mixing unless foo.nil? and if foo syntax like this.

Are there any strong reasons why we couldn't rephrase this as:

arr_opts << opts[:ref] if opts[:ref]

The only semantic difference I can think of with these two forms is that the previous will let you pass in false as a ref, but... that doesn't really make much sense anyway.

No problem! I like the use of unless to express the idea of We're going to add this, unless this thing is nil. Of course your suggestion is totally reasonable: We're going to add this if there is something to add. I updated the pr for the style changes 👍

Thanks @taquitos. I agree that the unless pattern can make the code more readable, I just want things to be uniform.

taquitos · 2018-06-25T15:53:21Z

@perlun ping 😄

perlun

Thanks! Merging.

Joshua Liebowitz added 3 commits April 19, 2018 09:59

Allow fetch operation to receive a ref param

c16862b

addresses #361

Update remote object

d9f7da0

remove .ruby-version

37fede4

taquitos mentioned this pull request Apr 20, 2018

Use github's ref they set up for prs fastlane/ci#706

Merged

kylemacey suggested changes Apr 26, 2018

View reviewed changes

Joshua Liebowitz and others added 2 commits June 19, 2018 11:00

Use options instead of separate param

1510bce

Merge branch 'master' into custom_fetch

eac3010

taquitos commented Jun 19, 2018

View reviewed changes

Update for comments

c413f21

Merge branch 'master' into custom_fetch

f361b5d

kylemacey approved these changes Jun 20, 2018

View reviewed changes

perlun requested changes Jun 21, 2018

View reviewed changes

Update for style

33c9f1d

perlun approved these changes Jun 25, 2018

View reviewed changes

perlun merged commit e7f1880 into ruby-git:master Jun 25, 2018

taquitos deleted the custom_fetch branch June 25, 2018 19:00

Allow fetch operation to receive a ref param #362

Allow fetch operation to receive a ref param #362

Uh oh!

Conversation

taquitos commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kylemacey left a comment

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!

taquitos commented Jun 19, 2018

Uh oh!

kylemacey commented Jun 20, 2018

Uh oh!

kylemacey commented Jun 20, 2018

Uh oh!

perlun left a comment

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!

taquitos commented Jun 25, 2018

Uh oh!

perlun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Allow fetch operation to receive a `ref` param #362

Allow fetch operation to receive a `ref` param #362

taquitos commented Apr 19, 2018 •

edited

Loading