Re: [PATCH] completion: resolve svn remote upstream refs

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 3, 2012, 20:40 UTC
Message-ID: <7v7gtfbtfn.fsf@alter.siamese.dyndns.org>
In-Reply-To: <501B9FD1.8090204@zetam.org>

Administrivia:

 - "Content-Type: text/plain; charset=ISO-8859-1; format=flowed" in
   the header tells us that the patch is corrupted and will not
   apply cleanly, but we still can discuss the contents of the
   patch.

 - "git shortlog --since=6.months -n --no-merges contrib/completion"
   tells us who the area experts who may be able to help you move
   this patch forward.  Also people who are involved in git-svn may
   be able to help with the configurations you are reading from.  By
   Cc'ing them, your patch may have a better chance of getting
   polished more quickly (I added Cc to relevant people).

Yves Blusseau <blusseau@zetam.org> writes:

> Updated version of my previous patch

Everything before the "---" line we see below will be for people who read "git log" history 6 months down the road, when they have to understand what you did and why (perhaps because they found bugs or they want to further enhance what your patch did).

Because your previous patch will not appear in that "git log" output, the above is not an appropriate thing to say here.

> This fix is used to return the svn reference of the remote svn upstream
> branch when the git repository is a clone of a svn repository that was
> created with the --stdlayout and --prefix options of git svn command.

Before using the word "fix", please describe what the current (i.e. without the "fix") behaviour visible to the users, why it is bad, and what behaviour you want to give your users instead. And then describe what change you made to give that desired behaviour.

Show 6 quoted lines

> * completion/git-prompt.sh: add function to resolve svn branches into
>   git remote refs using paths declared in svn-remote.*.fetch and
>   svn-remote.*.branches configurations
>
> Signed-off-by: Yves Blusseau <blusseau@zetam.org>
> ---

Here, immediately after "---", is where you would say "Updated
version of my patch.  The difference from the previous round are
1. fixed foo, 2. fixed bar, 3. reworded baz...." if you want to.

>  contrib/completion/git-prompt.sh | 56
> +++++++++++++++++++++++++++++++++++++++-

Line-wrapped (comes from "format=flawed" we saw in the Content-Type header).

Show 16 quoted lines

>  1 file changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-prompt.sh
> b/contrib/completion/git-prompt.sh
> index 29b1ec9..f9a3421 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -73,17 +73,65 @@ __gitdir ()
>  	fi
>  }
>  +# resolve svn refs
> +# The function accepts 2 arguments:
> +# 1: An array containing the translation ref(s) (ie
> branches/*:refs/remotes/svn/*)
> +# 2: the ref to be translated (ie. branches/prod)
> +# returns the remote refs or original ref if it could not be translated

We see "translation ref(s)", which is an unheard-of term, and it is unclear what is translated and into what other thing. Is this the moral equivalent of the ref mapping that maps refs at the remote repository to remote tracking refs we have at our local repository?

If the first item of above were written like this:

    # 1: An array containing the mapping from branches in the subversion
    #  repository to remote tracking branches in this repository (e.g
    #  "branches/*:refs/remotes/svn/*").

I wouldn't have had to ask the above question, but it is unclear if that is what you meant, hence this question.

Show 7 quoted lines

> +__git_resolve_svn_refs ()
> +{
> +	local idx ref_globs left_part_ref_glob right_part_ref_glob
> left right value
> +
> +	local all_ref_globs=("${!1}")
> +	local ref="$2"

Bash/Zsh reusability czars might want to comment on the way all_ref_globs is declared and initialized.

> +	for (( idx=0 ; idx < ${#all_ref_globs[@]}; idx++ ));do
> +		ref_globs="${all_ref_globs[$idx]}"
> +		# get the left part ref glob (before the :refs/)
> +		left_part_ref_glob="${ref_globs%%:refs/*}"

Is doubling of %% significant here? I am assuming there isn't, as there would be no colon in $ref_globs other than the boundary between LHS and RHS.

> +		# If the ref match the left part we can resolve the ref directly
> +		if [[ "$ref" == "$left_part_ref_glob" ]];then

The coding standard used in this file is kept deliberately lax compared to the remainder of the codebase, but I think the lack of SP between ";" and "then" goes a bit too far.

Show 7 quoted lines

> +			# resolve the ref into the right part (after the :)
> +			ref="${ref_globs/${left_part_ref_glob}:/}"
> +			break
> +		else
> +			case $ref in
> +				# check if the ref match the glob
> +				$left_part_ref_glob)

case/esac and case arms should align at the same column (otherwise the body will be indented unreadably too deep to the right, like this code).

Show 9 quoted lines

>  # 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 svn_refs_globs=()
>  	local upstream=git legacy="" verbose=""
>   	svn_remote=()

Doesn't this have the same problem as the one fixed by 471dcfd (contrib/completion: "local var=()" is misinterpreted as func-decl by zsh, 2011-09-01)?

Thanks.