Re: [PATCH v4] remote rename/remove: gently handle remote.pushDefault config

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Bert,

On Sat, 1 Feb 2020, Bert Wesarg wrote:

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

Very clear commit message. Thank you!

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

We will have to be careful to not segfault if a user has this in their
config:

	[remote]
		pushDefault

i.e. we have to insert `!value || ` before the call to `strcmp()`.

It does not make much sense not to specify a value, of course, but we
should not segfault in such a case, either.

> +
> +	info->scope = current_config_scope();

Do we want to care about the case where above-mentioned invalid
`remote.pushDefault` is configured _and_ overrides an otherwise-valid
setting in `~/.gitconfig`?

Or for that matter, shouldn't we be careful to handle the case where `git
config --global remote.pushDefault` returns `old_name` but that is
overridden by a different `git config --local remote.pushDefault`?

Concretely, I believe that the patched code will misbehave in this
scenario:

	git config --global remote.pushDefault january
	git config remote.pushDefault february
	git remote rename january march

If I read the patch right, this will incorrectly warn about the
`pushDefault` setting in the user-wide config.

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

That name probably wants to convey better that the push default is handled
in the `mv`/`rm` commands here, not in any other command. Maybe
`handle_modified_push_default_remote()`?

> +{
> +	struct push_default_info push_default = {
> +		old_name, CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };

Personally, I would prefer the closing bracket to be on a new line,
followed by an empty line to separate the variable declaration from the
following statements.

> +	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");

Isn't this more like a `BUG()`? Or do you see any valid scenario where
this could happen? If you do, it may make a lot of sense to call
`die_errno()` here, to give the user _some_ sort of an actionable insight
as to what went wrong.

> +		else if (!new_name && result && result != CONFIG_NOTHING_SET)
> +			die(_("could not unset '%s'"), "remote.pushDefault");

Same here.

> +	} 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..dda81b7d07 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -734,6 +734,7 @@ 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 &&
> @@ -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 &&

I am not sure that a full clone is warranted here. Maybe use
`--no-checkout` here and in the subsequent test cases? Omitting that
option makes the tests only slower for no gain.

> +	(
> +		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' '
> +	test_config_global remote.pushDefault other &&
> +	git clone one four.2 &&
> +	(
> +		cd four.2 &&
> +		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' '
> +	test_config_global remote.pushDefault origin &&
> +	git clone one four.3 &&
> +	(
> +		cd four.3 &&
> +		git config remote.pushDefault origin &&
> +		git remote rename origin upstream &&
> +		test "$(git config --global remote.pushDefault)" = "origin" &&
> +		test "$(git config --local remote.pushDefault)" = "upstream"

A lot of tests added. Personally, I would have probably only extended the
existing `rename a remote` to verify that a repository-local
`remote.pushDefault` _is_ renamed, and then added one test case that
verifies that not only is a user-wide `remote.pushDefault` left alone but
also warned about.

>  	)
>  '
>
> @@ -787,6 +823,7 @@ 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 &&
> @@ -794,7 +831,42 @@ test_expect_success 'remove a remote' '
>  		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

Now that I see this sort of "in action", I have to wonder whether I would
be okay with a `remote.pushDefault` simply vanishing. I think that would
puzzle me ("Didn't I set a push default before? I must be getting old and
delusional."). In the least, I would want to see a warning here ("As the
remote 'xyz' was deleted, so was the `remote.pushDefault = xyz` setting"
or some such).

It strikes me as a fundamentally different thing whether we simply
re-target or delete a `pushDefault` setting. The former needs no further
warning, but the latter does.

> +	)
> +'
> +
> +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 &&
> +		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' '
> +	test_config_global remote.pushDefault origin &&
> +	git clone one four.five.3 &&
> +	(
> +		cd four.five.3 &&
> +		git config remote.pushDefault origin &&
> +		git remote remove origin &&
> +		test "$(git config --global remote.pushDefault)" = "origin" &&
> +		test_must_fail git config --local remote.pushDefault

Since the `mv` case already covers those code paths, I would be a lot more
parsimonious in adding test cases for `rm`.

There are voices claiming that adding regression tests is always a good
thing, but I would counter that it has to strike a balance between
coverage and runtime. We see a more and more contributions -- even from
long-time contributors -- where the test suite obviously has not been run
(because the CI builds are failing, and there is no doubt that seasoned
contributors in particular would fix those failures before contributing
their patches, _if_ they were aware of those test failures), proving just
how costly our test suite has become.

Other than that, the patch looks fine to me. Thank you,
Dscho

>  	)
>  '
>
> --
> 2.25.0.30.g00ce2e43d4
>
>