update-bash: git related improvement #48508
Conversation
| @@ -2,22 +2,38 @@ brew() { | |||
| "$HOMEBREW_BREW_FILE" "$@" | |||
| } | |||
|
|
|||
| git() { | |||
| "$HOMEBREW_GIT" "$@" | |||
There was a problem hiding this comment.
Might want to run which_git if this isn't already set.
|
Updated and tested locally. |
| @@ -2,22 +2,41 @@ brew() { | |||
| "$HOMEBREW_BREW_FILE" "$@" | |||
| } | |||
|
|
|||
| git() { | |||
| [[ -z "$HOMEBREW_GIT" ]] && HOMEBREW_GIT="$(which_git 2>/dev/null)" | |||
There was a problem hiding this comment.
I think this should just error out if it's empty; it won't produce an error as-is.
There was a problem hiding this comment.
I wonder that we should add set -e and set -o pipefail in bin/brew. It's important especially in subshell.
There was a problem hiding this comment.
The problem is it's not very user friendly; things will just fail and it's nearly impossible to tell what and why.
There was a problem hiding this comment.
But without it, error here when git is run in subshell will have strange result.
There was a problem hiding this comment.
Yeh, I get that, I just think it's a pretty major change that should be handled in another PR along with e.g. a trap to improve the messaging.
|
Any comment before I 🚢 |
|
|
||
| if [[ -n "$git_path" ]] | ||
| then | ||
| git_path="$(chdir "${git_path%/*}" && pwd -P)/${git_path##*/}" |
There was a problem hiding this comment.
Are you sure ${git_path%/*} behaves the same as dirname? I'd probably rather we used dirname and the raw git string here; the performance implications are minimal (this is not a critical path) and the readability is better using them.
There was a problem hiding this comment.
Are you sure ${git_path%/*} behaves the same as dirname?
Yes. In fact, we use this for years in bin/brew.
There was a problem hiding this comment.
It behaves differently if the string doesn't contain a slash at all, but otherwise it's doing the same. For the mentioned case dirname will return . while ${git_path%/*} will return the original string.
There was a problem hiding this comment.
OK, I will change it to dirname when pulling.
There was a problem hiding this comment.
OK, I will change it to
dirnamewhen pulling.
That's not what I was trying to imply. Does it make sense to have something like git-2.2 in one of the variables, with the expectation that it will be looked up in PATH? If yes, the whole construct doesn't make sense and will fail, no matter if we use dirname or not.
There was a problem hiding this comment.
How about:
if [[ -n "$HOMEBREW_GIT" ]]
then
git_path="$HOMEBREW_GIT"
elif [[ -n "$GIT" ]]
then
git_path="$GIT"
else
git_path="git"
fi
git_path="$(which "$git_path" 2>/dev/null)"There was a problem hiding this comment.
This looks like something that would work in all scenarios I can currently think of. And it should be guaranteed to return a path that contains a slash. 👍
|
One comment otherwise 👍 |
| @@ -2,22 +2,41 @@ brew() { | |||
| "$HOMEBREW_BREW_FILE" "$@" | |||
| } | |||
|
|
|||
| git() { | |||
| [[ -n "$HOMEBREW_GIT" ]] || exit 1 | |||
There was a problem hiding this comment.
Why not use odie instead?
* Use git function instead of refreshing bash cache on `git` path.
* Better `which_git`:
* Take user's setting of `HOMEBREW_GIT` and `GIT` env variable into
account.
* Always expand git path.
* Only check Xcode installation for OS X.
gitpath.which_git:HOMEBREW_GITas result.HOMEBREW_GITandGITenv variable intoaccount.
cc @MikeMcQuaid @UniqMartin