| Message ID | 20191211233820.185153-1-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] config: add string mapping for enum config_scope | expand |
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.
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
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?
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
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
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
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);
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(+)