Re: [PATCHv4] bash completion: Support "divergence from upstream" messages in __git_ps1

[Junio C Hamano <gitster@pobox.com>]

Andrew Sayers <andrew-git@pileofstuff.org> writes:

> Add a notification in the command prompt specifying whether (and optionally how
> far) your branch has diverged from its upstream.  This is especially helpful in
> small teams that very frequently (forget to) push to each other.
>
> Support git-svn upstream detection as a special case, as migrators from
> centralised version control systems are especially likely to forget to push.
>
> Support for other types of upstream than SVN should be easy to add if anyone is
> so inclined.
> ---

Sign-off?

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 57245a8..dabcdaa 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -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, and
> +#       "<>" 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.

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?

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.

> +#       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.

> +# stores the divergence from upstream in $p
> +# used by GIT_PS1_SHOWUPSTREAM
> +__git_ps1_show_upstream ()
> +{
> +	local key value
> +	local svn_remote=() svn_url_pattern count n
> +	local upstream=git legacy verbose
> +
> +	# get some config options from git-config
> +	while read key value; do
> +		case "$key" in
> +			bash.showupstream)
> +				GIT_PS1_SHOWUPSTREAM="$value"
> +				if [[ -z "${GIT_PS1_SHOWUPSTREAM}" ]]; then
> +					p=""
> +					return
> +				fi

This is the "backwards" part.

> +				;;
> +			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_SHOWUPSTREAM at all, no?

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?

Oh, also, all of your case arms are one-indent too deep.  Please write
them like this:

	case foo in
        arm1)
        	stmt1
                ;;
	esac

> +		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"?

> +	# parse configuration values
> +	for option in ${GIT_PS1_SHOWUPSTREAM}; do

Is this safe under "set -u"?  See 25a31f8 (bash-completion: Support
running when set -u is enabled, 2009-01-15).