[v2,3/4] config: clarify meaning of command line scoping

Commit Message

Linus Arver via GitGitGadget Jan. 9, 2020, 10:16 a.m. UTC

From: Matthew Rogers <[e-mail]>

CONFIG_SCOPE_CMDLINE is generally used in the code to refer to config
values passed in via the -c option.  This is a little bit too specific
as there are other methods to pass config values so that the last for a
single command (namely --file and --blob).  As the "visibility" of config
values passed by these situations is common, we unify them as having a
scope of "command" rather than "command line".

Signed-off-by: Matthew Rogers <[e-mail]>
---
 config.c               | 2 +-
 config.h               | 2 +-
 t/helper/test-config.c | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

Comments

Junio C Hamano Jan. 9, 2020, 7:13 p.m. UTC | #1

"Matthew Rogers via GitGitGadget" <[e-mail]> writes:

> From: Matthew Rogers <[e-mail]>
>
> CONFIG_SCOPE_CMDLINE is generally used in the code to refer to config
> values passed in via the -c option.  This is a little bit too specific
> as there are other methods to pass config values so that the last for a
> single command (namely --file and --blob).

Sorry, but I cannot parse this, especially around "so that the last
for ..." part.

> As the "visibility" of config
> values passed by these situations is common, we unify them as having a
> scope of "command" rather than "command line".

Is the "unification" something done by this patch?  It does not
appear to be so.  The existing code already was using CMDLINE to
call "git -c VAR=VAL" and also something else (which is not clear to
me, unfortunately, probably because I failed to parse the above
X-<), and this step renames CMDLINE to COMMAND perhaps because
CMDLINE has too strong connotation with the "-c" thing and much less
with the other thing (which is not clear to me, unfortunately) and
you found that renaming it to COMMAND would cover both cases better?

I also do not get what you meant by "visibility", but it probably is
primarily because it is not clear what "the other thing" is.



> Signed-off-by: Matthew Rogers <[e-mail]>
> ---
>  config.c               | 2 +-
>  config.h               | 2 +-
>  t/helper/test-config.c | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/config.c b/config.c
> index 447a013a15..f319a3d6a0 100644
> --- a/config.c
> +++ b/config.c
> @@ -1737,7 +1737,7 @@ static int do_git_config_sequence(const struct config_options *opts,
>  		free(path);
>  	}
>  
> -	current_parsing_scope = CONFIG_SCOPE_CMDLINE;
> +	current_parsing_scope = CONFIG_SCOPE_COMMAND;
>  	if (!opts->ignore_cmdline && git_config_from_parameters(fn, data) < 0)
>  		die(_("unable to parse command-line config"));
>  
> diff --git a/config.h b/config.h
> index 284d92fb0e..f383a71404 100644
> --- a/config.h
> +++ b/config.h
> @@ -300,7 +300,7 @@ enum config_scope {
>  	CONFIG_SCOPE_GLOBAL,
>  	CONFIG_SCOPE_LOCAL,
>  	CONFIG_SCOPE_WORKTREE,
> -	CONFIG_SCOPE_CMDLINE,
> +	CONFIG_SCOPE_COMMAND,
>  };
>  
>  enum config_scope current_config_scope(void);
> diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> index 6695e463eb..78bbb9eb98 100644
> --- a/t/helper/test-config.c
> +++ b/t/helper/test-config.c
> @@ -48,8 +48,8 @@ static const char *scope_name(enum config_scope scope)
>  		return "repo";
>  	case CONFIG_SCOPE_WORKTREE:
>  		return "worktree";
> -	case CONFIG_SCOPE_CMDLINE:
> -		return "cmdline";
> +	case CONFIG_SCOPE_COMMAND:
> +		return "command";
>  	default:
>  		return "unknown";
>  	}

Matt Rogers Jan. 9, 2020, 11:41 p.m. UTC | #2

On Thu, Jan 9, 2020 at 2:14 PM Junio C Hamano <[e-mail]> wrote:
>
> "Matthew Rogers via GitGitGadget" <[e-mail]> writes:
>
> > From: Matthew Rogers <[e-mail]>
> >
> > CONFIG_SCOPE_CMDLINE is generally used in the code to refer to config
> > values passed in via the -c option.  This is a little bit too specific
> > as there are other methods to pass config values so that the last for a
> > single command (namely --file and --blob).
>
> Sorry, but I cannot parse this, especially around "so that the last
> for ..." part.
>

My bad, I guess I must have not read as carefully as I thought I did.
It should read:
"... there are other methods to pass config values so that _they_ last
for a single command ..."

> > As the "visibility" of config
> > values passed by these situations is common, we unify them as having a
> > scope of "command" rather than "command line".
>
> Is the "unification" something done by this patch?  It does not
> appear to be so.  The existing code already was using CMDLINE to
> call "git -c VAR=VAL" and also something else (which is not clear to
> me, unfortunately, probably because I failed to parse the above
> X-<), and this step renames CMDLINE to COMMAND perhaps because
> CMDLINE has too strong connotation with the "-c" thing and much less
> with the other thing (which is not clear to me, unfortunately) and
> you found that renaming it to COMMAND would cover both cases better?

Essentially, the "unification" was referring to more the unification of all the
things that affect configuration only for the duration of a specific command.

The gist of this patch was to say "There are a few ways besides -c to pass
a configuration option that lasts for a single command, so it makes sense
to broaden that scope.".  The change is definitely justified in that COMMAND
communicates that much clearer than CMDLINE as REPO, GLOBAL, SYSTEM
all describe the thing that can see the configuration options, and
it's specifically the
command the can see the -c options and not the command line as a whole.

>
> I also do not get what you meant by "visibility", but it probably is
> primarily because it is not clear what "the other thing" is.

I was using visibility as another way to conceptualize scoping.  A
scope more or less determines who can "see" a thing, so maybe that
language was a little bit too much in my head.

The issue is that currently the code doesn't care about any of that (as only -c
options actually have COMMAND scoping), so maybe I should roll it into the
next patch of the series?  As that introduces code that actually cares about the
difference?

Patch

diff --git a/config.c b/config.c
index 447a013a15..f319a3d6a0 100644
--- a/config.c
+++ b/config.c
@@ -1737,7 +1737,7 @@  static int do_git_config_sequence(const struct config_options *opts,
 		free(path);
 	}
 
-	current_parsing_scope = CONFIG_SCOPE_CMDLINE;
+	current_parsing_scope = CONFIG_SCOPE_COMMAND;
 	if (!opts->ignore_cmdline && git_config_from_parameters(fn, data) < 0)
 		die(_("unable to parse command-line config"));
 
diff --git a/config.h b/config.h
index 284d92fb0e..f383a71404 100644
--- a/config.h
+++ b/config.h
@@ -300,7 +300,7 @@  enum config_scope {
 	CONFIG_SCOPE_GLOBAL,
 	CONFIG_SCOPE_LOCAL,
 	CONFIG_SCOPE_WORKTREE,
-	CONFIG_SCOPE_CMDLINE,
+	CONFIG_SCOPE_COMMAND,
 };
 
 enum config_scope current_config_scope(void);
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 6695e463eb..78bbb9eb98 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -48,8 +48,8 @@  static const char *scope_name(enum config_scope scope)
 		return "repo";
 	case CONFIG_SCOPE_WORKTREE:
 		return "worktree";
-	case CONFIG_SCOPE_CMDLINE:
-		return "cmdline";
+	case CONFIG_SCOPE_COMMAND:
+		return "command";
 	default:
 		return "unknown";
 	}