[PATCH v2] config: add string mapping for enum config_scope

[PATCH v2] config: add string mapping for enum config_scope

[Emily Shaffer]

If a user is interacting with their config files primarily by the 'git
config' command, using the location flags (--global, --system, etc) then
they may be more interested to see the scope of the config file they are
editing, rather than the filepath.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---

Since v1, the only change is removing locale markers around the strings
returned from the helper.

As mentioned in lore.kernel.org/git/20191211232540.GE8464@google.com I'm
still not sure whether it's better to return "local" for
CONFIG_SCOPE_REPO. Since that's the scope returned for both local and
worktree (.git/config, .git/config.worktree) configs, I'm happy to leave
it the way it is to indicate "one of the configs in the repo".

 - Emily

 config.c | 17 +++++++++++++++++
 config.h |  1 +
 2 files changed, 18 insertions(+)

diff --git a/config.c b/config.c
index e7052b3977..baab4a916e 100644
--- a/config.c
+++ b/config.c
@@ -3312,6 +3312,23 @@ enum config_scope current_config_scope(void)
 		return current_parsing_scope;
 }
 
+const char *config_scope_to_string(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";
+	case CONFIG_SCOPE_UNKNOWN:
+	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..c8bf296dcc 100644
--- a/config.h
+++ b/config.h
@@ -303,6 +303,7 @@ enum config_scope {
 };
 
 enum config_scope current_config_scope(void);
+const char *config_scope_to_string(enum config_scope);
 const char *current_config_origin_type(void);
 const char *current_config_name(void);
 
-- 
2.24.0.525.g8f36a354ae-goog

Re: [PATCH v2] config: add string mapping for enum config_scope

[Matt Rogers]

On Wed, Dec 11, 2019 at 6:38 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> If a user is interacting with their config files primarily by the 'git
> config' command, using the location flags (--global, --system, etc) then
> they may be more interested to see the scope of the config file they are
> editing, rather than the filepath.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>
> Since v1, the only change is removing locale markers around the strings
> returned from the helper.
>
> As mentioned in lore.kernel.org/git/20191211232540.GE8464@google.com I'm
> still not sure whether it's better to return "local" for
> CONFIG_SCOPE_REPO. Since that's the scope returned for both local and
> worktree (.git/config, .git/config.worktree) configs, I'm happy to leave
> it the way it is to indicate "one of the configs in the repo".
>
>  - Emily
>
>  config.c | 17 +++++++++++++++++
>  config.h |  1 +
>  2 files changed, 18 insertions(+)
>
> diff --git a/config.c b/config.c
> index e7052b3977..baab4a916e 100644
> --- a/config.c
> +++ b/config.c
> @@ -3312,6 +3312,23 @@ enum config_scope current_config_scope(void)
>                 return current_parsing_scope;
>  }
>
> +const char *config_scope_to_string(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";
> +       case CONFIG_SCOPE_UNKNOWN:
> +       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..c8bf296dcc 100644
> --- a/config.h
> +++ b/config.h
> @@ -303,6 +303,7 @@ enum config_scope {
>  };
>
>  enum config_scope current_config_scope(void);
> +const char *config_scope_to_string(enum config_scope);
>  const char *current_config_origin_type(void);
>  const char *current_config_name(void);
>
> --
> 2.24.0.525.g8f36a354ae-goog
>

Okay Good to know, I'm using gmail right now, so the default is to
start at the top

I wouldn't really consider it a bug so much as just something that
hasn't been an issue before.  The way config.c is set up kind of makes
it hard to pass in the information that it uses the --local option, so
that may need to be refactored, since it just passes in the
appropriate file based on the --local, etc. flags to builtin\config.c
which has never really needed that information before now.  Anyways, I
don't think it needs to be addressed right now, and I'm working on
something that would address it anyways... I just need to find some
more time to work on it.

-- 
Matthew Rogers

Re: [PATCH v2] config: add string mapping for enum config_scope

[Jeff King]

On Wed, Dec 11, 2019 at 03:38:20PM -0800, Emily Shaffer wrote:

> If a user is interacting with their config files primarily by the 'git
> config' command, using the location flags (--global, --system, etc) then
> they may be more interested to see the scope of the config file they are
> editing, rather than the filepath.

I don't mind adding this in, but I think we did discuss this same
concept when we did "config --show-origin", and ended up showing the
whole path, to help with a few cases:

  - you know you're getting a value from the "system" config, but you
    don't know where that is (e.g., because the baked-in sysconfdir path
    is something you didn't expect)

  - you're in a scope like "global", but the value actually comes from
    an included file

Of course there's a flip-side, which is that showing "/etc/gitconfig"
doesn't tell you that this is the "--system" file; the user has to infer
that themselves.

There are no callers added here, so I'm not sure exactly how the new
function is meant to be used. But I'd caution that it might be worth
showing the scope _and_ the path, instead of one or the other.

-Peff

RE: [PATCH v2] config: add string mapping for enum config_scope

[mattr94]

From: Jeff King <peff@peff.net> 
Sent: Wednesday, December 11, 2019 10:10 PM
To: Emily Shaffer <emilyshaffer@google.com>
Cc: git@vger.kernel.org; Matthew Rogers <mattr94@gmail.com>; Philip Oakley <philipoakley@iee.email>
Subject: Re: [PATCH v2] config: add string mapping for enum config_scope

On Wed, Dec 11, 2019 at 03:38:20PM -0800, Emily Shaffer wrote:

> If a user is interacting with their config files primarily by the 'git 
> config' command, using the location flags (--global, --system, etc) 
> then they may be more interested to see the scope of the config file 
> they are editing, rather than the filepath.

I don't mind adding this in, but I think we did discuss this same concept when we did "config --show-origin", and ended up showing the whole path, to help with a few cases:

  - you know you're getting a value from the "system" config, but you
    don't know where that is (e.g., because the baked-in sysconfdir path
    is something you didn't expect)

  - you're in a scope like "global", but the value actually comes from
    an included file

Of course there's a flip-side, which is that showing "/etc/gitconfig"
doesn't tell you that this is the "--system" file; the user has to infer that themselves.

There are no callers added here, so I'm not sure exactly how the new function is meant to be used. But I'd caution that it might be worth showing the scope _and_ the path, instead of one or the other.

-Peff

Just to mention, that I am working on submitting a feature to expand config with a --show-scope option via gitgitgadget.  I still have some kinks to iron out before it's ready for submission, but maybe it would make sense to wait for that? Or to wait for the rest of the patches this was taken from to actually materialize?

Re: [PATCH v2] config: add string mapping for enum config_scope

[Jeff King]

On Wed, Dec 11, 2019 at 10:40:37PM -0500, mattr94@gmail.com wrote:

> Just to mention, that I am working on submitting a feature to expand
> config with a --show-scope option via gitgitgadget.

That makes sense (hopefully it can be combined with --show-origin, too,
to get all of the information).

> I still have some kinks to iron out before it's ready for submission,
> but maybe it would make sense to wait for that? Or to wait for the
> rest of the patches this was taken from to actually materialize?

In general, yes, I think it makes sense for a patch like this to come as
part of a larger topic, that all gets merged at once. That way we don't
end up with unused cruft.

It sounds like the issue is that there are two topics that use this.
The simplest thing may be to have a common topic branch that both build
on, but with a note never to merge it directly (that last part would be
done by Junio, but the submitter would want to give that guidance along
with the patch).

I _think_ that would be easy to handle with the way Junio handles the
topics, but he may correct me.

-Peff

Re: [PATCH v2] config: add string mapping for enum config_scope

[Emily Shaffer]

On Wed, Dec 11, 2019 at 10:10:03PM -0500, Jeff King wrote:
> On Wed, Dec 11, 2019 at 03:38:20PM -0800, Emily Shaffer wrote:
> 
> > If a user is interacting with their config files primarily by the 'git
> > config' command, using the location flags (--global, --system, etc) then
> > they may be more interested to see the scope of the config file they are
> > editing, rather than the filepath.
> 
> I don't mind adding this in, but I think we did discuss this same
> concept when we did "config --show-origin", and ended up showing the
> whole path, to help with a few cases:
> 
>   - you know you're getting a value from the "system" config, but you
>     don't know where that is (e.g., because the baked-in sysconfdir path
>     is something you didn't expect)
> 
>   - you're in a scope like "global", but the value actually comes from
>     an included file
> 
> Of course there's a flip-side, which is that showing "/etc/gitconfig"
> doesn't tell you that this is the "--system" file; the user has to infer
> that themselves.
> 
> There are no callers added here, so I'm not sure exactly how the new
> function is meant to be used. But I'd caution that it might be worth
> showing the scope _and_ the path, instead of one or the other.

Yeah, I hear you - I had added this originally to the config-based hooks
topic to get an output like this:

$ git hook --list pre-commit
001	global	~/foo.sh
002	local	~/bar.sh

That's a scenario where it might be handy to add the path, especially if
it's coming in via an import, sure. (For brevity I think I'd want to
turn it on via an argument.)

As I was working through the comments on v3 of git-bugreport, though, I
saw a request to add the origin of the configs - and that's a case where
I don't necessarily want someone to see, say:

[Selected Configs]
user.name (/home/emily/robot-revolution/stairclimber/.git/config) : Emily Shaffer

when I mail that bugreport to the Git list.

So, I think I hear what you're saying - use wisely - but I think it's OK
for a user to say:

  printf("%s (%s): %s = %s\n",
         current_config_name(),
	 config_scope_to_string(current_config_scope()),
	 var,
	 value);

That is, I don't think the right solution is to make
current_config_name() provide a stringification of
current_config_scope() as well.

Or, I guess we can decide that the bugreport scenario is different
enough that this helper should exist only there, and everybody else
should use current_config_name().

 - Emily

Re: [PATCH v2] config: add string mapping for enum config_scope

[Jeff King]

On Wed, Dec 11, 2019 at 07:45:47PM -0800, Emily Shaffer wrote:

> > There are no callers added here, so I'm not sure exactly how the new
> > function is meant to be used. But I'd caution that it might be worth
> > showing the scope _and_ the path, instead of one or the other.
> 
> Yeah, I hear you - I had added this originally to the config-based hooks
> topic to get an output like this:
> 
> $ git hook --list pre-commit
> 001	global	~/foo.sh
> 002	local	~/bar.sh
> 
> That's a scenario where it might be handy to add the path, especially if
> it's coming in via an import, sure. (For brevity I think I'd want to
> turn it on via an argument.)

Yeah, there I think showing the file path would be helpful.

> As I was working through the comments on v3 of git-bugreport, though, I
> saw a request to add the origin of the configs - and that's a case where
> I don't necessarily want someone to see, say:
> 
> [Selected Configs]
> user.name (/home/emily/robot-revolution/stairclimber/.git/config) : Emily Shaffer
> 
> when I mail that bugreport to the Git list.

Yeah, agreed. You'd want less information there, because we should be
sensitive to sharing user filesystem paths.

> So, I think I hear what you're saying - use wisely - but I think it's OK
> for a user to say:
> 
>   printf("%s (%s): %s = %s\n",
>          current_config_name(),
> 	 config_scope_to_string(current_config_scope()),
> 	 var,
> 	 value);
> 
> That is, I don't think the right solution is to make
> current_config_name() provide a stringification of
> current_config_scope() as well.

Yes, I think the way you're thinking about it all makes sense. We
definitely can't just change current_config_name(); its output ends up
in --show-origin, which is plumbing.

> Or, I guess we can decide that the bugreport scenario is different
> enough that this helper should exist only there, and everybody else
> should use current_config_name().

No, I think this helper makes sense in config.c. And then callers can
choose how much (and in what format) to show the various bits as
appropriate.

So this seems like the right direction.

-Peff