| Message ID | 61ff3c64b5bdec4aaca71caa63efc23888713315.1579275102.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: > +--show-scope:: > + Similar to `--show-origin` in that it augments the output of > + all queried config options with the scope of that value > + (local, global, system, command). > + > --get-colorbool name [stdout-is-tty]:: > > Find the color setting for `name` (e.g. `color.diff`) and output > diff --git a/builtin/config.c b/builtin/config.c > index 52a904cfb1..d5931061e8 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -33,6 +33,7 @@ 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) > @@ -155,6 +156,7 @@ static struct option builtin_config_options[] = { > 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 (worktree, local, global, system, command)")), > OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), > OPT_END(), > }; > @@ -189,11 +191,43 @@ static void show_config_origin(struct strbuf *buf) > strbuf_addch(buf, term); > } > > +static const char *scope_to_string(enum config_scope scope) > +{ > + switch (scope) { > + case CONFIG_SCOPE_LOCAL: > + return "local"; > + case CONFIG_SCOPE_GLOBAL: > + return "global"; > + case CONFIG_SCOPE_SYSTEM: > + return "system"; > + case CONFIG_SCOPE_WORKTREE: > + return "worktree"; > + case CONFIG_SCOPE_COMMAND: > + return "command"; > + case CONFIG_SCOPE_SUBMODULE: > + return "submodule"; > + default: > + return "unknown"; > + } > +} It is a shame that with this defined in the main part of the system test-tool still needs to carry its own private copy. I wonder if it results in a better system if we rename this to const char *config_scope_name(enum config_scope scope) made it externally visible, and use it in t/helper/test-config.c and get rid of the private copy scope_name() there? > diff --git a/config.h b/config.h > index f383a71404..91f851e925 100644 > --- a/config.h > +++ b/config.h > @@ -35,10 +35,21 @@ struct object_id; > > #define CONFIG_REGEX_NONE ((void *)1) > > +enum config_scope { > + CONFIG_SCOPE_UNKNOWN = 0, > + CONFIG_SCOPE_SYSTEM, > + CONFIG_SCOPE_GLOBAL, > + CONFIG_SCOPE_LOCAL, > + CONFIG_SCOPE_WORKTREE, > + CONFIG_SCOPE_COMMAND, > + CONFIG_SCOPE_SUBMODULE, > +}; > + And this is the logical place to make an external definition, if we were to go that route. > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > index 983a0a1583..b654a6d2f9 100755 > --- a/t/t1300-config.sh > +++ b/t/t1300-config.sh > @@ -1766,6 +1766,64 @@ 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 user.cmdline=true > + EOF The HERE-DOC is over-indented. All the body lines should align with cat and EOF. The same comment applies to many "<<-" added by this patch---mimick the "<<-" that appear in earlier part of the same file. > +test_expect_success !MINGW '--show-scope with --blob' ' > + blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") && > + cat >expect <<-EOF && > + command user.custom=true > + EOF > + git config --blob=$blob --show-scope --list >output && > + test_cmp expect output > +' Missing blank line between two tests. I wonder if we want to revamp the tests for --show-origin that wants to make sure a funny filename is quoted properly. For that purpose, CUSTOM_CONFIG_FILE is given a funny pathname, and it would have been OK to use that only for a single "do we quote properly?" test, but instead we use that same funnily-named file as the source in many places where we do not *care* how --show-origin quotes the pathname at all, and end up having to skip with !MINGW. It's a bad tradition that started at 45bf3297 ("t1300: fix the new --show-origin tests on Windows", 2016-03-23), I guess, and is not a new problem introduced by this patch, but it should stop so that we can have a better test coverage everywhere. Other than that, I think that the new feature added by this series is a sensible one. Thanks for working on it.
On Fri, Jan 17, 2020 at 10:21 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Matthew Rogers via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > +--show-scope:: > > + Similar to `--show-origin` in that it augments the output of > > + all queried config options with the scope of that value > > + (local, global, system, command). > > + > > --get-colorbool name [stdout-is-tty]:: > > > > Find the color setting for `name` (e.g. `color.diff`) and output > > diff --git a/builtin/config.c b/builtin/config.c > > index 52a904cfb1..d5931061e8 100644 > > --- a/builtin/config.c > > +++ b/builtin/config.c > > @@ -33,6 +33,7 @@ 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) > > @@ -155,6 +156,7 @@ static struct option builtin_config_options[] = { > > 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 (worktree, local, global, system, command)")), > > OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), > > OPT_END(), > > }; > > @@ -189,11 +191,43 @@ static void show_config_origin(struct strbuf *buf) > > strbuf_addch(buf, term); > > } > > > > +static const char *scope_to_string(enum config_scope scope) > > +{ > > + switch (scope) { > > + case CONFIG_SCOPE_LOCAL: > > + return "local"; > > + case CONFIG_SCOPE_GLOBAL: > > + return "global"; > > + case CONFIG_SCOPE_SYSTEM: > > + return "system"; > > + case CONFIG_SCOPE_WORKTREE: > > + return "worktree"; > > + case CONFIG_SCOPE_COMMAND: > > + return "command"; > > + case CONFIG_SCOPE_SUBMODULE: > > + return "submodule"; > > + default: > > + return "unknown"; > > + } > > +} > > It is a shame that with this defined in the main part of the system > test-tool still needs to carry its own private copy. > > I wonder if it results in a better system if we rename this to > > const char *config_scope_name(enum config_scope scope) > > made it externally visible, and use it in t/helper/test-config.c > and get rid of the private copy scope_name() there? > +1 and please keep the order in config_scope_name the same as in the enum definition. Will base my 'remote rename & branch.<name>.pushRemote' series on this then. Bert > > diff --git a/config.h b/config.h > > index f383a71404..91f851e925 100644 > > --- a/config.h > > +++ b/config.h > > @@ -35,10 +35,21 @@ struct object_id; > > > > #define CONFIG_REGEX_NONE ((void *)1) > > > > +enum config_scope { > > + CONFIG_SCOPE_UNKNOWN = 0, > > + CONFIG_SCOPE_SYSTEM, > > + CONFIG_SCOPE_GLOBAL, > > + CONFIG_SCOPE_LOCAL, > > + CONFIG_SCOPE_WORKTREE, > > + CONFIG_SCOPE_COMMAND, > > + CONFIG_SCOPE_SUBMODULE, > > +}; > > + > > And this is the logical place to make an external definition, if we > were to go that route. > > > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > > index 983a0a1583..b654a6d2f9 100755 > > --- a/t/t1300-config.sh > > +++ b/t/t1300-config.sh > > @@ -1766,6 +1766,64 @@ 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 user.cmdline=true > > + EOF > > The HERE-DOC is over-indented. All the body lines should align with > cat and EOF. The same comment applies to many "<<-" added by this > patch---mimick the "<<-" that appear in earlier part of the same > file. > > > +test_expect_success !MINGW '--show-scope with --blob' ' > > + blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") && > > + cat >expect <<-EOF && > > + command user.custom=true > > + EOF > > + git config --blob=$blob --show-scope --list >output && > > + test_cmp expect output > > +' > > Missing blank line between two tests. > > I wonder if we want to revamp the tests for --show-origin that wants > to make sure a funny filename is quoted properly. For that purpose, > CUSTOM_CONFIG_FILE is given a funny pathname, and it would have been > OK to use that only for a single "do we quote properly?" test, but > instead we use that same funnily-named file as the source in many > places where we do not *care* how --show-origin quotes the pathname > at all, and end up having to skip with !MINGW. > > It's a bad tradition that started at 45bf3297 ("t1300: fix the new > --show-origin tests on Windows", 2016-03-23), I guess, and is not a > new problem introduced by this patch, but it should stop so that we > can have a better test coverage everywhere. > > Other than that, I think that the new feature added by this series > is a sensible one. Thanks for working on it. >
> The HERE-DOC is over-indented. All the body lines should align with > cat and EOF. The same comment applies to many "<<-" added by this > patch---mimick the "<<-" that appear in earlier part of the same > file. > I was mimicking the style of the --show-origin tests, and didn't realize this was a style violation. This is done in a few tests, so I'll add a patch that just goes through the file and fixes that for the existing tests as well. > I wonder if we want to revamp the tests for --show-origin that wants > to make sure a funny filename is quoted properly. For that purpose, > CUSTOM_CONFIG_FILE is given a funny pathname, and it would have been > OK to use that only for a single "do we quote properly?" test, but > instead we use that same funnily-named file as the source in many > places where we do not *care* how --show-origin quotes the pathname > at all, and end up having to skip with !MINGW. > > It's a bad tradition that started at 45bf3297 ("t1300: fix the new > --show-origin tests on Windows", 2016-03-23), I guess, and is not a > new problem introduced by this patch, but it should stop so that we > can have a better test coverage everywhere. > Agreed. > Other than that, I think that the new feature added by this series > is a sensible one. Thanks for working on it. > You're welcome! And thank you for the careful review.
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 899e92a1c9..2e47765aab 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -9,18 +9,18 @@ git-config - Get and set repository or global options SYNOPSIS -------- [verse] -'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] name [value [value_regex]] +'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]] 'git config' [<file-option>] [--type=<type>] --add name value 'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex] -'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get name [value_regex] -'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get-all name [value_regex] -'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex] +'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get name [value_regex] +'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get-all name [value_regex] +'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value_regex] 'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL 'git config' [<file-option>] --unset name [value_regex] 'git config' [<file-option>] --unset-all name [value_regex] 'git config' [<file-option>] --rename-section old_name new_name 'git config' [<file-option>] --remove-section name -'git config' [<file-option>] [--show-origin] [-z|--null] [--name-only] -l | --list +'git config' [<file-option>] [--show-origin] [--show-scope] [-z|--null] [--name-only] -l | --list 'git config' [<file-option>] --get-color name [default] 'git config' [<file-option>] --get-colorbool name [stdout-is-tty] 'git config' [<file-option>] -e | --edit @@ -222,6 +222,11 @@ Valid `<type>`'s include: the actual origin (config file path, ref, or blob id if applicable). +--show-scope:: + Similar to `--show-origin` in that it augments the output of + all queried config options with the scope of that value + (local, global, system, command). + --get-colorbool name [stdout-is-tty]:: Find the color setting for `name` (e.g. `color.diff`) and output diff --git a/builtin/config.c b/builtin/config.c index 52a904cfb1..d5931061e8 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -33,6 +33,7 @@ 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) @@ -155,6 +156,7 @@ static struct option builtin_config_options[] = { 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 (worktree, local, global, system, command)")), OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), OPT_END(), }; @@ -189,11 +191,43 @@ static void show_config_origin(struct strbuf *buf) strbuf_addch(buf, term); } +static const char *scope_to_string(enum config_scope scope) +{ + switch (scope) { + case CONFIG_SCOPE_LOCAL: + return "local"; + case CONFIG_SCOPE_GLOBAL: + return "global"; + case CONFIG_SCOPE_SYSTEM: + return "system"; + case CONFIG_SCOPE_WORKTREE: + return "worktree"; + case CONFIG_SCOPE_COMMAND: + return "command"; + case CONFIG_SCOPE_SUBMODULE: + return "submodule"; + default: + 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_addstr(buf, N_(scope)); + 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); + if (show_scope) + show_config_scope(&buf); + if (show_origin) + show_config_origin(&buf); /* Use fwrite as "buf" can contain \0's if "end_null" is set. */ fwrite(buf.buf, 1, buf.len, stdout); strbuf_release(&buf); @@ -213,6 +247,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) @@ -622,6 +658,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) !strcmp(given_config_source.file, "-")) { given_config_source.file = NULL; given_config_source.use_stdin = 1; + given_config_source.scope = CONFIG_SCOPE_COMMAND; } if (use_global_config) { @@ -637,6 +674,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) */ die(_("$HOME not set")); + given_config_source.scope = CONFIG_SCOPE_GLOBAL; + if (access_or_warn(user_config, R_OK, 0) && xdg_config && !access_or_warn(xdg_config, R_OK, 0)) { given_config_source.file = xdg_config; @@ -646,11 +685,13 @@ int cmd_config(int argc, const char **argv, const char *prefix) free(xdg_config); } } - else if (use_system_config) + else if (use_system_config) { given_config_source.file = git_etc_gitconfig(); - else if (use_local_config) + given_config_source.scope = CONFIG_SCOPE_SYSTEM; + } else if (use_local_config) { given_config_source.file = git_pathdup("config"); - else if (use_worktree_config) { + given_config_source.scope = CONFIG_SCOPE_LOCAL; + } else if (use_worktree_config) { struct worktree **worktrees = get_worktrees(0); if (repository_format_worktree_config) given_config_source.file = git_pathdup("config.worktree"); @@ -662,13 +703,18 @@ int cmd_config(int argc, const char **argv, const char *prefix) "section in \"git help worktree\" for details")); else given_config_source.file = git_pathdup("config"); + given_config_source.scope = CONFIG_SCOPE_LOCAL; free_worktrees(worktrees); } else if (given_config_source.file) { if (!is_absolute_path(given_config_source.file) && prefix) given_config_source.file = prefix_filename(prefix, given_config_source.file); + given_config_source.scope = CONFIG_SCOPE_COMMAND; + } else if (given_config_source.blob) { + given_config_source.scope = CONFIG_SCOPE_COMMAND; } + if (respect_includes_opt == -1) config_options.respect_includes = !given_config_source.file; else diff --git a/config.c b/config.c index f319a3d6a0..035fc8fb29 100644 --- a/config.c +++ b/config.c @@ -1702,6 +1702,7 @@ static int do_git_config_sequence(const struct config_options *opts, char *xdg_config = xdg_config_home("config"); char *user_config = expand_user_path("~/.gitconfig", 0); char *repo_config; + enum config_scope prev_parsing_scope = current_parsing_scope; if (opts->commondir) repo_config = mkpathdup("%s/config", opts->commondir); @@ -1741,7 +1742,7 @@ static int do_git_config_sequence(const struct config_options *opts, if (!opts->ignore_cmdline && git_config_from_parameters(fn, data) < 0) die(_("unable to parse command-line config")); - current_parsing_scope = CONFIG_SCOPE_UNKNOWN; + current_parsing_scope = prev_parsing_scope; free(xdg_config); free(user_config); free(repo_config); @@ -1762,6 +1763,9 @@ int config_with_options(config_fn_t fn, void *data, data = &inc; } + if (config_source) + current_parsing_scope = config_source->scope; + /* * If we have a specific filename, use it. Otherwise, follow the * regular lookup sequence. diff --git a/config.h b/config.h index f383a71404..91f851e925 100644 --- a/config.h +++ b/config.h @@ -35,10 +35,21 @@ struct object_id; #define CONFIG_REGEX_NONE ((void *)1) +enum config_scope { + CONFIG_SCOPE_UNKNOWN = 0, + CONFIG_SCOPE_SYSTEM, + CONFIG_SCOPE_GLOBAL, + CONFIG_SCOPE_LOCAL, + CONFIG_SCOPE_WORKTREE, + CONFIG_SCOPE_COMMAND, + CONFIG_SCOPE_SUBMODULE, +}; + struct git_config_source { unsigned int use_stdin:1; const char *file; const char *blob; + enum config_scope scope; }; enum config_origin_type { @@ -294,15 +305,6 @@ int config_error_nonbool(const char *); int git_config_parse_parameter(const char *, config_fn_t fn, void *data); -enum config_scope { - CONFIG_SCOPE_UNKNOWN = 0, - CONFIG_SCOPE_SYSTEM, - CONFIG_SCOPE_GLOBAL, - CONFIG_SCOPE_LOCAL, - CONFIG_SCOPE_WORKTREE, - CONFIG_SCOPE_COMMAND, -}; - enum config_scope current_config_scope(void); const char *current_config_origin_type(void); const char *current_config_name(void); diff --git a/submodule-config.c b/submodule-config.c index 85064810b2..b8e97d8ae8 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -635,7 +635,9 @@ static void submodule_cache_check_init(struct repository *repo) static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data) { if (repo->worktree) { - struct git_config_source config_source = { 0 }; + struct git_config_source config_source = { + 0, .scope = CONFIG_SCOPE_SUBMODULE + }; const struct config_options opts = { 0 }; struct object_id oid; char *file; diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 78bbb9eb98..c93bee15c3 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -48,6 +48,8 @@ static const char *scope_name(enum config_scope scope) return "repo"; case CONFIG_SCOPE_WORKTREE: return "worktree"; + case CONFIG_SCOPE_SUBMODULE: + return "submodule"; case CONFIG_SCOPE_COMMAND: return "command"; default: diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 983a0a1583..b654a6d2f9 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1766,6 +1766,64 @@ 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 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 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 getting a single value' ' + cat >expect <<-\EOF && + local true + EOF + git config --show-scope --get user.local >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 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"