Re: [PATCH] remote: rename also remotes in the branch.<name>.pushRemote config

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

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> Subject: Re: [PATCH] remote: rename also remotes in the branch.<name>.pushRemote config
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>

This is way under-explained.

It is not even clear what "also" in "remote: rename also remotes..."
refers to.  It hints that something that is not "remotes" is
renamed, but the readers do not know what it is.  It is not clear
when that said renaming is done, either.  

Is it supposed to be a bugfix?  If so, readers would need the issue
being addressed described, perhaps like so:

	When X is done, Y is renamed, but at the same time Z should
	also be renamed, but it is not.

> ---
> Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/remote.c  | 16 ++++++++++++++--
>  t/t5505-remote.sh |  4 +++-
>  2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 96bbe828fe..ddceba868a 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -246,7 +246,7 @@ static int add(int argc, const char **argv)
>  }
>  
>  struct branch_info {
> -	char *remote_name;
> +	char *remote_name, *push_remote_name;
>  	struct string_list merge;
>  	enum {
>  		NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES
> @@ -269,13 +269,16 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>  		char *name;
>  		struct string_list_item *item;
>  		struct branch_info *info;
> -		enum { REMOTE, MERGE, REBASE } type;
> +		enum { REMOTE, PUSH_REMOTE, MERGE, REBASE } type;

Is there a reason why this new one has to come between REMOTE and
MERGE?  Otherwise, the usual convention is either to add
alphabetically (if the list is sorted alphabetically from the
beginning) or add to the end (otherwise).

>  		size_t key_len;
>  
>  		key += 7;
>  		if (strip_suffix(key, ".remote", &key_len)) {
>  			name = xmemdupz(key, key_len);
>  			type = REMOTE;
> +		} else if (strip_suffix(key, ".pushremote", &key_len)) {
> +			name = xmemdupz(key, key_len);
> +			type = PUSH_REMOTE;
>  		} else if (strip_suffix(key, ".merge", &key_len)) {
>  			name = xmemdupz(key, key_len);
>  			type = MERGE;
> @@ -294,6 +297,10 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>  			if (info->remote_name)
>  				warning(_("more than one %s"), orig_key);
>  			info->remote_name = xstrdup(value);
> +		} else if (type == PUSH_REMOTE) {
> +			if (info->push_remote_name)
> +				warning(_("more than one %s"), orig_key);
> +			info->push_remote_name = xstrdup(value);
>  		} else if (type == MERGE) {
>  			char *space = strchr(value, ' ');
>  			value = abbrev_branch(value);
> @@ -680,6 +687,11 @@ static int mv(int argc, const char **argv)
>  			strbuf_addf(&buf, "branch.%s.remote", item->string);
>  			git_config_set(buf.buf, rename.new_name);
>  		}
> +		if (info->push_remote_name && !strcmp(info->push_remote_name, rename.old_name)) {
> +			strbuf_reset(&buf);
> +			strbuf_addf(&buf, "branch.%s.pushremote", item->string);
> +			git_config_set(buf.buf, rename.new_name);
> +		}
>  	}
>  
>  	if (!refspec_updated)
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index 883b32efa0..59a1681636 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -737,12 +737,14 @@ test_expect_success 'rename a remote' '
>  	git clone one four &&
>  	(
>  		cd four &&
> +		git config branch.master.pushRemote origin &&
>  		git remote rename origin upstream &&
>  		test -z "$(git for-each-ref refs/remotes/origin)" &&
>  		test "$(git symbolic-ref refs/remotes/upstream/HEAD)" = "refs/remotes/upstream/master" &&
>  		test "$(git rev-parse upstream/master)" = "$(git rev-parse master)" &&
>  		test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/remotes/upstream/*" &&
> -		test "$(git config branch.master.remote)" = "upstream"
> +		test "$(git config branch.master.remote)" = "upstream" &&
> +		test "$(git config branch.master.pushRemote)" = "upstream"
>  	)
>  '

OK, so the issue the patch wants to address is

	git remote rename X Y

ought to rename branch.X.Z (for any value of Z) to branch.Y.Z but it
forgot to do so for Z==pushRemote?

If that is the case, I have to wonder if the patch is making the
situation better or even worse.  Shouldn't we be not even caring
what Z is by not having to list these specific keys?  I dunno.