Features/git cleanups #256

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

richo commented Feb 28, 2013

Implements #fetch! on a repo.

Return full refs from #resolve to fix both some inconsistent behaviour and to do the most reasonable thing since it's used internally.

richo referenced this pull request in benhoskings/babushka.me Mar 1, 2013

Closed

Allow specification of an origin and ref to install from #7

Owner

benhoskings commented Mar 26, 2013

Sorry this has sat here. Comments inline.

@benhoskings benhoskings commented on the diff Mar 26, 2013

deps/babushka.rb
@@ -31,7 +31,7 @@ def resolved_ref
requires 'resolvable ref.babushka'.with(from, path, ref)
met? {
- (repo.current_head == resolved_ref).tap {|result|
+ (resolved_ref.start_with? repo.current_head).tap {|result|
@benhoskings

benhoskings Mar 26, 2013

Owner

#start_with? isn't a method, this isn't going to work :)

@richo

richo Mar 26, 2013

Contributor

Are you sure?

irb(main):001:0> "asdf".start_with? "a"
=> true
irb(main):002:0> RUBY_VERSION
=> "1.8.7"
irb(main):003:0>
[1] pry(main)> "asdf".start_with? "a"
=> true
[2] pry(main)> RUBY_VERSION
=> "1.9.3"
[3] pry(main)>

Unless I'm an idiot and managed to monkeypatch String in an init file somewhere.

@benhoskings

benhoskings Mar 26, 2013

Owner

Ahh, no, it's me that's the idiot; I patched #starts_with? on ages ago and thought this was a typo of that. Carry on.

@benhoskings benhoskings commented on the diff Mar 26, 2013

lib/babushka/git_repo.rb
def resolve ref
- repo_shell?("git rev-parse --short #{ref}")
+ repo_shell?("git rev-parse #{ref}")
@benhoskings

benhoskings Mar 26, 2013

Owner

What's the rationale behind this change?

Owner

benhoskings commented Mar 26, 2013

What's the inconsistent behaviour you've seen with resolve?

Contributor

richo commented Mar 26, 2013

Resolve isn't deterministic, it might return a longer string if you call it later because the short string is no longer unique.

Worse, the string it returned initially may not uniquely identify an object any more in which case any subsequent operation that uses the result will fail.

Owner

benhoskings commented Mar 26, 2013

Fair enough.

(I'm aware of all that; I'm just assuming that commits being made while babushka runs is an edge case.)

I agree that it's better for #resolve to return the full SHA. I'll need to spelunk around and see where it's used, though, because I'm nervous about changing an existing method.

Owner

benhoskings commented Jun 15, 2013

I had a think about this. GitRepo#resolve is useful as-is, IMO: a big part of its use for me is logging, which is much more readable with short SHAs. I've added GitRepo#resolve_full though, to mirror #current_head / #current_full_head: 2de036f

As for #fetch, I think that's better done via GitRepo#repo_shell:

repo.repo_shell('git fetch remote')

I'd rather keep the helpers to things where options are wrapped up, like #track!.

Contributor

richo commented Jun 15, 2013

WRT resolve, I at the very least think it's worth adding a gotcha that if you store and try to use it's return values, you'll get burned eventually.

As for fetch, I'm not super stressed. From memory it's been on our fork for a while now.

Owner

benhoskings commented Jun 16, 2013

Good point, I'll add a note to that method's comments. But it is the edgiest of cases—it can't be hit unless you're creating commits during babushka runs, at the very least.

On Sat, Jun 15, 2013 at 10:05 PM, Richo Healey notifications@github.com
wrote:

WRT resolve, I at the very least think it's worth adding a gotcha that if you store and try to use it's return values, you'll get burned eventually.

As for fetch, I'm not super stressed. From memory it's been on our fork for a while now.

Reply to this email directly or view it on GitHub:
#256 (comment)

Owner

benhoskings commented Jun 16, 2013

Added a note here: 3a2da55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment