Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fix: Git provider on_branch? retains trailing newline #109

Merged
merged 1 commit into from
Dec 5, 2013
Merged

Bug fix: Git provider on_branch? retains trailing newline #109

merged 1 commit into from
Dec 5, 2013

Conversation

mikegerwitz
Copy link

I submitted a patch yesterday for using --abbrev-ref for on_branch?; unfortunately, there is a trailing newline, which causes problems when the repository already exists:

Error: /Stage[main]//Vcsrepo[...]: Could not evaluate: Execution of '/usr/local/bin/git rev-parse origin/master
' returned 128: fatal: ambiguous argument 'origin/master
': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
origin/master

@mikegerwitz
Copy link
Author

I'm wholly inexperienced with Ruby, so if there's a better way of doing this than strip (which on_branch? was doing originally), feel free to say so.

@mikegerwitz mikegerwitz mentioned this pull request Dec 4, 2013
@sodabrew
Copy link

sodabrew commented Dec 4, 2013

Does on_branch? return something besides a boolean? Typical Ruby style would be a method ending in ? returns true or false.

@sodabrew
Copy link

sodabrew commented Dec 4, 2013

Oh, on_branch? was pre-existing, let's leave it as is. Looking around, the more common pattern in this file is at_path { git_with_identify(stuff).chomp } - could you switch to that instead? Please also change from if branch == '' to 'if !branch`. Thanks for working on this!

I see your python roots :) Instead of:
return get_revision("#{@resource.value(:remote)}/%s" % branch)
Ruby-ish consistency would be:
return get_revision("#{@resource.value(:remote)}/#{branch}")

@mikegerwitz
Copy link
Author

Thanks for your reply, sodabrew; I made the chomp change.

I see your python roots :)

Actually, no Python here---that wasn't my line, but I did make the formatting changes that you requested.

@@ -253,7 +253,7 @@ def branches
end

def on_branch?
at_path { git_with_identity('rev-parse', '--abbrev-ref', 'HEAD') }
at_path { git_with_identity('rev-parse', '--abbrev-ref', 'HEAD') }.chomp
Copy link

Choose a reason for hiding this comment

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

nit: chomp inside the brackets

@sodabrew
Copy link

sodabrew commented Dec 5, 2013

Thanks @mikegerwitz!

context "retrieving the current revision" do
before do
expects_chdir
provider.expects(:git).with('rev-parse', '--abbrev-ref', 'HEAD').returns("foo\n")
Copy link

Choose a reason for hiding this comment

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

Nice test for the newline!

@mikegerwitz
Copy link
Author

Heh, whoops---thanks; corrected.

@sodabrew
Copy link

sodabrew commented Dec 5, 2013

One final request, could you use 'git rebase -i master' to squash the three commits together?

This commit also contains git provider `latest' method formatting changes;
squashed by request.
sodabrew added a commit that referenced this pull request Dec 5, 2013
Bug fix: Git provider on_branch? retains trailing newline
@sodabrew sodabrew merged commit 8356fc9 into puppetlabs:master Dec 5, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants