| Message ID | ec699bb3e64c74e6e87a20bbb5efac12a13cb077.1576631464.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config: allow user to know scope of config options | expand |
"Matthew Rogers via GitGitGadget" <gitgitgadget@gmail.com> writes: > -static int end_null; > +static int end_nul; > - OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")), > + OPT_BOOL('z', "null", &end_nul, N_("terminate values with NUL byte")), > - const char term = end_null ? '\0' : '\t'; > + const char term = end_nul ? '\0' : '\t'; > - if (end_null) > + if (end_nul) These are correct changes, but is unrelated noise in the context of the theme of the patch, no? > +static const char *scope_to_string(enum config_scope scope) { > + /* > + * --local, --global, and --system work the same as --file so there's > + * no easy way for the parser to tell the difference when it is > + * setting the scope, so we use our information about which options > + * were passed > + */ > + if (use_local_config || scope == CONFIG_SCOPE_REPO) { > + return "local"; > + } else if (use_global_config || scope == CONFIG_SCOPE_GLOBAL) { > + return "global"; > + } else if (use_system_config || scope == CONFIG_SCOPE_SYSTEM) { > + return "system"; The above is slightly tricky; --global/--system/--local are made mutually exclusive in the higher level, so if any of them is set, we do not even need to look at the "scope" to tell what kind of source we are reading from. > + } else if (given_config_source.use_stdin || > + given_config_source.blob || > + given_config_source.file || > + scope == CONFIG_SCOPE_CMDLINE) { > + return "command line"; I am not sure what the implication of saying "they came from the command line" when we read from the standard input or from a blob. > + } else { > + return "unknown"; > + } > +} In any case, the need for such logic that says "scope might not say it is REPO, but when use_local_config is true, we are doing a local config" implies that "scope" parameter the caller of this function has is not set correctly when these options are used---would that be the real bug that needs fixing, rather than getting "worked around" with a code like this? It almost makes me point fingers at config.c::config_with_options() where config_source is inspected and git_config_from_*() helpers are called without setting the current_parsing_scope. Unlike these cases, when do_git_config_sequence() is called from that function, the scope is recorded in the variable before each standard config source file is opened and read. What would we be breaking if we taught the function to set the current_parsing_scope variable correctly even when reading from the config_source? That would certainly simplify this function quite a lot, but if the other parts of the codebase relies on the current behaviour, we cannot make such a change lightly. > +static void show_config_scope(struct strbuf *buf) > +{ > + const char term = end_nul ? '\0' : '\t'; > + const char *scope = scope_to_string(current_config_scope()); > + > + strbuf_addch(buf, '('); > + if (end_nul) > + strbuf_addstr(buf, N_(scope)); > + else > + quote_c_style(scope, buf, NULL, 0); Isn't this overkill? I think this code was copied-and-pasted from the other function that needs to show an arbitrary end-user supplied data which is a pathname, so it makes perfect sense to use c-style quoting, but the token scope_to_string() returns is taken from a bounded set that doesn't require such quoting, no? > + strbuf_addch(buf, ')'); > + strbuf_addch(buf, term); > +} > +
Hi Matthew, On 18/12/2019 01:11, Matthew Rogers via GitGitGadget wrote: > From: Matthew Rogers <mattr94@gmail.com> > > Add new option --show-scope which allows a user to know what the scope > of listed config options are (local/global/system/etc.). > > Signed-off-by: Matthew Rogers <mattr94@gmail.com> > --- > builtin/config.c | 60 ++++++++++++++++++++++++++++++++++++++++------- > t/t1300-config.sh | 51 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 103 insertions(+), 8 deletions(-) Doesn't this also need a man page update as well for adding the option to the synopsis. The commit message doesn't fully highlight that the config list will often be all the users config values, so each value will be disambiguated/identified as to it's origin. Philip
Junio, >These are correct changes, but is unrelated noise in the context of >the theme of the patch, no? I think that's the case, would the recommended course of action be to move these changes into its own commit? >> + if (use_local_config || scope == CONFIG_SCOPE_REPO) { >> + return "local"; >> + } else if (use_global_config || scope == CONFIG_SCOPE_GLOBAL) { >> + return "global"; >> + } else if (use_system_config || scope == CONFIG_SCOPE_SYSTEM) { >> + return "system"; > >The above is slightly tricky; --global/--system/--local are made >mutually exclusive in the higher level, so if any of them is set, >we do not even need to look at the "scope" to tell what kind of >source we are reading from. So the way I structured these was to mirror the way other parts of this file check if we should be doing a --local, etc. and mirrored that here. This could definitely be cleaned up if we change the behavior with how --local, etc. set the current_config_scope. >> + } else if (given_config_source.use_stdin || >> + given_config_source.blob || >> + given_config_source.file || >> + scope == CONFIG_SCOPE_CMDLINE) { >> + return "command line"; > >I am not sure what the implication of saying "they came from the >command line" when we read from the standard input or from a blob. I agree with you here, I only put this as "command line" because they came from a place that was ultimately fed in from command line options (--file/--blob). I wouldn't have a problem with calling them out as their own scope ("file", "blob", "stdin"). >> + } else { >> + return "unknown"; >> + } >> +} > >In any case, the need for such logic that says "scope might not say >it is REPO, but when use_local_config is true, we are doing a local >config" implies that "scope" parameter the caller of this function >has is not set correctly when these options are used---would that be >the real bug that needs fixing, rather than getting "worked around" >with a code like this? > >It almost makes me point fingers at config.c::config_with_options() >where config_source is inspected and git_config_from_*() helpers are >called without setting the current_parsing_scope. Unlike these >cases, when do_git_config_sequence() is called from that function, >the scope is recorded in the variable before each standard config >source file is opened and read. What would we be breaking if we >taught the function to set the current_parsing_scope variable >correctly even when reading from the config_source? That would >certainly simplify this function quite a lot, but if the other parts >of the codebase relies on the current behaviour, we cannot make such >a change lightly. From what I can tell from a cursory glance. the only two clients of this function are remote.c and upload-pack.c. The usecase for remote.c mostly seems to be to determine the result of `remote_is_configured()` which (more importantly) seems to be done when that iterates through the relevant configuration options. Similarly for upload-pack.c. I don't think it would be harmful for git config --local, etc. to set that as we would normally intuit. >> +static void show_config_scope(struct strbuf *buf) >> +{ >> + const char term = end_nul ? '\0' : '\t'; >> + const char *scope = scope_to_string(current_config_scope()); >> + >> + strbuf_addch(buf, '('); >> + if (end_nul) >> + strbuf_addstr(buf, N_(scope)); >> + else >> + quote_c_style(scope, buf, NULL, 0); > >Isn't this overkill? I think this code was copied-and-pasted from >the other function that needs to show an arbitrary end-user supplied >data which is a pathname, so it makes perfect sense to use c-style >quoting, but the token scope_to_string() returns is taken from a >bounded set that doesn't require such quoting, no? Yeah, I guess that is a copy+paste mistake. I don't think its necessary since we control the input into this function, So I'll fix that. Philip, >Doesn't this also need a man page update as well for adding the option >to the synopsis. > >The commit message doesn't fully highlight that the config list will >often be all the users config values, so each value will be >disambiguated/identified as to it's origin. I'm agreed on these. So I'll look to readjust that.
On Wed, Dec 18, 2019 at 11:46:06AM -0800, Junio C Hamano wrote: > It almost makes me point fingers at config.c::config_with_options() > where config_source is inspected and git_config_from_*() helpers are > called without setting the current_parsing_scope. Unlike these > cases, when do_git_config_sequence() is called from that function, > the scope is recorded in the variable before each standard config > source file is opened and read. What would we be breaking if we > taught the function to set the current_parsing_scope variable > correctly even when reading from the config_source? That would > certainly simplify this function quite a lot, but if the other parts > of the codebase relies on the current behaviour, we cannot make such > a change lightly. As the author of the SCOPE enum, I think this is the right direction. The only users of current_config_scope() are in config callbacks (e.g., upload_pack_config() checks it for the "packobjectshook"), which you couldn't trigger via "git config" anyway. The main reason I didn't set the scope before in config_with_options() is that it's not given the scope at all; git-config resolves it to the filename. So if git_config_source grows an enum to select the type, and all that filename-resolution gets pushed down into config_with_options(), the whole thing would fall out naturally, I think. -Peff
Jeff King <peff@peff.net> writes: > ... So if git_config_source grows an enum to select the type, and > all that filename-resolution gets pushed down into config_with_options(), > the whole thing would fall out naturally, I think. Makes sense.
<mattr94@gmail.com> writes: >>These are correct changes, but is unrelated noise in the context of >>the theme of the patch, no? > > I think that's the case, would the recommended course of action be to > move these changes into its own commit? > Yup, and it generally is a good idea to make such a clean-up patch either early in the series, or as a standalone patch (with a note under three-dash lines that another topic is coming on top of the cleanup), because it would be much less likely to introduce new bugs and can be merged quicly to 'next' and then to 'master' to serve as a base to build your other changes on top.
Philip, >The commit message doesn't fully highlight that the config list will >often be all the users config values, so each value will be >disambiguated/identified as to it's origin. I don't really understand what you mean by "it's origin" here. When you say origin, do you mean in the "--show-origin" sense of "file/blob/etc." or something else? Because scope is kind of an orthogonal concept to origin in that sense as you can have files with different origins but the same scope. Thanks, Matt On Thu, Dec 19, 2019 at 12:56 PM Junio C Hamano <gitster@pobox.com> wrote: > > <mattr94@gmail.com> writes: > > >>These are correct changes, but is unrelated noise in the context of > >>the theme of the patch, no? > > > > I think that's the case, would the recommended course of action be to > > move these changes into its own commit? > > > > Yup, and it generally is a good idea to make such a clean-up patch > either early in the series, or as a standalone patch (with a note > under three-dash lines that another topic is coming on top of the > cleanup), because it would be much less likely to introduce new bugs > and can be merged quicly to 'next' and then to 'master' to serve as > a base to build your other changes on top. >
Matt Rogers <mattr94@gmail.com> writes: > Philip, > >>The commit message doesn't fully highlight that the config list will >>often be all the users config values, so each value will be >>disambiguated/identified as to it's origin. > > I don't really understand what you mean by "it's origin" here. When > you say origin, do you mean in the "--show-origin" sense of "file/blob/etc." > or something else? Because scope is kind of an orthogonal concept to origin > in that sense as you can have files with different origins but the same scope. I do not think origin and scope are orghogonal, though. Can the same file appear as the source for different configuration var-value pair in two different scopes? It is likely that you can _guess_ with high precision that given a pathname reported by --show-origin what scope it is in. It on the other hand is not so trivial given a scope to guess which exact file a var-value pair came from, I would think.\
On Fri, Dec 20, 2019 at 9:37 PM Junio C Hamano <gitster@pobox.com> wrote: > > I do not think origin and scope are orghogonal, though. Can the > same file appear as the source for different configuration var-value > pair in two different scopes? I meant orthogonal in two senses: That given the current implementation you don't need to have both options active at the same time but can have them active at both times. And that origin and scope correlate, but aren't necessarily one-for-one. For example, --show-origin lists in a known order, but it follows includes and lists the origin as the included file. so if you include a file globally which has includeif "gitdir:..." directives then it can get hairy when all your config files are structured like that. Although, to be fair I doubt that that kind of situation is normal > > It is likely that you can _guess_ with high precision that given a > pathname reported by --show-origin what scope it is in. It on the > other hand is not so trivial given a scope to guess which exact file > a var-value pair came from, I would think.\ Normally yes, but things can get complicating depending on your configuration/include situation.
Matt Rogers <mattr94@gmail.com> writes: > And that origin and scope correlate, but aren't necessarily > one-for-one. Yes, exactly. When I see "orthogonal", I expect the word describes things that do not correlate. I can see values in the option that gives scope but not path in order to write scripts that limits var-value pairs to be used (e.g. "I do not want to be affected by the per-repository values", "I do not trust settings that comes system-wide"). I also can see values in the option that gives only path when debugging the configuration file. I briefly wondered, for the purpose of such "I do want to see only those settings coming from these scopes" script, it may make more sense to have the command _filter_ the var-value pairs, instead of showing all of them with label, but that feature always exists ;-)
diff --git a/builtin/config.c b/builtin/config.c index 98d65bc0ad..9a9c2d12f2 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -29,10 +29,11 @@ static int use_worktree_config; static struct git_config_source given_config_source; static int actions, type; static char *default_value; -static int end_null; +static int end_nul; static int respect_includes_opt = -1; static struct config_options config_options; static int show_origin; +static int show_scope; #define ACTION_GET (1<<0) #define ACTION_GET_ALL (1<<1) @@ -151,10 +152,11 @@ static struct option builtin_config_options[] = { OPT_CALLBACK_VALUE(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH), OPT_CALLBACK_VALUE(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE), OPT_GROUP(N_("Other")), - OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")), + OPT_BOOL('z', "null", &end_nul, N_("terminate values with NUL byte")), OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")), OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")), + OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (system, global, local, command line)")), OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), OPT_END(), }; @@ -178,23 +180,63 @@ static void check_argc(int argc, int min, int max) static void show_config_origin(struct strbuf *buf) { - const char term = end_null ? '\0' : '\t'; + const char term = end_nul ? '\0' : '\t'; strbuf_addstr(buf, current_config_origin_type()); strbuf_addch(buf, ':'); - if (end_null) + if (end_nul) strbuf_addstr(buf, current_config_name()); else quote_c_style(current_config_name(), buf, NULL, 0); strbuf_addch(buf, term); } +static const char *scope_to_string(enum config_scope scope) { + /* + * --local, --global, and --system work the same as --file so there's + * no easy way for the parser to tell the difference when it is + * setting the scope, so we use our information about which options + * were passed + */ + if (use_local_config || scope == CONFIG_SCOPE_REPO) { + return "local"; + } else if (use_global_config || scope == CONFIG_SCOPE_GLOBAL) { + return "global"; + } else if (use_system_config || scope == CONFIG_SCOPE_SYSTEM) { + return "system"; + } else if (given_config_source.use_stdin || + given_config_source.blob || + given_config_source.file || + scope == CONFIG_SCOPE_CMDLINE) { + return "command line"; + } else { + return "unknown"; + } +} + +static void show_config_scope(struct strbuf *buf) +{ + const char term = end_nul ? '\0' : '\t'; + const char *scope = scope_to_string(current_config_scope()); + + strbuf_addch(buf, '('); + if (end_nul) + strbuf_addstr(buf, N_(scope)); + else + quote_c_style(scope, buf, NULL, 0); + strbuf_addch(buf, ')'); + strbuf_addch(buf, term); +} + static int show_all_config(const char *key_, const char *value_, void *cb) { - if (show_origin) { + if (show_origin || show_scope) { struct strbuf buf = STRBUF_INIT; - show_config_origin(&buf); - /* Use fwrite as "buf" can contain \0's if "end_null" is set. */ + if (show_scope) + show_config_scope(&buf); + if (show_origin) + show_config_origin(&buf); + /* Use fwrite as "buf" can contain \0's if "end_nul" is set. */ fwrite(buf.buf, 1, buf.len, stdout); strbuf_release(&buf); } @@ -213,6 +255,8 @@ struct strbuf_list { static int format_config(struct strbuf *buf, const char *key_, const char *value_) { + if (show_scope) + show_config_scope(buf); if (show_origin) show_config_origin(buf); if (show_keys) @@ -678,7 +722,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) config_options.git_dir = get_git_dir(); } - if (end_null) { + if (end_nul) { term = '\0'; delim = '\n'; key_delim = '\n'; diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 983a0a1583..098f305bdd 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1766,6 +1766,57 @@ test_expect_success !MINGW '--show-origin blob ref' ' test_cmp expect output ' + +test_expect_success '--show-scope with --list' ' + cat >expect <<-EOF && + (global) user.global=true + (global) user.override=global + (global) include.path=$INCLUDE_DIR/absolute.include + (global) user.absolute=include + (local) user.local=true + (local) user.override=local + (local) include.path=../include/relative.include + (local) user.relative=include + (command line) user.cmdline=true + EOF + git -c user.cmdline=true config --list --show-scope >output && + test_cmp expect output +' + +test_expect_success !MINGW '--show-scope with --blob' ' + blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") && + cat >expect <<-EOF && + (command line) user.custom=true + EOF + git config --blob=$blob --show-scope --list >output && + test_cmp expect output +' +test_expect_success '--show-scope with --local' ' + cat >expect <<-\EOF && + (local) user.local=true + (local) user.override=local + (local) include.path=../include/relative.include + EOF + git config --local --list --show-scope >output && + test_cmp expect output +' + +test_expect_success '--show-scope with --show-origin' ' + cat >expect <<-EOF && + (global) file:$HOME/.gitconfig user.global=true + (global) file:$HOME/.gitconfig user.override=global + (global) file:$HOME/.gitconfig include.path=$INCLUDE_DIR/absolute.include + (global) file:$INCLUDE_DIR/absolute.include user.absolute=include + (local) file:.git/config user.local=true + (local) file:.git/config user.override=local + (local) file:.git/config include.path=../include/relative.include + (local) file:.git/../include/relative.include user.relative=include + (command line) command line: user.cmdline=true + EOF + git -c user.cmdline=true config --list --show-origin --show-scope >output && + test_cmp expect output +' + test_expect_success '--local requires a repo' ' # we expect 128 to ensure that we do not simply # fail to find anything and return code "1"