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

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

[Bert Wesarg]

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

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

[Junio C Hamano]

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.

[PATCH v2] remote rename: rename branch.<name>.pushRemote config values too

[Bert Wesarg]

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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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

Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too

[Johannes Schindelin]

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?

Thanks,
Dscho

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

Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too

[Bert Wesarg]

Hi,

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?

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

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.

Bert

>
> Thanks,
> Dscho
>

Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too

[Johannes Schindelin]

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.

Ciao,
Dscho

Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too

[Bert Wesarg]

Hi,

On Fri, Jan 17, 2020 at 2:30 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> 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.

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.

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

Glad I can squash this into this one, instead of making it a single
patch out of it.

Bert

>
> Ciao,
> Dscho

Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too

[Junio C Hamano]

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

> 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

This makes sense now.  Thanks for an updated description.

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

> @@ -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);
	}

Thanks.

Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too

[Bert Wesarg]

Junio,

On Fri, Jan 17, 2020 at 7:48 PM Junio C Hamano <gitster@pobox.com> wrote:
> > @@ -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);
>         }

can you give me an heads up about your expected number of patches for
this clean up. Rather detailed or just one?

Thanks in advance.

Best,
Bert

>
> Thanks.

Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too

[Junio C Hamano]

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

> can you give me an heads up about your expected number of patches for
> this clean up. Rather detailed or just one?

That's up to the contributor X-<.

I would expect that some folks can write it up as a single patch and
still keep it easily understandable, and others might have trouble
explaining what they did well to the others and may need to split
them into a few pieces.

Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too

[Johannes Schindelin]

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

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

Ciao,
Dscho

Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too

[Bert Wesarg]

Hi Dscho,

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?

Best,
Bert

>
> Ciao,
> Dscho

Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too

[Johannes Schindelin]

Hi Bert,

On Mon, 20 Jan 2020, Bert Wesarg wrote:

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

I guess you could go for that, even if it is not exactly elegant (and not
thread-safe, but who cares about that in `git remote`...). It would also
do way more work than you need.

If I were to implement this, I would definitely go for
`git_config_from_file(git_path("index"), ...)`.

Ciao,
Dscho

[PATCH 0/7] remote rename: improve handling of configuration values

[Bert Wesarg]

While fixing that 'git remote rename X Y' does not rename the values for
'branch.*.pushRemote', it opened the possibility to more improvements in
this area:

 - 'remote rename' did not accept single-letter abbreviations for
   'branch.*.rebase' like 'pull --rebase' does

 - minor clean-ups the config callback

 - patch 5 will be replaced by/rebased on Matthew's work in 'config: allow user to
   know scope of config options', once 'config_scope_name' is available

 - gently handling the rename of 'remote.pushDefault'

Bert Wesarg (7):
  pull --rebase/remote rename: document and honor single-letter
    abbreviations rebase types
  remote: clean-up by returning early to avoid one indentation
  remote: clean-up config callback
  remote rename: rename branch.<name>.pushRemote config values too
  [RFC] config: make `scope_name` global as `config_scope_name`
  config: provide access to the current line number
  remote rename: gently handle remote.pushDefault config

 Documentation/config/branch.txt |   7 +-
 Documentation/config/pull.txt   |   7 +-
 Makefile                        |   1 +
 builtin/pull.c                  |  29 +-----
 builtin/remote.c                | 168 +++++++++++++++++++++-----------
 config.c                        |  24 +++++
 config.h                        |   2 +
 rebase.c                        |  24 +++++
 rebase.h                        |  15 +++
 t/helper/test-config.c          |  18 +---
 t/t1308-config-set.sh           |  14 ++-
 t/t5505-remote.sh               |  52 +++++++++-
 12 files changed, 254 insertions(+), 107 deletions(-)
 create mode 100644 rebase.c
 create mode 100644 rebase.h

-- 
2.24.1.497.g9abd7b20b4.dirty

[PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types

[Bert Wesarg]

When 46af44b07d (pull --rebase=<type>: allow single-letter abbreviations
for the type, 2018-08-04) landed in Git, it had the side effect that
not only 'pull --rebase=<type>' accepted the single-letter abbreviations
but also the 'pull.rebase' and 'branch.<name>.rebase' configurations.

Secondly, 'git remote rename' did not honor these single-letter
abbreviations when reading the 'branch.*.rebase' configurations.

We now document the single-letter abbreviations and both code places
share a common function to parse the values of 'git pull --rebase=*',
'pull.rebase', and 'branches.*.rebase'.

The only functional change is the handling of the `branch_info::rebase`
value. Before it was an unsigned enum, thus the truth value could be
checked with `branch_info::rebase != 0`. But `enum rebase_type` is
signed, thus the truth value must now be checked with
`branch_info::rebase >= REBASE_TRUE`.

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

---
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>

In case this is considered a BUG, then sharing the function is nevertheless
a good thing. The function could than learn a new flag, indicating whether
the single-letter abbreviations are accepted or not.
---
 Documentation/config/branch.txt |  7 ++++---
 Documentation/config/pull.txt   |  7 ++++---
 Makefile                        |  1 +
 builtin/pull.c                  | 29 ++++-------------------------
 builtin/remote.c                | 26 ++++++++------------------
 rebase.c                        | 24 ++++++++++++++++++++++++
 rebase.h                        | 15 +++++++++++++++
 7 files changed, 60 insertions(+), 49 deletions(-)
 create mode 100644 rebase.c
 create mode 100644 rebase.h

diff --git a/Documentation/config/branch.txt b/Documentation/config/branch.txt
index a592d522a7..cc5f3249fc 100644
--- a/Documentation/config/branch.txt
+++ b/Documentation/config/branch.txt
@@ -81,15 +81,16 @@ branch.<name>.rebase::
 	"git pull" is run. See "pull.rebase" for doing this in a non
 	branch-specific manner.
 +
-When `merges`, pass the `--rebase-merges` option to 'git rebase'
+When `merges` (or just 'm'), pass the `--rebase-merges` option to 'git rebase'
 so that the local merge commits are included in the rebase (see
 linkgit:git-rebase[1] for details).
 +
-When `preserve` (deprecated in favor of `merges`), also pass
+When `preserve` (or just 'p', deprecated in favor of `merges`), also pass
 `--preserve-merges` along to 'git rebase' so that locally committed merge
 commits will not be flattened by running 'git pull'.
 +
-When the value is `interactive`, the rebase is run in interactive mode.
+When the value is `interactive` (or just 'i'), the rebase is run in interactive
+mode.
 +
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
diff --git a/Documentation/config/pull.txt b/Documentation/config/pull.txt
index b87cab31b3..5404830609 100644
--- a/Documentation/config/pull.txt
+++ b/Documentation/config/pull.txt
@@ -14,15 +14,16 @@ pull.rebase::
 	pull" is run. See "branch.<name>.rebase" for setting this on a
 	per-branch basis.
 +
-When `merges`, pass the `--rebase-merges` option to 'git rebase'
+When `merges` (or just 'm'), pass the `--rebase-merges` option to 'git rebase'
 so that the local merge commits are included in the rebase (see
 linkgit:git-rebase[1] for details).
 +
-When `preserve` (deprecated in favor of `merges`), also pass
+When `preserve` (or just 'p', deprecated in favor of `merges`), also pass
 `--preserve-merges` along to 'git rebase' so that locally committed merge
 commits will not be flattened by running 'git pull'.
 +
-When the value is `interactive`, the rebase is run in interactive mode.
+When the value is `interactive` (or just 'i'), the rebase is run in interactive
+mode.
 +
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
diff --git a/Makefile b/Makefile
index 09f98b777c..96ced97bff 100644
--- a/Makefile
+++ b/Makefile
@@ -954,6 +954,7 @@ LIB_OBJS += quote.o
 LIB_OBJS += range-diff.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
+LIB_OBJS += rebase.o
 LIB_OBJS += rebase-interactive.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
diff --git a/builtin/pull.c b/builtin/pull.c
index d25ff13a60..888181c07c 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -15,6 +15,7 @@
 #include "sha1-array.h"
 #include "remote.h"
 #include "dir.h"
+#include "rebase.h"
 #include "refs.h"
 #include "refspec.h"
 #include "revision.h"
@@ -26,15 +27,6 @@
 #include "commit-reach.h"
 #include "sequencer.h"
 
-enum rebase_type {
-	REBASE_INVALID = -1,
-	REBASE_FALSE = 0,
-	REBASE_TRUE,
-	REBASE_PRESERVE,
-	REBASE_MERGES,
-	REBASE_INTERACTIVE
-};
-
 /**
  * Parses the value of --rebase. If value is a false value, returns
  * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is
@@ -45,22 +37,9 @@ enum rebase_type {
 static enum rebase_type parse_config_rebase(const char *key, const char *value,
 		int fatal)
 {
-	int v = git_parse_maybe_bool(value);
-
-	if (!v)
-		return REBASE_FALSE;
-	else if (v > 0)
-		return REBASE_TRUE;
-	else if (!strcmp(value, "preserve") || !strcmp(value, "p"))
-		return REBASE_PRESERVE;
-	else if (!strcmp(value, "merges") || !strcmp(value, "m"))
-		return REBASE_MERGES;
-	else if (!strcmp(value, "interactive") || !strcmp(value, "i"))
-		return REBASE_INTERACTIVE;
-	/*
-	 * Please update _git_config() in git-completion.bash when you
-	 * add new rebase modes.
-	 */
+	enum rebase_type v = rebase_parse_value(value);
+	if (v != REBASE_INVALID)
+		return v;
 
 	if (fatal)
 		die(_("Invalid value for %s: %s"), key, value);
diff --git a/builtin/remote.c b/builtin/remote.c
index 96bbe828fe..2830c4ab33 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -6,6 +6,7 @@
 #include "string-list.h"
 #include "strbuf.h"
 #include "run-command.h"
+#include "rebase.h"
 #include "refs.h"
 #include "refspec.h"
 #include "object-store.h"
@@ -248,9 +249,7 @@ static int add(int argc, const char **argv)
 struct branch_info {
 	char *remote_name;
 	struct string_list merge;
-	enum {
-		NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES
-	} rebase;
+	enum rebase_type rebase;
 };
 
 static struct string_list branch_list = STRING_LIST_INIT_NODUP;
@@ -305,17 +304,8 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 				space = strchr(value, ' ');
 			}
 			string_list_append(&info->merge, xstrdup(value));
-		} else {
-			int v = git_parse_maybe_bool(value);
-			if (v >= 0)
-				info->rebase = v;
-			else if (!strcmp(value, "preserve"))
-				info->rebase = NORMAL_REBASE;
-			else if (!strcmp(value, "merges"))
-				info->rebase = REBASE_MERGES;
-			else if (!strcmp(value, "interactive"))
-				info->rebase = INTERACTIVE_REBASE;
-		}
+		} else
+			info->rebase = rebase_parse_value(value);
 	}
 	return 0;
 }
@@ -943,7 +933,7 @@ static int add_local_to_show_info(struct string_list_item *branch_item, void *cb
 		return 0;
 	if ((n = strlen(branch_item->string)) > show_info->width)
 		show_info->width = n;
-	if (branch_info->rebase)
+	if (branch_info->rebase >= REBASE_TRUE)
 		show_info->any_rebase = 1;
 
 	item = string_list_insert(show_info->list, branch_item->string);
@@ -960,16 +950,16 @@ static int show_local_info_item(struct string_list_item *item, void *cb_data)
 	int width = show_info->width + 4;
 	int i;
 
-	if (branch_info->rebase && branch_info->merge.nr > 1) {
+	if (branch_info->rebase >= REBASE_TRUE && branch_info->merge.nr > 1) {
 		error(_("invalid branch.%s.merge; cannot rebase onto > 1 branch"),
 			item->string);
 		return 0;
 	}
 
 	printf("    %-*s ", show_info->width, item->string);
-	if (branch_info->rebase) {
+	if (branch_info->rebase >= REBASE_TRUE) {
 		const char *msg;
-		if (branch_info->rebase == INTERACTIVE_REBASE)
+		if (branch_info->rebase == REBASE_INTERACTIVE)
 			msg = _("rebases interactively onto remote %s");
 		else if (branch_info->rebase == REBASE_MERGES)
 			msg = _("rebases interactively (with merges) onto "
diff --git a/rebase.c b/rebase.c
new file mode 100644
index 0000000000..a9ab27205a
--- /dev/null
+++ b/rebase.c
@@ -0,0 +1,24 @@
+#include "rebase.h"
+#include "config.h"
+
+enum rebase_type rebase_parse_value(const char *value)
+{
+	int v = git_parse_maybe_bool(value);
+
+	if (!v)
+		return REBASE_FALSE;
+	else if (v > 0)
+		return REBASE_TRUE;
+	else if (!strcmp(value, "preserve") || !strcmp(value, "p"))
+		return REBASE_PRESERVE;
+	else if (!strcmp(value, "merges") || !strcmp(value, "m"))
+		return REBASE_MERGES;
+	else if (!strcmp(value, "interactive") || !strcmp(value, "i"))
+		return REBASE_INTERACTIVE;
+	/*
+	 * Please update _git_config() in git-completion.bash when you
+	 * add new rebase modes.
+	 */
+
+	return REBASE_INVALID;
+}
diff --git a/rebase.h b/rebase.h
new file mode 100644
index 0000000000..cc723d4748
--- /dev/null
+++ b/rebase.h
@@ -0,0 +1,15 @@
+#ifndef REBASE_H
+#define REBASE_H
+
+enum rebase_type {
+	REBASE_INVALID = -1,
+	REBASE_FALSE = 0,
+	REBASE_TRUE,
+	REBASE_PRESERVE,
+	REBASE_MERGES,
+	REBASE_INTERACTIVE
+};
+
+enum rebase_type rebase_parse_value(const char *value);
+
+#endif /* REBASE */
-- 
2.24.1.497.g9abd7b20b4.dirty

[PATCH 2/7] remote: clean-up by returning early to avoid one indentation

[Bert Wesarg]

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

---
Cc: Junio C Hamano <gitster@pobox.com>
---
 builtin/remote.c | 86 +++++++++++++++++++++++++-----------------------
 1 file changed, 44 insertions(+), 42 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 2830c4ab33..a8bdaca4f4 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -263,50 +263,52 @@ static const char *abbrev_ref(const char *name, const char *prefix)
 
 static int config_read_branches(const char *key, const char *value, void *cb)
 {
-	if (starts_with(key, "branch.")) {
-		const char *orig_key = key;
-		char *name;
-		struct string_list_item *item;
-		struct branch_info *info;
-		enum { 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, ".merge", &key_len)) {
-			name = xmemdupz(key, key_len);
-			type = MERGE;
-		} else if (strip_suffix(key, ".rebase", &key_len)) {
-			name = xmemdupz(key, key_len);
-			type = REBASE;
-		} else
-			return 0;
+	if (!starts_with(key, "branch."))
+		return 0;
 
-		item = string_list_insert(&branch_list, name);
+	const char *orig_key = key;
+	char *name;
+	struct string_list_item *item;
+	struct branch_info *info;
+	enum { 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, ".merge", &key_len)) {
+		name = xmemdupz(key, key_len);
+		type = MERGE;
+	} else if (strip_suffix(key, ".rebase", &key_len)) {
+		name = xmemdupz(key, key_len);
+		type = REBASE;
+	} else
+		return 0;
+
+	item = string_list_insert(&branch_list, name);
+
+	if (!item->util)
+		item->util = xcalloc(1, sizeof(struct branch_info));
+	info = item->util;
+	if (type == REMOTE) {
+		if (info->remote_name)
+			warning(_("more than one %s"), orig_key);
+		info->remote_name = xstrdup(value);
+	} else if (type == MERGE) {
+		char *space = strchr(value, ' ');
+		value = abbrev_branch(value);
+		while (space) {
+			char *merge;
+			merge = xstrndup(value, space - value);
+			string_list_append(&info->merge, merge);
+			value = abbrev_branch(space + 1);
+			space = strchr(value, ' ');
+		}
+		string_list_append(&info->merge, xstrdup(value));
+	} else
+		info->rebase = rebase_parse_value(value);
 
-		if (!item->util)
-			item->util = xcalloc(1, sizeof(struct branch_info));
-		info = item->util;
-		if (type == REMOTE) {
-			if (info->remote_name)
-				warning(_("more than one %s"), orig_key);
-			info->remote_name = xstrdup(value);
-		} else if (type == MERGE) {
-			char *space = strchr(value, ' ');
-			value = abbrev_branch(value);
-			while (space) {
-				char *merge;
-				merge = xstrndup(value, space - value);
-				string_list_append(&info->merge, merge);
-				value = abbrev_branch(space + 1);
-				space = strchr(value, ' ');
-			}
-			string_list_append(&info->merge, xstrdup(value));
-		} else
-			info->rebase = rebase_parse_value(value);
-	}
 	return 0;
 }
 
-- 
2.24.1.497.g9abd7b20b4.dirty

[PATCH 3/7] remote: clean-up config callback

[Bert Wesarg]

Some minor clean-ups in function `config_read_branches`:

 * remove hardcoded length in `key += 7`
 * call `xmemdupz` only once
 * use a switch to handle the configuration type and add a `BUG()`

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>

---
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/remote.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index a8bdaca4f4..9466e32b3d 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -273,29 +273,29 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 	enum { REMOTE, MERGE, REBASE } type;
 	size_t key_len;
 
-	key += 7;
-	if (strip_suffix(key, ".remote", &key_len)) {
-		name = xmemdupz(key, key_len);
+	key += strlen("branch.");
+	if (strip_suffix(key, ".remote", &key_len))
 		type = REMOTE;
-	} else if (strip_suffix(key, ".merge", &key_len)) {
-		name = xmemdupz(key, key_len);
+	else if (strip_suffix(key, ".merge", &key_len))
 		type = MERGE;
-	} else if (strip_suffix(key, ".rebase", &key_len)) {
-		name = xmemdupz(key, key_len);
+	else if (strip_suffix(key, ".rebase", &key_len))
 		type = REBASE;
-	} else
+	else
 		return 0;
+	name = xmemdupz(key, key_len);
 
 	item = string_list_insert(&branch_list, name);
 
 	if (!item->util)
 		item->util = xcalloc(1, sizeof(struct branch_info));
 	info = item->util;
-	if (type == REMOTE) {
+	switch (type) {
+	case REMOTE:
 		if (info->remote_name)
 			warning(_("more than one %s"), orig_key);
 		info->remote_name = xstrdup(value);
-	} else if (type == MERGE) {
+		break;
+	case MERGE: {
 		char *space = strchr(value, ' ');
 		value = abbrev_branch(value);
 		while (space) {
@@ -306,8 +306,14 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 			space = strchr(value, ' ');
 		}
 		string_list_append(&info->merge, xstrdup(value));
-	} else
+		break;
+	}
+	case REBASE:
 		info->rebase = rebase_parse_value(value);
+		break;
+	default:
+		BUG("unexpected type=%d", type);
+	}
 
 	return 0;
 }
-- 
2.24.1.497.g9abd7b20b4.dirty

[PATCH v3 4/7] remote rename: rename branch.<name>.pushRemote config values too

[Bert Wesarg]

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  | 15 ++++++++++++++-
 t/t5505-remote.sh |  4 +++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 9466e32b3d..0cb930fe00 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -250,6 +250,7 @@ struct branch_info {
 	char *remote_name;
 	struct string_list merge;
 	enum rebase_type rebase;
+	char *push_remote_name;
 };
 
 static struct string_list branch_list = STRING_LIST_INIT_NODUP;
@@ -270,7 +271,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 += strlen("branch.");
@@ -280,6 +281,8 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 		type = MERGE;
 	else if (strip_suffix(key, ".rebase", &key_len))
 		type = REBASE;
+	if (strip_suffix(key, ".pushremote", &key_len))
+		type = PUSH_REMOTE;
 	else
 		return 0;
 	name = xmemdupz(key, key_len);
@@ -311,6 +314,11 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 	case REBASE:
 		info->rebase = rebase_parse_value(value);
 		break;
+	case PUSH_REMOTE:
+		if (info->push_remote_name)
+			warning(_("more than one %s"), orig_key);
+		info->push_remote_name = xstrdup(value);
+		break;
 	default:
 		BUG("unexpected type=%d", type);
 	}
@@ -678,6 +686,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

[PATCH 5/7] [RFC] config: make `scope_name` global as `config_scope_name`

[Bert Wesarg]

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
Will be replaced by Matthew Rogers.

Cc: Matthew Rogers <mattr94@gmail.com>
---
 config.c               | 16 ++++++++++++++++
 config.h               |  1 +
 t/helper/test-config.c | 17 +----------------
 3 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/config.c b/config.c
index d75f88ca0c..4c461bb7a3 100644
--- a/config.c
+++ b/config.c
@@ -3317,6 +3317,22 @@ enum config_scope current_config_scope(void)
 		return current_parsing_scope;
 }
 
+const char *config_scope_name(enum config_scope scope)
+{
+	switch (scope) {
+	case CONFIG_SCOPE_SYSTEM:
+		return "system";
+	case CONFIG_SCOPE_GLOBAL:
+		return "global";
+	case CONFIG_SCOPE_REPO:
+		return "repo";
+	case CONFIG_SCOPE_CMDLINE:
+		return "cmdline";
+	default:
+		return "unknown";
+	}
+}
+
 int lookup_config(const char **mapping, int nr_mapping, const char *var)
 {
 	int i;
diff --git a/config.h b/config.h
index 91fd4c5e96..c063f33ff6 100644
--- a/config.h
+++ b/config.h
@@ -301,6 +301,7 @@ enum config_scope {
 	CONFIG_SCOPE_REPO,
 	CONFIG_SCOPE_CMDLINE,
 };
+const char *config_scope_name(enum config_scope scope);
 
 enum config_scope current_config_scope(void);
 const char *current_config_origin_type(void);
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 214003d5b2..1e3bc7c8f4 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -37,21 +37,6 @@
  *
  */
 
-static const char *scope_name(enum config_scope scope)
-{
-	switch (scope) {
-	case CONFIG_SCOPE_SYSTEM:
-		return "system";
-	case CONFIG_SCOPE_GLOBAL:
-		return "global";
-	case CONFIG_SCOPE_REPO:
-		return "repo";
-	case CONFIG_SCOPE_CMDLINE:
-		return "cmdline";
-	default:
-		return "unknown";
-	}
-}
 static int iterate_cb(const char *var, const char *value, void *data)
 {
 	static int nr;
@@ -63,7 +48,7 @@ static int iterate_cb(const char *var, const char *value, void *data)
 	printf("value=%s\n", value ? value : "(null)");
 	printf("origin=%s\n", current_config_origin_type());
 	printf("name=%s\n", current_config_name());
-	printf("scope=%s\n", scope_name(current_config_scope()));
+	printf("scope=%s\n", config_scope_name(current_config_scope()));
 
 	return 0;
 }
-- 
2.24.1.497.g9abd7b20b4.dirty

[PATCH 6/7] config: provide access to the current line number

[Bert Wesarg]

Users are nowadays trained to see message from CLI tools in the form

    <file>:<lno>: …

To be able to give such messages when notifying the user about
configurations in any config file, it is currently only possible to get
the file name (if the value originates from a file to begin with) via
`current_config_name()`. Now it is also possible to query the current line
number for the configuration.

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
 config.c               |  8 ++++++++
 config.h               |  1 +
 t/helper/test-config.c |  1 +
 t/t1308-config-set.sh  | 14 ++++++++++++--
 4 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 4c461bb7a3..5d1d6b5871 100644
--- a/config.c
+++ b/config.c
@@ -3333,6 +3333,14 @@ const char *config_scope_name(enum config_scope scope)
 	}
 }
 
+int current_config_line(void)
+{
+	if (current_config_kvi)
+		return current_config_kvi->linenr;
+	else
+		return cf->linenr;
+}
+
 int lookup_config(const char **mapping, int nr_mapping, const char *var)
 {
 	int i;
diff --git a/config.h b/config.h
index c063f33ff6..371f7f2dd0 100644
--- a/config.h
+++ b/config.h
@@ -306,6 +306,7 @@ const char *config_scope_name(enum config_scope scope);
 enum config_scope current_config_scope(void);
 const char *current_config_origin_type(void);
 const char *current_config_name(void);
+int current_config_line(void);
 
 /**
  * Include Directives
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 1e3bc7c8f4..234c722b48 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -48,6 +48,7 @@ static int iterate_cb(const char *var, const char *value, void *data)
 	printf("value=%s\n", value ? value : "(null)");
 	printf("origin=%s\n", current_config_origin_type());
 	printf("name=%s\n", current_config_name());
+	printf("lno=%d\n", current_config_line());
 	printf("scope=%s\n", config_scope_name(current_config_scope()));
 
 	return 0;
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 7b4e1a63eb..9e36e7a590 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -238,8 +238,8 @@ test_expect_success 'error on modifying repo config without repo' '
 
 cmdline_config="'foo.bar=from-cmdline'"
 test_expect_success 'iteration shows correct origins' '
-	echo "[foo]bar = from-repo" >.git/config &&
-	echo "[foo]bar = from-home" >.gitconfig &&
+	printf "[ignore]\n\tthis = please\n[foo]bar = from-repo\n" >.git/config &&
+	printf "[foo]\n\tbar = from-home\n" >.gitconfig &&
 	if test_have_prereq MINGW
 	then
 		# Use Windows path (i.e. *not* $HOME)
@@ -253,18 +253,28 @@ test_expect_success 'iteration shows correct origins' '
 	value=from-home
 	origin=file
 	name=$HOME_GITCONFIG
+	lno=2
 	scope=global
 
+	key=ignore.this
+	value=please
+	origin=file
+	name=.git/config
+	lno=2
+	scope=repo
+
 	key=foo.bar
 	value=from-repo
 	origin=file
 	name=.git/config
+	lno=3
 	scope=repo
 
 	key=foo.bar
 	value=from-cmdline
 	origin=command line
 	name=
+	lno=-1
 	scope=cmdline
 	EOF
 	GIT_CONFIG_PARAMETERS=$cmdline_config test-tool config iterate >actual &&
-- 
2.24.1.497.g9abd7b20b4.dirty

[PATCH 7/7] remote rename: gently handle remote.pushDefault config

[Bert Wesarg]

When renaming a remote with

    git remote rename X Y

Git already renames any branch.<name>.remote and branch.<name>.pushRemote
values from X to Y.

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 when set in a repo configuration
though.

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

Matthew, you are in Cc because of your current work 'config: allow user to
know scope of config options'. I think I'm correct to assuming an ordering
of the enum config_scope.

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

diff --git a/builtin/remote.c b/builtin/remote.c
index 0cb930fe00..52172e523a 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -611,6 +611,29 @@ static int migrate_file(struct remote *remote)
 	return 0;
 }
 
+struct push_default_info
+{
+	struct rename_info *rename;
+	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->rename->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 int mv(int argc, const char **argv)
 {
 	struct option options[] = {
@@ -746,6 +769,26 @@ static int mv(int argc, const char **argv)
 			die(_("creating '%s' failed"), buf.buf);
 	}
 	string_list_clear(&remote_branches, 1);
+
+	struct push_default_info push_default;
+	push_default.rename = &rename;
+	push_default.scope = CONFIG_SCOPE_UNKNOWN;
+	push_default.origin = &buf;
+	git_config(config_read_push_default, &push_default);
+	if (push_default.scope >= CONFIG_SCOPE_CMDLINE)
+		; /* pass */
+	else if (push_default.scope >= CONFIG_SCOPE_REPO) {
+		git_config_set("remote.pushDefault", rename.new_name);
+	} 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,
+			rename.old_name);
+	}
+
 	return 0;
 }
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 59a1681636..3b84c7bf9b 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,54 @@ 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 remote.pushDefault)" = "upstream"
+	)
+'
+
+test_expect_success 'rename a remote keeps global remote.pushDefault' '
+	git clone one four.2 &&
+	(
+		cd four.2 &&
+		test_config_global remote.pushDefault origin &&
+		git config remote.pushDefault other &&
+		git remote rename origin upstream &&
+		test "$(git config --global remote.pushDefault)" = "origin" &&
+		test "$(git config remote.pushDefault)" = "other"
+	)
+'
+
+test_expect_success 'rename a remote renames repo remote.pushDefault but ignores global' '
+	git clone one four.3 &&
+	(
+		cd four.3 &&
+		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 remote.pushDefault)" = "upstream"
+	)
+'
+
+test_expect_success 'rename a remote renames repo remote.pushDefault but keeps global' '
+	git clone one four.4 &&
+	(
+		cd four.4 &&
+		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 remote.pushDefault)" = "upstream"
 	)
 '
 
-- 
2.24.1.497.g9abd7b20b4.dirty

Re: [PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types

[Junio C Hamano]

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

> When 46af44b07d (pull --rebase=<type>: allow single-letter abbreviations
> for the type, 2018-08-04) landed in Git, it had the side effect that
> not only 'pull --rebase=<type>' accepted the single-letter abbreviations
> but also the 'pull.rebase' and 'branch.<name>.rebase' configurations.
>
> Secondly, 'git remote rename' did not honor these single-letter
> abbreviations when reading the 'branch.*.rebase' configurations.

Hmph, do you mean s/Secondly/However/ instead?

> The only functional change is the handling of the `branch_info::rebase`
> value. Before it was an unsigned enum, thus the truth value could be
> checked with `branch_info::rebase != 0`. But `enum rebase_type` is
> signed, thus the truth value must now be checked with
> `branch_info::rebase >= REBASE_TRUE`.

I think there is another hidden one, but I do not know offhand the
implications of the change.  It could well be benign.

>  /**
>   * Parses the value of --rebase. If value is a false value, returns
>   * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is
> @@ -45,22 +37,9 @@ enum rebase_type {
>  static enum rebase_type parse_config_rebase(const char *key, const char *value,
>  		int fatal)
>  {
> -	int v = git_parse_maybe_bool(value);
> -
> -	if (!v)
> -		return REBASE_FALSE;
> -	else if (v > 0)
> -		return REBASE_TRUE;
> -	else if (!strcmp(value, "preserve") || !strcmp(value, "p"))
> -		return REBASE_PRESERVE;
> -	else if (!strcmp(value, "merges") || !strcmp(value, "m"))
> -		return REBASE_MERGES;
> -	else if (!strcmp(value, "interactive") || !strcmp(value, "i"))
> -		return REBASE_INTERACTIVE;
> -	/*
> -	 * Please update _git_config() in git-completion.bash when you
> -	 * add new rebase modes.
> -	 */

I see all of the above, including the "Please update" comment, has
become rebase_parse_value(), which is very good.

> diff --git a/builtin/remote.c b/builtin/remote.c
> index 96bbe828fe..2830c4ab33 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -6,6 +6,7 @@
> ...
> -	enum {
> -		NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES
> -	} rebase;
> +	enum rebase_type rebase;

Good to see the duplicate go.

> @@ -305,17 +304,8 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>  				space = strchr(value, ' ');
>  			}
>  			string_list_append(&info->merge, xstrdup(value));
> -		} else {
> -			int v = git_parse_maybe_bool(value);
> -			if (v >= 0)
> -				info->rebase = v;
> -			else if (!strcmp(value, "preserve"))
> -				info->rebase = NORMAL_REBASE;
> -			else if (!strcmp(value, "merges"))
> -				info->rebase = REBASE_MERGES;
> -			else if (!strcmp(value, "interactive"))
> -				info->rebase = INTERACTIVE_REBASE;
> -		}
> +		} else
> +			info->rebase = rebase_parse_value(value);

Here, we never had info->rebase == REBASE_INVALID.  The field was
left intact when the configuration file had a rebase type that is
not known to this version of git.  Now it has become possible that
info->rebase to be REBASE_INVALID.  Would the code after this part
returns be prepared to handle it, and if so how?  At least I think
it deserves a comment here, or in rebase_parse_value(), to say (1)
that unknown rebase value is treated as false for most of the code
that do not need to differentiate between false and unknown, and (2)
that assigning a negative value to REBASE_INVALID and always
checking if the value is the same or greater than REBASE_TRUE helps
to maintain the convention.


> diff --git a/rebase.h b/rebase.h
> new file mode 100644
> index 0000000000..cc723d4748
> --- /dev/null
> +++ b/rebase.h
> @@ -0,0 +1,15 @@
> +#ifndef REBASE_H
> +#define REBASE_H
> +
> +enum rebase_type {
> +	REBASE_INVALID = -1,
> +	REBASE_FALSE = 0,
> +	REBASE_TRUE,
> +	REBASE_PRESERVE,
> +	REBASE_MERGES,
> +	REBASE_INTERACTIVE
> +};
> +
> +enum rebase_type rebase_parse_value(const char *value);
> +
> +#endif /* REBASE */

Re: [PATCH 5/7] [RFC] config: make `scope_name` global as `config_scope_name`

[Matt Rogers]

Logos good to me...

As I'm a bit new, what would be the best way for me to work this into
my workflow?

On Tue, Jan 21, 2020 at 4:25 AM Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> ---
> Will be replaced by Matthew Rogers.
>
> Cc: Matthew Rogers <mattr94@gmail.com>
> ---
>  config.c               | 16 ++++++++++++++++
>  config.h               |  1 +
>  t/helper/test-config.c | 17 +----------------
>  3 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/config.c b/config.c
> index d75f88ca0c..4c461bb7a3 100644
> --- a/config.c
> +++ b/config.c
> @@ -3317,6 +3317,22 @@ enum config_scope current_config_scope(void)
>                 return current_parsing_scope;
>  }
>
> +const char *config_scope_name(enum config_scope scope)
> +{
> +       switch (scope) {
> +       case CONFIG_SCOPE_SYSTEM:
> +               return "system";
> +       case CONFIG_SCOPE_GLOBAL:
> +               return "global";
> +       case CONFIG_SCOPE_REPO:
> +               return "repo";
> +       case CONFIG_SCOPE_CMDLINE:
> +               return "cmdline";
> +       default:
> +               return "unknown";
> +       }
> +}
> +
>  int lookup_config(const char **mapping, int nr_mapping, const char *var)
>  {
>         int i;
> diff --git a/config.h b/config.h
> index 91fd4c5e96..c063f33ff6 100644
> --- a/config.h
> +++ b/config.h
> @@ -301,6 +301,7 @@ enum config_scope {
>         CONFIG_SCOPE_REPO,
>         CONFIG_SCOPE_CMDLINE,
>  };
> +const char *config_scope_name(enum config_scope scope);
>
>  enum config_scope current_config_scope(void);
>  const char *current_config_origin_type(void);
> diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> index 214003d5b2..1e3bc7c8f4 100644
> --- a/t/helper/test-config.c
> +++ b/t/helper/test-config.c
> @@ -37,21 +37,6 @@
>   *
>   */
>
> -static const char *scope_name(enum config_scope scope)
> -{
> -       switch (scope) {
> -       case CONFIG_SCOPE_SYSTEM:
> -               return "system";
> -       case CONFIG_SCOPE_GLOBAL:
> -               return "global";
> -       case CONFIG_SCOPE_REPO:
> -               return "repo";
> -       case CONFIG_SCOPE_CMDLINE:
> -               return "cmdline";
> -       default:
> -               return "unknown";
> -       }
> -}
>  static int iterate_cb(const char *var, const char *value, void *data)
>  {
>         static int nr;
> @@ -63,7 +48,7 @@ static int iterate_cb(const char *var, const char *value, void *data)
>         printf("value=%s\n", value ? value : "(null)");
>         printf("origin=%s\n", current_config_origin_type());
>         printf("name=%s\n", current_config_name());
> -       printf("scope=%s\n", scope_name(current_config_scope()));
> +       printf("scope=%s\n", config_scope_name(current_config_scope()));
>
>         return 0;
>  }
> --
> 2.24.1.497.g9abd7b20b4.dirty
>


-- 
Matthew Rogers

Re: [PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types

[Bert Wesarg]

Dear Junio,

On Wed, Jan 22, 2020 at 12:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
> > When 46af44b07d (pull --rebase=<type>: allow single-letter abbreviations
> > for the type, 2018-08-04) landed in Git, it had the side effect that
> > not only 'pull --rebase=<type>' accepted the single-letter abbreviations
> > but also the 'pull.rebase' and 'branch.<name>.rebase' configurations.
> >
> > Secondly, 'git remote rename' did not honor these single-letter
> > abbreviations when reading the 'branch.*.rebase' configurations.
>
> Hmph, do you mean s/Secondly/However/ instead?

thanks, that now reads smoothly.

> > @@ -305,17 +304,8 @@ static int config_read_branches(const char *key, const char *value, void *cb)
> >                               space = strchr(value, ' ');
> >                       }
> >                       string_list_append(&info->merge, xstrdup(value));
> > -             } else {
> > -                     int v = git_parse_maybe_bool(value);
> > -                     if (v >= 0)
> > -                             info->rebase = v;
> > -                     else if (!strcmp(value, "preserve"))
> > -                             info->rebase = NORMAL_REBASE;
> > -                     else if (!strcmp(value, "merges"))
> > -                             info->rebase = REBASE_MERGES;
> > -                     else if (!strcmp(value, "interactive"))
> > -                             info->rebase = INTERACTIVE_REBASE;
> > -             }
> > +             } else
> > +                     info->rebase = rebase_parse_value(value);
>
> Here, we never had info->rebase == REBASE_INVALID.  The field was
> left intact when the configuration file had a rebase type that is
> not known to this version of git.  Now it has become possible that
> info->rebase to be REBASE_INVALID.  Would the code after this part
> returns be prepared to handle it, and if so how?  At least I think
> it deserves a comment here, or in rebase_parse_value(), to say (1)
> that unknown rebase value is treated as false for most of the code
> that do not need to differentiate between false and unknown, and (2)
> that assigning a negative value to REBASE_INVALID and always
> checking if the value is the same or greater than REBASE_TRUE helps
> to maintain the convention.

Its true that we never had 'info->rebase == REBASE_INVALID', but the
previous code also considered unknown values as false. 'info' is
allocated with 'xcalloc', thus 'info->rebase' defaults to false. Thus
it remains false.

While my change may set 'info->rebase' implicitly to 'REBASE_INVALID'
I also changed all truth value checks to '>= REBASE_TRUE'. Therefore,
(and I must admit) incidentally, I did not introduced a function
change. Both versions handle unknown '.rebase' values as false.

If this is the expected behavior, I will add a comment to the line,
with that finding. If not, I will map 'REBASE_INVALID' to
'REBASE_TRUE' in that case.

Bert

Re: [PATCH 5/7] [RFC] config: make `scope_name` global as `config_scope_name`

[Bert Wesarg]

On Wed, Jan 22, 2020 at 1:12 AM Matt Rogers <mattr94@gmail.com> wrote:
>
> Logos good to me...
>
> As I'm a bit new, what would be the best way for me to work this into
> my workflow?

if you have done that change already locally, then you can ignore my
patch. I will wait for your re-roll and put my changes on top of
yours. If not, you could replace your patch with this one in your
series. Your call.

Bert

>
> On Tue, Jan 21, 2020 at 4:25 AM Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> >
> > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> > ---
> > Will be replaced by Matthew Rogers.
> >
> > Cc: Matthew Rogers <mattr94@gmail.com>
> > ---
> >  config.c               | 16 ++++++++++++++++
> >  config.h               |  1 +
> >  t/helper/test-config.c | 17 +----------------
> >  3 files changed, 18 insertions(+), 16 deletions(-)
> >
> > diff --git a/config.c b/config.c
> > index d75f88ca0c..4c461bb7a3 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -3317,6 +3317,22 @@ enum config_scope current_config_scope(void)
> >                 return current_parsing_scope;
> >  }
> >
> > +const char *config_scope_name(enum config_scope scope)
> > +{
> > +       switch (scope) {
> > +       case CONFIG_SCOPE_SYSTEM:
> > +               return "system";
> > +       case CONFIG_SCOPE_GLOBAL:
> > +               return "global";
> > +       case CONFIG_SCOPE_REPO:
> > +               return "repo";
> > +       case CONFIG_SCOPE_CMDLINE:
> > +               return "cmdline";
> > +       default:
> > +               return "unknown";
> > +       }
> > +}
> > +
> >  int lookup_config(const char **mapping, int nr_mapping, const char *var)
> >  {
> >         int i;
> > diff --git a/config.h b/config.h
> > index 91fd4c5e96..c063f33ff6 100644
> > --- a/config.h
> > +++ b/config.h
> > @@ -301,6 +301,7 @@ enum config_scope {
> >         CONFIG_SCOPE_REPO,
> >         CONFIG_SCOPE_CMDLINE,
> >  };
> > +const char *config_scope_name(enum config_scope scope);
> >
> >  enum config_scope current_config_scope(void);
> >  const char *current_config_origin_type(void);
> > diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> > index 214003d5b2..1e3bc7c8f4 100644
> > --- a/t/helper/test-config.c
> > +++ b/t/helper/test-config.c
> > @@ -37,21 +37,6 @@
> >   *
> >   */
> >
> > -static const char *scope_name(enum config_scope scope)
> > -{
> > -       switch (scope) {
> > -       case CONFIG_SCOPE_SYSTEM:
> > -               return "system";
> > -       case CONFIG_SCOPE_GLOBAL:
> > -               return "global";
> > -       case CONFIG_SCOPE_REPO:
> > -               return "repo";
> > -       case CONFIG_SCOPE_CMDLINE:
> > -               return "cmdline";
> > -       default:
> > -               return "unknown";
> > -       }
> > -}
> >  static int iterate_cb(const char *var, const char *value, void *data)
> >  {
> >         static int nr;
> > @@ -63,7 +48,7 @@ static int iterate_cb(const char *var, const char *value, void *data)
> >         printf("value=%s\n", value ? value : "(null)");
> >         printf("origin=%s\n", current_config_origin_type());
> >         printf("name=%s\n", current_config_name());
> > -       printf("scope=%s\n", scope_name(current_config_scope()));
> > +       printf("scope=%s\n", config_scope_name(current_config_scope()));
> >
> >         return 0;
> >  }
> > --
> > 2.24.1.497.g9abd7b20b4.dirty
> >
>
>
> --
> Matthew Rogers

Re: [PATCH 0/7] remote rename: improve handling of configuration values

[Bert Wesarg]

All,

I think 'git remote remove X' needs similar improvements to
'handle.*.pushremote = X' and 'push.default = X'. Will be handled in
the re-roll.

Bert

On Tue, Jan 21, 2020 at 10:24 AM Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>
> While fixing that 'git remote rename X Y' does not rename the values for
> 'branch.*.pushRemote', it opened the possibility to more improvements in
> this area:
>
>  - 'remote rename' did not accept single-letter abbreviations for
>    'branch.*.rebase' like 'pull --rebase' does
>
>  - minor clean-ups the config callback
>
>  - patch 5 will be replaced by/rebased on Matthew's work in 'config: allow user to
>    know scope of config options', once 'config_scope_name' is available
>
>  - gently handling the rename of 'remote.pushDefault'
>
> Bert Wesarg (7):
>   pull --rebase/remote rename: document and honor single-letter
>     abbreviations rebase types
>   remote: clean-up by returning early to avoid one indentation
>   remote: clean-up config callback
>   remote rename: rename branch.<name>.pushRemote config values too
>   [RFC] config: make `scope_name` global as `config_scope_name`
>   config: provide access to the current line number
>   remote rename: gently handle remote.pushDefault config
>
>  Documentation/config/branch.txt |   7 +-
>  Documentation/config/pull.txt   |   7 +-
>  Makefile                        |   1 +
>  builtin/pull.c                  |  29 +-----
>  builtin/remote.c                | 168 +++++++++++++++++++++-----------
>  config.c                        |  24 +++++
>  config.h                        |   2 +
>  rebase.c                        |  24 +++++
>  rebase.h                        |  15 +++
>  t/helper/test-config.c          |  18 +---
>  t/t1308-config-set.sh           |  14 ++-
>  t/t5505-remote.sh               |  52 +++++++++-
>  12 files changed, 254 insertions(+), 107 deletions(-)
>  create mode 100644 rebase.c
>  create mode 100644 rebase.h
>
> --
> 2.24.1.497.g9abd7b20b4.dirty
>

Re: [PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types

[Junio C Hamano]

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

> Dear Junio,
>
> On Wed, Jan 22, 2020 at 12:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>>
>> > When 46af44b07d (pull --rebase=<type>: allow single-letter abbreviations
>> > for the type, 2018-08-04) landed in Git, it had the side effect that
>> > not only 'pull --rebase=<type>' accepted the single-letter abbreviations
>> > but also the 'pull.rebase' and 'branch.<name>.rebase' configurations.
>> >
>> > Secondly, 'git remote rename' did not honor these single-letter
>> > abbreviations when reading the 'branch.*.rebase' configurations.
>>
>> Hmph, do you mean s/Secondly/However/ instead?
>
> thanks, that now reads smoothly.
>
>> > @@ -305,17 +304,8 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>> >                               space = strchr(value, ' ');
>> >                       }
>> >                       string_list_append(&info->merge, xstrdup(value));
>> > -             } else {
>> > -                     int v = git_parse_maybe_bool(value);
>> > -                     if (v >= 0)
>> > -                             info->rebase = v;
>> > -                     else if (!strcmp(value, "preserve"))
>> > -                             info->rebase = NORMAL_REBASE;
>> > -                     else if (!strcmp(value, "merges"))
>> > -                             info->rebase = REBASE_MERGES;
>> > -                     else if (!strcmp(value, "interactive"))
>> > -                             info->rebase = INTERACTIVE_REBASE;
>> > -             }
>> > +             } else
>> > +                     info->rebase = rebase_parse_value(value);
>>
>> Here, we never had info->rebase == REBASE_INVALID.  The field was
>> left intact when the configuration file had a rebase type that is
>> not known to this version of git.  Now it has become possible that
>> info->rebase to be REBASE_INVALID.  Would the code after this part
>> returns be prepared to handle it, and if so how?  At least I think
>> it deserves a comment here, or in rebase_parse_value(), to say (1)
>> that unknown rebase value is treated as false for most of the code
>> that do not need to differentiate between false and unknown, and (2)
>> that assigning a negative value to REBASE_INVALID and always
>> checking if the value is the same or greater than REBASE_TRUE helps
>> to maintain the convention.
>
> Its true that we never had 'info->rebase == REBASE_INVALID', but the
> previous code also considered unknown values as false. 'info' is
> allocated with 'xcalloc', thus 'info->rebase' defaults to false. Thus
> it remains false.

Yes, that is why I was not opposed to the new code.  It was just
that it was not clear, without some comments I suggested in the
latter half of my paragraph you responded above, why it is correct
to unconditionally assign to info->rebase and the code the control
reaches after this part gets executed does not need any adjustment
and simply "works".

Thinking about it again, I think the two points I thought need
highlighting in the above belong to the in-code comment for the new
helper rebase_parse_value().

    *** in rebase.h ***
    enum rebase_type {
            REBASE_INVALID = -1,
            REBASE_FALSE = 0,
            REBASE_TRUE,
            REBASE_PRESERVE,
            REBASE_MERGES,
            REBASE_INTERACTIVE
    };

    /*
     * Parses textual value for pull.rebase, branch.<name>.rebase, etc.
     * Unrecognised value yields REBASE_INVALID, which traditionally is
     * treated the same way as REBASE_FALSE.
     *
     * The callers that care if (any) rebase is requested should say
     *   if (REBASE_TRUE <= rebase_parse_value(string))
     *
     * The callers that want to differenciate an unrecognised value and
     * false can do so by treating _INVALID and _FALSE differently.
     */
    enum rebase_type rebase_parse_value(const char *value);

or something like that, perhaps.

Re: [PATCH 5/7] [RFC] config: make `scope_name` global as `config_scope_name`

[Matt Rogers]

I'll just put it into my local changes, and then reroll.  I should
have it up within a couple of days at the latest.

Re: [PATCH 2/7] remote: clean-up by returning early to avoid one indentation

[Junio C Hamano]

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

> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>
> ---
> Cc: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/remote.c | 86 +++++++++++++++++++++++++-----------------------
>  1 file changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 2830c4ab33..a8bdaca4f4 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -263,50 +263,52 @@ static const char *abbrev_ref(const char *name, const char *prefix)
>  
>  static int config_read_branches(const char *key, const char *value, void *cb)
>  {
> -	if (starts_with(key, "branch.")) {
> -		const char *orig_key = key;
> -		char *name;
> -		struct string_list_item *item;
> -		struct branch_info *info;
> -		enum { 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, ".merge", &key_len)) {
> -			name = xmemdupz(key, key_len);
> -			type = MERGE;
> -		} else if (strip_suffix(key, ".rebase", &key_len)) {
> -			name = xmemdupz(key, key_len);
> -			type = REBASE;
> -		} else
> -			return 0;
> +	if (!starts_with(key, "branch."))
> +		return 0;

That's way too early.  We must have all decl/defn before the first
statement (see Documentation/CodingGuidelines).

> -		item = string_list_insert(&branch_list, name);
> +	const char *orig_key = key;
> +	char *name;
> +	struct string_list_item *item;
> +	struct branch_info *info;
> +	enum { REMOTE, MERGE, REBASE } type;
> +	size_t key_len;

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

[Junio C Hamano]

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

> @@ -746,6 +769,26 @@ static int mv(int argc, const char **argv)
>  			die(_("creating '%s' failed"), buf.buf);
>  	}
>  	string_list_clear(&remote_branches, 1);
> +
> +	struct push_default_info push_default;

Likewise.  decl-after-stmt is not allowed.

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

[Bert Wesarg]

On Fri, Jan 24, 2020 at 12:03 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
> > @@ -746,6 +769,26 @@ static int mv(int argc, const char **argv)
> >                       die(_("creating '%s' failed"), buf.buf);
> >       }
> >       string_list_clear(&remote_branches, 1);
> > +
> > +     struct push_default_info push_default;
>
> Likewise.  decl-after-stmt is not allowed.

thanks. Its time for a re-roll.

Bert