bash completion: Support "divergence from upstream" messages in __git_ps1
Get series with b4
b4 am [email protected]Versions newest first · linked by author and title
- v5 Latest2 patches 2 repliesIn review
- v41 patch 2 repliesSuperseded
- v11 patch 1 replySuperseded
Patches in v5 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/2 | bash completion: Support "divergence from upstream" messages in __git_ps1contrib/ | +143 | 0 replies | |
| 2/2 | bash-completion: Fix __git_ps1 to work with "set -u"contrib/ | +8 | 0 replies |
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v5 yet.
Cover letter [PATCHv5 0/2] bash completion: Support "divergence from upstream" messages in __git_ps1
I agree with all the points I haven't specifically replied to. The first patch makes the appropriate changes. The second patch fixes largely unrelated "set -u" issues I stumbled over while running tests.
Quoted 24 lines · Junio C Hamano
Quoted 11 lines
contrib/completion/git-completion.bash +3
-0@@ -42,6 +42,23 @@ # set GIT_PS1_SHOWUNTRACKEDFILES to a nonempty value. If there're # untracked files, then a '%' will be shown next to the branch name. # +# If you would like to see the difference between HEAD and its +# upstream, set GIT_PS1_SHOWUPSTREAM to a nonempty value. A "<" +# indicates you are behind, ">" indicates you are ahead, andcontrib/completion/git-completion.bash line 57
Quoted 10 lines
contrib/completion/git-completion.bash +10
-0+# "<>" indicates you have diverged. You can further control +# behaviour by setting GIT_PS1_SHOWUPSTREAM to a space-separated +# list of values: +# git compare HEAD to @{upstream} +# svn compare HEAD to your SVN upstream +# verbose show number of commits ahead/behind (+/-) upstream +# legacy don't use the '--count' option available in recent +# versions of git-rev-list +# By default, __git_ps1 will compare HEAD to your SVN upstream +# if it can find one, or @{upstream} otherwise.This feels somewhat weird.
Quoted 8 lines
I can sort-of read from the above that I can set the variable to a random string, e.g. "garbage", if I only want a simple show-upstream feature without frills (i.e. I don't want it to be verbose, I don't want it to restrict the comparison only to "git" upstream nor "svn" upstream, and I don't think I would ever use ancient git that lack "rev-list --count"). But the description does not assure me that the random string I happened to choose (in this case "garbage") is a safe one. Perhaps list (and implement) "default" as a safe, otherwise-no-op value?
I agree this would improve the documentation, but I've used "auto" instead of "default", to give a hint that that the code is being a bit automagical. I don't see how adding code would help though - "GIT_PS1_SHOWUPSTREAM=auto" is already covered in the default case of an unrecognised string, and adding code to make "GIT_PS1_SHOWUPSTREAM=1" do nothing or print a warning would just confuse people that skip-read the documentation and set the value to see what happened.
Quoted 5 lines
How much overhead are we shaving if you specify "git" (without "svn") or "svn" (without "git") to the variable? I suspect that the bulk of the time is spent by reading from "git config" to look for svn-remote.*.url, which you seem to unconditionally do even when "git" was asked for anyway.
In my tests, a single invocation of git-config took an average of roughly 0.005s with a 30-line .git/config, and roughly 0.030s with a .git/config that contained about 17,600 extra nonsense lines (aaa = aaa, aab = aab, etc.). In both cases, the extra test for svn-remote.*.url made no significant difference to the time taken, whereas a second invocation of git config (obviously) doubled the time taken.
Checking the SVN upstream with git log --first-parent -1 --grep="^git-svn-id: \(${svn_url_pattern:2}\)" is actually quite a serious time issue, especially if you have made many commits since your upstream. A test with 100 empty commits since the SVN upstream took roughly 0.012 seconds on average. A test on git itself (>22,000 commits) took roughly 0.29 seconds to determine there was no SVN upstream.
Speed (and user confidence in speed) isn't the main reason to allow the user to force "git" or "svn". If someone had e.g. imported their old SVN history into a git project, or did clever git tricks on a branch they regularly merged into SVN, they would want to override the default behaviour. This is probably quite rare now I think about it, and I've rejigged the documentation a bit to reflect that.
Quoted 10 lines
contrib/completion/git-completion.bash line 60
Quoted 4 lines
contrib/completion/git-completion.bash +3
-0+# You can +# override the value of GIT_PS1_SHOWUPSTREAM on a per-repository +# basis by setting the bash.showUpstream config variable.That's totally backwards from it should be, isn't it?
Usually configuration variables are used to give you the default, and you use environment variables to override them.
I basically agree with Thomas here. Going down that route without parsing all the options in one big --get-regexp mess would take O(n) time, where n consists mostly of config options I don't care about.
Quoted 12 lines
contrib/completion/git-completion.bash
Quoted 6 lines
contrib/completion/git-completion.bash +6
-0+ ;; + svn-remote.*.url) + svn_remote[ $((${#svn_remote[@]} + 1)) ]="$value" + svn_url_pattern+="\\|$value" + upstream=svn # default upstream is SVN if available + ;;I expected that (1) when on a branch that is a fork of a svn upstream, you would use the svn magic; (2) otherwise when on a branch that is a fork of a git upstream, you would use "@{upstream}". That way, the users do not even have to say "git" or "svn" in
GIT_PS1_SHOWUPSTREAMat all, no?
Quoted 4 lines
But that does not seem to be what is happening here. Your loop seems to force "upstream=svn" if I have one branch that is a fork from svn upstream, even if my current branch does not have anything to do with that branch nor svn upstream. Is that what was intended?
I'm not sure I understand how you would detect if something is a fork of an SVN/git upstream. It's certainly deliberate not to do a git log if it's avoidable, for the efficiency reasons I mentioned above. But it seems like a good idea to use @{upstream} in the "auto" case if no SVN upstream was found, so I've changed the patch to do that. Please let me know if there's some applicable magic I'm not aware of :)
Quoted 4 lines
contrib/completion/git-completion.bash
Quoted 2 lines
contrib/completion/git-completion.bash +2
-0+ esac + done < <(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n ')If you "tr" to trash "\0" anyway, do you need to run "config -z"?
The tr is there to work around issues like this:
git config bash.showUpstream $'svn\nlegacy' git config bash.showUpstream | tr '\0\n' '\n '
The end result is a format that can be easily parsed with read. Without the -z, I'd have no way to tell the difference between the end of a config item and a literal newline.
Having said that, I have no strong opinion about which is the more appropriate thing to do here - slow down everyone's prompt to deal with an edge case, or break otherwise-valid behaviour because the solution is ugly. The latest patch still has it in - let me know if you'd prefer it out.
Quoted 6 lines
contrib/completion/git-completion.bash
Quoted 3 lines
contrib/completion/git-completion.bash +2
-0+ # parse configuration values + for option in ${GIT_PS1_SHOWUPSTREAM}; doIs this safe under "set -u"? See 25a31f8 (bash-completion: Support running when set -u is enabled, 2009-01-15).
This is safe under "set -u", as this function is only called if $GIT_PS1_SHOWUPSTREAM is defined. But testing showed several issues that make me suspect __git_ps1 never worked under "set -u". My second patch fixes those issues.
- Andrew