threads / patch / 52672

patch, 7 partsremote rename: improve handling of configuration values

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

## tl;dr

18 messages between Jan 21, 2020 and Jan 24, 2020. Diffs are folded; open one to read it.

replies: 17people: 3as markdown or json

Bert Wesarg· Jan 21, 2020, 09:24 UTC · lore

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
Bert Wesarg· Jan 21, 2020, 09:24 UTC · re: Bert Wesarg · lore

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

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(-)
Show changes to builtin/remote.c +44 −42
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
Junio C Hamano· Jan 23, 2020, 23:02 UTC · re: Bert Wesarg · lore

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

Bert Wesarg <bert.wesarg@googlemail.com> writes:
Show 38 quoted lines
> 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).

Show 7 quoted lines
> -		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;
Bert Wesarg· Jan 21, 2020, 09:24 UTC · re: Bert Wesarg · lore

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

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
Show changes to 7 files +60 −49

Documentation/config/branch.txt, Documentation/config/pull.txt, Makefile, builtin/pull.c, builtin/remote.c, rebase.c, 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
Junio C Hamano· Jan 21, 2020, 23:26 UTC · re: Bert Wesarg · lore

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

Bert Wesarg <bert.wesarg@googlemail.com> writes:
Show 7 quoted lines
> 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?
Show 5 quoted lines
> 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.

Show 23 quoted lines
>  /**
>   * 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.

Show 10 quoted lines
> 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.
Show 17 quoted lines
> @@ -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.

Show 21 quoted lines
> 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 */
Bert Wesarg· Jan 22, 2020, 07:34 UTC · re: Junio C Hamano · lore

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

Dear Junio,
On Wed, Jan 22, 2020 at 12:26 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
>
> 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.
Show 29 quoted lines
> > @@ -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
Junio C Hamano· Jan 22, 2020, 19:43 UTC · re: Bert Wesarg · lore

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

Bert Wesarg <bert.wesarg@googlemail.com> writes:
Show 52 quoted lines
> 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.
Bert Wesarg· Jan 21, 2020, 09:24 UTC · re: Bert Wesarg · lore

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

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(-)
Show changes to builtin/remote.c +17 −11
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
Bert Wesarg· Jan 21, 2020, 09:24 UTC · re: Bert Wesarg · lore

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

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(-)
Show changes to 3 files +18 −16

config.c, config.h, t/helper/test-config.c

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
Matt Rogers· Jan 22, 2020, 00:12 UTC · re: Bert Wesarg · lore

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

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:
Show 89 quoted lines
>
> 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
Bert Wesarg· Jan 22, 2020, 07:37 UTC · re: Matt Rogers · lore

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

On Wed, Jan 22, 2020 at 1:12 AM Matt Rogers <mattr94@gmail.com> wrote:
Show 5 quoted lines
>
> 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
Show 95 quoted lines
>
> 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
Matt Rogers· Jan 23, 2020, 01:30 UTC · re: Bert Wesarg · lore

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

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.

Bert Wesarg· Jan 21, 2020, 09:24 UTC · re: Bert Wesarg · lore

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

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(-)
Show changes to 2 files +17 −2

builtin/remote.c, t/t5505-remote.sh

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
Bert Wesarg· Jan 21, 2020, 09:24 UTC · re: Bert Wesarg · lore

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

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(-)
Show changes to 2 files +92 −1

builtin/remote.c, t/t5505-remote.sh

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
Junio C Hamano· Jan 23, 2020, 23:03 UTC · re: Bert Wesarg · lore

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

Bert Wesarg <bert.wesarg@googlemail.com> writes:
Show 6 quoted lines
> @@ -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.
Bert Wesarg· Jan 24, 2020, 08:49 UTC · re: Junio C Hamano · lore

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

On Fri, Jan 24, 2020 at 12:03 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
>
> 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
Bert Wesarg· Jan 21, 2020, 09:24 UTC · re: Bert Wesarg · lore

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

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(-)
Show changes to 4 files +22 −2

config.c, config.h, t/helper/test-config.c, t/t1308-config-set.sh

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
Bert Wesarg· Jan 22, 2020, 15:26 UTC · re: Bert Wesarg · lore

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

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:
Show 44 quoted lines
>
> 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
>

← back to recent threads