[v2,2/4] config: fix config scope enum
diff mbox series

Message ID e8e05f39407365a1bf5008820267d362e0cbffd6.1578565001.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • config: allow user to know scope of config options
Related show

Commit Message

Ralf Thielow via GitGitGadget Jan. 9, 2020, 10:16 a.m. UTC
From: Matthew Rogers <mattr94@gmail.com>

Previously when iterating through git config variables, worktree config
and local config were both considered "CONFIG_SCOPE_REPO".  This was
never a problem before as no one had needed to differentiate between the
two cases.

Additionally we rename what was CONFIG_SCOPE_REPO to CONFIG_SCOPE_LOCAL
to reflect its new, more specific meaning.

The clients of 'current_config_scope()' who cared about
CONFIG_SCOPE_REPO are also modified to similarly care about
CONFIG_SCOPE_WORKTREE and CONFIG_SCOPE_LOCAL to preserve previous behavior.

Signed-off-by: Matthew Rogers <mattr94@gmail.com>
---
 config.c               | 7 ++-----
 config.h               | 3 ++-
 remote.c               | 3 ++-
 t/helper/test-config.c | 4 +++-
 upload-pack.c          | 3 ++-
 5 files changed, 11 insertions(+), 9 deletions(-)

Comments

Junio C Hamano Jan. 9, 2020, 7:06 p.m. UTC | #1
"Matthew Rogers via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Matthew Rogers <mattr94@gmail.com>
>
> Previously when iterating through git config variables, worktree config
> and local config were both considered "CONFIG_SCOPE_REPO".  This was
> never a problem before as no one had needed to differentiate between the
> two cases.

Hmph, then "fix" on the title is a bit misleading, no?  

The enum may not have been as fine grained as you would have liked,
but if there was nothing broken, then we are doing this not to "fix"
anything.  

A more important thing to explain would probably be why we
(i.e. those who propose this change, those who give favoriable
reviews to it, and those who accept it change to the system) would
want to see finer-grained classification.  What do we expect to be
able to do with the resulting finer-grained set that we wouldn't be
able to with the original, and why is it a good thing?  That is what
readers of the proposed log message of this change would want to
see, I would think.

> Additionally we rename what was CONFIG_SCOPE_REPO to CONFIG_SCOPE_LOCAL
> to reflect its new, more specific meaning.
>
> The clients of 'current_config_scope()' who cared about
> CONFIG_SCOPE_REPO are also modified to similarly care about
> CONFIG_SCOPE_WORKTREE and CONFIG_SCOPE_LOCAL to preserve previous behavior.
>
> Signed-off-by: Matthew Rogers <mattr94@gmail.com>
> ---
>  config.c               | 7 ++-----
>  config.h               | 3 ++-
>  remote.c               | 3 ++-
>  t/helper/test-config.c | 4 +++-
>  upload-pack.c          | 3 ++-
>  5 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/config.c b/config.c
> index d75f88ca0c..447a013a15 100644
> --- a/config.c
> +++ b/config.c
> @@ -1724,15 +1724,12 @@ static int do_git_config_sequence(const struct config_options *opts,
>  	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
>  		ret += git_config_from_file(fn, user_config, data);
>  
> -	current_parsing_scope = CONFIG_SCOPE_REPO;
> +	current_parsing_scope = CONFIG_SCOPE_LOCAL;
>  	if (!opts->ignore_repo && repo_config &&
>  	    !access_or_die(repo_config, R_OK, 0))
>  		ret += git_config_from_file(fn, repo_config, data);
>  
> -	/*
> -	 * Note: this should have a new scope, CONFIG_SCOPE_WORKTREE.
> -	 * But let's not complicate things before it's actually needed.
> -	 */
> +	current_parsing_scope = CONFIG_SCOPE_WORKTREE;
>  	if (!opts->ignore_worktree && repository_format_worktree_config) {
>  		char *path = git_pathdup("config.worktree");
>  		if (!access_or_die(path, R_OK, 0))
> diff --git a/config.h b/config.h
> index 91fd4c5e96..284d92fb0e 100644
> --- a/config.h
> +++ b/config.h
> @@ -298,7 +298,8 @@ enum config_scope {
>  	CONFIG_SCOPE_UNKNOWN = 0,
>  	CONFIG_SCOPE_SYSTEM,
>  	CONFIG_SCOPE_GLOBAL,
> -	CONFIG_SCOPE_REPO,
> +	CONFIG_SCOPE_LOCAL,
> +	CONFIG_SCOPE_WORKTREE,
>  	CONFIG_SCOPE_CMDLINE,
>  };
>  
> diff --git a/remote.c b/remote.c
> index 5c4666b53a..593ce297ed 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -369,7 +369,8 @@ static int handle_config(const char *key, const char *value, void *cb)
>  	}
>  	remote = make_remote(name, namelen);
>  	remote->origin = REMOTE_CONFIG;
> -	if (current_config_scope() == CONFIG_SCOPE_REPO)
> +	if (current_config_scope() == CONFIG_SCOPE_LOCAL ||
> +	current_config_scope() == CONFIG_SCOPE_WORKTREE)
>  		remote->configured_in_repo = 1;
>  	if (!strcmp(subkey, "mirror"))
>  		remote->mirror = git_config_bool(key, value);
> diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> index 214003d5b2..6695e463eb 100644
> --- a/t/helper/test-config.c
> +++ b/t/helper/test-config.c
> @@ -44,8 +44,10 @@ static const char *scope_name(enum config_scope scope)
>  		return "system";
>  	case CONFIG_SCOPE_GLOBAL:
>  		return "global";
> -	case CONFIG_SCOPE_REPO:
> +	case CONFIG_SCOPE_LOCAL:
>  		return "repo";
> +	case CONFIG_SCOPE_WORKTREE:
> +		return "worktree";
>  	case CONFIG_SCOPE_CMDLINE:
>  		return "cmdline";
>  	default:
> diff --git a/upload-pack.c b/upload-pack.c
> index a00d7ece6b..c53249cac1 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1073,7 +1073,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
>  		precomposed_unicode = git_config_bool(var, value);
>  	}
>  
> -	if (current_config_scope() != CONFIG_SCOPE_REPO) {
> +	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
> +	current_config_scope() != CONFIG_SCOPE_WORKTREE) {
>  		if (!strcmp("uploadpack.packobjectshook", var))
>  			return git_config_string(&pack_objects_hook, var, value);
>  	}
Matt Rogers Jan. 9, 2020, 11:29 p.m. UTC | #2
On Thu, Jan 9, 2020 at 2:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Matthew Rogers via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Matthew Rogers <mattr94@gmail.com>
> >
> > Previously when iterating through git config variables, worktree config
> > and local config were both considered "CONFIG_SCOPE_REPO".  This was
> > never a problem before as no one had needed to differentiate between the
> > two cases.
>
> Hmph, then "fix" on the title is a bit misleading, no?
>
> The enum may not have been as fine grained as you would have liked,
> but if there was nothing broken, then we are doing this not to "fix"
> anything.
>

I see where you're coming from, but I would definitely consider this a "fix"
in that it's something that (as explained in the deleted comment) should
have been this way from the start but was unnecessary as no one had a
need for it yet.  But I definitely wouldn't be against changing the phrasing
to something like "clean up" or whatever your preferred wording would be.

> A more important thing to explain would probably be why we
> (i.e. those who propose this change, those who give favoriable
> reviews to it, and those who accept it change to the system) would
> want to see finer-grained classification.  What do we expect to be
> able to do with the resulting finer-grained set that we wouldn't be
> able to with the original, and why is it a good thing?  That is what
> readers of the proposed log message of this change would want to
> see, I would think.
>

This is really more prep for patch 4 later on in this series, as a user who
ran the proposed '--show-scope' later on in the series would care what
was "worktree" vs "local".

Regardless, I think the two options I have would be to amend the commit
message with that extra information or roll it together with patch 4

Patch
diff mbox series

diff --git a/config.c b/config.c
index d75f88ca0c..447a013a15 100644
--- a/config.c
+++ b/config.c
@@ -1724,15 +1724,12 @@  static int do_git_config_sequence(const struct config_options *opts,
 	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
 		ret += git_config_from_file(fn, user_config, data);
 
-	current_parsing_scope = CONFIG_SCOPE_REPO;
+	current_parsing_scope = CONFIG_SCOPE_LOCAL;
 	if (!opts->ignore_repo && repo_config &&
 	    !access_or_die(repo_config, R_OK, 0))
 		ret += git_config_from_file(fn, repo_config, data);
 
-	/*
-	 * Note: this should have a new scope, CONFIG_SCOPE_WORKTREE.
-	 * But let's not complicate things before it's actually needed.
-	 */
+	current_parsing_scope = CONFIG_SCOPE_WORKTREE;
 	if (!opts->ignore_worktree && repository_format_worktree_config) {
 		char *path = git_pathdup("config.worktree");
 		if (!access_or_die(path, R_OK, 0))
diff --git a/config.h b/config.h
index 91fd4c5e96..284d92fb0e 100644
--- a/config.h
+++ b/config.h
@@ -298,7 +298,8 @@  enum config_scope {
 	CONFIG_SCOPE_UNKNOWN = 0,
 	CONFIG_SCOPE_SYSTEM,
 	CONFIG_SCOPE_GLOBAL,
-	CONFIG_SCOPE_REPO,
+	CONFIG_SCOPE_LOCAL,
+	CONFIG_SCOPE_WORKTREE,
 	CONFIG_SCOPE_CMDLINE,
 };
 
diff --git a/remote.c b/remote.c
index 5c4666b53a..593ce297ed 100644
--- a/remote.c
+++ b/remote.c
@@ -369,7 +369,8 @@  static int handle_config(const char *key, const char *value, void *cb)
 	}
 	remote = make_remote(name, namelen);
 	remote->origin = REMOTE_CONFIG;
-	if (current_config_scope() == CONFIG_SCOPE_REPO)
+	if (current_config_scope() == CONFIG_SCOPE_LOCAL ||
+	current_config_scope() == CONFIG_SCOPE_WORKTREE)
 		remote->configured_in_repo = 1;
 	if (!strcmp(subkey, "mirror"))
 		remote->mirror = git_config_bool(key, value);
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 214003d5b2..6695e463eb 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -44,8 +44,10 @@  static const char *scope_name(enum config_scope scope)
 		return "system";
 	case CONFIG_SCOPE_GLOBAL:
 		return "global";
-	case CONFIG_SCOPE_REPO:
+	case CONFIG_SCOPE_LOCAL:
 		return "repo";
+	case CONFIG_SCOPE_WORKTREE:
+		return "worktree";
 	case CONFIG_SCOPE_CMDLINE:
 		return "cmdline";
 	default:
diff --git a/upload-pack.c b/upload-pack.c
index a00d7ece6b..c53249cac1 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1073,7 +1073,8 @@  static int upload_pack_config(const char *var, const char *value, void *unused)
 		precomposed_unicode = git_config_bool(var, value);
 	}
 
-	if (current_config_scope() != CONFIG_SCOPE_REPO) {
+	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
+	current_config_scope() != CONFIG_SCOPE_WORKTREE) {
 		if (!strcmp("uploadpack.packobjectshook", var))
 			return git_config_string(&pack_objects_hook, var, value);
 	}