remote: rename also remotes in the branch.<name>.pushRemote config
diff mbox series

Message ID 5a8791ef1e262d2078a4ca26b87bfbd777bd4432.1579209398.git.bert.wesarg@googlemail.com
State New
Headers show
Series
  • remote: rename also remotes in the branch.<name>.pushRemote config
Related show

Commit Message

Bert Wesarg Jan. 16, 2020, 9:25 p.m. UTC
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(-)

Comments

Junio C Hamano Jan. 16, 2020, 11:14 p.m. UTC | #1
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.
Johannes Schindelin Jan. 17, 2020, 9:45 a.m. UTC | #2
Hi Bert,

On Thu, 16 Jan 2020, Bert Wesarg wrote:

> 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(-)

The patch looks obviously good to me. I would have wished for slightly
more information in the commit message, but it is okay as it is.

Thanks,
Dscho

>
> 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
>
>
Johannes Schindelin Jan. 17, 2020, 9:49 a.m. UTC | #3
Hi Junio,

On Thu, 16 Jan 2020, Junio C Hamano wrote:

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

I think what the code does is not rename branch.X.Z to branch.Y.Z, but for
every `branch.B.pushRemote` whose value is `X`, it will now be set to `Y`.

So we really should not touch, say, `branch.X.description` because it has
nothing to do with any renamed remote.

And yes, I agree, the commit message would have made for a splendid place
to explain something like this.

Ciao,
Dscho

Patch
diff mbox series

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