diff mbox series

[v3,6/6] remote rename/remove: gently handle remote.pushDefault config

Message ID 965b587f5834c88532476b56da95ead605d16000.1580110970.git.bert.wesarg@googlemail.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Bert Wesarg Jan. 27, 2020, 8:15 a.m. UTC 
When renaming a remote with

    git remote rename X Y
    git remote remove X

Git already renames or removes any branch.<name>.remote and
branch.<name>.pushRemote configurations if their value is X.

However remote.pushDefault needs a more gentle approach, as this may be
set in a non-repo configuration file. In such a case only a warning is
printed, such as:

warning: The global configuration remote.pushDefault in:
	$HOME/.gitconfig:35
now names the non-existent remote origin

It is changed to remote.pushDefault = Y or removed when set in a repo
configuration though.

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---

Sorry, <029dd071038db1daf590d5224076c089cfd060bc.1580108477.git.bert.wesarg@googlemail.com>
missed the ammend to let it work with mr/show-config-scope. Please drop that in
favor of this one.

Changes since v1:
 * handle also 'git remote remove'

Cc: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: Matthew Rogers <mattr94@gmail.com>
---
 builtin/remote.c  | 54 +++++++++++++++++++++++++++++++++
 t/t5505-remote.sh | 76 +++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 128 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Jan. 29, 2020, 6:57 a.m. UTC | #1 
Bert Wesarg <bert.wesarg@googlemail.com> writes:

> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index 082042b05a..bbff8c5770 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -737,6 +737,7 @@ test_expect_success 'rename a remote' '
>  	git clone one four &&
>  	(
>  		cd four &&
> +		test_config_global remote.pushDefault origin &&
>  		git config branch.master.pushRemote origin &&
>  		git remote rename origin upstream &&
>  		test -z "$(git for-each-ref refs/remotes/origin)" &&

You cannot use test_config and test_config_global inside a subshell,
as they rely on test_when_finished to rewind their effect, which
cannot be used inside a subshell.  As you are doing "global" config,
there is no reason to make "git config --global" call in a particular
repository anyway, so just do this upfront as the first thing in the
test sequence.  There are a few others in this file.

Tentatively I applied the following fix-up on top of the series to
unblock tonight's integration cycle.

Thanks.

 t/t5505-remote.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index bbff8c5770..dda81b7d07 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -734,10 +734,10 @@ test_expect_success 'reject adding remote with an invalid name' '
 # the last two ones check if the config is updated.
 
 test_expect_success 'rename a remote' '
+	test_config_global remote.pushDefault origin &&
 	git clone one four &&
 	(
 		cd four &&
-		test_config_global remote.pushDefault origin &&
 		git config branch.master.pushRemote origin &&
 		git remote rename origin upstream &&
 		test -z "$(git for-each-ref refs/remotes/origin)" &&
@@ -761,10 +761,10 @@ test_expect_success 'rename a remote renames repo remote.pushDefault' '
 '
 
 test_expect_success 'rename a remote renames repo remote.pushDefault but ignores global' '
+	test_config_global remote.pushDefault other &&
 	git clone one four.2 &&
 	(
 		cd four.2 &&
-		test_config_global remote.pushDefault other &&
 		git config remote.pushDefault origin &&
 		git remote rename origin upstream &&
 		test "$(git config --global remote.pushDefault)" = "other" &&
@@ -773,10 +773,10 @@ test_expect_success 'rename a remote renames repo remote.pushDefault but ignores
 '
 
 test_expect_success 'rename a remote renames repo remote.pushDefault but keeps global' '
+	test_config_global remote.pushDefault origin &&
 	git clone one four.3 &&
 	(
 		cd four.3 &&
-		test_config_global remote.pushDefault origin &&
 		git config remote.pushDefault origin &&
 		git remote rename origin upstream &&
 		test "$(git config --global remote.pushDefault)" = "origin" &&
@@ -823,10 +823,10 @@ test_expect_success 'rename succeeds with existing remote.<target>.prune' '
 '
 
 test_expect_success 'remove a remote' '
+	test_config_global remote.pushDefault origin &&
 	git clone one four.five &&
 	(
 		cd four.five &&
-		test_config_global remote.pushDefault origin &&
 		git config branch.master.pushRemote origin &&
 		git remote remove origin &&
 		test -z "$(git for-each-ref refs/remotes/origin)" &&
@@ -847,10 +847,10 @@ test_expect_success 'remove a remote removes repo remote.pushDefault' '
 '
 
 test_expect_success 'remove a remote removes repo remote.pushDefault but ignores global' '
+	test_config_global remote.pushDefault other &&
 	git clone one four.five.2 &&
 	(
 		cd four.five.2 &&
-		test_config_global remote.pushDefault other &&
 		git config remote.pushDefault origin &&
 		git remote remove origin &&
 		test "$(git config --global remote.pushDefault)" = "other" &&
@@ -859,10 +859,10 @@ test_expect_success 'remove a remote removes repo remote.pushDefault but ignores
 '
 
 test_expect_success 'remove a remote removes repo remote.pushDefault but keeps global' '
+	test_config_global remote.pushDefault origin &&
 	git clone one four.five.3 &&
 	(
 		cd four.five.3 &&
-		test_config_global remote.pushDefault origin &&
 		git config remote.pushDefault origin &&
 		git remote remove origin &&
 		test "$(git config --global remote.pushDefault)" = "origin" &&
Bert Wesarg Jan. 29, 2020, 9:16 a.m. UTC | #2 
On Wed, Jan 29, 2020 at 7:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
> > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> > index 082042b05a..bbff8c5770 100755
> > --- a/t/t5505-remote.sh
> > +++ b/t/t5505-remote.sh
> > @@ -737,6 +737,7 @@ test_expect_success 'rename a remote' '
> >       git clone one four &&
> >       (
> >               cd four &&
> > +             test_config_global remote.pushDefault origin &&
> >               git config branch.master.pushRemote origin &&
> >               git remote rename origin upstream &&
> >               test -z "$(git for-each-ref refs/remotes/origin)" &&
>
> You cannot use test_config and test_config_global inside a subshell,
> as they rely on test_when_finished to rewind their effect, which
> cannot be used inside a subshell.  As you are doing "global" config,
> there is no reason to make "git config --global" call in a particular
> repository anyway, so just do this upfront as the first thing in the
> test sequence.  There are a few others in this file.
>
> Tentatively I applied the following fix-up on top of the series to
> unblock tonight's integration cycle.
>
> Thanks.

Thanks. Will squash them and re-roll.

Bert

>
>  t/t5505-remote.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index bbff8c5770..dda81b7d07 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -734,10 +734,10 @@ test_expect_success 'reject adding remote with an invalid name' '
>  # the last two ones check if the config is updated.
>
>  test_expect_success 'rename a remote' '
> +       test_config_global remote.pushDefault origin &&
>         git clone one four &&
>         (
>                 cd four &&
> -               test_config_global remote.pushDefault origin &&
>                 git config branch.master.pushRemote origin &&
>                 git remote rename origin upstream &&
>                 test -z "$(git for-each-ref refs/remotes/origin)" &&
> @@ -761,10 +761,10 @@ test_expect_success 'rename a remote renames repo remote.pushDefault' '
>  '
>
>  test_expect_success 'rename a remote renames repo remote.pushDefault but ignores global' '
> +       test_config_global remote.pushDefault other &&
>         git clone one four.2 &&
>         (
>                 cd four.2 &&
> -               test_config_global remote.pushDefault other &&
>                 git config remote.pushDefault origin &&
>                 git remote rename origin upstream &&
>                 test "$(git config --global remote.pushDefault)" = "other" &&
> @@ -773,10 +773,10 @@ test_expect_success 'rename a remote renames repo remote.pushDefault but ignores
>  '
>
>  test_expect_success 'rename a remote renames repo remote.pushDefault but keeps global' '
> +       test_config_global remote.pushDefault origin &&
>         git clone one four.3 &&
>         (
>                 cd four.3 &&
> -               test_config_global remote.pushDefault origin &&
>                 git config remote.pushDefault origin &&
>                 git remote rename origin upstream &&
>                 test "$(git config --global remote.pushDefault)" = "origin" &&
> @@ -823,10 +823,10 @@ test_expect_success 'rename succeeds with existing remote.<target>.prune' '
>  '
>
>  test_expect_success 'remove a remote' '
> +       test_config_global remote.pushDefault origin &&
>         git clone one four.five &&
>         (
>                 cd four.five &&
> -               test_config_global remote.pushDefault origin &&
>                 git config branch.master.pushRemote origin &&
>                 git remote remove origin &&
>                 test -z "$(git for-each-ref refs/remotes/origin)" &&
> @@ -847,10 +847,10 @@ test_expect_success 'remove a remote removes repo remote.pushDefault' '
>  '
>
>  test_expect_success 'remove a remote removes repo remote.pushDefault but ignores global' '
> +       test_config_global remote.pushDefault other &&
>         git clone one four.five.2 &&
>         (
>                 cd four.five.2 &&
> -               test_config_global remote.pushDefault other &&
>                 git config remote.pushDefault origin &&
>                 git remote remove origin &&
>                 test "$(git config --global remote.pushDefault)" = "other" &&
> @@ -859,10 +859,10 @@ test_expect_success 'remove a remote removes repo remote.pushDefault but ignores
>  '
>
>  test_expect_success 'remove a remote removes repo remote.pushDefault but keeps global' '
> +       test_config_global remote.pushDefault origin &&
>         git clone one four.five.3 &&
>         (
>                 cd four.five.3 &&
> -               test_config_global remote.pushDefault origin &&
>                 git config remote.pushDefault origin &&
>                 git remote remove origin &&
>                 test "$(git config --global remote.pushDefault)" = "origin" &&
diff mbox series

Patch

diff --git a/builtin/remote.c b/builtin/remote.c
index a2379a14bf..5af06b74a7 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -615,6 +615,55 @@  static int migrate_file(struct remote *remote)
 	return 0;
 }
 
+struct push_default_info
+{
+	const char *old_name;
+	enum config_scope scope;
+	struct strbuf origin;
+	int linenr;
+};
+
+static int config_read_push_default(const char *key, const char *value,
+	void *cb)
+{
+	struct push_default_info* info = cb;
+	if (strcmp(key, "remote.pushdefault") || strcmp(value, info->old_name))
+		return 0;
+
+	info->scope = current_config_scope();
+	strbuf_reset(&info->origin);
+	strbuf_addstr(&info->origin, current_config_name());
+	info->linenr = current_config_line();
+
+	return 0;
+}
+
+static void handle_push_default(const char* old_name, const char* new_name)
+{
+	struct push_default_info push_default = {
+		old_name, CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };
+	git_config(config_read_push_default, &push_default);
+	if (push_default.scope >= CONFIG_SCOPE_COMMAND)
+		; /* pass */
+	else if (push_default.scope >= CONFIG_SCOPE_LOCAL) {
+		int result = git_config_set_gently("remote.pushDefault",
+						   new_name);
+		if (new_name && result && result != CONFIG_NOTHING_SET)
+			die(_("could not set '%s'"), "remote.pushDefault");
+		else if (!new_name && result && result != CONFIG_NOTHING_SET)
+			die(_("could not unset '%s'"), "remote.pushDefault");
+	} else if (push_default.scope >= CONFIG_SCOPE_SYSTEM) {
+		/* warn */
+		warning(_("The %s configuration remote.pushDefault in:\n"
+			  "\t%s:%d\n"
+			  "now names the non-existent remote '%s'"),
+			config_scope_name(push_default.scope),
+			push_default.origin.buf, push_default.linenr,
+			old_name);
+	}
+}
+
+
 static int mv(int argc, const char **argv)
 {
 	struct option options[] = {
@@ -750,6 +799,9 @@  static int mv(int argc, const char **argv)
 			die(_("creating '%s' failed"), buf.buf);
 	}
 	string_list_clear(&remote_branches, 1);
+
+	handle_push_default(rename.old_name, rename.new_name);
+
 	return 0;
 }
 
@@ -835,6 +887,8 @@  static int rm(int argc, const char **argv)
 		strbuf_addf(&buf, "remote.%s", remote->name);
 		if (git_config_rename_section(buf.buf, NULL) < 1)
 			return error(_("Could not remove config section '%s'"), buf.buf);
+
+		handle_push_default(remote->name, NULL);
 	}
 
 	return result;
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 082042b05a..bbff8c5770 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -737,6 +737,7 @@  test_expect_success 'rename a remote' '
 	git clone one four &&
 	(
 		cd four &&
+		test_config_global remote.pushDefault origin &&
 		git config branch.master.pushRemote origin &&
 		git remote rename origin upstream &&
 		test -z "$(git for-each-ref refs/remotes/origin)" &&
@@ -744,7 +745,42 @@  test_expect_success 'rename a remote' '
 		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.pushRemote)" = "upstream"
+		test "$(git config branch.master.pushRemote)" = "upstream" &&
+		test "$(git config --global remote.pushDefault)" = "origin"
+	)
+'
+
+test_expect_success 'rename a remote renames repo remote.pushDefault' '
+	git clone one four.1 &&
+	(
+		cd four.1 &&
+		git config remote.pushDefault origin &&
+		git remote rename origin upstream &&
+		test "$(git config --local remote.pushDefault)" = "upstream"
+	)
+'
+
+test_expect_success 'rename a remote renames repo remote.pushDefault but ignores global' '
+	git clone one four.2 &&
+	(
+		cd four.2 &&
+		test_config_global remote.pushDefault other &&
+		git config remote.pushDefault origin &&
+		git remote rename origin upstream &&
+		test "$(git config --global remote.pushDefault)" = "other" &&
+		test "$(git config --local remote.pushDefault)" = "upstream"
+	)
+'
+
+test_expect_success 'rename a remote renames repo remote.pushDefault but keeps global' '
+	git clone one four.3 &&
+	(
+		cd four.3 &&
+		test_config_global remote.pushDefault origin &&
+		git config remote.pushDefault origin &&
+		git remote rename origin upstream &&
+		test "$(git config --global remote.pushDefault)" = "origin" &&
+		test "$(git config --local remote.pushDefault)" = "upstream"
 	)
 '
 
@@ -790,11 +826,47 @@  test_expect_success 'remove a remote' '
 	git clone one four.five &&
 	(
 		cd four.five &&
+		test_config_global remote.pushDefault origin &&
 		git config branch.master.pushRemote origin &&
 		git remote remove origin &&
 		test -z "$(git for-each-ref refs/remotes/origin)" &&
 		test_must_fail git config branch.master.remote &&
-		test_must_fail git config branch.master.pushRemote
+		test_must_fail git config branch.master.pushRemote &&
+		test "$(git config --global remote.pushDefault)" = "origin"
+	)
+'
+
+test_expect_success 'remove a remote removes repo remote.pushDefault' '
+	git clone one four.five.1 &&
+	(
+		cd four.five.1 &&
+		git config remote.pushDefault origin &&
+		git remote remove origin &&
+		test_must_fail git config --local remote.pushDefault
+	)
+'
+
+test_expect_success 'remove a remote removes repo remote.pushDefault but ignores global' '
+	git clone one four.five.2 &&
+	(
+		cd four.five.2 &&
+		test_config_global remote.pushDefault other &&
+		git config remote.pushDefault origin &&
+		git remote remove origin &&
+		test "$(git config --global remote.pushDefault)" = "other" &&
+		test_must_fail git config --local remote.pushDefault
+	)
+'
+
+test_expect_success 'remove a remote removes repo remote.pushDefault but keeps global' '
+	git clone one four.five.3 &&
+	(
+		cd four.five.3 &&
+		test_config_global remote.pushDefault origin &&
+		git config remote.pushDefault origin &&
+		git remote remove origin &&
+		test "$(git config --global remote.pushDefault)" = "origin" &&
+		test_must_fail git config --local remote.pushDefault
 	)
 '