-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add heuristic for finding default remote name #2318
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
base: master
Are you sure you want to change the base?
Add heuristic for finding default remote name #2318
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems legit...
@@ -951,6 +951,24 @@ function pathmunge() { | |||
fi | |||
} | |||
|
|||
function _remote_name() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked over the repo, looks like there is already code covering remote name detection in themes/githelpers.theme.bash
Maybe it's not a bad idea for code reuse, to move this function library to the general libraries and have those functions available globally and not just for a small number of themes that bother to source it.
Any thoughts, @akinomyoga ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(either way, if we keep this, maybe name it a bit more descrive, like _get_git_default_remote()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the target repositories are different. The functions _bash-it-{update-,version}
modified in this PR are intended to retrieve the remote in the repository of Bash-It itself, while githelpers.theme.sh
is supposed to retrieve information in the repository of the current working directory.
edit: I think it would be valid to prepare a function to get the remote name in the current git tree and use it everywhere. For the present purpose, the function can be used as $(cd "$BASH_IT"; _bash-it-git-get-remote-host)
. However, the feature to determine the remote name doesn't seem to exist in githelpers.theme.sh
. I think unless the same feature is required by both themes and the bash-it
command, we don't necessarily have to define the function in a single place.
@@ -951,6 +951,24 @@ function pathmunge() { | |||
fi | |||
} | |||
|
|||
function _remote_name() { | |||
local branch | |||
branch=$(git branch --show-current) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this get the information about the git repository of the current working directory? Shouldn't this get the information in the Bash-it repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -951,6 +951,24 @@ function pathmunge() { | |||
fi | |||
} | |||
|
|||
function _remote_name() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how much the contribution guideline is strictly applied to the project, but Coding Style - docs/contributing.rst
says
- When creating new functions, please use a dash ("-") to separate the words of the function's name, e.g.
my-new-function
. Don't use underscores, e.g.my_new_function
.- Internal functions that aren't to be used by the end user should start with an underscore, e.g.
_my-new-internal-function
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, also maybe call it _get-git-default-remote-name is a bit more descriptive, and add a remark to doc the function.
Description, Motivation and Context
Fixes #2317
Implementation:
HEAD
is not detached, it infers the remote name by the current branchHEAD
is detached, then it infers the remote name by parsinggit remote -v
, but only if every entry has the same remote nameI've made similar PRs in other projects, so this does work in general.
How Has This Been Tested?
Manual testing
Types of changes
Checklist:
clean_files.txt
and formatted it usinglint_clean_files.sh
.