patchremote: rename also remotes in the branch.<name>.pushRemote config

> ---
> 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;

>  		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"
>  	)
>  '

> When renaming a remote with
>
>     git remote rename X Y
>
> Git already renames any config values from
>
>     branch.<name>.remote = X
>
> to
>
>     branch.<name>.remote = Y
>
> As branch.<name>.pushRemote also names a remote, it now also renames
> these config values from
>
>     branch.<name>.pushRemote = X
>
> to
>
>     branch.<name>.pushRemote = Y

> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>
> ---
> Cc: Junio C Hamano <gitster@pobox.com>
> Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/remote.c  | 17 +++++++++++++++--
>  t/t5505-remote.sh |  4 +++-
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 96bbe828fe..a74aac344f 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -251,6 +251,7 @@ struct branch_info {
>  	enum {
>  		NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES
>  	} rebase;
> +	char *push_remote_name;
>  };
>
>  static struct string_list branch_list = STRING_LIST_INIT_NODUP;
> @@ -269,7 +270,7 @@ 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, MERGE, REBASE, PUSH_REMOTE } type;
>  		size_t key_len;
>
>  		key += 7;
> @@ -282,6 +283,9 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>  		} else if (strip_suffix(key, ".rebase", &key_len)) {
>  			name = xmemdupz(key, key_len);
>  			type = REBASE;
> +		} else if (strip_suffix(key, ".pushremote", &key_len)) {
> +			name = xmemdupz(key, key_len);
> +			type = PUSH_REMOTE;
>  		} else
>  			return 0;
>
> @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>  				space = strchr(value, ' ');
>  			}
>  			string_list_append(&info->merge, xstrdup(value));
> -		} else {
> +		} else if (type == REBASE) {
>  			int v = git_parse_maybe_bool(value);
>  			if (v >= 0)
>  				info->rebase = v;
> @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>  				info->rebase = REBASE_MERGES;
>  			else if (!strcmp(value, "interactive"))
>  				info->rebase = INTERACTIVE_REBASE;
> +		} else {
> +			if (info->push_remote_name)
> +				warning(_("more than one %s"), orig_key);
> +			info->push_remote_name = xstrdup(value);

>  		}
>  	}
>  	return 0;
> @@ -680,6 +688,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"
>  	)
>  '
>
> --
> 2.24.1.497.g9abd7b20b4.dirty
>
>

>
> Hi Bert,
>
> On Fri, 17 Jan 2020, Bert Wesarg wrote:
>
> > When renaming a remote with
> >
> >     git remote rename X Y
> >
> > Git already renames any config values from
> >
> >     branch.<name>.remote = X
> >
> > to
> >
> >     branch.<name>.remote = Y
> >
> > As branch.<name>.pushRemote also names a remote, it now also renames
> > these config values from
> >
> >     branch.<name>.pushRemote = X
> >
> > to
> >
> >     branch.<name>.pushRemote = Y
>
> Should we warn if remote.pushDefault = X?

>
> Other than that, I am still okay with the patch (even after the
> re-ordering of the enum ;-)
>
> Just one more suggestion:
>
> > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> >
> > ---
> > Cc: Junio C Hamano <gitster@pobox.com>
> > Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/remote.c  | 17 +++++++++++++++--
> >  t/t5505-remote.sh |  4 +++-
> >  2 files changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/remote.c b/builtin/remote.c
> > index 96bbe828fe..a74aac344f 100644
> > --- a/builtin/remote.c
> > +++ b/builtin/remote.c
> > @@ -251,6 +251,7 @@ struct branch_info {
> >       enum {
> >               NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES
> >       } rebase;
> > +     char *push_remote_name;
> >  };
> >
> >  static struct string_list branch_list = STRING_LIST_INIT_NODUP;
> > @@ -269,7 +270,7 @@ 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, MERGE, REBASE, PUSH_REMOTE } type;
> >               size_t key_len;
> >
> >               key += 7;
> > @@ -282,6 +283,9 @@ static int config_read_branches(const char *key, const char *value, void *cb)
> >               } else if (strip_suffix(key, ".rebase", &key_len)) {
> >                       name = xmemdupz(key, key_len);
> >                       type = REBASE;
> > +             } else if (strip_suffix(key, ".pushremote", &key_len)) {
> > +                     name = xmemdupz(key, key_len);
> > +                     type = PUSH_REMOTE;
> >               } else
> >                       return 0;
> >
> > @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb)
> >                               space = strchr(value, ' ');
> >                       }
> >                       string_list_append(&info->merge, xstrdup(value));
> > -             } else {
> > +             } else if (type == REBASE) {
> >                       int v = git_parse_maybe_bool(value);
> >                       if (v >= 0)
> >                               info->rebase = v;
> > @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb)
> >                               info->rebase = REBASE_MERGES;
> >                       else if (!strcmp(value, "interactive"))
> >                               info->rebase = INTERACTIVE_REBASE;
> > +             } else {
> > +                     if (info->push_remote_name)
> > +                             warning(_("more than one %s"), orig_key);
> > +                     info->push_remote_name = xstrdup(value);
>
> It makes me a bit nervous that this is an `else` without an `if (type ==
> PUSH_REMOTE)`... Maybe do that, just to be on the safe side, even if
> another type is introduced in the future?

> On Fri, Jan 17, 2020 at 12:50 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > Hi Bert,
> >
> > On Fri, 17 Jan 2020, Bert Wesarg wrote:
> >
> > > When renaming a remote with
> > >
> > >     git remote rename X Y
> > >
> > > Git already renames any config values from
> > >
> > >     branch.<name>.remote = X
> > >
> > > to
> > >
> > >     branch.<name>.remote = Y
> > >
> > > As branch.<name>.pushRemote also names a remote, it now also renames
> > > these config values from
> > >
> > >     branch.<name>.pushRemote = X
> > >
> > > to
> > >
> > >     branch.<name>.pushRemote = Y
> >
> > Should we warn if remote.pushDefault = X?
>
> AFAIU, the value of remote.pushDefault wont be renamed yet. So you
> suggest to issue a warning in case remote.pushDefault is X. But as X
> does not exists anymore after the rename, the value of
> remote.pushDefault is invalid. So why not rename it too?

> > > @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb)
> > >                               space = strchr(value, ' ');
> > >                       }
> > >                       string_list_append(&info->merge, xstrdup(value));
> > > -             } else {
> > > +             } else if (type == REBASE) {
> > >                       int v = git_parse_maybe_bool(value);
> > >                       if (v >= 0)
> > >                               info->rebase = v;
> > > @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb)
> > >                               info->rebase = REBASE_MERGES;
> > >                       else if (!strcmp(value, "interactive"))
> > >                               info->rebase = INTERACTIVE_REBASE;
> > > +             } else {
> > > +                     if (info->push_remote_name)
> > > +                             warning(_("more than one %s"), orig_key);
> > > +                     info->push_remote_name = xstrdup(value);
> >
> > It makes me a bit nervous that this is an `else` without an `if (type ==
> > PUSH_REMOTE)`... Maybe do that, just to be on the safe side, even if
> > another type is introduced in the future?
>
> I'm not a friend of this last 'else' either, but it was so to begin
> with. I'm all for changing it to an 'else if'. Though while it is
> impossible that 'type' has a value different than one from the enum, I
> would be even more comfortable with having BUG at the end.

>
> Hi Bert,
>
> On Fri, 17 Jan 2020, Bert Wesarg wrote:
>
> > On Fri, Jan 17, 2020 at 12:50 PM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > Hi Bert,
> > >
> > > On Fri, 17 Jan 2020, Bert Wesarg wrote:
> > >
> > > > When renaming a remote with
> > > >
> > > >     git remote rename X Y
> > > >
> > > > Git already renames any config values from
> > > >
> > > >     branch.<name>.remote = X
> > > >
> > > > to
> > > >
> > > >     branch.<name>.remote = Y
> > > >
> > > > As branch.<name>.pushRemote also names a remote, it now also renames
> > > > these config values from
> > > >
> > > >     branch.<name>.pushRemote = X
> > > >
> > > > to
> > > >
> > > >     branch.<name>.pushRemote = Y
> > >
> > > Should we warn if remote.pushDefault = X?
> >
> > AFAIU, the value of remote.pushDefault wont be renamed yet. So you
> > suggest to issue a warning in case remote.pushDefault is X. But as X
> > does not exists anymore after the rename, the value of
> > remote.pushDefault is invalid. So why not rename it too?
>
> If this setting was usually a repository-specific one, I would suggest to
> change its value, too. But it is my understanding that this might be set
> in `~/.gitconfig` more often than not, so I recommend a warning instead.

>
> > > > @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb)
> > > >                               space = strchr(value, ' ');
> > > >                       }
> > > >                       string_list_append(&info->merge, xstrdup(value));
> > > > -             } else {
> > > > +             } else if (type == REBASE) {
> > > >                       int v = git_parse_maybe_bool(value);
> > > >                       if (v >= 0)
> > > >                               info->rebase = v;
> > > > @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb)
> > > >                               info->rebase = REBASE_MERGES;
> > > >                       else if (!strcmp(value, "interactive"))
> > > >                               info->rebase = INTERACTIVE_REBASE;
> > > > +             } else {
> > > > +                     if (info->push_remote_name)
> > > > +                             warning(_("more than one %s"), orig_key);
> > > > +                     info->push_remote_name = xstrdup(value);
> > >
> > > It makes me a bit nervous that this is an `else` without an `if (type ==
> > > PUSH_REMOTE)`... Maybe do that, just to be on the safe side, even if
> > > another type is introduced in the future?
> >
> > I'm not a friend of this last 'else' either, but it was so to begin
> > with. I'm all for changing it to an 'else if'. Though while it is
> > impossible that 'type' has a value different than one from the enum, I
> > would be even more comfortable with having BUG at the end.
>
> My thinking was: even if this was a bare `else` before, why not fix that
> issue while we're already in the area? I like the `BUG` idea.

> On Fri, Jan 17, 2020 at 2:30 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Fri, 17 Jan 2020, Bert Wesarg wrote:
> >
> > > On Fri, Jan 17, 2020 at 12:50 PM Johannes Schindelin
> > > <Johannes.Schindelin@gmx.de> wrote:
> > > >
> > > > On Fri, 17 Jan 2020, Bert Wesarg wrote:
> > > >
> > > > > When renaming a remote with
> > > > >
> > > > >     git remote rename X Y
> > > > >
> > > > > Git already renames any config values from
> > > > >
> > > > >     branch.<name>.remote = X
> > > > >
> > > > > to
> > > > >
> > > > >     branch.<name>.remote = Y
> > > > >
> > > > > As branch.<name>.pushRemote also names a remote, it now also renames
> > > > > these config values from
> > > > >
> > > > >     branch.<name>.pushRemote = X
> > > > >
> > > > > to
> > > > >
> > > > >     branch.<name>.pushRemote = Y
> > > >
> > > > Should we warn if remote.pushDefault = X?
> > >
> > > AFAIU, the value of remote.pushDefault wont be renamed yet. So you
> > > suggest to issue a warning in case remote.pushDefault is X. But as X
> > > does not exists anymore after the rename, the value of
> > > remote.pushDefault is invalid. So why not rename it too?
> >
> > If this setting was usually a repository-specific one, I would suggest to
> > change its value, too. But it is my understanding that this might be set
> > in `~/.gitconfig` more often than not, so I recommend a warning instead.
>
> than why not rename it, if its a repository-specific setting and warn
> if it is a global one? If this is detectable at all.

>
> Hi Bert,
>
> On Fri, 17 Jan 2020, Bert Wesarg wrote:
>
> > On Fri, Jan 17, 2020 at 2:30 PM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > On Fri, 17 Jan 2020, Bert Wesarg wrote:
> > >
> > > > On Fri, Jan 17, 2020 at 12:50 PM Johannes Schindelin
> > > > <Johannes.Schindelin@gmx.de> wrote:
> > > > AFAIU, the value of remote.pushDefault wont be renamed yet. So you
> > > > suggest to issue a warning in case remote.pushDefault is X. But as X
> > > > does not exists anymore after the rename, the value of
> > > > remote.pushDefault is invalid. So why not rename it too?
> > >
> > > If this setting was usually a repository-specific one, I would suggest to
> > > change its value, too. But it is my understanding that this might be set
> > > in `~/.gitconfig` more often than not, so I recommend a warning instead.
> >
> > than why not rename it, if its a repository-specific setting and warn
> > if it is a global one? If this is detectable at all.
>
> Sure, but you might need to re-parse the config to detect that (and you
> have to use `git_config_from_file()` to make sure that you know that you
> are looking at the repository config and not at anything else).

> On Mon, Jan 20, 2020 at 12:25 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > Hi Bert,
> >
> > On Fri, 17 Jan 2020, Bert Wesarg wrote:
> >
> > > On Fri, Jan 17, 2020 at 2:30 PM Johannes Schindelin
> > > <Johannes.Schindelin@gmx.de> wrote:
> > > >
> > > > On Fri, 17 Jan 2020, Bert Wesarg wrote:
> > > >
> > > > > On Fri, Jan 17, 2020 at 12:50 PM Johannes Schindelin
> > > > > <Johannes.Schindelin@gmx.de> wrote:
> > > > > AFAIU, the value of remote.pushDefault wont be renamed yet. So you
> > > > > suggest to issue a warning in case remote.pushDefault is X. But as X
> > > > > does not exists anymore after the rename, the value of
> > > > > remote.pushDefault is invalid. So why not rename it too?
> > > >
> > > > If this setting was usually a repository-specific one, I would suggest to
> > > > change its value, too. But it is my understanding that this might be set
> > > > in `~/.gitconfig` more often than not, so I recommend a warning instead.
> > >
> > > than why not rename it, if its a repository-specific setting and warn
> > > if it is a global one? If this is detectable at all.
> >
> > Sure, but you might need to re-parse the config to detect that (and you
> > have to use `git_config_from_file()` to make sure that you know that you
> > are looking at the repository config and not at anything else).
>
> I found current_config_scope() which serves the purpose for me.
> Anything wrong with this approach?

> When renaming a remote with
>
>     git remote rename X Y
>
> Git already renames any config values from
>
>     branch.<name>.remote = X
>
> to
>
>     branch.<name>.remote = Y
>
> As branch.<name>.pushRemote also names a remote, it now also renames
> these config values from
>
>     branch.<name>.pushRemote = X
>
> to
>
>     branch.<name>.pushRemote = Y

> @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>  				space = strchr(value, ' ');
>  			}
>  			string_list_append(&info->merge, xstrdup(value));
> -		} else {
> +		} else if (type == REBASE) {
>  			int v = git_parse_maybe_bool(value);
>  			if (v >= 0)
>  				info->rebase = v;
> @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>  				info->rebase = REBASE_MERGES;
>  			else if (!strcmp(value, "interactive"))
>  				info->rebase = INTERACTIVE_REBASE;
> +		} else {
> +			if (info->push_remote_name)
> +				warning(_("more than one %s"), orig_key);
> +			info->push_remote_name = xstrdup(value);
>  		}

> > @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb)
> >                               space = strchr(value, ' ');
> >                       }
> >                       string_list_append(&info->merge, xstrdup(value));
> > -             } else {
> > +             } else if (type == REBASE) {
> >                       int v = git_parse_maybe_bool(value);
> >                       if (v >= 0)
> >                               info->rebase = v;
> > @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb)
> >                               info->rebase = REBASE_MERGES;
> >                       else if (!strcmp(value, "interactive"))
> >                               info->rebase = INTERACTIVE_REBASE;
> > +             } else {
> > +                     if (info->push_remote_name)
> > +                             warning(_("more than one %s"), orig_key);
> > +                     info->push_remote_name = xstrdup(value);
> >               }
>
> This is perfectly fine for now, as it follows the existing "now we
> have handled X, and Y, so the remainder must be Z" mentality, but at
> some point we may want to make sure that we are protected against
> seeing an unexpected 'type', iow
>
>                         ...
>                 } else if (type == PUSH_REMOTE) {
>                         ...
>                 } else {
>                         BUG("unexpected type=%d", type);
>                 }
>
> as we learn more "type"s.  Better yet, this if/elseif/ cascade may
> become clearer if it is rewritten to a switch statement.
>
> I was about to conclude this message with "but that is all outside
> the scope of this fix, so I'll queue it as-is " before noticing
> that you two seem to be leaning towards clean-up at the same time.
> If we are to clean up the code structure along these lines, I'd
> prefer to see it done as a preparatory patch before pushremote
> handling gets introduced.
>
> Taking some other clean-up ideas on this function, e.g.:
>
>  * key += 7 should better be done without hardcoded length of "branch."
>  * By leaving early, we can save one indentation level.
>  * name does not have to be computed for each branch.
>
> the resulting body of the function might look more like this:
>
>         if (!skip_prefix(key, "branch.", &key))
>                 return 0;
>
>         if (strip_suffix(key, ".remote", &key_len))
>                 type = REMOTE;
>         else if (strip_suffix(key, ".merge", &key_len))
>                 type = MERGE;
>         ...
>         else
>                 return 0;
>         name = xmemdupz(key, key_len);
>         item = string_list_insert(&branch_list, name);
>         ...
>
>         switch (type) {
>         case REMOTE:
>                 ...
>         default:
>                 BUG("unhandled type %d", type);
>         }

> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
> > 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.

> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> ---
> 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;
>  		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"
>  	)
>  '
>
> --
> 2.24.1.497.g9abd7b20b4.dirty
>
>