Honor repo default branch #142
Conversation
…feature tests to verify the behavior.
| @@ -18,7 +18,17 @@ def self.remote_branch_differ?(repo, local_branch, remote_branch) | |||
| repo.diff("#{local_branch}..origin/#{remote_branch}").any? | |||
| end | |||
|
|
|||
| def self.default_branch(repo) | |||
| symbolic_ref = repo.branches.find { |b| b.full =~ %r{remotes/origin/HEAD} } | |||
There was a problem hiding this comment.
This assumes you use origin as a remote. I always use upstream as a remote so I'd appreciate if that was overridable somehow but I do see that assumption is already present in a few places.
There was a problem hiding this comment.
You're right, it is hard coded in a number of places. Shall we handle it in this PR or a future one?
There was a problem hiding this comment.
Since it's pre-existing code I'd handle it in a separate PR.
lib/modulesync/git.rb
Outdated
| @@ -100,6 +110,7 @@ def self.update(name, files, options) | |||
| module_root = "#{options[:project_root]}/#{name}" | |||
| message = options[:message] | |||
| repo = ::Git.open(module_root) | |||
| options[:branch] ||= (repo.current_branch || 'master') | |||
There was a problem hiding this comment.
I dislike changing options this way and think a local variable is cleaner. Same below.
There was a problem hiding this comment.
You're right, that's crap. I started to change it to a local variable and then put it back to this for some reason. I know that I had some questions about why the branch is being checked out again here and in update_noop after switch_branch is called in pull. Looking over the code in ModuleSync::update again I suppose ModuleSync::Git::update_noop could be called in offline mode so you may need to check out a branch then.
… and checkout to a helper method so Rubocop does not complain about cyclomatic complexity or the ABC metric on ModuleSync::Git::update.
lib/modulesync/cli.rb
Outdated
| @@ -53,8 +53,8 @@ class Base < Thor | |||
| :desc => 'A regular expression to skip repositories.' | |||
| class_option :branch, | |||
| :aliases => '-b', | |||
| :desc => 'Branch name to make the changes in. Defaults to master.', | |||
| :default => CLI.defaults[:branch] || 'master' | |||
| :desc => 'Branch name to make the changes in. Defaults to the default branch of the upstream repository.', | |||
| @@ -95,12 +105,18 @@ def self.tag(repo, version, tag_pattern) | |||
| repo.push('origin', tag) | |||
| end | |||
|
|
|||
| def self.checkout_branch(repo, branch) | |||
There was a problem hiding this comment.
The name is quite similar to switch_branch. Maybe this should be a private method to clearly indicate it's a helper?
|
@ekohl I have made those changed you requested. |
|
@ekohl It's been a couple weeks. Either it's fine or nobody cares. I suppose both could be true. 😄 |
|
@ekohl I'm fine if you want to get others to review this. Just want to make sure it's not forgotten. Thanks! |
|
Apologies, it's been a very busy couple of weeks. |
|
Thanks a lot! |
|
I can appreciate that, thanks for the review/merge and for (I assume) volunteering your time! |
Fixes #133 #76.
I had to disable
Metrics/ModuleLengthbecause it was over by 11 lines butModulesync::Gitcould use some refactoring I think. I wasn't sure how far I should go with it so I figured I would file the PR and see where the discussion leads.