# [PATCH v5 1/8] doc: grep: unify info on configuration variables 30 messages from 2020-09-02 to 2021-02-10. Participants: Matheus Tavares, Eric Sunshine, Matheus Tavares Bernardino, Jonathan Nieder, Junio C Hamano, Elijah Newren. Thread: https://gitlist.dev/t/54177 ## Matheus Tavares, 2020-09-02 06:17 Subject: [PATCH v5 1/8] doc: grep: unify info on configuration variables Message-ID: <70c9a4e7415f532c2aa6fdd460a927b5e677cd97.1599026986.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/70c9a4e7415f532c2aa6fdd460a927b5e677cd97.1599026986.git.matheus.bernardino%40usp.br In-Reply-To: ``` Explanations about the configuration variables for git-grep are duplicated in "Documentation/git-grep.txt" and "Documentation/config/grep.txt", which can make maintenance difficult. The first also contains a definition not present in the latter (grep.fullName). To avoid problems like this, let's unify the information in the second file and include it in the first. Signed-off-by: Matheus Tavares --- Documentation/config/grep.txt | 10 ++++++++-- Documentation/git-grep.txt | 36 ++++++----------------------------- 2 files changed, 14 insertions(+), 32 deletions(-) diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt index 44abe45a7c..dd51db38e1 100644 --- a/Documentation/config/grep.txt +++ b/Documentation/config/grep.txt @@ -16,8 +16,14 @@ grep.extendedRegexp:: other than 'default'. grep.threads:: - Number of grep worker threads to use. - See `grep.threads` in linkgit:git-grep[1] for more information. + Number of grep worker threads to use. See `--threads` +ifndef::git-grep[] + in linkgit:git-grep[1] +endif::git-grep[] + for more information. + +grep.fullName:: + If set to true, enable `--full-name` option by default. grep.fallbackToNoIndex:: If set to true, fall back to git grep --no-index if git grep diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt index a7f9bc99ea..9bdf807584 100644 --- a/Documentation/git-grep.txt +++ b/Documentation/git-grep.txt @@ -41,34 +41,8 @@ characters. An empty string as search expression matches all lines. CONFIGURATION ------------- -grep.lineNumber:: - If set to true, enable `-n` option by default. - -grep.column:: - If set to true, enable the `--column` option by default. - -grep.patternType:: - Set the default matching behavior. Using a value of 'basic', 'extended', - 'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`, - `--fixed-strings`, or `--perl-regexp` option accordingly, while the - value 'default' will return to the default matching behavior. - -grep.extendedRegexp:: - If set to true, enable `--extended-regexp` option by default. This - option is ignored when the `grep.patternType` option is set to a value - other than 'default'. - -grep.threads:: - Number of grep worker threads to use. If unset (or set to 0), Git will - use as many threads as the number of logical cores available. - -grep.fullName:: - If set to true, enable `--full-name` option by default. - -grep.fallbackToNoIndex:: - If set to true, fall back to git grep --no-index if git grep - is executed outside of a git repository. Defaults to false. - +:git-grep: 1 +include::config/grep.txt[] OPTIONS ------- @@ -269,8 +243,10 @@ providing this option will cause it to die. found. --threads :: - Number of grep worker threads to use. - See `grep.threads` in 'CONFIGURATION' for more information. + Number of grep worker threads to use. If not provided (or set to + 0), Git will use as many worker threads as the number of logical + cores available. The default value can also be set with the + `grep.threads` configuration. -f :: Read patterns from , one per line. -- 2.28.0 ``` ## Matheus Tavares, 2020-09-02 06:17 Subject: [PATCH v5 0/8] grep: honor sparse checkout and add option to ignore it Message-ID: URL: https://gitlist.dev/e/cover.1599026986.git.matheus.bernardino%40usp.br In-Reply-To: ``` This series makes git-grep restrict its output to the sparsity patterns when requested by the user. A new global option is added to control this behavior in grep and hopefully more commands in the future. There are also a couple fixes in t/helper/test-config and in a test that uses it. Changes since v4: - Rebased on top of master to use repo_config_get_string_tmp(), added in jk/leakfix, in t/helper/test-config (patch 6). - Added patch 2, to make sure a test that relies on test-config checks its output in addition to the exit code, to avoid false positives. - Split patch "t/helper/test-config: return exit codes consistently" into three separated ones, as these are in fact three non-related changes: "t/helper/test-config: unify exit labels" "t/helper/test-config: check argc before accessing argv" "t/helper/test-config: be consistent with exit codes" - Removed TC_USAGE_ERROR in favor of calling die(). Also removed the test_config_exit_code enum. - On "config: correctly read worktree configs in submodules": * Improved commit message to focus on the problem instead of the implementation and remove section about the grep example. * Made use of the parse_options API * Allocated subrepo struct in the stack instead of malloc()'ing. Matheus Tavares (8): doc: grep: unify info on configuration variables t1308-config-set: avoid false positives when using test-config t/helper/test-config: be consistent with exit codes t/helper/test-config: check argc before accessing argv t/helper/test-config: unify exit labels config: correctly read worktree configs in submodules grep: honor sparse checkout patterns config: add setting to ignore sparsity patterns in some cmds Documentation/config.txt | 2 + Documentation/config/grep.txt | 18 +- Documentation/config/sparse.txt | 20 ++ Documentation/git-grep.txt | 36 +-- Documentation/git.txt | 4 + Makefile | 1 + builtin/grep.c | 134 ++++++++++- config.c | 21 +- contrib/completion/git-completion.bash | 2 + git.c | 5 + sparse-checkout.c | 18 ++ sparse-checkout.h | 11 + t/helper/test-config.c | 126 ++++++---- t/t1308-config-set.sh | 8 +- t/t2404-worktree-config.sh | 16 ++ t/t7011-skip-worktree-reading.sh | 9 - t/t7817-grep-sparse-checkout.sh | 321 +++++++++++++++++++++++++ t/t9902-completion.sh | 4 +- 18 files changed, 652 insertions(+), 104 deletions(-) create mode 100644 Documentation/config/sparse.txt create mode 100644 sparse-checkout.c create mode 100644 sparse-checkout.h create mode 100755 t/t7817-grep-sparse-checkout.sh Range-diff against v4: 1: fc47a96bfa = 1: 70c9a4e741 doc: grep: unify info on configuration variables -: ---------- > 2: f53782f14c t1308-config-set: avoid false positives when using test-config -: ---------- > 3: 85e1588d6c t/helper/test-config: be consistent with exit codes -: ---------- > 4: 0750191342 t/helper/test-config: check argc before accessing argv 2: 874aab36dd ! 5: 56535b0e36 t/helper/test-config: return exit codes consistently @@ Metadata Author: Matheus Tavares ## Commit message ## - t/helper/test-config: return exit codes consistently + t/helper/test-config: unify exit labels - The test-config helper may exit with a variety of at least four - different codes, to reflect the status of the requested operations. - These codes are sometimes checked in the tests, but not all of the codes - are returned consistently by the helper: 1 will usually refer to a - "value not found", but usage errors can also return 1 or 128. Moreover, - 128 is also expected on errors within the configset functions. These - inconsistent uses of the exit codes can lead to false positives in the - tests. Although all tests which expect errors and check the helper's - exit code currently also check the output, it's still better to - standardize the exit codes and avoid future problems in new tests. - While we are here, let's also check that we have the expected argc for - configset_get_value and configset_get_value_multi, before trying to use - argv. - - Note: this change is implemented with the unification of the exit - labels. This might seem unnecessary, for now, but it will benefit the - next patch, which will increase the cleanup section. + test-config's main function has three different exit labels, all of + which have to perform the same cleanup code before returning. Unify the + labels in preparation for a future patch which will increase the cleanup + section. Signed-off-by: Matheus Tavares ## t/helper/test-config.c ## -@@ - * iterate -> iterate over all values using git_config(), and print some - * data for each - * -+ * Exit codes: -+ * 0: success -+ * 1: value not found for the given config key -+ * 2: config file path given as argument is inaccessible or doesn't exist -+ * 129: test-config usage error -+ * -+ * Note: tests may also expect 128 for die() calls in the config machinery. -+ * - * Examples: - * - * To print the value with highest priority for key "foo.bAr Baz.rock": @@ t/helper/test-config.c: static int early_config_cb(const char *var, const char *value, void *vdata) return 0; } -+enum test_config_exit_code { -+ TC_SUCCESS = 0, -+ TC_VALUE_NOT_FOUND = 1, -+ TC_CONFIG_FILE_ERROR = 2, -+ TC_USAGE_ERROR = 129, -+}; ++#define TC_VALUE_NOT_FOUND 1 ++#define TC_CONFIG_FILE_ERROR 2 + int cmd__config(int argc, const char **argv) { - int i, val; +- int i, val; ++ int i, val, ret = 0; const char *v; const struct string_list *strptr; struct config_set cs; -+ enum test_config_exit_code ret = TC_SUCCESS; if (argc == 3 && !strcmp(argv[1], "read_early_config")) { read_early_config(early_config_cb, (void *)argv[2]); - return 0; -+ return TC_SUCCESS; ++ return ret; } setup_git_directory(); - - git_configset_init(&cs); - -- if (argc < 2) { -- fprintf(stderr, "Please, provide a command name on the command-line\n"); -- goto exit1; -- } else if (argc == 3 && !strcmp(argv[1], "get_value")) { -+ if (argc < 2) -+ goto print_usage_error; -+ -+ if (argc == 3 && !strcmp(argv[1], "get_value")) { - if (!git_config_get_value(argv[2], &v)) { - if (!v) +@@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) printf("(NULL)\n"); else printf("%s\n", v); @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) - goto exit1; + ret = TC_VALUE_NOT_FOUND; } -- } else if (!strcmp(argv[1], "configset_get_value")) { -+ } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { + } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) - goto exit1; + ret = TC_VALUE_NOT_FOUND; } -- } else if (!strcmp(argv[1], "configset_get_value_multi")) { -+ } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) { + } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) { for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) git_config(iterate_cb, NULL); - goto exit0; + } else { -+print_usage_error: -+ fprintf(stderr, "Invalid syntax. Usage: test-tool config [args]\n"); -+ ret = TC_USAGE_ERROR; ++ die("%s: Please check the syntax and the function name", argv[0]); } - die("%s: Please check the syntax and the function name", argv[0]); 3: c5093099f3 < -: ---------- t/helper/test-config: facilitate addition of new cli options 4: b510de0de0 ! 6: 3e02e1bd24 config: correctly read worktree configs in submodules @@ Metadata ## Commit message ## config: correctly read worktree configs in submodules - One of the steps in do_git_config_sequence() is to load the - worktree-specific config file. Although the function receives a git_dir - string, it relies on git_pathdup(), which uses the_repository->git_dir, - to make the path to the file. Furthermore, it also checks that - extensions.worktreeConfig is set through the - repository_format_worktree_config variable, which refers to - the_repository only. Thus, when a submodule has worktree-specific - settings, a command executed in the superproject that recurses into the - submodule won't find the said settings. + The config machinery is not able to read worktree configs from a + submodule in a process where the_repository represents the superproject. + Furthermore, when extensions.worktreeConfig is set on the superproject, + querying for a worktree config in a submodule will, instead, return + the value set at the superproject. - This will be especially important in the next patch: git-grep will learn - to honor sparse checkouts and, when running with --recurse-submodules, - the submodule's sparse checkout settings must be loaded. As these - settings are stored in the config.worktree file, they would be ignored - without this patch. So let's fix this by reading the right - config.worktree file and extensions.worktreeConfig setting, based on the - git_dir and commondir paths given to do_git_config_sequence(). Also - add a test to avoid any regressions. + The problem resides in do_git_config_sequence(). Although the function + receives a git_dir string, it uses the_repository->git_dir when making + the path to the worktree config file. And when checking if + extensions.worktreeConfig is set, it uses the global + repository_format_worktree_config variable, which refers to + the_repository only. So let's fix this by using the git_dir given to the + function and reading the extension value from the right place. Also add + a test to avoid any regressions. Signed-off-by: Matheus Tavares @@ t/helper/test-config.c #include "config.h" #include "string-list.h" +#include "submodule-config.h" ++#include "parse-options.h" /* * This program exposes the C API of the configuration mechanism @@ t/helper/test-config.c * * get_value -> prints the value with highest priority for the entered key * -@@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) +@@ t/helper/test-config.c: static int early_config_cb(const char *var, const char *value, void *vdata) + #define TC_VALUE_NOT_FOUND 1 + #define TC_CONFIG_FILE_ERROR 2 + ++static const char *test_config_usage[] = { ++ "test-tool config [--submodule=] []", ++ NULL ++}; ++ + int cmd__config(int argc, const char **argv) + { + int i, val, ret = 0; + const char *v; const struct string_list *strptr; - struct config_set cs = { .hash_initialized = 0 }; - enum test_config_exit_code ret = TC_SUCCESS; -+ struct repository *repo = the_repository; + struct config_set cs; ++ struct repository subrepo, *repo = the_repository; + const char *subrepo_path = NULL; - - argc--; /* skip over "config" */ - argv++; -@@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) - if (argc == 0) - goto print_usage_error; - -+ if (skip_prefix(*argv, "--submodule=", &subrepo_path)) { -+ argc--; -+ argv++; -+ if (argc == 0) -+ goto print_usage_error; -+ } + - if (argc == 2 && !strcmp(argv[0], "read_early_config")) { -+ if (subrepo_path) { -+ fprintf(stderr, "Cannot use --submodule with read_early_config\n"); -+ return TC_USAGE_ERROR; -+ } - read_early_config(early_config_cb, (void *)argv[1]); - return TC_SUCCESS; ++ struct option options[] = { ++ OPT_STRING(0, "submodule", &subrepo_path, "path", ++ "run on the submodule at "), ++ OPT_END() ++ }; ++ ++ argc = parse_options(argc, argv, NULL, options, test_config_usage, ++ PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_STOP_AT_NON_OPTION); ++ if (argc < 2) ++ die("Please, provide a command name on the command-line"); + + if (argc == 3 && !strcmp(argv[1], "read_early_config")) { ++ if (subrepo_path) ++ die("cannot use --submodule with read_early_config"); + read_early_config(early_config_cb, (void *)argv[2]); + return ret; } @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) - setup_git_directory(); + git_configset_init(&cs); +- if (argc < 2) +- die("Please, provide a command name on the command-line"); + if (subrepo_path) { + const struct submodule *sub; -+ struct repository *subrepo = xcalloc(1, sizeof(*repo)); + + sub = submodule_from_path(the_repository, &null_oid, subrepo_path); -+ if (!sub || repo_submodule_init(subrepo, the_repository, sub)) { -+ fprintf(stderr, "Invalid argument to --submodule: '%s'\n", -+ subrepo_path); -+ free(subrepo); -+ ret = TC_USAGE_ERROR; -+ goto out; -+ } -+ repo = subrepo; -+ } ++ if (!sub || repo_submodule_init(&subrepo, the_repository, sub)) ++ die("invalid argument to --submodule: '%s'", subrepo_path); + - if (argc == 2 && !strcmp(argv[0], "get_value")) { -- if (!git_config_get_value(argv[1], &v)) { -+ if (!repo_config_get_value(repo, argv[1], &v)) { ++ repo = &subrepo; ++ } + + if (argc == 3 && !strcmp(argv[1], "get_value")) { +- if (!git_config_get_value(argv[2], &v)) { ++ if (!repo_config_get_value(repo, argv[2], &v)) { if (!v) printf("(NULL)\n"); else @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } - } else if (argc == 2 && !strcmp(argv[0], "get_value_multi")) { -- strptr = git_config_get_value_multi(argv[1]); -+ strptr = repo_config_get_value_multi(repo, argv[1]); + } else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) { +- strptr = git_config_get_value_multi(argv[2]); ++ strptr = repo_config_get_value_multi(repo, argv[2]); if (strptr) { for (i = 0; i < strptr->nr; i++) { v = strptr->items[i].string; @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } - } else if (argc == 2 && !strcmp(argv[0], "get_int")) { -- if (!git_config_get_int(argv[1], &val)) { -+ if (!repo_config_get_int(repo, argv[1], &val)) { + } else if (argc == 3 && !strcmp(argv[1], "get_int")) { +- if (!git_config_get_int(argv[2], &val)) { ++ if (!repo_config_get_int(repo, argv[2], &val)) { printf("%d\n", val); } else { - printf("Value not found for \"%s\"\n", argv[1]); + printf("Value not found for \"%s\"\n", argv[2]); ret = TC_VALUE_NOT_FOUND; } - } else if (argc == 2 && !strcmp(argv[0], "get_bool")) { -- if (!git_config_get_bool(argv[1], &val)) { -+ if (!repo_config_get_bool(repo, argv[1], &val)) { + } else if (argc == 3 && !strcmp(argv[1], "get_bool")) { +- if (!git_config_get_bool(argv[2], &val)) { ++ if (!repo_config_get_bool(repo, argv[2], &val)) { printf("%d\n", val); } else { + - printf("Value not found for \"%s\"\n", argv[1]); + printf("Value not found for \"%s\"\n", argv[2]); ret = TC_VALUE_NOT_FOUND; } - } else if (argc == 2 && !strcmp(argv[0], "get_string")) { -- if (!git_config_get_string_tmp(argv[1], &v)) { -+ if (!repo_config_get_string_tmp(repo, argv[1], &v)) { + } else if (argc == 3 && !strcmp(argv[1], "get_string")) { +- if (!git_config_get_string_tmp(argv[2], &v)) { ++ if (!repo_config_get_string_tmp(repo, argv[2], &v)) { printf("%s\n", v); } else { - printf("Value not found for \"%s\"\n", argv[1]); + printf("Value not found for \"%s\"\n", argv[2]); ret = TC_VALUE_NOT_FOUND; } - } else if (argc >= 2 && !strcmp(argv[0], "configset_get_value")) { -+ if (subrepo_path) { -+ fprintf(stderr, "Cannot use --submodule with configset_get_value\n"); -+ ret = TC_USAGE_ERROR; -+ goto out; -+ } - for (i = 2; i < argc; i++) { + } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { ++ if (subrepo_path) ++ die("cannot use --submodule with configset_get_value"); ++ + for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } - } else if (argc >= 2 && !strcmp(argv[0], "configset_get_value_multi")) { -+ if (subrepo_path) { -+ fprintf(stderr, "Cannot use --submodule with configset_get_value_multi\n"); -+ ret = TC_USAGE_ERROR; -+ goto out; -+ } - for (i = 2; i < argc; i++) { + } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) { ++ if (subrepo_path) ++ die("cannot use --submodule with configset_get_value_multi"); ++ + for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } - } else if (!strcmp(argv[0], "iterate")) { + } else if (!strcmp(argv[1], "iterate")) { - git_config(iterate_cb, NULL); + repo_config(repo, iterate_cb, NULL); } else { - print_usage_error: -- fprintf(stderr, "Invalid syntax. Usage: test-tool config [args]\n"); -+ fprintf(stderr, "Invalid syntax. Usage: test-tool config" -+ " [--submodule=] [args]\n"); - ret = TC_USAGE_ERROR; + die("%s: Please check the syntax and the function name", argv[0]); } out: git_configset_clear(&cs); -+ if (repo != the_repository) { ++ if (repo != the_repository) + repo_clear(repo); -+ free(repo); -+ } return ret; } 5: 6d9720abf5 = 7: 902556a7b6 grep: honor sparse checkout patterns 6: affb931d35 = 8: 70e7d7b90c config: add setting to ignore sparsity patterns in some cmds -- 2.28.0 ``` ## Matheus Tavares, 2020-09-02 06:17 Subject: [PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config Message-ID: URL: https://gitlist.dev/e/f53782f14c5f53da5d5537b369a810a94f9ce184.1599026986.git.matheus.bernardino%40usp.br In-Reply-To: ``` One test in t1308 expects test-config to fail with exit code 128 due to a parsing error in the config machinery. But test-config might also exit with 128 for any other reason that leads it to call die(). Therefore the test can potentially succeed for the wrong reason. To avoid false positives, let's check test-config's output, in addition to the exit code, and make sure that the cause of the error is the one we expect in this test. Moreover, the test was using the auxiliary function check_config which optionally takes a string to compare the test-config stdout against. Because this string is optional, there is a risk that future callers may also check only the exit code and not the output. To avoid that, make the string parameter of this function mandatory. Signed-off-by: Matheus Tavares --- t/t1308-config-set.sh | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index 3a527e3a84..cff17120dc 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -14,10 +14,7 @@ check_config () { expect_code=0 fi && op=$1 key=$2 && shift && shift && - if test $# != 0 - then - printf "%s\n" "$@" - fi >expect && + printf "%s\n" "$@" >expect && test_expect_code $expect_code test-tool config "$op" "$key" >actual && test_cmp expect actual } @@ -130,7 +127,8 @@ test_expect_success 'check line error when NULL string is queried' ' ' test_expect_success 'find integer if value is non parse-able' ' - check_config expect_code 128 get_int lamb.head + test_expect_code 128 test-tool config get_int lamb.head 2>result && + test_i18ngrep "fatal: bad numeric config value '\'none\'' for '\'lamb.head\''" result ' test_expect_success 'find bool value for the entered key' ' -- 2.28.0 ``` ## Matheus Tavares, 2020-09-02 06:17 Subject: [PATCH v5 3/8] t/helper/test-config: be consistent with exit codes Message-ID: <85e1588d6c5431c32fe0262671df1d247d98b83e.1599026986.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/85e1588d6c5431c32fe0262671df1d247d98b83e.1599026986.git.matheus.bernardino%40usp.br In-Reply-To: ``` The test-config helper can return at least three different exit codes to reflect the status of the requested operation. And these codes are checked in some of the tests. But there is an inconsistent place in the helper where an usage error returns the same code as a "value not found" error. Let's fix that and, while we are here, document the meaning of each exit code in the file's header. Signed-off-by: Matheus Tavares --- t/helper/test-config.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/t/helper/test-config.c b/t/helper/test-config.c index a6e936721f..9e9d50099a 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -30,6 +30,11 @@ * iterate -> iterate over all values using git_config(), and print some * data for each * + * Exit codes: + * 0: success + * 1: value not found for the given config key + * 2: config file path given as argument is inaccessible or doesn't exist + * * Examples: * * To print the value with highest priority for key "foo.bAr Baz.rock": @@ -80,10 +85,10 @@ int cmd__config(int argc, const char **argv) git_configset_init(&cs); - if (argc < 2) { - fprintf(stderr, "Please, provide a command name on the command-line\n"); - goto exit1; - } else if (argc == 3 && !strcmp(argv[1], "get_value")) { + if (argc < 2) + die("Please, provide a command name on the command-line"); + + if (argc == 3 && !strcmp(argv[1], "get_value")) { if (!git_config_get_value(argv[2], &v)) { if (!v) printf("(NULL)\n"); -- 2.28.0 ``` ## Matheus Tavares, 2020-09-02 06:17 Subject: [PATCH v5 4/8] t/helper/test-config: check argc before accessing argv Message-ID: <0750191342754bcca398c6fdad522616b0f3fbc3.1599026986.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/0750191342754bcca398c6fdad522616b0f3fbc3.1599026986.git.matheus.bernardino%40usp.br In-Reply-To: ``` Check that we have the expected argc in 'configset_get_value' and 'configset_get_value_multi' before trying to access argv elements. Signed-off-by: Matheus Tavares --- t/helper/test-config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 9e9d50099a..26d9c2ac4c 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -138,7 +138,7 @@ int cmd__config(int argc, const char **argv) printf("Value not found for \"%s\"\n", argv[2]); goto exit1; } - } else if (!strcmp(argv[1], "configset_get_value")) { + } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { @@ -156,7 +156,7 @@ int cmd__config(int argc, const char **argv) printf("Value not found for \"%s\"\n", argv[2]); goto exit1; } - } else if (!strcmp(argv[1], "configset_get_value_multi")) { + } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) { for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { -- 2.28.0 ``` ## Matheus Tavares, 2020-09-02 06:17 Subject: [PATCH v5 5/8] t/helper/test-config: unify exit labels Message-ID: <56535b0e36e94dc73aa570f4f3c0466305c6432f.1599026986.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/56535b0e36e94dc73aa570f4f3c0466305c6432f.1599026986.git.matheus.bernardino%40usp.br In-Reply-To: ``` test-config's main function has three different exit labels, all of which have to perform the same cleanup code before returning. Unify the labels in preparation for the next patch which will increase the cleanup section. Signed-off-by: Matheus Tavares --- t/helper/test-config.c | 51 +++++++++++++++++------------------------- 1 file changed, 20 insertions(+), 31 deletions(-) diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 26d9c2ac4c..8fe43e9775 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -69,16 +69,19 @@ static int early_config_cb(const char *var, const char *value, void *vdata) return 0; } +#define TC_VALUE_NOT_FOUND 1 +#define TC_CONFIG_FILE_ERROR 2 + int cmd__config(int argc, const char **argv) { - int i, val; + int i, val, ret = 0; const char *v; const struct string_list *strptr; struct config_set cs; if (argc == 3 && !strcmp(argv[1], "read_early_config")) { read_early_config(early_config_cb, (void *)argv[2]); - return 0; + return ret; } setup_git_directory(); @@ -94,10 +97,9 @@ int cmd__config(int argc, const char **argv) printf("(NULL)\n"); else printf("%s\n", v); - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) { strptr = git_config_get_value_multi(argv[2]); @@ -109,41 +111,38 @@ int cmd__config(int argc, const char **argv) else printf("%s\n", v); } - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_int")) { if (!git_config_get_int(argv[2], &val)) { printf("%d\n", val); - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_bool")) { if (!git_config_get_bool(argv[2], &val)) { printf("%d\n", val); - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_string")) { if (!git_config_get_string_tmp(argv[2], &v)) { printf("%s\n", v); - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { fprintf(stderr, "Error (%d) reading configuration file %s.\n", err, argv[i]); - goto exit2; + ret = TC_CONFIG_FILE_ERROR; + goto out; } } if (!git_configset_get_value(&cs, argv[2], &v)) { @@ -151,17 +150,17 @@ int cmd__config(int argc, const char **argv) printf("(NULL)\n"); else printf("%s\n", v); - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) { for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { fprintf(stderr, "Error (%d) reading configuration file %s.\n", err, argv[i]); - goto exit2; + ret = TC_CONFIG_FILE_ERROR; + goto out; } } strptr = git_configset_get_value_multi(&cs, argv[2]); @@ -173,27 +172,17 @@ int cmd__config(int argc, const char **argv) else printf("%s\n", v); } - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (!strcmp(argv[1], "iterate")) { git_config(iterate_cb, NULL); - goto exit0; + } else { + die("%s: Please check the syntax and the function name", argv[0]); } - die("%s: Please check the syntax and the function name", argv[0]); - -exit0: - git_configset_clear(&cs); - return 0; - -exit1: - git_configset_clear(&cs); - return 1; - -exit2: +out: git_configset_clear(&cs); - return 2; + return ret; } -- 2.28.0 ``` ## Matheus Tavares, 2020-09-02 06:17 Subject: [PATCH v5 6/8] config: correctly read worktree configs in submodules Message-ID: <3e02e1bd248438e0b435a19d857432edcaa15a2c.1599026986.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/3e02e1bd248438e0b435a19d857432edcaa15a2c.1599026986.git.matheus.bernardino%40usp.br In-Reply-To: ``` The config machinery is not able to read worktree configs from a submodule in a process where the_repository represents the superproject. Furthermore, when extensions.worktreeConfig is set on the superproject, querying for a worktree config in a submodule will, instead, return the value set at the superproject. The problem resides in do_git_config_sequence(). Although the function receives a git_dir string, it uses the_repository->git_dir when making the path to the worktree config file. And when checking if extensions.worktreeConfig is set, it uses the global repository_format_worktree_config variable, which refers to the_repository only. So let's fix this by using the git_dir given to the function and reading the extension value from the right place. Also add a test to avoid any regressions. Signed-off-by: Matheus Tavares --- config.c | 21 ++++++++++--- t/helper/test-config.c | 62 ++++++++++++++++++++++++++++++++------ t/t2404-worktree-config.sh | 16 ++++++++++ 3 files changed, 85 insertions(+), 14 deletions(-) diff --git a/config.c b/config.c index 2bdff4457b..e1e7fab6dc 100644 --- a/config.c +++ b/config.c @@ -1747,11 +1747,22 @@ static int do_git_config_sequence(const struct config_options *opts, ret += git_config_from_file(fn, repo_config, data); current_parsing_scope = CONFIG_SCOPE_WORKTREE; - if (!opts->ignore_worktree && repository_format_worktree_config) { - char *path = git_pathdup("config.worktree"); - if (!access_or_die(path, R_OK, 0)) - ret += git_config_from_file(fn, path, data); - free(path); + if (!opts->ignore_worktree && repo_config && opts->git_dir) { + struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT; + struct strbuf buf = STRBUF_INIT; + + read_repository_format(&repo_fmt, repo_config); + + if (!verify_repository_format(&repo_fmt, &buf) && + repo_fmt.worktree_config) { + char *path = mkpathdup("%s/config.worktree", opts->git_dir); + if (!access_or_die(path, R_OK, 0)) + ret += git_config_from_file(fn, path, data); + free(path); + } + + strbuf_release(&buf); + clear_repository_format(&repo_fmt); } current_parsing_scope = CONFIG_SCOPE_COMMAND; diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 8fe43e9775..2924c09c21 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -2,12 +2,20 @@ #include "cache.h" #include "config.h" #include "string-list.h" +#include "submodule-config.h" +#include "parse-options.h" /* * This program exposes the C API of the configuration mechanism * as a set of simple commands in order to facilitate testing. * - * Reads stdin and prints result of command to stdout: + * Usage: test-tool config [--submodule=] [] + * + * If --submodule= is given, will operate on the submodule at the + * given . This option is not valid for the commands: read_early_config, + * configset_get_value and configset_get_value_multi. + * + * Possible cmds are: * * get_value -> prints the value with highest priority for the entered key * @@ -72,14 +80,34 @@ static int early_config_cb(const char *var, const char *value, void *vdata) #define TC_VALUE_NOT_FOUND 1 #define TC_CONFIG_FILE_ERROR 2 +static const char *test_config_usage[] = { + "test-tool config [--submodule=] []", + NULL +}; + int cmd__config(int argc, const char **argv) { int i, val, ret = 0; const char *v; const struct string_list *strptr; struct config_set cs; + struct repository subrepo, *repo = the_repository; + const char *subrepo_path = NULL; + + struct option options[] = { + OPT_STRING(0, "submodule", &subrepo_path, "path", + "run on the submodule at "), + OPT_END() + }; + + argc = parse_options(argc, argv, NULL, options, test_config_usage, + PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_STOP_AT_NON_OPTION); + if (argc < 2) + die("Please, provide a command name on the command-line"); if (argc == 3 && !strcmp(argv[1], "read_early_config")) { + if (subrepo_path) + die("cannot use --submodule with read_early_config"); read_early_config(early_config_cb, (void *)argv[2]); return ret; } @@ -88,11 +116,18 @@ int cmd__config(int argc, const char **argv) git_configset_init(&cs); - if (argc < 2) - die("Please, provide a command name on the command-line"); + if (subrepo_path) { + const struct submodule *sub; + + sub = submodule_from_path(the_repository, &null_oid, subrepo_path); + if (!sub || repo_submodule_init(&subrepo, the_repository, sub)) + die("invalid argument to --submodule: '%s'", subrepo_path); + + repo = &subrepo; + } if (argc == 3 && !strcmp(argv[1], "get_value")) { - if (!git_config_get_value(argv[2], &v)) { + if (!repo_config_get_value(repo, argv[2], &v)) { if (!v) printf("(NULL)\n"); else @@ -102,7 +137,7 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) { - strptr = git_config_get_value_multi(argv[2]); + strptr = repo_config_get_value_multi(repo, argv[2]); if (strptr) { for (i = 0; i < strptr->nr; i++) { v = strptr->items[i].string; @@ -116,27 +151,31 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_int")) { - if (!git_config_get_int(argv[2], &val)) { + if (!repo_config_get_int(repo, argv[2], &val)) { printf("%d\n", val); } else { printf("Value not found for \"%s\"\n", argv[2]); ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_bool")) { - if (!git_config_get_bool(argv[2], &val)) { + if (!repo_config_get_bool(repo, argv[2], &val)) { printf("%d\n", val); } else { + printf("Value not found for \"%s\"\n", argv[2]); ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_string")) { - if (!git_config_get_string_tmp(argv[2], &v)) { + if (!repo_config_get_string_tmp(repo, argv[2], &v)) { printf("%s\n", v); } else { printf("Value not found for \"%s\"\n", argv[2]); ret = TC_VALUE_NOT_FOUND; } } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { + if (subrepo_path) + die("cannot use --submodule with configset_get_value"); + for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { @@ -155,6 +194,9 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) { + if (subrepo_path) + die("cannot use --submodule with configset_get_value_multi"); + for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { @@ -177,12 +219,14 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (!strcmp(argv[1], "iterate")) { - git_config(iterate_cb, NULL); + repo_config(repo, iterate_cb, NULL); } else { die("%s: Please check the syntax and the function name", argv[0]); } out: git_configset_clear(&cs); + if (repo != the_repository) + repo_clear(repo); return ret; } diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh index 9536d10919..1e32c93735 100755 --- a/t/t2404-worktree-config.sh +++ b/t/t2404-worktree-config.sh @@ -78,4 +78,20 @@ test_expect_success 'config.worktree no longer read without extension' ' test_cmp_config -C wt2 shared this.is ' +test_expect_success 'correctly read config.worktree from submodules' ' + test_unconfig extensions.worktreeConfig && + git init sub && + ( + cd sub && + test_commit A && + git config extensions.worktreeConfig true && + git config --worktree wtconfig.sub test-value + ) && + git submodule add ./sub && + git commit -m "add sub" && + echo test-value >expect && + test-tool config --submodule=sub get_value wtconfig.sub >actual && + test_cmp expect actual +' + test_done -- 2.28.0 ``` ## Matheus Tavares, 2020-09-02 06:17 Subject: [PATCH v5 7/8] grep: honor sparse checkout patterns Message-ID: <902556a7b60974539ea2ce390bfd8f161d998270.1599026986.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/902556a7b60974539ea2ce390bfd8f161d998270.1599026986.git.matheus.bernardino%40usp.br In-Reply-To: ``` One of the main uses for a sparse checkout is to allow users to focus on the subset of files in a repository in which they are interested. But git-grep currently ignores the sparsity patterns and reports all matches found outside this subset, which kind of goes in the opposite direction. There are some use cases for ignoring the sparsity patterns and the next commit will add an option to obtain this behavior, but here we start by making grep honor the sparsity boundaries in every case where this is relevant: - git grep in worktree - git grep --cached - git grep $REVISION For the worktree and cached cases, we iterate over paths without the SKIP_WORKTREE bit set, and limit our searches to these paths. For the $REVISION case, we limit the paths we search to those that match the sparsity patterns. (We do not check the SKIP_WORKTREE bit for the $REVISION case, because $REVISION may contain paths that do not exist in HEAD and thus for which we have no SKIP_WORKTREE bit to consult. The sparsity patterns tell us how the SKIP_WORKTREE bit would be set if we were to check out $REVISION, so we consult those. Also, we don't use the sparsity patterns with the worktree or cached cases, both because we have a bit we can check directly and more efficiently, and because unmerged entries from a merge or a rebase could cause more files to temporarily be present than the sparsity patterns would normally select.) Note that there is a special case here: `git grep $TREE`. In this case, we cannot know whether $TREE corresponds to the root of the repository or some sub-tree, and thus there is no way for us to know which sparsity patterns, if any, apply. So the $TREE case will not use sparsity patterns or any SKIP_WORKTREE bits and will instead always search all files within the $TREE. Signed-off-by: Matheus Tavares --- builtin/grep.c | 125 ++++++++++++++++++-- t/t7011-skip-worktree-reading.sh | 9 -- t/t7817-grep-sparse-checkout.sh | 195 +++++++++++++++++++++++++++++++ 3 files changed, 312 insertions(+), 17 deletions(-) create mode 100755 t/t7817-grep-sparse-checkout.sh diff --git a/builtin/grep.c b/builtin/grep.c index f58979bc3f..a32815de0a 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -410,7 +410,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int cached); static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, struct tree_desc *tree, struct strbuf *base, int tn_len, - int check_attr); + int is_root_tree); static int grep_submodule(struct grep_opt *opt, const struct pathspec *pathspec, @@ -508,6 +508,10 @@ static int grep_cache(struct grep_opt *opt, for (nr = 0; nr < repo->index->cache_nr; nr++) { const struct cache_entry *ce = repo->index->cache[nr]; + + if (ce_skip_worktree(ce)) + continue; + strbuf_setlen(&name, name_base_len); strbuf_addstr(&name, ce->name); @@ -520,8 +524,7 @@ static int grep_cache(struct grep_opt *opt, * cache entry are identical, even if worktree file has * been modified, so use cache version instead */ - if (cached || (ce->ce_flags & CE_VALID) || - ce_skip_worktree(ce)) { + if (cached || (ce->ce_flags & CE_VALID)) { if (ce_stage(ce) || ce_intent_to_add(ce)) continue; hit |= grep_oid(opt, &ce->oid, name.buf, @@ -552,9 +555,76 @@ static int grep_cache(struct grep_opt *opt, return hit; } -static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, - struct tree_desc *tree, struct strbuf *base, int tn_len, - int check_attr) +static struct pattern_list *get_sparsity_patterns(struct repository *repo) +{ + struct pattern_list *patterns; + char *sparse_file; + int sparse_config, cone_config; + + if (repo_config_get_bool(repo, "core.sparsecheckout", &sparse_config) || + !sparse_config) { + return NULL; + } + + sparse_file = repo_git_path(repo, "info/sparse-checkout"); + patterns = xcalloc(1, sizeof(*patterns)); + + if (repo_config_get_bool(repo, "core.sparsecheckoutcone", &cone_config)) + cone_config = 0; + patterns->use_cone_patterns = cone_config; + + if (add_patterns_from_file_to_list(sparse_file, "", 0, patterns, NULL)) { + if (file_exists(sparse_file)) { + warning(_("failed to load sparse-checkout file: '%s'"), + sparse_file); + } + free(sparse_file); + free(patterns); + return NULL; + } + + free(sparse_file); + return patterns; +} + +static int path_in_sparse_checkout(struct strbuf *path, int prefix_len, + unsigned int entry_mode, + struct index_state *istate, + struct pattern_list *sparsity, + enum pattern_match_result parent_match, + enum pattern_match_result *match) +{ + int dtype = DT_UNKNOWN; + int is_dir = S_ISDIR(entry_mode); + + if (parent_match == MATCHED_RECURSIVE) { + *match = parent_match; + return 1; + } + + if (is_dir && !is_dir_sep(path->buf[path->len - 1])) + strbuf_addch(path, '/'); + + *match = path_matches_pattern_list(path->buf, path->len, + path->buf + prefix_len, &dtype, + sparsity, istate); + if (*match == UNDECIDED) + *match = parent_match; + + if (is_dir) + strbuf_trim_trailing_dir_sep(path); + + if (*match == NOT_MATCHED && + (!is_dir || (is_dir && sparsity->use_cone_patterns))) + return 0; + + return 1; +} + +static int do_grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, + struct tree_desc *tree, struct strbuf *base, int tn_len, + int check_attr, struct pattern_list *sparsity, + enum pattern_match_result default_sparsity_match) { struct repository *repo = opt->repo; int hit = 0; @@ -570,6 +640,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, while (tree_entry(tree, &entry)) { int te_len = tree_entry_len(&entry); + enum pattern_match_result sparsity_match = 0; if (match != all_entries_interesting) { strbuf_addstr(&name, base->buf + tn_len); @@ -586,6 +657,19 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, strbuf_add(base, entry.path, te_len); + if (sparsity) { + struct strbuf path = STRBUF_INIT; + strbuf_addstr(&path, base->buf + tn_len); + + if (!path_in_sparse_checkout(&path, old_baselen - tn_len, + entry.mode, repo->index, + sparsity, default_sparsity_match, + &sparsity_match)) { + strbuf_setlen(base, old_baselen); + continue; + } + } + if (S_ISREG(entry.mode)) { hit |= grep_oid(opt, &entry.oid, base->buf, tn_len, check_attr ? base->buf + tn_len : NULL); @@ -602,8 +686,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, strbuf_addch(base, '/'); init_tree_desc(&sub, data, size); - hit |= grep_tree(opt, pathspec, &sub, base, tn_len, - check_attr); + hit |= do_grep_tree(opt, pathspec, &sub, base, tn_len, + check_attr, sparsity, sparsity_match); free(data); } else if (recurse_submodules && S_ISGITLINK(entry.mode)) { hit |= grep_submodule(opt, pathspec, &entry.oid, @@ -621,6 +705,31 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, return hit; } +/* + * Note: sparsity patterns and paths' attributes will only be considered if + * is_root_tree has true value. (Otherwise, we cannot properly perform pattern + * matching on paths.) + */ +static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, + struct tree_desc *tree, struct strbuf *base, int tn_len, + int is_root_tree) +{ + struct pattern_list *patterns = NULL; + int ret; + + if (is_root_tree) + patterns = get_sparsity_patterns(opt->repo); + + ret = do_grep_tree(opt, pathspec, tree, base, tn_len, is_root_tree, + patterns, 0); + + if (patterns) { + clear_pattern_list(patterns); + free(patterns); + } + return ret; +} + static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, struct object *obj, const char *name, const char *path) { diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh index 37525cae3a..26852586ac 100755 --- a/t/t7011-skip-worktree-reading.sh +++ b/t/t7011-skip-worktree-reading.sh @@ -109,15 +109,6 @@ test_expect_success 'ls-files --modified' ' test -z "$(git ls-files -m)" ' -test_expect_success 'grep with skip-worktree file' ' - git update-index --no-skip-worktree 1 && - echo test > 1 && - git update-index 1 && - git update-index --skip-worktree 1 && - rm 1 && - test "$(git grep --no-ext-grep test)" = "1:test" -' - echo ":000000 100644 $ZERO_OID $EMPTY_BLOB A 1" > expected test_expect_success 'diff-index does not examine skip-worktree absent entries' ' setup_absent && diff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh new file mode 100755 index 0000000000..b3109e3479 --- /dev/null +++ b/t/t7817-grep-sparse-checkout.sh @@ -0,0 +1,195 @@ +#!/bin/sh + +test_description='grep in sparse checkout + +This test creates a repo with the following structure: + +. +|-- a +|-- b +|-- dir +| `-- c +|-- sub +| |-- A +| | `-- a +| `-- B +| `-- b +`-- sub2 + `-- a + +Where the outer repository has non-cone mode sparsity patterns, sub is a +submodule with cone mode sparsity patterns and sub2 is a submodule that is +excluded by the superproject sparsity patterns. The resulting sparse checkout +should leave the following structure in the working tree: + +. +|-- a +|-- sub +| `-- B +| `-- b +`-- sub2 + `-- a + +But note that sub2 should have the SKIP_WORKTREE bit set. +' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo "text" >a && + echo "text" >b && + mkdir dir && + echo "text" >dir/c && + + git init sub && + ( + cd sub && + mkdir A B && + echo "text" >A/a && + echo "text" >B/b && + git add A B && + git commit -m sub && + git sparse-checkout init --cone && + git sparse-checkout set B + ) && + + git init sub2 && + ( + cd sub2 && + echo "text" >a && + git add a && + git commit -m sub2 + ) && + + git submodule add ./sub && + git submodule add ./sub2 && + git add a b dir && + git commit -m super && + git sparse-checkout init --no-cone && + git sparse-checkout set "/*" "!b" "!/*/" "sub" && + + git tag -am tag-to-commit tag-to-commit HEAD && + tree=$(git rev-parse HEAD^{tree}) && + git tag -am tag-to-tree tag-to-tree $tree && + + test_path_is_missing b && + test_path_is_missing dir && + test_path_is_missing sub/A && + test_path_is_file a && + test_path_is_file sub/B/b && + test_path_is_file sub2/a +' + +# The test below checks a special case: the sparsity patterns exclude '/b' +# and sparse checkout is enabled, but the path exists in the working tree (e.g. +# manually created after `git sparse-checkout init`). In this case, grep should +# skip it. +test_expect_success 'grep in working tree should honor sparse checkout' ' + cat >expect <<-EOF && + a:text + EOF + echo "new-text" >b && + test_when_finished "rm b" && + git grep "text" >actual && + test_cmp expect actual +' + +test_expect_success 'grep unmerged file despite not matching sparsity patterns' ' + cat >expect <<-EOF && + b:modified-b-in-branchX + b:modified-b-in-branchY + EOF + test_when_finished "test_might_fail git merge --abort && \ + git checkout master" && + + git sparse-checkout disable && + git checkout -b branchY master && + test_commit modified-b-in-branchY b && + git checkout -b branchX master && + test_commit modified-b-in-branchX b && + + git sparse-checkout init && + test_path_is_missing b && + test_must_fail git merge branchY && + git grep "modified-b" >actual && + test_cmp expect actual +' + +test_expect_success 'grep --cached should honor sparse checkout' ' + cat >expect <<-EOF && + a:text + EOF + git grep --cached "text" >actual && + test_cmp expect actual +' + +test_expect_success 'grep should honor sparse checkout' ' + commit=$(git rev-parse HEAD) && + cat >expect_commit <<-EOF && + $commit:a:text + EOF + cat >expect_tag-to-commit <<-EOF && + tag-to-commit:a:text + EOF + git grep "text" $commit >actual_commit && + test_cmp expect_commit actual_commit && + git grep "text" tag-to-commit >actual_tag-to-commit && + test_cmp expect_tag-to-commit actual_tag-to-commit +' + +test_expect_success 'grep should ignore sparsity patterns' ' + commit=$(git rev-parse HEAD) && + tree=$(git rev-parse HEAD^{tree}) && + cat >expect_tree <<-EOF && + $tree:a:text + $tree:b:text + $tree:dir/c:text + EOF + cat >expect_tag-to-tree <<-EOF && + tag-to-tree:a:text + tag-to-tree:b:text + tag-to-tree:dir/c:text + EOF + git grep "text" $tree >actual_tree && + test_cmp expect_tree actual_tree && + git grep "text" tag-to-tree >actual_tag-to-tree && + test_cmp expect_tag-to-tree actual_tag-to-tree +' + +# Note that sub2/ is present in the worktree but it is excluded by the sparsity +# patterns, so grep should not recurse into it. +test_expect_success 'grep --recurse-submodules should honor sparse checkout in submodule' ' + cat >expect <<-EOF && + a:text + sub/B/b:text + EOF + git grep --recurse-submodules "text" >actual && + test_cmp expect actual +' + +test_expect_success 'grep --recurse-submodules --cached should honor sparse checkout in submodule' ' + cat >expect <<-EOF && + a:text + sub/B/b:text + EOF + git grep --recurse-submodules --cached "text" >actual && + test_cmp expect actual +' + +test_expect_success 'grep --recurse-submodules should honor sparse checkout in submodule' ' + commit=$(git rev-parse HEAD) && + cat >expect_commit <<-EOF && + $commit:a:text + $commit:sub/B/b:text + EOF + cat >expect_tag-to-commit <<-EOF && + tag-to-commit:a:text + tag-to-commit:sub/B/b:text + EOF + git grep --recurse-submodules "text" $commit >actual_commit && + test_cmp expect_commit actual_commit && + git grep --recurse-submodules "text" tag-to-commit >actual_tag-to-commit && + test_cmp expect_tag-to-commit actual_tag-to-commit +' + +test_done -- 2.28.0 ``` ## Matheus Tavares, 2020-09-02 06:17 Subject: [PATCH v5 8/8] config: add setting to ignore sparsity patterns in some cmds Message-ID: <70e7d7b90c22452d018acf6fe2a1825813b6448e.1599026986.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/70e7d7b90c22452d018acf6fe2a1825813b6448e.1599026986.git.matheus.bernardino%40usp.br In-Reply-To: ``` When sparse checkout is enabled, some users expect the output of certain commands (such as grep, diff, and log) to be also restricted within the sparsity patterns. This would allow them to effectively work only on the subset of files in which they are interested; and allow some commands to possibly perform better, by not considering uninteresting paths. For this reason, we taught grep to honor the sparsity patterns, in the previous patch. But, on the other hand, allowing grep and the other commands mentioned to optionally ignore the patterns also make for some interesting use cases. E.g. using grep to search for a function documentation that resides outside the sparse checkout. In any case, there is no current way for users to configure the behavior they want for these commands. Aiming to provide this flexibility, let's introduce the sparse.restrictCmds setting (and the analogous --[no]-restrict-to-sparse-paths global option). The default value is true. For now, grep is the only one affected by this setting, but the goal is to have support for more commands, in the future. Helped-by: Elijah Newren Signed-off-by: Matheus Tavares --- Documentation/config.txt | 2 + Documentation/config/grep.txt | 8 ++ Documentation/config/sparse.txt | 20 ++++ Documentation/git.txt | 4 + Makefile | 1 + builtin/grep.c | 13 ++- contrib/completion/git-completion.bash | 2 + git.c | 5 + sparse-checkout.c | 18 ++++ sparse-checkout.h | 11 +++ t/t7817-grep-sparse-checkout.sh | 132 ++++++++++++++++++++++++- t/t9902-completion.sh | 4 +- 12 files changed, 214 insertions(+), 6 deletions(-) create mode 100644 Documentation/config/sparse.txt create mode 100644 sparse-checkout.c create mode 100644 sparse-checkout.h diff --git a/Documentation/config.txt b/Documentation/config.txt index 3042d80978..3b6e0901b8 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -438,6 +438,8 @@ include::config/sequencer.txt[] include::config/showbranch.txt[] +include::config/sparse.txt[] + include::config/splitindex.txt[] include::config/ssh.txt[] diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt index dd51db38e1..a3275ab4b7 100644 --- a/Documentation/config/grep.txt +++ b/Documentation/config/grep.txt @@ -28,3 +28,11 @@ grep.fullName:: grep.fallbackToNoIndex:: If set to true, fall back to git grep --no-index if git grep is executed outside of a git repository. Defaults to false. + +ifdef::git-grep[] +sparse.restrictCmds:: + See base definition in linkgit:git-config[1]. grep honors + sparse.restrictCmds by limiting searches to the sparsity paths in three + cases: when searching the working tree, when searching the index with + --cached, and when searching a specified commit. +endif::git-grep[] diff --git a/Documentation/config/sparse.txt b/Documentation/config/sparse.txt new file mode 100644 index 0000000000..494761526e --- /dev/null +++ b/Documentation/config/sparse.txt @@ -0,0 +1,20 @@ +sparse.restrictCmds:: + Only meaningful in conjunction with core.sparseCheckout. This option + extends sparse checkouts (which limit which paths are written to the + working tree), so that output and operations are also limited to the + sparsity paths where possible and implemented. The purpose of this + option is to (1) focus output for the user on the portion of the + repository that is of interest to them, and (2) enable potentially + dramatic performance improvements, especially in conjunction with + partial clones. ++ +When this option is true (default), some git commands may limit their behavior +to the paths specified by the sparsity patterns, or to the intersection of +those paths and any (like `*.c`) that the user might also specify on the +command line. When false, the affected commands will work on full trees, +ignoring the sparsity patterns. For now, only git-grep honors this setting. ++ +Note: commands which export, integrity check, or create history will always +operate on full trees (e.g. fast-export, format-patch, fsck, commit, etc.), +unaffected by any sparsity patterns. Also, writing commands such as +sparse-checkout and read-tree will not be affected by this configuration. diff --git a/Documentation/git.txt b/Documentation/git.txt index 2fc92586b5..d857509573 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -180,6 +180,10 @@ If you just want to run git as if it was started in `` then use Do not perform optional operations that require locks. This is equivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`. +--[no-]restrict-to-sparse-paths:: + Overrides the sparse.restrictCmds configuration (see + linkgit:git-config[1]) for this execution. + --list-cmds=group[,group...]:: List commands by group. This is an internal/experimental option and may change or be removed in the future. Supported diff --git a/Makefile b/Makefile index 65f8cfb236..778c9e499e 100644 --- a/Makefile +++ b/Makefile @@ -982,6 +982,7 @@ LIB_OBJS += sha1-name.o LIB_OBJS += shallow.o LIB_OBJS += sideband.o LIB_OBJS += sigchain.o +LIB_OBJS += sparse-checkout.o LIB_OBJS += split-index.o LIB_OBJS += stable-qsort.o LIB_OBJS += strbuf.o diff --git a/builtin/grep.c b/builtin/grep.c index a32815de0a..3fa364e91c 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -25,6 +25,7 @@ #include "submodule-config.h" #include "object-store.h" #include "packfile.h" +#include "sparse-checkout.h" static char const * const grep_usage[] = { N_("git grep [] [-e] [...] [[--] ...]"), @@ -498,6 +499,7 @@ static int grep_cache(struct grep_opt *opt, int nr; struct strbuf name = STRBUF_INIT; int name_base_len = 0; + int sparse_paths_only = restrict_to_sparse_paths(repo); if (repo->submodule_prefix) { name_base_len = strlen(repo->submodule_prefix); strbuf_addstr(&name, repo->submodule_prefix); @@ -509,7 +511,7 @@ static int grep_cache(struct grep_opt *opt, for (nr = 0; nr < repo->index->cache_nr; nr++) { const struct cache_entry *ce = repo->index->cache[nr]; - if (ce_skip_worktree(ce)) + if (sparse_paths_only && ce_skip_worktree(ce)) continue; strbuf_setlen(&name, name_base_len); @@ -715,9 +717,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, int is_root_tree) { struct pattern_list *patterns = NULL; + int sparse_paths_only = restrict_to_sparse_paths(opt->repo); int ret; - if (is_root_tree) + if (is_root_tree && sparse_paths_only) patterns = get_sparsity_patterns(opt->repo); ret = do_grep_tree(opt, pathspec, tree, base, tn_len, is_root_tree, @@ -1258,6 +1261,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix) if (!use_index || untracked) { int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude; + + if (opt_restrict_to_sparse_paths >= 0) { + die(_("--[no-]restrict-to-sparse-paths is incompatible" + " with --no-index and --untracked")); + } + hit = grep_directory(&opt, &pathspec, use_exclude, use_index); } else if (0 <= opt_exclude) { die(_("--[no-]exclude-standard cannot be used for tracked contents")); diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 9147fba3d5..de12766a70 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -3402,6 +3402,8 @@ __git_main () --namespace= --no-replace-objects --help + --restrict-to-sparse-paths + --no-restrict-to-sparse-paths " ;; *) diff --git a/git.c b/git.c index 8bd1d7551d..81206b424c 100644 --- a/git.c +++ b/git.c @@ -5,6 +5,7 @@ #include "run-command.h" #include "alias.h" #include "shallow.h" +#include "sparse-checkout.h" #define RUN_SETUP (1<<0) #define RUN_SETUP_GENTLY (1<<1) @@ -311,6 +312,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) } else { exit(list_cmds(cmd)); } + } else if (!strcmp(cmd, "--restrict-to-sparse-paths")) { + opt_restrict_to_sparse_paths = 1; + } else if (!strcmp(cmd, "--no-restrict-to-sparse-paths")) { + opt_restrict_to_sparse_paths = 0; } else { fprintf(stderr, _("unknown option: %s\n"), cmd); usage(git_usage_string); diff --git a/sparse-checkout.c b/sparse-checkout.c new file mode 100644 index 0000000000..96c5ed5446 --- /dev/null +++ b/sparse-checkout.c @@ -0,0 +1,18 @@ +#include "cache.h" +#include "config.h" +#include "sparse-checkout.h" + +int opt_restrict_to_sparse_paths = -1; + +int restrict_to_sparse_paths(struct repository *repo) +{ + int ret; + + if (opt_restrict_to_sparse_paths >= 0) + return opt_restrict_to_sparse_paths; + + if (repo_config_get_bool(repo, "sparse.restrictcmds", &ret)) + ret = 1; + + return ret; +} diff --git a/sparse-checkout.h b/sparse-checkout.h new file mode 100644 index 0000000000..a4805e443a --- /dev/null +++ b/sparse-checkout.h @@ -0,0 +1,11 @@ +#ifndef SPARSE_CHECKOUT_H +#define SPARSE_CHECKOUT_H + +struct repository; + +extern int opt_restrict_to_sparse_paths; + +/* Whether or not cmds should restrict behavior on sparse paths, in this repo */ +int restrict_to_sparse_paths(struct repository *repo); + +#endif /* SPARSE_CHECKOUT_H */ diff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh index b3109e3479..f93a4f71d1 100755 --- a/t/t7817-grep-sparse-checkout.sh +++ b/t/t7817-grep-sparse-checkout.sh @@ -80,10 +80,10 @@ test_expect_success 'setup' ' test_path_is_file sub2/a ' -# The test below checks a special case: the sparsity patterns exclude '/b' +# The two tests below check a special case: the sparsity patterns exclude '/b' # and sparse checkout is enabled, but the path exists in the working tree (e.g. # manually created after `git sparse-checkout init`). In this case, grep should -# skip it. +# skip the file by default, but not with --no-restrict-to-sparse-paths. test_expect_success 'grep in working tree should honor sparse checkout' ' cat >expect <<-EOF && a:text @@ -93,6 +93,16 @@ test_expect_success 'grep in working tree should honor sparse checkout' ' git grep "text" >actual && test_cmp expect actual ' +test_expect_success 'grep w/ --no-restrict-to-sparse-paths for sparsely excluded but present paths' ' + cat >expect <<-EOF && + a:text + b:new-text + EOF + echo "new-text" >b && + test_when_finished "rm b" && + git --no-restrict-to-sparse-paths grep "text" >actual && + test_cmp expect actual +' test_expect_success 'grep unmerged file despite not matching sparsity patterns' ' cat >expect <<-EOF && @@ -157,7 +167,7 @@ test_expect_success 'grep should ignore sparsity patterns' ' ' # Note that sub2/ is present in the worktree but it is excluded by the sparsity -# patterns, so grep should not recurse into it. +# patterns, so grep should only recurse into it with --no-restrict-to-sparse-paths. test_expect_success 'grep --recurse-submodules should honor sparse checkout in submodule' ' cat >expect <<-EOF && a:text @@ -166,6 +176,15 @@ test_expect_success 'grep --recurse-submodules should honor sparse checkout in s git grep --recurse-submodules "text" >actual && test_cmp expect actual ' +test_expect_success 'grep --recurse-submodules should search in excluded submodules w/ --no-restrict-to-sparse-paths' ' + cat >expect <<-EOF && + a:text + sub/B/b:text + sub2/a:text + EOF + git --no-restrict-to-sparse-paths grep --recurse-submodules "text" >actual && + test_cmp expect actual +' test_expect_success 'grep --recurse-submodules --cached should honor sparse checkout in submodule' ' cat >expect <<-EOF && @@ -192,4 +211,111 @@ test_expect_success 'grep --recurse-submodules should honor sparse test_cmp expect_tag-to-commit actual_tag-to-commit ' +for cmd in 'git --no-restrict-to-sparse-paths grep' \ + 'git -c sparse.restrictCmds=false grep' \ + 'git -c sparse.restrictCmds=true --no-restrict-to-sparse-paths grep' +do + + test_expect_success "$cmd --cached should ignore sparsity patterns" ' + cat >expect <<-EOF && + a:text + b:text + dir/c:text + EOF + $cmd --cached "text" >actual && + test_cmp expect actual + ' + + test_expect_success "$cmd should ignore sparsity patterns" ' + commit=$(git rev-parse HEAD) && + cat >expect_commit <<-EOF && + $commit:a:text + $commit:b:text + $commit:dir/c:text + EOF + cat >expect_tag-to-commit <<-EOF && + tag-to-commit:a:text + tag-to-commit:b:text + tag-to-commit:dir/c:text + EOF + $cmd "text" $commit >actual_commit && + test_cmp expect_commit actual_commit && + $cmd "text" tag-to-commit >actual_tag-to-commit && + test_cmp expect_tag-to-commit actual_tag-to-commit + ' +done + +test_expect_success 'grep --recurse-submodules --cached w/ --no-restrict-to-sparse-paths' ' + cat >expect <<-EOF && + a:text + b:text + dir/c:text + sub/A/a:text + sub/B/b:text + sub2/a:text + EOF + git --no-restrict-to-sparse-paths grep --recurse-submodules --cached \ + "text" >actual && + test_cmp expect actual +' + +test_expect_success 'grep --recurse-submodules w/ --no-restrict-to-sparse-paths' ' + commit=$(git rev-parse HEAD) && + cat >expect_commit <<-EOF && + $commit:a:text + $commit:b:text + $commit:dir/c:text + $commit:sub/A/a:text + $commit:sub/B/b:text + $commit:sub2/a:text + EOF + cat >expect_tag-to-commit <<-EOF && + tag-to-commit:a:text + tag-to-commit:b:text + tag-to-commit:dir/c:text + tag-to-commit:sub/A/a:text + tag-to-commit:sub/B/b:text + tag-to-commit:sub2/a:text + EOF + git --no-restrict-to-sparse-paths grep --recurse-submodules "text" \ + $commit >actual_commit && + test_cmp expect_commit actual_commit && + git --no-restrict-to-sparse-paths grep --recurse-submodules "text" \ + tag-to-commit >actual_tag-to-commit && + test_cmp expect_tag-to-commit actual_tag-to-commit +' + +test_expect_success 'should respect the sparse.restrictCmds values from submodules' ' + cat >expect <<-EOF && + a:text + sub/A/a:text + sub/B/b:text + EOF + test_config -C sub sparse.restrictCmds false && + git grep --cached --recurse-submodules "text" >actual && + test_cmp expect actual +' + +test_expect_success 'should propagate --[no]-restrict-to-sparse-paths to submodules' ' + cat >expect <<-EOF && + a:text + b:text + dir/c:text + sub/A/a:text + sub/B/b:text + sub2/a:text + EOF + test_config -C sub sparse.restrictCmds true && + git --no-restrict-to-sparse-paths grep --cached --recurse-submodules "text" >actual && + test_cmp expect actual +' + +for opt in '--untracked' '--no-index' +do + test_expect_success "--[no]-restrict-to-sparse-paths and $opt are incompatible" " + test_must_fail git --restrict-to-sparse-paths grep $opt . 2>actual && + test_i18ngrep 'restrict-to-sparse-paths is incompatible with' actual + " +done + test_done diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 8425b9a531..a8c2ac9d70 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1928,6 +1928,8 @@ test_expect_success 'double dash "git" itself' ' --namespace= --no-replace-objects Z --help Z + --restrict-to-sparse-paths Z + --no-restrict-to-sparse-paths Z EOF ' @@ -1970,7 +1972,7 @@ test_expect_success 'general options' ' test_completion "git --nam" "--namespace=" && test_completion "git --bar" "--bare " && test_completion "git --inf" "--info-path " && - test_completion "git --no-r" "--no-replace-objects " + test_completion "git --no-rep" "--no-replace-objects " ' test_expect_success 'general options plus command' ' -- 2.28.0 ``` ## Eric Sunshine, 2020-09-02 06:57 Subject: Re: [PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSLjMabeLgkg9N7%2BZJ1jotbpJx0FAnjkpNSt0Lf%2BQ0wNQ%40mail.gmail.com In-Reply-To: ``` On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares wrote: > One test in t1308 expects test-config to fail with exit code 128 due to > a parsing error in the config machinery. But test-config might also exit > with 128 for any other reason that leads it to call die(). Therefore the > test can potentially succeed for the wrong reason. To avoid false > positives, let's check test-config's output, in addition to the exit > code, and make sure that the cause of the error is the one we expect in > this test. > > Moreover, the test was using the auxiliary function check_config which > optionally takes a string to compare the test-config stdout against. > Because this string is optional, there is a risk that future callers may > also check only the exit code and not the output. To avoid that, make > the string parameter of this function mandatory. > > Signed-off-by: Matheus Tavares > --- > diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh > @@ -14,10 +14,7 @@ check_config () { > expect_code=0 > fi && > op=$1 key=$2 && shift && shift && > - if test $# != 0 > - then > - printf "%s\n" "$@" > - fi >expect && > + printf "%s\n" "$@" >expect && This change in behavior is quite subtle. With the original code, "expect" will be entirely empty if no argument is provided, whereas with the revised code, "expect" will contain a single newline. This could be improved by making the argument genuinely mandatory as stated in the commit message. Perhaps something like this: if test $# -eq 0 then BUG "check_config 'value' argument missing" fi && printf "%s\n" "$@" >expect && > @@ -130,7 +127,8 @@ test_expect_success 'check line error when NULL string is queried' ' > test_expect_success 'find integer if value is non parse-able' ' > - check_config expect_code 128 get_int lamb.head > + test_expect_code 128 test-tool config get_int lamb.head 2>result && > + test_i18ngrep "fatal: bad numeric config value '\'none\'' for '\'lamb.head\''" result > ' The complex '\'quoting\'' magic leaves and re-enters the single-quote context of the test body and makes it difficult to reason about. Since this is a pattern argument to grep, a simpler alternative would be: test_i18ngrep "fatal: bad numeric config value .none. for .lamb.head." result Aside from that, do I understand correctly that all other callers which expect a non-zero exit code will find the error message on stdout, but this case will find it on stderr? That makes one wonder if, rather than dropping use of check_config() here, instead check_config() should be enhanced to accept an additional option, such as 'stderr' which causes it to check stderr rather than stdout (similar to how 'expect_code' allows the caller to override the expected exit code). But perhaps that would be overengineered if this case is not expected to come up again as more callers are added in the future? ``` ## Eric Sunshine, 2020-09-02 07:18 Subject: Re: [PATCH v5 4/8] t/helper/test-config: check argc before accessing argv Message-ID: URL: https://gitlist.dev/e/CAPig%2BcT7bO97kWgQCftetR_4i_eTU4FsGejkdkY%2Bcbd5K78oLA%40mail.gmail.com In-Reply-To: <0750191342754bcca398c6fdad522616b0f3fbc3.1599026986.git.matheus.bernardino@usp.br> ``` On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares wrote: > Check that we have the expected argc in 'configset_get_value' and > 'configset_get_value_multi' before trying to access argv elements. > > Signed-off-by: Matheus Tavares > --- > diff --git a/t/helper/test-config.c b/t/helper/test-config.c > @@ -138,7 +138,7 @@ int cmd__config(int argc, const char **argv) > - } else if (!strcmp(argv[1], "configset_get_value")) { > + } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { > for (i = 3; i < argc; i++) { > @@ -156,7 +156,7 @@ int cmd__config(int argc, const char **argv) > printf("Value not found for \"%s\"\n", argv[2]); This is certainly a bug fix since it was accessing argv[2] without checking that that element was even present, but the more significant outcome of this change is that it now correctly diagnoses when these two commands are called with the wrong number of arguments (just like all the other commands -- except "iterate" -- diagnose incorrect number of arguments). It might make sense, therefore, for the commit message to focus on that improvement and mention the out-of-bounds array access fix as a side-effect. However, that itself is not worth a re-roll. ``` ## Eric Sunshine, 2020-09-02 07:30 Subject: Re: [PATCH v5 5/8] t/helper/test-config: unify exit labels Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSZFOXuLBKBLrNkZ%3DdTmQUrp-QiUb7wYGoza-YmEFiM9A%40mail.gmail.com In-Reply-To: <56535b0e36e94dc73aa570f4f3c0466305c6432f.1599026986.git.matheus.bernardino@usp.br> ``` On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares wrote: > test-config's main function has three different exit labels, all of > which have to perform the same cleanup code before returning. Unify the > labels in preparation for the next patch which will increase the cleanup > section. > > Signed-off-by: Matheus Tavares > --- > diff --git a/t/helper/test-config.c b/t/helper/test-config.c > @@ -69,16 +69,19 @@ static int early_config_cb(const char *var, const char *value, void *vdata) > +#define TC_VALUE_NOT_FOUND 1 > +#define TC_CONFIG_FILE_ERROR 2 > + > int cmd__config(int argc, const char **argv) > { > + int i, val, ret = 0; > > if (argc == 3 && !strcmp(argv[1], "read_early_config")) { > read_early_config(early_config_cb, (void *)argv[2]); > - return 0; > + return ret; > } This one change feels both fragile and increases cognitive load, thus does not seem desirable. It feels fragile because someone could come along and insert code above this conditional which assigns some value other than 0 to 'ret' (not realizing that this conditional wants to return 0), thus breaking it. It increases cognitive load because it forces the reader to scan all the code leading up to this point to determine what value this conditional really wants to return. Nevertheless, this is a minor objection, not necessarily worth a re-roll. > @@ -94,10 +97,9 @@ int cmd__config(int argc, const char **argv) > printf("Value not found for \"%s\"\n", argv[2]); > - goto exit1; > + ret = TC_VALUE_NOT_FOUND; This sort of change, on the other hand, does not increase cognitive load because it's quite obvious what return value this conditional wants to return (because it's assigning it explicitly). ``` ## Matheus Tavares Bernardino, 2020-09-02 16:16 Subject: Re: [PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config Message-ID: URL: https://gitlist.dev/e/CAHd-oW5gk16%3DTmhi_e43vVCufM9%3Dzd8jtZJ_EFJVLmC4dJ06%3Dw%40mail.gmail.com In-Reply-To: ``` On Wed, Sep 2, 2020 at 3:57 AM Eric Sunshine wrote: > > On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares > wrote: > > One test in t1308 expects test-config to fail with exit code 128 due to > > a parsing error in the config machinery. But test-config might also exit > > with 128 for any other reason that leads it to call die(). Therefore the > > test can potentially succeed for the wrong reason. To avoid false > > positives, let's check test-config's output, in addition to the exit > > code, and make sure that the cause of the error is the one we expect in > > this test. > > > > Moreover, the test was using the auxiliary function check_config which > > optionally takes a string to compare the test-config stdout against. > > Because this string is optional, there is a risk that future callers may > > also check only the exit code and not the output. To avoid that, make > > the string parameter of this function mandatory. > > > > Signed-off-by: Matheus Tavares > > --- > > diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh > > @@ -14,10 +14,7 @@ check_config () { > > expect_code=0 > > fi && > > op=$1 key=$2 && shift && shift && > > - if test $# != 0 > > - then > > - printf "%s\n" "$@" > > - fi >expect && > > + printf "%s\n" "$@" >expect && > > This change in behavior is quite subtle. With the original code, > "expect" will be entirely empty if no argument is provided, whereas > with the revised code, "expect" will contain a single newline. This > could be improved by making the argument genuinely mandatory as stated > in the commit message. Perhaps something like this: > > if test $# -eq 0 > then > BUG "check_config 'value' argument missing" > fi && > printf "%s\n" "$@" >expect && Thanks for catching this. I will add the check. > > @@ -130,7 +127,8 @@ test_expect_success 'check line error when NULL string is queried' ' > > test_expect_success 'find integer if value is non parse-able' ' > > - check_config expect_code 128 get_int lamb.head > > + test_expect_code 128 test-tool config get_int lamb.head 2>result && > > + test_i18ngrep "fatal: bad numeric config value '\'none\'' for '\'lamb.head\''" result > > ' >exit > The complex '\'quoting\'' magic leaves and re-enters the single-quote > context of the test body and makes it difficult to reason about. Since > this is a pattern argument to grep, a simpler alternative would be: > > test_i18ngrep "fatal: bad numeric config value .none. for > .lamb.head." result Will do, thanks. > Aside from that, do I understand correctly that all other callers > which expect a non-zero exit code will find the error message on > stdout, but this case will find it on stderr? Right. This happens because, for a "value not found" error, test-config will exit with code 1 and print to stdout. This is the only case where it exits with a non-zero code and prints to stdout instead of stderr. With that said, I'm wondering now whether we should change the function's signature from: `check_config [expect_code ] ` to: `check_config ` `check_config expect_not_found ` The second form would then automatically expect exit code 1 and check stdout for the message 'Value not found for ""'. With this we can avoid wrong uses of check_config to check an arbitrary error code without also checking stderr. > That makes one wonder > if, rather than dropping use of check_config() here, instead > check_config() should be enhanced to accept an additional option, such > as 'stderr' which causes it to check stderr rather than stdout > (similar to how 'expect_code' allows the caller to override the > expected exit code). But perhaps that would be overengineered if this > case is not expected to come up again as more callers are added in the > future? That's an interesting idea. However, because some callers may want to use test_i18ngrep instead of test_cmp, I think the required logic would become too complex. ``` ## Eric Sunshine, 2020-09-02 16:38 Subject: Re: [PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSdAp%3DgaJ01MSxhxc%2Bj%3D%2BOFkiPK7e6c960RZwUU22sKhQ%40mail.gmail.com In-Reply-To: ``` On Wed, Sep 2, 2020 at 12:16 PM Matheus Tavares Bernardino wrote: > With that said, I'm wondering now whether we should change the > function's signature from: > > `check_config [expect_code ] ` > > to: > > `check_config ` > `check_config expect_not_found ` > > The second form would then automatically expect exit code 1 and check > stdout for the message 'Value not found for ""'. With this we > can avoid wrong uses of check_config to check an arbitrary error code > without also checking stderr. Yes, that seems more straightforward. In fact, at this point, you could just have two distinct functions and eliminate the ugly complexity of the existing check_config() implementation. Perhaps something like this (typed in email): check_config () { test_tool config "$1" "$2" >actual && shift && shift && printf "%s\n" "$@" >expect && test_cmp expect actual } check_not_found () { test_expect_code 1 test_tool config "$1" "$2" >actual && echo "Value not found for \"$2\"" >expect && test_cmp expect actual } ``` ## Jonathan Nieder, 2020-09-02 20:15 Subject: Re: [PATCH v5 6/8] config: correctly read worktree configs in submodules Message-ID: <20200902201523.GA3941032@google.com> URL: https://gitlist.dev/e/20200902201523.GA3941032%40google.com In-Reply-To: <3e02e1bd248438e0b435a19d857432edcaa15a2c.1599026986.git.matheus.bernardino@usp.br> ``` Matheus Tavares wrote: > The config machinery is not able to read worktree configs from a > submodule in a process where the_repository represents the superproject. ... where the_repository represents the superproject and extensions.worktreeConfig is not set there, right? > Furthermore, when extensions.worktreeConfig is set on the superproject, > querying for a worktree config in a submodule will, instead, return > the value set at the superproject. > > The problem resides in do_git_config_sequence(). Although the function > receives a git_dir string, it uses the_repository->git_dir when making This part of the commit message seems to be rephrasing what the patch says; for that kind of thing, it seems better to let the patch speak for itself. Can we describe what is happening at a higher level (in other words the intent instead of the details of how that is manifested in code)? For example, The relevant code is in do_git_config_sequence. Although it is designed to act on an arbitrary repository, specified by the passed-in git_dir string, it accidentally depends on the_repository in two places: - it reads the global variable `repository_format_worktree_config`, which is set based on the content of the_repository, to determine whether extensions.worktreeConfig is set - it uses the git_pathdup helper to find the config.worktree file, instead of making a path using the passed-in git_dir falue Sever these dependencies. [...] > --- a/config.c > +++ b/config.c > @@ -1747,11 +1747,22 @@ static int do_git_config_sequence(const struct config_options *opts, > ret += git_config_from_file(fn, repo_config, data); > > current_parsing_scope = CONFIG_SCOPE_WORKTREE; > - if (!opts->ignore_worktree && repository_format_worktree_config) { > + if (!opts->ignore_worktree && repo_config && opts->git_dir) { repo_config is non-NULL if and only if commondir is non-NULL and commondir is always non-NUlL if git_dir is non-NULL (as checked higher in the function), right? I think that means this condition could be written more simply as if (!opts->ignore_worktree && opts->git_dir) { which I think should be easier for the reader to understand. > + struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT; > + struct strbuf buf = STRBUF_INIT; > + > + read_repository_format(&repo_fmt, repo_config); > + > + if (!verify_repository_format(&repo_fmt, &buf) && > + repo_fmt.worktree_config) { In the common case where we are acting on the_repository, this add extra complexity and slows the routine down. Would passing in the 'struct repository *' to allow distinguishing that case help? Something like this: diff --git i/builtin/config.c w/builtin/config.c index 5e39f618854..ca4caedf33a 100644 --- i/builtin/config.c +++ w/builtin/config.c @@ -699,10 +699,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) config_options.respect_includes = !given_config_source.file; else config_options.respect_includes = respect_includes_opt; - if (!nongit) { - config_options.commondir = get_git_common_dir(); - config_options.git_dir = get_git_dir(); - } + if (!nongit) + config_options.repo = the_repository; if (end_nul) { term = '\0'; diff --git i/config.c w/config.c index 2bdff4457be..70a1dd0ad3f 100644 --- i/config.c +++ w/config.c @@ -222,8 +222,8 @@ static int include_by_gitdir(const struct config_options *opts, const char *git_dir; int already_tried_absolute = 0; - if (opts->git_dir) - git_dir = opts->git_dir; + if (opts->repo && opts->repo->gitdir) + git_dir = opts->repo->gitdir; else goto done; @@ -1720,10 +1720,10 @@ static int do_git_config_sequence(const struct config_options *opts, char *repo_config; enum config_scope prev_parsing_scope = current_parsing_scope; - if (opts->commondir) - repo_config = mkpathdup("%s/config", opts->commondir); - else if (opts->git_dir) - BUG("git_dir without commondir"); + if (opts->repo && opts->repo->commondir) + repo_config = mkpathdup("%s/config", opts->repo->commondir); + else if (opts->repo && opts->repo->gitdir) + BUG("gitdir without commondir"); else repo_config = NULL; @@ -1824,27 +1824,33 @@ void read_early_config(config_fn_t cb, void *data) struct config_options opts = {0}; struct strbuf commondir = STRBUF_INIT; struct strbuf gitdir = STRBUF_INIT; + struct repository the_early_repo = {0}; opts.respect_includes = 1; if (have_git_dir()) { - opts.commondir = get_git_common_dir(); - opts.git_dir = get_git_dir(); + opts.repo = the_repository; /* * When setup_git_directory() was not yet asked to discover the * GIT_DIR, we ask discover_git_directory() to figure out whether there * is any repository config we should use (but unlike - * setup_git_directory_gently(), no global state is changed, most + * setup_git_directory_gently(), no global state is changed; most * notably, the current working directory is still the same after the * call). + * + * NEEDSWORK: There is some duplicate work between + * discover_git_directory and repo_init. Update to use a variant of + * repo_init that does its own repository discovery once available. */ } else if (!discover_git_directory(&commondir, &gitdir)) { - opts.commondir = commondir.buf; - opts.git_dir = gitdir.buf; + repo_init(&the_early_repo, gitdir.buf, NULL); + opts.repo = &the_early_repo; } config_with_options(cb, data, NULL, &opts); + if (the_early_repo.settings.initialized) + repo_clear(&the_early_repo); strbuf_release(&commondir); strbuf_release(&gitdir); } @@ -2097,8 +2103,7 @@ static void repo_read_config(struct repository *repo) struct config_options opts = { 0 }; opts.respect_includes = 1; - opts.commondir = repo->commondir; - opts.git_dir = repo->gitdir; + opts.repo = repo; if (!repo->config) repo->config = xcalloc(1, sizeof(struct config_set)); diff --git i/config.h w/config.h index 91cdfbfb414..e56293fb29f 100644 --- i/config.h +++ w/config.h @@ -21,6 +21,7 @@ */ struct object_id; +struct repository; /* git_config_parse_key() returns these negated: */ #define CONFIG_INVALID_KEY 1 @@ -87,8 +88,7 @@ struct config_options { unsigned int ignore_worktree : 1; unsigned int ignore_cmdline : 1; unsigned int system_gently : 1; - const char *commondir; - const char *git_dir; + struct repository *repo; config_parser_event_fn_t event_fn; void *event_fn_data; enum config_error_action { ==== >8 ==== [...] > --- a/t/helper/test-config.c > +++ b/t/helper/test-config.c [...] > @@ -72,14 +80,34 @@ static int early_config_cb(const char *var, const char *value, void *vdata) > #define TC_VALUE_NOT_FOUND 1 > #define TC_CONFIG_FILE_ERROR 2 > > +static const char *test_config_usage[] = { > + "test-tool config [--submodule=] []", > + NULL > +}; > + > int cmd__config(int argc, const char **argv) > { > int i, val, ret = 0; > const char *v; > const struct string_list *strptr; > struct config_set cs; > + struct repository subrepo, *repo = the_repository; > + const char *subrepo_path = NULL; > + > + struct option options[] = { > + OPT_STRING(0, "submodule", &subrepo_path, "path", > + "run on the submodule at "), > + OPT_END() > + }; Nice. > + > + argc = parse_options(argc, argv, NULL, options, test_config_usage, > + PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_STOP_AT_NON_OPTION); > + if (argc < 2) > + die("Please, provide a command name on the command-line"); optional nit: can use usage_with_options here. It produces a better error message than any other I can think of (all I can think of are things like "need a "). This is from existing code, but the use of parse_options opens up the possibility of taking advantage of the parse-options generated message. :) [...] > --- a/t/t2404-worktree-config.sh > +++ b/t/t2404-worktree-config.sh > @@ -78,4 +78,20 @@ test_expect_success 'config.worktree no longer read without extension' ' > test_cmp_config -c wt2 shared this.is > ' > > +test_expect_success 'correctly read config.worktree from submodules' ' > + test_unconfig extensions.worktreeconfig && > + git init sub && > + ( > + cd sub && > + test_commit a && > + git config extensions.worktreeconfig true && > + git config --worktree wtconfig.sub test-value > + ) && > + git submodule add ./sub && > + git commit -m "add sub" && > + echo test-value >expect && > + test-tool config --submodule=sub get_value wtconfig.sub >actual && > + test_cmp expect actual > +' Lovely. Summary: I like the direction this change goes in. I think we can do it without repeating repository format discovery in the the_repository case and without duplicating repository format discovery code in the submodule case. If it proves too fussy, then a NEEDSWORK comment would be helpful to help the reader see what is going on. Thanks and hope that helps, Jonathan ``` ## Matheus Tavares Bernardino, 2020-09-09 13:04 Subject: Re: [PATCH v5 6/8] config: correctly read worktree configs in submodules Message-ID: URL: https://gitlist.dev/e/CAHd-oW6jCiaXdsKnhde0iBDWXpZyh4cf5j_8zW7x%3DW3BaZ9Kbg%40mail.gmail.com In-Reply-To: <20200902201523.GA3941032@google.com> ``` Hi, Jonathan Sorry for the late reply, last week was quite busy. On Wed, Sep 2, 2020 at 5:15 PM Jonathan Nieder wrote: > > Matheus Tavares wrote: > > > The config machinery is not able to read worktree configs from a > > submodule in a process where the_repository represents the superproject. > > ... where the_repository represents the superproject and > extensions.worktreeConfig is not set there, right? > > > Furthermore, when extensions.worktreeConfig is set on the superproject, > > querying for a worktree config in a submodule will, instead, return > > the value set at the superproject. > > > > The problem resides in do_git_config_sequence(). Although the function > > receives a git_dir string, it uses the_repository->git_dir when making > > This part of the commit message seems to be rephrasing what the patch > says; for that kind of thing, it seems better to let the patch speak > for itself. Can we describe what is happening at a higher level (in > other words the intent instead of the details of how that is > manifested in code)? For example, > > The relevant code is in do_git_config_sequence. Although it is designed > to act on an arbitrary repository, specified by the passed-in git_dir > string, it accidentally depends on the_repository in two places: > > - it reads the global variable `repository_format_worktree_config`, > which is set based on the content of the_repository, to determine > whether extensions.worktreeConfig is set > > - it uses the git_pathdup helper to find the config.worktree file, > instead of making a path using the passed-in git_dir falue > > Sever these dependencies. Yeah, much better, thanks! :) > [...] > > --- a/config.c > > +++ b/config.c > > @@ -1747,11 +1747,22 @@ static int do_git_config_sequence(const struct config_options *opts, > > ret += git_config_from_file(fn, repo_config, data); > > > > current_parsing_scope = CONFIG_SCOPE_WORKTREE; > > - if (!opts->ignore_worktree && repository_format_worktree_config) { > > + if (!opts->ignore_worktree && repo_config && opts->git_dir) { > > repo_config is non-NULL if and only if commondir is non-NULL and > commondir is always non-NUlL if git_dir is non-NULL (as checked higher > in the function), right? I think that means this condition could be > written more simply as > > if (!opts->ignore_worktree && opts->git_dir) { > > which I think should be easier for the reader to understand. Nice, thanks. > > + struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT; > > + struct strbuf buf = STRBUF_INIT; > > + > > + read_repository_format(&repo_fmt, repo_config); > > + > > + if (!verify_repository_format(&repo_fmt, &buf) && > > + repo_fmt.worktree_config) { > > In the common case where we are acting on the_repository, this add > extra complexity and slows the routine down. > > Would passing in the 'struct repository *' to allow distinguishing > that case help? Something like this: > > diff --git i/builtin/config.c w/builtin/config.c > index 5e39f618854..ca4caedf33a 100644 > --- i/builtin/config.c > +++ w/builtin/config.c > @@ -699,10 +699,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) > config_options.respect_includes = !given_config_source.file; > else > config_options.respect_includes = respect_includes_opt; > - if (!nongit) { > - config_options.commondir = get_git_common_dir(); > - config_options.git_dir = get_git_dir(); > - } > + if (!nongit) > + config_options.repo = the_repository; > > if (end_nul) { > term = '\0'; > diff --git i/config.c w/config.c > index 2bdff4457be..70a1dd0ad3f 100644 > --- i/config.c > +++ w/config.c > @@ -222,8 +222,8 @@ static int include_by_gitdir(const struct config_options *opts, > const char *git_dir; > int already_tried_absolute = 0; > > - if (opts->git_dir) > - git_dir = opts->git_dir; > + if (opts->repo && opts->repo->gitdir) > + git_dir = opts->repo->gitdir; > else > goto done; > > @@ -1720,10 +1720,10 @@ static int do_git_config_sequence(const struct config_options *opts, > char *repo_config; > enum config_scope prev_parsing_scope = current_parsing_scope; > > - if (opts->commondir) > - repo_config = mkpathdup("%s/config", opts->commondir); > - else if (opts->git_dir) > - BUG("git_dir without commondir"); > + if (opts->repo && opts->repo->commondir) > + repo_config = mkpathdup("%s/config", opts->repo->commondir); > + else if (opts->repo && opts->repo->gitdir) > + BUG("gitdir without commondir"); > else > repo_config = NULL; > > @@ -1824,27 +1824,33 @@ void read_early_config(config_fn_t cb, void *data) > struct config_options opts = {0}; > struct strbuf commondir = STRBUF_INIT; > struct strbuf gitdir = STRBUF_INIT; > + struct repository the_early_repo = {0}; > > opts.respect_includes = 1; > > if (have_git_dir()) { > - opts.commondir = get_git_common_dir(); > - opts.git_dir = get_git_dir(); > + opts.repo = the_repository; I'm not very familiar with the code in setup.c so I apologize for the noob question: have_git_dir() returns `startup_info->have_repository || the_repository->gitdir`; so is it possible that it returns true but the_repository->gitdir is not initialized yet? If so, should we also check the_repository->gitdir here (before assigning opts.repo), and call BUG() when it is NULL, like get_git_dir() does? Hmm, nevertheless, I see that you already check `opts.repo && opts.repo->gitdir` before trying to use it in do_git_config_sequence(). So it should already cover this case, right? > /* > * When setup_git_directory() was not yet asked to discover the > * GIT_DIR, we ask discover_git_directory() to figure out whether there > * is any repository config we should use (but unlike > - * setup_git_directory_gently(), no global state is changed, most > + * setup_git_directory_gently(), no global state is changed; most > * notably, the current working directory is still the same after the > * call). > + * > + * NEEDSWORK: There is some duplicate work between > + * discover_git_directory and repo_init. Update to use a variant of > + * repo_init that does its own repository discovery once available. > */ > } else if (!discover_git_directory(&commondir, &gitdir)) { > - opts.commondir = commondir.buf; > - opts.git_dir = gitdir.buf; > + repo_init(&the_early_repo, gitdir.buf, NULL); > + opts.repo = &the_early_repo; > } > > config_with_options(cb, data, NULL, &opts); > > + if (the_early_repo.settings.initialized) > + repo_clear(&the_early_repo); > > strbuf_release(&commondir); > strbuf_release(&gitdir); > } > @@ -2097,8 +2103,7 @@ static void repo_read_config(struct repository *repo) > struct config_options opts = { 0 }; > > opts.respect_includes = 1; > - opts.commondir = repo->commondir; > - opts.git_dir = repo->gitdir; > + opts.repo = repo; > > if (!repo->config) > repo->config = xcalloc(1, sizeof(struct config_set)); > diff --git i/config.h w/config.h > index 91cdfbfb414..e56293fb29f 100644 > --- i/config.h > +++ w/config.h > @@ -21,6 +21,7 @@ > */ > > struct object_id; > +struct repository; > > /* git_config_parse_key() returns these negated: */ > #define CONFIG_INVALID_KEY 1 > @@ -87,8 +88,7 @@ struct config_options { > unsigned int ignore_worktree : 1; > unsigned int ignore_cmdline : 1; > unsigned int system_gently : 1; > - const char *commondir; > - const char *git_dir; > + struct repository *repo; > config_parser_event_fn_t event_fn; > void *event_fn_data; > enum config_error_action { > ==== >8 ==== Thanks a lot for this :) I was thinking of adding it as a preparatory patch before the fix itself. May I have your S-o-B as the author? Best, Matheus ``` ## Jonathan Nieder, 2020-09-09 23:32 Subject: Re: [PATCH v5 6/8] config: correctly read worktree configs in submodules Message-ID: <20200909233247.GA667601@google.com> URL: https://gitlist.dev/e/20200909233247.GA667601%40google.com In-Reply-To: ``` Hi, Matheus Tavares Bernardino wrote: > Sorry for the late reply, last week was quite busy. No problem. It's an unusual time for everyone. [...] > On Wed, Sep 2, 2020 at 5:15 PM Jonathan Nieder wrote: >> @@ -1824,27 +1824,33 @@ void read_early_config(config_fn_t cb, void *data) >> struct config_options opts = {0}; >> struct strbuf commondir = STRBUF_INIT; >> struct strbuf gitdir = STRBUF_INIT; >> + struct repository the_early_repo = {0}; >> >> opts.respect_includes = 1; >> >> if (have_git_dir()) { >> - opts.commondir = get_git_common_dir(); >> - opts.git_dir = get_git_dir(); >> + opts.repo = the_repository; > > I'm not very familiar with the code in setup.c so I apologize for the > noob question: have_git_dir() returns `startup_info->have_repository > || the_repository->gitdir`; so is it possible that it returns true but > the_repository->gitdir is not initialized yet? If so, should we also > check the_repository->gitdir here (before assigning opts.repo), and > call BUG() when it is NULL, like get_git_dir() does? > > Hmm, nevertheless, I see that you already check `opts.repo && > opts.repo->gitdir` before trying to use it in > do_git_config_sequence(). So it should already cover this case, right? Right --- the main point is that a BUG() call represents "this can't happen", or in other words, it's an assertion failure. As a matter of defensive coding functions like get_git_dir() guard against such cases to make debugging a little easier and exploitation a little more difficult when the impossible happens. [...] > Thanks a lot for this :) I was thinking of adding it as a preparatory > patch before the fix itself. May I have your S-o-B as the author? Sure! Signed-off-by: Jonathan Nieder Thanks, Jonathan ``` ## Matheus Tavares, 2020-09-10 17:21 Subject: [PATCH v6 0/9] grep: honor sparse checkout and add option to ignore it Message-ID: URL: https://gitlist.dev/e/cover.1599758167.git.matheus.bernardino%40usp.br In-Reply-To: ``` This series makes git-grep restrict its output to the sparsity patterns when requested by the user. A new global option is added to control this behavior in grep and hopefully more commands in the future. There is also a fix in config.c, to correctly read worktree-specific settings on submodules. Changes since v5: Patch 2: - Avoid complex '\'quoting\'' by using '.', as the string is going to be used in a grep search. - Split the check_config auxiliary function into two, to remove some unecessary logic and future-proof them against wrong usages of test-config. Patch 4: reword commit message to focus on correctly diagnosing missing arguments. Patch 5: don't replace explicit `return 0` with `return ret`. Added patch 6, passing a `struct repository` to do_git_config_sequence(). Patch 6: - Removed global `repository_format_worktree_config` and cached extensions.worktreeConfig value in `struct repository`, to avoid repeating repository format discovery. - Improved commit message as suggested by Jonathan. - Use usage_with_options() Jonathan Nieder (1): config: make do_git_config_sequence receive a 'struct repository' Matheus Tavares (8): doc: grep: unify info on configuration variables t1308-config-set: avoid false positives when using test-config t/helper/test-config: be consistent with exit codes t/helper/test-config: diagnose missing arguments t/helper/test-config: unify exit labels config: correctly read worktree configs in submodules grep: honor sparse checkout patterns config: add setting to ignore sparsity patterns in some cmds Documentation/config.txt | 2 + Documentation/config/grep.txt | 18 +- Documentation/config/sparse.txt | 20 ++ Documentation/git-grep.txt | 36 +-- Documentation/git.txt | 4 + Makefile | 1 + builtin/config.c | 8 +- builtin/grep.c | 134 ++++++++++- cache.h | 1 - config.c | 47 ++-- config.h | 4 +- contrib/completion/git-completion.bash | 2 + environment.c | 1 - git.c | 5 + repository.c | 1 + repository.h | 1 + setup.c | 4 +- sparse-checkout.c | 18 ++ sparse-checkout.h | 11 + t/helper/test-config.c | 124 ++++++---- t/t1308-config-set.sh | 28 +-- t/t2404-worktree-config.sh | 16 ++ t/t7011-skip-worktree-reading.sh | 9 - t/t7817-grep-sparse-checkout.sh | 321 +++++++++++++++++++++++++ t/t9902-completion.sh | 4 +- 25 files changed, 682 insertions(+), 138 deletions(-) create mode 100644 Documentation/config/sparse.txt create mode 100644 sparse-checkout.c create mode 100644 sparse-checkout.h create mode 100755 t/t7817-grep-sparse-checkout.sh Range-diff against v5: 1: 70c9a4e741 = 1: 70c9a4e741 doc: grep: unify info on configuration variables 2: f53782f14c < -: ---------- t1308-config-set: avoid false positives when using test-config -: ---------- > 2: 3c2d722152 t1308-config-set: avoid false positives when using test-config 3: 85e1588d6c = 3: 45d13744b7 t/helper/test-config: be consistent with exit codes 4: 0750191342 ! 4: 51656e43c3 t/helper/test-config: check argc before accessing argv @@ Metadata Author: Matheus Tavares ## Commit message ## - t/helper/test-config: check argc before accessing argv + t/helper/test-config: diagnose missing arguments - Check that we have the expected argc in 'configset_get_value' and - 'configset_get_value_multi' before trying to access argv elements. + test-config verifies that the correct number of arguments was given for + all of its commands except for 'configset_get_value' and + 'configset_get_value_multi'. Add the check to these two, so that we + properly report missing arguments and prevent out-of-bounds access to + argv[]. Signed-off-by: Matheus Tavares 5: 56535b0e36 ! 5: 924d2e8ceb t/helper/test-config: unify exit labels @@ t/helper/test-config.c: static int early_config_cb(const char *var, const char * const char *v; const struct string_list *strptr; struct config_set cs; - - if (argc == 3 && !strcmp(argv[1], "read_early_config")) { - read_early_config(early_config_cb, (void *)argv[2]); -- return 0; -+ return ret; - } - - setup_git_directory(); @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) printf("(NULL)\n"); else -: ---------- > 6: f5c0fc3336 config: make do_git_config_sequence receive a 'struct repository' 6: 3e02e1bd24 ! 7: 3a28b8e608 config: correctly read worktree configs in submodules @@ Commit message config: correctly read worktree configs in submodules The config machinery is not able to read worktree configs from a - submodule in a process where the_repository represents the superproject. - Furthermore, when extensions.worktreeConfig is set on the superproject, - querying for a worktree config in a submodule will, instead, return - the value set at the superproject. - - The problem resides in do_git_config_sequence(). Although the function - receives a git_dir string, it uses the_repository->git_dir when making - the path to the worktree config file. And when checking if - extensions.worktreeConfig is set, it uses the global - repository_format_worktree_config variable, which refers to - the_repository only. So let's fix this by using the git_dir given to the - function and reading the extension value from the right place. Also add - a test to avoid any regressions. + submodule in a process where the_repository represents the superproject + and extensions.worktreeConfig is not set there. Furthermore, when + extensions.worktreeConfig is set on the superproject, querying for a + worktree config in a submodule will, instead, return the value set at + the superproject. + + The relevant code is in do_git_config_sequence(). Although it is + designed to act on an arbitrary repository, specified in the passed-in + `struct config_options`, it accidentally depends on the_repository in + two places: + + - it reads the global variable `repository_format_worktree_config`, + which is set based on the content of the_repository, to determine + whether extensions.worktreeConfig is set. + + - it uses the git_pathdup() helper to find the config.worktree file, + instead of making a path using the passed-in repository. + + Sever these dependencies and add a regression test. Also, to avoid + future misuses of `repository_format_worktree_config` like this one, + remove this global variable and store the config value on + `struct repository` itself. Signed-off-by: Matheus Tavares + ## builtin/config.c ## +@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix) + given_config_source.scope = CONFIG_SCOPE_LOCAL; + } else if (use_worktree_config) { + struct worktree **worktrees = get_worktrees(); +- if (repository_format_worktree_config) ++ if (!nongit && the_repository->worktree_config_extension) + given_config_source.file = git_pathdup("config.worktree"); + else if (worktrees[0] && worktrees[1]) + die(_("--worktree cannot be used with multiple " + + ## cache.h ## +@@ cache.h: extern int grafts_replace_parents; + #define GIT_REPO_VERSION 0 + #define GIT_REPO_VERSION_READ 1 + extern int repository_format_precious_objects; +-extern int repository_format_worktree_config; + + /* + * You _have_ to initialize a `struct repository_format` using + ## config.c ## @@ config.c: static int do_git_config_sequence(const struct config_options *opts, ret += git_config_from_file(fn, repo_config, data); @@ config.c: static int do_git_config_sequence(const struct config_options *opts, - if (!access_or_die(path, R_OK, 0)) - ret += git_config_from_file(fn, path, data); - free(path); -+ if (!opts->ignore_worktree && repo_config && opts->git_dir) { -+ struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT; -+ struct strbuf buf = STRBUF_INIT; -+ -+ read_repository_format(&repo_fmt, repo_config); -+ -+ if (!verify_repository_format(&repo_fmt, &buf) && -+ repo_fmt.worktree_config) { -+ char *path = mkpathdup("%s/config.worktree", opts->git_dir); -+ if (!access_or_die(path, R_OK, 0)) -+ ret += git_config_from_file(fn, path, data); -+ free(path); -+ } -+ -+ strbuf_release(&buf); -+ clear_repository_format(&repo_fmt); ++ if (!opts->ignore_worktree && opts->repo && opts->repo->gitdir && ++ opts->repo->worktree_config_extension) { ++ struct strbuf path = STRBUF_INIT; ++ strbuf_repo_git_path(&path, opts->repo, "config.worktree"); ++ if (!access_or_die(path.buf, R_OK, 0)) ++ ret += git_config_from_file(fn, path.buf, data); ++ strbuf_release(&path); } current_parsing_scope = CONFIG_SCOPE_COMMAND; + ## environment.c ## +@@ environment.c: int warn_ambiguous_refs = 1; + int warn_on_object_refname_ambiguity = 1; + int ref_paranoia = -1; + int repository_format_precious_objects; +-int repository_format_worktree_config; + const char *git_commit_encoding; + const char *git_log_output_encoding; + char *apply_default_whitespace; + + ## repository.c ## +@@ repository.c: int repo_init(struct repository *repo, + goto error; + + repo_set_hash_algo(repo, format.hash_algo); ++ repo->worktree_config_extension = format.worktree_config; + + if (worktree) + repo_set_worktree(repo, worktree); + + ## repository.h ## +@@ repository.h: struct repository { + + /* Indicate if a repository has a different 'commondir' from 'gitdir' */ + unsigned different_commondir:1; ++ unsigned worktree_config_extension:1; + }; + + extern struct repository *the_repository; + + ## setup.c ## +@@ setup.c: static int check_repository_format_gently(const char *gitdir, struct repository_ + + repository_format_precious_objects = candidate->precious_objects; + set_repository_format_partial_clone(candidate->partial_clone); +- repository_format_worktree_config = candidate->worktree_config; ++ the_repository->worktree_config_extension = candidate->worktree_config; + string_list_clear(&candidate->unknown_extensions, 0); + string_list_clear(&candidate->v1_only_extensions, 0); + +- if (repository_format_worktree_config) { ++ if (the_repository->worktree_config_extension) { + /* + * pick up core.bare and core.worktree from per-worktree + * config if present + ## t/helper/test-config.c ## @@ #include "cache.h" @@ t/helper/test-config.c: static int early_config_cb(const char *var, const char * + argc = parse_options(argc, argv, NULL, options, test_config_usage, + PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_STOP_AT_NON_OPTION); + if (argc < 2) -+ die("Please, provide a command name on the command-line"); ++ usage_with_options(test_config_usage, options); if (argc == 3 && !strcmp(argv[1], "read_early_config")) { + if (subrepo_path) + die("cannot use --submodule with read_early_config"); read_early_config(early_config_cb, (void *)argv[2]); - return ret; + return 0; } @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv) 7: 902556a7b6 = 8: 2fc889c9c2 grep: honor sparse checkout patterns 8: 70e7d7b90c = 9: 92bc5351cf config: add setting to ignore sparsity patterns in some cmds -- 2.28.0 ``` ## Matheus Tavares, 2020-09-10 17:21 Subject: [PATCH v6 2/9] t1308-config-set: avoid false positives when using test-config Message-ID: <3c2d722152575d8a4f7aa01d958823a66c5ba26e.1599758167.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/3c2d722152575d8a4f7aa01d958823a66c5ba26e.1599758167.git.matheus.bernardino%40usp.br In-Reply-To: ``` One test in t1308 expects test-config to fail with exit code 128 due to a parsing error in the config machinery. But test-config might also exit with 128 for any other reason that leads it to call die(). Therefore the test can potentially succeed for the wrong reason. To avoid false positives, let's check test-config's stderr, in addition to the exit code, and make sure that the cause of the error is the one we expect in this test. Moreover, the test was using the auxiliary function check_config, which optionally takes a number to compare with test-config's exit code, and a string to compare with its stdout. Because the function does not check stderr, it can induce improper uses, like the one corrected in this patch. To avoid this, remove the optional expect_code parameter, disallowing tests that expect an error from test-config to use this helper function. There is one error, though, which is printed to stdout despite returning a non-zero code: "value not found" (exit code 1). For this one, let's add another function which properly checks stdout and the code. Helped-by: Eric Sunshine Signed-off-by: Matheus Tavares --- t/t1308-config-set.sh | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index 3a527e3a84..66c6363080 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -7,18 +7,15 @@ test_description='Test git config-set API in different settings' # 'check_config get_* section.key value' verifies that the entry for # section.key is 'value' check_config () { - if test "$1" = expect_code - then - expect_code="$2" && shift && shift - else - expect_code=0 - fi && - op=$1 key=$2 && shift && shift && - if test $# != 0 - then - printf "%s\n" "$@" - fi >expect && - test_expect_code $expect_code test-tool config "$op" "$key" >actual && + test-tool config "$1" "$2" >actual && + shift && shift && + printf "%s\n" "$@" >expect && + test_cmp expect actual +} + +check_not_found () { + test_expect_code 1 test-tool config "$1" "$2" >actual && + echo "Value not found for \"$2\"" >expect && test_cmp expect actual } @@ -108,7 +105,7 @@ test_expect_success 'key with case insensitive section header & variable' ' ' test_expect_success 'find value with misspelled key' ' - check_config expect_code 1 get_value "my.fOo Bar.hi" "Value not found for \"my.fOo Bar.hi\"" + check_not_found get_value "my.fOo Bar.hi" ' test_expect_success 'find value with the highest priority' ' @@ -121,7 +118,7 @@ test_expect_success 'find integer value for a key' ' test_expect_success 'find string value for a key' ' check_config get_string case.baz hask && - check_config expect_code 1 get_string case.ba "Value not found for \"case.ba\"" + check_not_found get_string case.ba ' test_expect_success 'check line error when NULL string is queried' ' @@ -130,7 +127,8 @@ test_expect_success 'check line error when NULL string is queried' ' ' test_expect_success 'find integer if value is non parse-able' ' - check_config expect_code 128 get_int lamb.head + test_expect_code 128 test-tool config get_int lamb.head 2>result && + test_i18ngrep "fatal: bad numeric config value .none. for .lamb\.head." result ' test_expect_success 'find bool value for the entered key' ' -- 2.28.0 ``` ## Matheus Tavares, 2020-09-10 17:21 Subject: [PATCH v6 4/9] t/helper/test-config: diagnose missing arguments Message-ID: <51656e43c35e5b83a004dcd7ed34499a0ebed137.1599758167.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/51656e43c35e5b83a004dcd7ed34499a0ebed137.1599758167.git.matheus.bernardino%40usp.br In-Reply-To: ``` test-config verifies that the correct number of arguments was given for all of its commands except for 'configset_get_value' and 'configset_get_value_multi'. Add the check to these two, so that we properly report missing arguments and prevent out-of-bounds access to argv[]. Signed-off-by: Matheus Tavares --- t/helper/test-config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 9e9d50099a..26d9c2ac4c 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -138,7 +138,7 @@ int cmd__config(int argc, const char **argv) printf("Value not found for \"%s\"\n", argv[2]); goto exit1; } - } else if (!strcmp(argv[1], "configset_get_value")) { + } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { @@ -156,7 +156,7 @@ int cmd__config(int argc, const char **argv) printf("Value not found for \"%s\"\n", argv[2]); goto exit1; } - } else if (!strcmp(argv[1], "configset_get_value_multi")) { + } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) { for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { -- 2.28.0 ``` ## Matheus Tavares, 2020-09-10 17:21 Subject: [PATCH v6 7/9] config: correctly read worktree configs in submodules Message-ID: <3a28b8e608c727d0e184fadf34ba4113933517c5.1599758167.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/3a28b8e608c727d0e184fadf34ba4113933517c5.1599758167.git.matheus.bernardino%40usp.br In-Reply-To: ``` The config machinery is not able to read worktree configs from a submodule in a process where the_repository represents the superproject and extensions.worktreeConfig is not set there. Furthermore, when extensions.worktreeConfig is set on the superproject, querying for a worktree config in a submodule will, instead, return the value set at the superproject. The relevant code is in do_git_config_sequence(). Although it is designed to act on an arbitrary repository, specified in the passed-in `struct config_options`, it accidentally depends on the_repository in two places: - it reads the global variable `repository_format_worktree_config`, which is set based on the content of the_repository, to determine whether extensions.worktreeConfig is set. - it uses the git_pathdup() helper to find the config.worktree file, instead of making a path using the passed-in repository. Sever these dependencies and add a regression test. Also, to avoid future misuses of `repository_format_worktree_config` like this one, remove this global variable and store the config value on `struct repository` itself. Signed-off-by: Matheus Tavares --- builtin/config.c | 2 +- cache.h | 1 - config.c | 12 +++++--- environment.c | 1 - repository.c | 1 + repository.h | 1 + setup.c | 4 +-- t/helper/test-config.c | 62 ++++++++++++++++++++++++++++++++------ t/t2404-worktree-config.sh | 16 ++++++++++ 9 files changed, 81 insertions(+), 19 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index ca4caedf33..586faad359 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -673,7 +673,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) given_config_source.scope = CONFIG_SCOPE_LOCAL; } else if (use_worktree_config) { struct worktree **worktrees = get_worktrees(); - if (repository_format_worktree_config) + if (!nongit && the_repository->worktree_config_extension) given_config_source.file = git_pathdup("config.worktree"); else if (worktrees[0] && worktrees[1]) die(_("--worktree cannot be used with multiple " diff --git a/cache.h b/cache.h index 4cad61ffa4..4cdce7b68f 100644 --- a/cache.h +++ b/cache.h @@ -1029,7 +1029,6 @@ extern int grafts_replace_parents; #define GIT_REPO_VERSION 0 #define GIT_REPO_VERSION_READ 1 extern int repository_format_precious_objects; -extern int repository_format_worktree_config; /* * You _have_ to initialize a `struct repository_format` using diff --git a/config.c b/config.c index 97f3022c92..38c177d1e5 100644 --- a/config.c +++ b/config.c @@ -1747,11 +1747,13 @@ static int do_git_config_sequence(const struct config_options *opts, ret += git_config_from_file(fn, repo_config, data); current_parsing_scope = CONFIG_SCOPE_WORKTREE; - if (!opts->ignore_worktree && repository_format_worktree_config) { - char *path = git_pathdup("config.worktree"); - if (!access_or_die(path, R_OK, 0)) - ret += git_config_from_file(fn, path, data); - free(path); + if (!opts->ignore_worktree && opts->repo && opts->repo->gitdir && + opts->repo->worktree_config_extension) { + struct strbuf path = STRBUF_INIT; + strbuf_repo_git_path(&path, opts->repo, "config.worktree"); + if (!access_or_die(path.buf, R_OK, 0)) + ret += git_config_from_file(fn, path.buf, data); + strbuf_release(&path); } current_parsing_scope = CONFIG_SCOPE_COMMAND; diff --git a/environment.c b/environment.c index bb518c61cd..5bfb8cf9c2 100644 --- a/environment.c +++ b/environment.c @@ -32,7 +32,6 @@ int warn_ambiguous_refs = 1; int warn_on_object_refname_ambiguity = 1; int ref_paranoia = -1; int repository_format_precious_objects; -int repository_format_worktree_config; const char *git_commit_encoding; const char *git_log_output_encoding; char *apply_default_whitespace; diff --git a/repository.c b/repository.c index a4174ddb06..8d18d9d3f2 100644 --- a/repository.c +++ b/repository.c @@ -170,6 +170,7 @@ int repo_init(struct repository *repo, goto error; repo_set_hash_algo(repo, format.hash_algo); + repo->worktree_config_extension = format.worktree_config; if (worktree) repo_set_worktree(repo, worktree); diff --git a/repository.h b/repository.h index 3c1f7d54bd..3d060bc3e6 100644 --- a/repository.h +++ b/repository.h @@ -136,6 +136,7 @@ struct repository { /* Indicate if a repository has a different 'commondir' from 'gitdir' */ unsigned different_commondir:1; + unsigned worktree_config_extension:1; }; extern struct repository *the_repository; diff --git a/setup.c b/setup.c index c04cd25a30..73cd42dbbe 100644 --- a/setup.c +++ b/setup.c @@ -567,11 +567,11 @@ static int check_repository_format_gently(const char *gitdir, struct repository_ repository_format_precious_objects = candidate->precious_objects; set_repository_format_partial_clone(candidate->partial_clone); - repository_format_worktree_config = candidate->worktree_config; + the_repository->worktree_config_extension = candidate->worktree_config; string_list_clear(&candidate->unknown_extensions, 0); string_list_clear(&candidate->v1_only_extensions, 0); - if (repository_format_worktree_config) { + if (the_repository->worktree_config_extension) { /* * pick up core.bare and core.worktree from per-worktree * config if present diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 06c61d91e1..87488aab6b 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -2,12 +2,20 @@ #include "cache.h" #include "config.h" #include "string-list.h" +#include "submodule-config.h" +#include "parse-options.h" /* * This program exposes the C API of the configuration mechanism * as a set of simple commands in order to facilitate testing. * - * Reads stdin and prints result of command to stdout: + * Usage: test-tool config [--submodule=] [] + * + * If --submodule= is given, will operate on the submodule at the + * given . This option is not valid for the commands: read_early_config, + * configset_get_value and configset_get_value_multi. + * + * Possible cmds are: * * get_value -> prints the value with highest priority for the entered key * @@ -72,14 +80,34 @@ static int early_config_cb(const char *var, const char *value, void *vdata) #define TC_VALUE_NOT_FOUND 1 #define TC_CONFIG_FILE_ERROR 2 +static const char *test_config_usage[] = { + "test-tool config [--submodule=] []", + NULL +}; + int cmd__config(int argc, const char **argv) { int i, val, ret = 0; const char *v; const struct string_list *strptr; struct config_set cs; + struct repository subrepo, *repo = the_repository; + const char *subrepo_path = NULL; + + struct option options[] = { + OPT_STRING(0, "submodule", &subrepo_path, "path", + "run on the submodule at "), + OPT_END() + }; + + argc = parse_options(argc, argv, NULL, options, test_config_usage, + PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_STOP_AT_NON_OPTION); + if (argc < 2) + usage_with_options(test_config_usage, options); if (argc == 3 && !strcmp(argv[1], "read_early_config")) { + if (subrepo_path) + die("cannot use --submodule with read_early_config"); read_early_config(early_config_cb, (void *)argv[2]); return 0; } @@ -88,11 +116,18 @@ int cmd__config(int argc, const char **argv) git_configset_init(&cs); - if (argc < 2) - die("Please, provide a command name on the command-line"); + if (subrepo_path) { + const struct submodule *sub; + + sub = submodule_from_path(the_repository, &null_oid, subrepo_path); + if (!sub || repo_submodule_init(&subrepo, the_repository, sub)) + die("invalid argument to --submodule: '%s'", subrepo_path); + + repo = &subrepo; + } if (argc == 3 && !strcmp(argv[1], "get_value")) { - if (!git_config_get_value(argv[2], &v)) { + if (!repo_config_get_value(repo, argv[2], &v)) { if (!v) printf("(NULL)\n"); else @@ -102,7 +137,7 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) { - strptr = git_config_get_value_multi(argv[2]); + strptr = repo_config_get_value_multi(repo, argv[2]); if (strptr) { for (i = 0; i < strptr->nr; i++) { v = strptr->items[i].string; @@ -116,27 +151,31 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_int")) { - if (!git_config_get_int(argv[2], &val)) { + if (!repo_config_get_int(repo, argv[2], &val)) { printf("%d\n", val); } else { printf("Value not found for \"%s\"\n", argv[2]); ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_bool")) { - if (!git_config_get_bool(argv[2], &val)) { + if (!repo_config_get_bool(repo, argv[2], &val)) { printf("%d\n", val); } else { + printf("Value not found for \"%s\"\n", argv[2]); ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_string")) { - if (!git_config_get_string_tmp(argv[2], &v)) { + if (!repo_config_get_string_tmp(repo, argv[2], &v)) { printf("%s\n", v); } else { printf("Value not found for \"%s\"\n", argv[2]); ret = TC_VALUE_NOT_FOUND; } } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { + if (subrepo_path) + die("cannot use --submodule with configset_get_value"); + for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { @@ -155,6 +194,9 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) { + if (subrepo_path) + die("cannot use --submodule with configset_get_value_multi"); + for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { @@ -177,12 +219,14 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (!strcmp(argv[1], "iterate")) { - git_config(iterate_cb, NULL); + repo_config(repo, iterate_cb, NULL); } else { die("%s: Please check the syntax and the function name", argv[0]); } out: git_configset_clear(&cs); + if (repo != the_repository) + repo_clear(repo); return ret; } diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh index 9536d10919..1e32c93735 100755 --- a/t/t2404-worktree-config.sh +++ b/t/t2404-worktree-config.sh @@ -78,4 +78,20 @@ test_expect_success 'config.worktree no longer read without extension' ' test_cmp_config -C wt2 shared this.is ' +test_expect_success 'correctly read config.worktree from submodules' ' + test_unconfig extensions.worktreeConfig && + git init sub && + ( + cd sub && + test_commit A && + git config extensions.worktreeConfig true && + git config --worktree wtconfig.sub test-value + ) && + git submodule add ./sub && + git commit -m "add sub" && + echo test-value >expect && + test-tool config --submodule=sub get_value wtconfig.sub >actual && + test_cmp expect actual +' + test_done -- 2.28.0 ``` ## Matheus Tavares, 2020-09-10 17:21 Subject: [PATCH v6 5/9] t/helper/test-config: unify exit labels Message-ID: <924d2e8ceb8b2131a13bafa356c5dc98eccae806.1599758167.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/924d2e8ceb8b2131a13bafa356c5dc98eccae806.1599758167.git.matheus.bernardino%40usp.br In-Reply-To: ``` test-config's main function has three different exit labels, all of which have to perform the same cleanup code before returning. Unify the labels in preparation for a future patch which will increase the cleanup section. Signed-off-by: Matheus Tavares --- t/helper/test-config.c | 49 ++++++++++++++++-------------------------- 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 26d9c2ac4c..06c61d91e1 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -69,9 +69,12 @@ static int early_config_cb(const char *var, const char *value, void *vdata) return 0; } +#define TC_VALUE_NOT_FOUND 1 +#define TC_CONFIG_FILE_ERROR 2 + int cmd__config(int argc, const char **argv) { - int i, val; + int i, val, ret = 0; const char *v; const struct string_list *strptr; struct config_set cs; @@ -94,10 +97,9 @@ int cmd__config(int argc, const char **argv) printf("(NULL)\n"); else printf("%s\n", v); - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) { strptr = git_config_get_value_multi(argv[2]); @@ -109,41 +111,38 @@ int cmd__config(int argc, const char **argv) else printf("%s\n", v); } - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_int")) { if (!git_config_get_int(argv[2], &val)) { printf("%d\n", val); - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_bool")) { if (!git_config_get_bool(argv[2], &val)) { printf("%d\n", val); - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_string")) { if (!git_config_get_string_tmp(argv[2], &v)) { printf("%s\n", v); - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { fprintf(stderr, "Error (%d) reading configuration file %s.\n", err, argv[i]); - goto exit2; + ret = TC_CONFIG_FILE_ERROR; + goto out; } } if (!git_configset_get_value(&cs, argv[2], &v)) { @@ -151,17 +150,17 @@ int cmd__config(int argc, const char **argv) printf("(NULL)\n"); else printf("%s\n", v); - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) { for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { fprintf(stderr, "Error (%d) reading configuration file %s.\n", err, argv[i]); - goto exit2; + ret = TC_CONFIG_FILE_ERROR; + goto out; } } strptr = git_configset_get_value_multi(&cs, argv[2]); @@ -173,27 +172,17 @@ int cmd__config(int argc, const char **argv) else printf("%s\n", v); } - goto exit0; } else { printf("Value not found for \"%s\"\n", argv[2]); - goto exit1; + ret = TC_VALUE_NOT_FOUND; } } else if (!strcmp(argv[1], "iterate")) { git_config(iterate_cb, NULL); - goto exit0; + } else { + die("%s: Please check the syntax and the function name", argv[0]); } - die("%s: Please check the syntax and the function name", argv[0]); - -exit0: - git_configset_clear(&cs); - return 0; - -exit1: - git_configset_clear(&cs); - return 1; - -exit2: +out: git_configset_clear(&cs); - return 2; + return ret; } -- 2.28.0 ``` ## Matheus Tavares, 2020-09-10 17:21 Subject: [PATCH v6 8/9] grep: honor sparse checkout patterns Message-ID: <2fc889c9c264fc10d878f31bd89cc44e79982516.1599758167.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/2fc889c9c264fc10d878f31bd89cc44e79982516.1599758167.git.matheus.bernardino%40usp.br In-Reply-To: ``` One of the main uses for a sparse checkout is to allow users to focus on the subset of files in a repository in which they are interested. But git-grep currently ignores the sparsity patterns and reports all matches found outside this subset, which kind of goes in the opposite direction. There are some use cases for ignoring the sparsity patterns and the next commit will add an option to obtain this behavior, but here we start by making grep honor the sparsity boundaries in every case where this is relevant: - git grep in worktree - git grep --cached - git grep $REVISION For the worktree and cached cases, we iterate over paths without the SKIP_WORKTREE bit set, and limit our searches to these paths. For the $REVISION case, we limit the paths we search to those that match the sparsity patterns. (We do not check the SKIP_WORKTREE bit for the $REVISION case, because $REVISION may contain paths that do not exist in HEAD and thus for which we have no SKIP_WORKTREE bit to consult. The sparsity patterns tell us how the SKIP_WORKTREE bit would be set if we were to check out $REVISION, so we consult those. Also, we don't use the sparsity patterns with the worktree or cached cases, both because we have a bit we can check directly and more efficiently, and because unmerged entries from a merge or a rebase could cause more files to temporarily be present than the sparsity patterns would normally select.) Note that there is a special case here: `git grep $TREE`. In this case, we cannot know whether $TREE corresponds to the root of the repository or some sub-tree, and thus there is no way for us to know which sparsity patterns, if any, apply. So the $TREE case will not use sparsity patterns or any SKIP_WORKTREE bits and will instead always search all files within the $TREE. Signed-off-by: Matheus Tavares --- builtin/grep.c | 125 ++++++++++++++++++-- t/t7011-skip-worktree-reading.sh | 9 -- t/t7817-grep-sparse-checkout.sh | 195 +++++++++++++++++++++++++++++++ 3 files changed, 312 insertions(+), 17 deletions(-) create mode 100755 t/t7817-grep-sparse-checkout.sh diff --git a/builtin/grep.c b/builtin/grep.c index f58979bc3f..a32815de0a 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -410,7 +410,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int cached); static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, struct tree_desc *tree, struct strbuf *base, int tn_len, - int check_attr); + int is_root_tree); static int grep_submodule(struct grep_opt *opt, const struct pathspec *pathspec, @@ -508,6 +508,10 @@ static int grep_cache(struct grep_opt *opt, for (nr = 0; nr < repo->index->cache_nr; nr++) { const struct cache_entry *ce = repo->index->cache[nr]; + + if (ce_skip_worktree(ce)) + continue; + strbuf_setlen(&name, name_base_len); strbuf_addstr(&name, ce->name); @@ -520,8 +524,7 @@ static int grep_cache(struct grep_opt *opt, * cache entry are identical, even if worktree file has * been modified, so use cache version instead */ - if (cached || (ce->ce_flags & CE_VALID) || - ce_skip_worktree(ce)) { + if (cached || (ce->ce_flags & CE_VALID)) { if (ce_stage(ce) || ce_intent_to_add(ce)) continue; hit |= grep_oid(opt, &ce->oid, name.buf, @@ -552,9 +555,76 @@ static int grep_cache(struct grep_opt *opt, return hit; } -static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, - struct tree_desc *tree, struct strbuf *base, int tn_len, - int check_attr) +static struct pattern_list *get_sparsity_patterns(struct repository *repo) +{ + struct pattern_list *patterns; + char *sparse_file; + int sparse_config, cone_config; + + if (repo_config_get_bool(repo, "core.sparsecheckout", &sparse_config) || + !sparse_config) { + return NULL; + } + + sparse_file = repo_git_path(repo, "info/sparse-checkout"); + patterns = xcalloc(1, sizeof(*patterns)); + + if (repo_config_get_bool(repo, "core.sparsecheckoutcone", &cone_config)) + cone_config = 0; + patterns->use_cone_patterns = cone_config; + + if (add_patterns_from_file_to_list(sparse_file, "", 0, patterns, NULL)) { + if (file_exists(sparse_file)) { + warning(_("failed to load sparse-checkout file: '%s'"), + sparse_file); + } + free(sparse_file); + free(patterns); + return NULL; + } + + free(sparse_file); + return patterns; +} + +static int path_in_sparse_checkout(struct strbuf *path, int prefix_len, + unsigned int entry_mode, + struct index_state *istate, + struct pattern_list *sparsity, + enum pattern_match_result parent_match, + enum pattern_match_result *match) +{ + int dtype = DT_UNKNOWN; + int is_dir = S_ISDIR(entry_mode); + + if (parent_match == MATCHED_RECURSIVE) { + *match = parent_match; + return 1; + } + + if (is_dir && !is_dir_sep(path->buf[path->len - 1])) + strbuf_addch(path, '/'); + + *match = path_matches_pattern_list(path->buf, path->len, + path->buf + prefix_len, &dtype, + sparsity, istate); + if (*match == UNDECIDED) + *match = parent_match; + + if (is_dir) + strbuf_trim_trailing_dir_sep(path); + + if (*match == NOT_MATCHED && + (!is_dir || (is_dir && sparsity->use_cone_patterns))) + return 0; + + return 1; +} + +static int do_grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, + struct tree_desc *tree, struct strbuf *base, int tn_len, + int check_attr, struct pattern_list *sparsity, + enum pattern_match_result default_sparsity_match) { struct repository *repo = opt->repo; int hit = 0; @@ -570,6 +640,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, while (tree_entry(tree, &entry)) { int te_len = tree_entry_len(&entry); + enum pattern_match_result sparsity_match = 0; if (match != all_entries_interesting) { strbuf_addstr(&name, base->buf + tn_len); @@ -586,6 +657,19 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, strbuf_add(base, entry.path, te_len); + if (sparsity) { + struct strbuf path = STRBUF_INIT; + strbuf_addstr(&path, base->buf + tn_len); + + if (!path_in_sparse_checkout(&path, old_baselen - tn_len, + entry.mode, repo->index, + sparsity, default_sparsity_match, + &sparsity_match)) { + strbuf_setlen(base, old_baselen); + continue; + } + } + if (S_ISREG(entry.mode)) { hit |= grep_oid(opt, &entry.oid, base->buf, tn_len, check_attr ? base->buf + tn_len : NULL); @@ -602,8 +686,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, strbuf_addch(base, '/'); init_tree_desc(&sub, data, size); - hit |= grep_tree(opt, pathspec, &sub, base, tn_len, - check_attr); + hit |= do_grep_tree(opt, pathspec, &sub, base, tn_len, + check_attr, sparsity, sparsity_match); free(data); } else if (recurse_submodules && S_ISGITLINK(entry.mode)) { hit |= grep_submodule(opt, pathspec, &entry.oid, @@ -621,6 +705,31 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, return hit; } +/* + * Note: sparsity patterns and paths' attributes will only be considered if + * is_root_tree has true value. (Otherwise, we cannot properly perform pattern + * matching on paths.) + */ +static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, + struct tree_desc *tree, struct strbuf *base, int tn_len, + int is_root_tree) +{ + struct pattern_list *patterns = NULL; + int ret; + + if (is_root_tree) + patterns = get_sparsity_patterns(opt->repo); + + ret = do_grep_tree(opt, pathspec, tree, base, tn_len, is_root_tree, + patterns, 0); + + if (patterns) { + clear_pattern_list(patterns); + free(patterns); + } + return ret; +} + static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, struct object *obj, const char *name, const char *path) { diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh index 37525cae3a..26852586ac 100755 --- a/t/t7011-skip-worktree-reading.sh +++ b/t/t7011-skip-worktree-reading.sh @@ -109,15 +109,6 @@ test_expect_success 'ls-files --modified' ' test -z "$(git ls-files -m)" ' -test_expect_success 'grep with skip-worktree file' ' - git update-index --no-skip-worktree 1 && - echo test > 1 && - git update-index 1 && - git update-index --skip-worktree 1 && - rm 1 && - test "$(git grep --no-ext-grep test)" = "1:test" -' - echo ":000000 100644 $ZERO_OID $EMPTY_BLOB A 1" > expected test_expect_success 'diff-index does not examine skip-worktree absent entries' ' setup_absent && diff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh new file mode 100755 index 0000000000..b3109e3479 --- /dev/null +++ b/t/t7817-grep-sparse-checkout.sh @@ -0,0 +1,195 @@ +#!/bin/sh + +test_description='grep in sparse checkout + +This test creates a repo with the following structure: + +. +|-- a +|-- b +|-- dir +| `-- c +|-- sub +| |-- A +| | `-- a +| `-- B +| `-- b +`-- sub2 + `-- a + +Where the outer repository has non-cone mode sparsity patterns, sub is a +submodule with cone mode sparsity patterns and sub2 is a submodule that is +excluded by the superproject sparsity patterns. The resulting sparse checkout +should leave the following structure in the working tree: + +. +|-- a +|-- sub +| `-- B +| `-- b +`-- sub2 + `-- a + +But note that sub2 should have the SKIP_WORKTREE bit set. +' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo "text" >a && + echo "text" >b && + mkdir dir && + echo "text" >dir/c && + + git init sub && + ( + cd sub && + mkdir A B && + echo "text" >A/a && + echo "text" >B/b && + git add A B && + git commit -m sub && + git sparse-checkout init --cone && + git sparse-checkout set B + ) && + + git init sub2 && + ( + cd sub2 && + echo "text" >a && + git add a && + git commit -m sub2 + ) && + + git submodule add ./sub && + git submodule add ./sub2 && + git add a b dir && + git commit -m super && + git sparse-checkout init --no-cone && + git sparse-checkout set "/*" "!b" "!/*/" "sub" && + + git tag -am tag-to-commit tag-to-commit HEAD && + tree=$(git rev-parse HEAD^{tree}) && + git tag -am tag-to-tree tag-to-tree $tree && + + test_path_is_missing b && + test_path_is_missing dir && + test_path_is_missing sub/A && + test_path_is_file a && + test_path_is_file sub/B/b && + test_path_is_file sub2/a +' + +# The test below checks a special case: the sparsity patterns exclude '/b' +# and sparse checkout is enabled, but the path exists in the working tree (e.g. +# manually created after `git sparse-checkout init`). In this case, grep should +# skip it. +test_expect_success 'grep in working tree should honor sparse checkout' ' + cat >expect <<-EOF && + a:text + EOF + echo "new-text" >b && + test_when_finished "rm b" && + git grep "text" >actual && + test_cmp expect actual +' + +test_expect_success 'grep unmerged file despite not matching sparsity patterns' ' + cat >expect <<-EOF && + b:modified-b-in-branchX + b:modified-b-in-branchY + EOF + test_when_finished "test_might_fail git merge --abort && \ + git checkout master" && + + git sparse-checkout disable && + git checkout -b branchY master && + test_commit modified-b-in-branchY b && + git checkout -b branchX master && + test_commit modified-b-in-branchX b && + + git sparse-checkout init && + test_path_is_missing b && + test_must_fail git merge branchY && + git grep "modified-b" >actual && + test_cmp expect actual +' + +test_expect_success 'grep --cached should honor sparse checkout' ' + cat >expect <<-EOF && + a:text + EOF + git grep --cached "text" >actual && + test_cmp expect actual +' + +test_expect_success 'grep should honor sparse checkout' ' + commit=$(git rev-parse HEAD) && + cat >expect_commit <<-EOF && + $commit:a:text + EOF + cat >expect_tag-to-commit <<-EOF && + tag-to-commit:a:text + EOF + git grep "text" $commit >actual_commit && + test_cmp expect_commit actual_commit && + git grep "text" tag-to-commit >actual_tag-to-commit && + test_cmp expect_tag-to-commit actual_tag-to-commit +' + +test_expect_success 'grep should ignore sparsity patterns' ' + commit=$(git rev-parse HEAD) && + tree=$(git rev-parse HEAD^{tree}) && + cat >expect_tree <<-EOF && + $tree:a:text + $tree:b:text + $tree:dir/c:text + EOF + cat >expect_tag-to-tree <<-EOF && + tag-to-tree:a:text + tag-to-tree:b:text + tag-to-tree:dir/c:text + EOF + git grep "text" $tree >actual_tree && + test_cmp expect_tree actual_tree && + git grep "text" tag-to-tree >actual_tag-to-tree && + test_cmp expect_tag-to-tree actual_tag-to-tree +' + +# Note that sub2/ is present in the worktree but it is excluded by the sparsity +# patterns, so grep should not recurse into it. +test_expect_success 'grep --recurse-submodules should honor sparse checkout in submodule' ' + cat >expect <<-EOF && + a:text + sub/B/b:text + EOF + git grep --recurse-submodules "text" >actual && + test_cmp expect actual +' + +test_expect_success 'grep --recurse-submodules --cached should honor sparse checkout in submodule' ' + cat >expect <<-EOF && + a:text + sub/B/b:text + EOF + git grep --recurse-submodules --cached "text" >actual && + test_cmp expect actual +' + +test_expect_success 'grep --recurse-submodules should honor sparse checkout in submodule' ' + commit=$(git rev-parse HEAD) && + cat >expect_commit <<-EOF && + $commit:a:text + $commit:sub/B/b:text + EOF + cat >expect_tag-to-commit <<-EOF && + tag-to-commit:a:text + tag-to-commit:sub/B/b:text + EOF + git grep --recurse-submodules "text" $commit >actual_commit && + test_cmp expect_commit actual_commit && + git grep --recurse-submodules "text" tag-to-commit >actual_tag-to-commit && + test_cmp expect_tag-to-commit actual_tag-to-commit +' + +test_done -- 2.28.0 ``` ## Matheus Tavares, 2020-09-10 17:21 Subject: [PATCH v6 9/9] config: add setting to ignore sparsity patterns in some cmds Message-ID: <92bc5351cf5fdbf7435ba4d99530d6ce02f957c4.1599758167.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/92bc5351cf5fdbf7435ba4d99530d6ce02f957c4.1599758167.git.matheus.bernardino%40usp.br In-Reply-To: ``` When sparse checkout is enabled, some users expect the output of certain commands (such as grep, diff, and log) to be also restricted within the sparsity patterns. This would allow them to effectively work only on the subset of files in which they are interested; and allow some commands to possibly perform better, by not considering uninteresting paths. For this reason, we taught grep to honor the sparsity patterns, in the previous patch. But, on the other hand, allowing grep and the other commands mentioned to optionally ignore the patterns also make for some interesting use cases. E.g. using grep to search for a function documentation that resides outside the sparse checkout. In any case, there is no current way for users to configure the behavior they want for these commands. Aiming to provide this flexibility, let's introduce the sparse.restrictCmds setting (and the analogous --[no]-restrict-to-sparse-paths global option). The default value is true. For now, grep is the only one affected by this setting, but the goal is to have support for more commands, in the future. Helped-by: Elijah Newren Signed-off-by: Matheus Tavares --- Documentation/config.txt | 2 + Documentation/config/grep.txt | 8 ++ Documentation/config/sparse.txt | 20 ++++ Documentation/git.txt | 4 + Makefile | 1 + builtin/grep.c | 13 ++- contrib/completion/git-completion.bash | 2 + git.c | 5 + sparse-checkout.c | 18 ++++ sparse-checkout.h | 11 +++ t/t7817-grep-sparse-checkout.sh | 132 ++++++++++++++++++++++++- t/t9902-completion.sh | 4 +- 12 files changed, 214 insertions(+), 6 deletions(-) create mode 100644 Documentation/config/sparse.txt create mode 100644 sparse-checkout.c create mode 100644 sparse-checkout.h diff --git a/Documentation/config.txt b/Documentation/config.txt index 3042d80978..3b6e0901b8 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -438,6 +438,8 @@ include::config/sequencer.txt[] include::config/showbranch.txt[] +include::config/sparse.txt[] + include::config/splitindex.txt[] include::config/ssh.txt[] diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt index dd51db38e1..a3275ab4b7 100644 --- a/Documentation/config/grep.txt +++ b/Documentation/config/grep.txt @@ -28,3 +28,11 @@ grep.fullName:: grep.fallbackToNoIndex:: If set to true, fall back to git grep --no-index if git grep is executed outside of a git repository. Defaults to false. + +ifdef::git-grep[] +sparse.restrictCmds:: + See base definition in linkgit:git-config[1]. grep honors + sparse.restrictCmds by limiting searches to the sparsity paths in three + cases: when searching the working tree, when searching the index with + --cached, and when searching a specified commit. +endif::git-grep[] diff --git a/Documentation/config/sparse.txt b/Documentation/config/sparse.txt new file mode 100644 index 0000000000..494761526e --- /dev/null +++ b/Documentation/config/sparse.txt @@ -0,0 +1,20 @@ +sparse.restrictCmds:: + Only meaningful in conjunction with core.sparseCheckout. This option + extends sparse checkouts (which limit which paths are written to the + working tree), so that output and operations are also limited to the + sparsity paths where possible and implemented. The purpose of this + option is to (1) focus output for the user on the portion of the + repository that is of interest to them, and (2) enable potentially + dramatic performance improvements, especially in conjunction with + partial clones. ++ +When this option is true (default), some git commands may limit their behavior +to the paths specified by the sparsity patterns, or to the intersection of +those paths and any (like `*.c`) that the user might also specify on the +command line. When false, the affected commands will work on full trees, +ignoring the sparsity patterns. For now, only git-grep honors this setting. ++ +Note: commands which export, integrity check, or create history will always +operate on full trees (e.g. fast-export, format-patch, fsck, commit, etc.), +unaffected by any sparsity patterns. Also, writing commands such as +sparse-checkout and read-tree will not be affected by this configuration. diff --git a/Documentation/git.txt b/Documentation/git.txt index 2fc92586b5..d857509573 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -180,6 +180,10 @@ If you just want to run git as if it was started in `` then use Do not perform optional operations that require locks. This is equivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`. +--[no-]restrict-to-sparse-paths:: + Overrides the sparse.restrictCmds configuration (see + linkgit:git-config[1]) for this execution. + --list-cmds=group[,group...]:: List commands by group. This is an internal/experimental option and may change or be removed in the future. Supported diff --git a/Makefile b/Makefile index 65f8cfb236..778c9e499e 100644 --- a/Makefile +++ b/Makefile @@ -982,6 +982,7 @@ LIB_OBJS += sha1-name.o LIB_OBJS += shallow.o LIB_OBJS += sideband.o LIB_OBJS += sigchain.o +LIB_OBJS += sparse-checkout.o LIB_OBJS += split-index.o LIB_OBJS += stable-qsort.o LIB_OBJS += strbuf.o diff --git a/builtin/grep.c b/builtin/grep.c index a32815de0a..3fa364e91c 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -25,6 +25,7 @@ #include "submodule-config.h" #include "object-store.h" #include "packfile.h" +#include "sparse-checkout.h" static char const * const grep_usage[] = { N_("git grep [] [-e] [...] [[--] ...]"), @@ -498,6 +499,7 @@ static int grep_cache(struct grep_opt *opt, int nr; struct strbuf name = STRBUF_INIT; int name_base_len = 0; + int sparse_paths_only = restrict_to_sparse_paths(repo); if (repo->submodule_prefix) { name_base_len = strlen(repo->submodule_prefix); strbuf_addstr(&name, repo->submodule_prefix); @@ -509,7 +511,7 @@ static int grep_cache(struct grep_opt *opt, for (nr = 0; nr < repo->index->cache_nr; nr++) { const struct cache_entry *ce = repo->index->cache[nr]; - if (ce_skip_worktree(ce)) + if (sparse_paths_only && ce_skip_worktree(ce)) continue; strbuf_setlen(&name, name_base_len); @@ -715,9 +717,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, int is_root_tree) { struct pattern_list *patterns = NULL; + int sparse_paths_only = restrict_to_sparse_paths(opt->repo); int ret; - if (is_root_tree) + if (is_root_tree && sparse_paths_only) patterns = get_sparsity_patterns(opt->repo); ret = do_grep_tree(opt, pathspec, tree, base, tn_len, is_root_tree, @@ -1258,6 +1261,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix) if (!use_index || untracked) { int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude; + + if (opt_restrict_to_sparse_paths >= 0) { + die(_("--[no-]restrict-to-sparse-paths is incompatible" + " with --no-index and --untracked")); + } + hit = grep_directory(&opt, &pathspec, use_exclude, use_index); } else if (0 <= opt_exclude) { die(_("--[no-]exclude-standard cannot be used for tracked contents")); diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 9147fba3d5..de12766a70 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -3402,6 +3402,8 @@ __git_main () --namespace= --no-replace-objects --help + --restrict-to-sparse-paths + --no-restrict-to-sparse-paths " ;; *) diff --git a/git.c b/git.c index 8bd1d7551d..81206b424c 100644 --- a/git.c +++ b/git.c @@ -5,6 +5,7 @@ #include "run-command.h" #include "alias.h" #include "shallow.h" +#include "sparse-checkout.h" #define RUN_SETUP (1<<0) #define RUN_SETUP_GENTLY (1<<1) @@ -311,6 +312,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) } else { exit(list_cmds(cmd)); } + } else if (!strcmp(cmd, "--restrict-to-sparse-paths")) { + opt_restrict_to_sparse_paths = 1; + } else if (!strcmp(cmd, "--no-restrict-to-sparse-paths")) { + opt_restrict_to_sparse_paths = 0; } else { fprintf(stderr, _("unknown option: %s\n"), cmd); usage(git_usage_string); diff --git a/sparse-checkout.c b/sparse-checkout.c new file mode 100644 index 0000000000..96c5ed5446 --- /dev/null +++ b/sparse-checkout.c @@ -0,0 +1,18 @@ +#include "cache.h" +#include "config.h" +#include "sparse-checkout.h" + +int opt_restrict_to_sparse_paths = -1; + +int restrict_to_sparse_paths(struct repository *repo) +{ + int ret; + + if (opt_restrict_to_sparse_paths >= 0) + return opt_restrict_to_sparse_paths; + + if (repo_config_get_bool(repo, "sparse.restrictcmds", &ret)) + ret = 1; + + return ret; +} diff --git a/sparse-checkout.h b/sparse-checkout.h new file mode 100644 index 0000000000..a4805e443a --- /dev/null +++ b/sparse-checkout.h @@ -0,0 +1,11 @@ +#ifndef SPARSE_CHECKOUT_H +#define SPARSE_CHECKOUT_H + +struct repository; + +extern int opt_restrict_to_sparse_paths; + +/* Whether or not cmds should restrict behavior on sparse paths, in this repo */ +int restrict_to_sparse_paths(struct repository *repo); + +#endif /* SPARSE_CHECKOUT_H */ diff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh index b3109e3479..f93a4f71d1 100755 --- a/t/t7817-grep-sparse-checkout.sh +++ b/t/t7817-grep-sparse-checkout.sh @@ -80,10 +80,10 @@ test_expect_success 'setup' ' test_path_is_file sub2/a ' -# The test below checks a special case: the sparsity patterns exclude '/b' +# The two tests below check a special case: the sparsity patterns exclude '/b' # and sparse checkout is enabled, but the path exists in the working tree (e.g. # manually created after `git sparse-checkout init`). In this case, grep should -# skip it. +# skip the file by default, but not with --no-restrict-to-sparse-paths. test_expect_success 'grep in working tree should honor sparse checkout' ' cat >expect <<-EOF && a:text @@ -93,6 +93,16 @@ test_expect_success 'grep in working tree should honor sparse checkout' ' git grep "text" >actual && test_cmp expect actual ' +test_expect_success 'grep w/ --no-restrict-to-sparse-paths for sparsely excluded but present paths' ' + cat >expect <<-EOF && + a:text + b:new-text + EOF + echo "new-text" >b && + test_when_finished "rm b" && + git --no-restrict-to-sparse-paths grep "text" >actual && + test_cmp expect actual +' test_expect_success 'grep unmerged file despite not matching sparsity patterns' ' cat >expect <<-EOF && @@ -157,7 +167,7 @@ test_expect_success 'grep should ignore sparsity patterns' ' ' # Note that sub2/ is present in the worktree but it is excluded by the sparsity -# patterns, so grep should not recurse into it. +# patterns, so grep should only recurse into it with --no-restrict-to-sparse-paths. test_expect_success 'grep --recurse-submodules should honor sparse checkout in submodule' ' cat >expect <<-EOF && a:text @@ -166,6 +176,15 @@ test_expect_success 'grep --recurse-submodules should honor sparse checkout in s git grep --recurse-submodules "text" >actual && test_cmp expect actual ' +test_expect_success 'grep --recurse-submodules should search in excluded submodules w/ --no-restrict-to-sparse-paths' ' + cat >expect <<-EOF && + a:text + sub/B/b:text + sub2/a:text + EOF + git --no-restrict-to-sparse-paths grep --recurse-submodules "text" >actual && + test_cmp expect actual +' test_expect_success 'grep --recurse-submodules --cached should honor sparse checkout in submodule' ' cat >expect <<-EOF && @@ -192,4 +211,111 @@ test_expect_success 'grep --recurse-submodules should honor sparse test_cmp expect_tag-to-commit actual_tag-to-commit ' +for cmd in 'git --no-restrict-to-sparse-paths grep' \ + 'git -c sparse.restrictCmds=false grep' \ + 'git -c sparse.restrictCmds=true --no-restrict-to-sparse-paths grep' +do + + test_expect_success "$cmd --cached should ignore sparsity patterns" ' + cat >expect <<-EOF && + a:text + b:text + dir/c:text + EOF + $cmd --cached "text" >actual && + test_cmp expect actual + ' + + test_expect_success "$cmd should ignore sparsity patterns" ' + commit=$(git rev-parse HEAD) && + cat >expect_commit <<-EOF && + $commit:a:text + $commit:b:text + $commit:dir/c:text + EOF + cat >expect_tag-to-commit <<-EOF && + tag-to-commit:a:text + tag-to-commit:b:text + tag-to-commit:dir/c:text + EOF + $cmd "text" $commit >actual_commit && + test_cmp expect_commit actual_commit && + $cmd "text" tag-to-commit >actual_tag-to-commit && + test_cmp expect_tag-to-commit actual_tag-to-commit + ' +done + +test_expect_success 'grep --recurse-submodules --cached w/ --no-restrict-to-sparse-paths' ' + cat >expect <<-EOF && + a:text + b:text + dir/c:text + sub/A/a:text + sub/B/b:text + sub2/a:text + EOF + git --no-restrict-to-sparse-paths grep --recurse-submodules --cached \ + "text" >actual && + test_cmp expect actual +' + +test_expect_success 'grep --recurse-submodules w/ --no-restrict-to-sparse-paths' ' + commit=$(git rev-parse HEAD) && + cat >expect_commit <<-EOF && + $commit:a:text + $commit:b:text + $commit:dir/c:text + $commit:sub/A/a:text + $commit:sub/B/b:text + $commit:sub2/a:text + EOF + cat >expect_tag-to-commit <<-EOF && + tag-to-commit:a:text + tag-to-commit:b:text + tag-to-commit:dir/c:text + tag-to-commit:sub/A/a:text + tag-to-commit:sub/B/b:text + tag-to-commit:sub2/a:text + EOF + git --no-restrict-to-sparse-paths grep --recurse-submodules "text" \ + $commit >actual_commit && + test_cmp expect_commit actual_commit && + git --no-restrict-to-sparse-paths grep --recurse-submodules "text" \ + tag-to-commit >actual_tag-to-commit && + test_cmp expect_tag-to-commit actual_tag-to-commit +' + +test_expect_success 'should respect the sparse.restrictCmds values from submodules' ' + cat >expect <<-EOF && + a:text + sub/A/a:text + sub/B/b:text + EOF + test_config -C sub sparse.restrictCmds false && + git grep --cached --recurse-submodules "text" >actual && + test_cmp expect actual +' + +test_expect_success 'should propagate --[no]-restrict-to-sparse-paths to submodules' ' + cat >expect <<-EOF && + a:text + b:text + dir/c:text + sub/A/a:text + sub/B/b:text + sub2/a:text + EOF + test_config -C sub sparse.restrictCmds true && + git --no-restrict-to-sparse-paths grep --cached --recurse-submodules "text" >actual && + test_cmp expect actual +' + +for opt in '--untracked' '--no-index' +do + test_expect_success "--[no]-restrict-to-sparse-paths and $opt are incompatible" " + test_must_fail git --restrict-to-sparse-paths grep $opt . 2>actual && + test_i18ngrep 'restrict-to-sparse-paths is incompatible with' actual + " +done + test_done diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 8425b9a531..a8c2ac9d70 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1928,6 +1928,8 @@ test_expect_success 'double dash "git" itself' ' --namespace= --no-replace-objects Z --help Z + --restrict-to-sparse-paths Z + --no-restrict-to-sparse-paths Z EOF ' @@ -1970,7 +1972,7 @@ test_expect_success 'general options' ' test_completion "git --nam" "--namespace=" && test_completion "git --bar" "--bare " && test_completion "git --inf" "--info-path " && - test_completion "git --no-r" "--no-replace-objects " + test_completion "git --no-rep" "--no-replace-objects " ' test_expect_success 'general options plus command' ' -- 2.28.0 ``` ## Matheus Tavares, 2020-09-10 17:21 Subject: [PATCH v6 6/9] config: make do_git_config_sequence receive a 'struct repository' Message-ID: URL: https://gitlist.dev/e/f5c0fc33365414987ff1cbeb8d38f11ab99b3004.1599758167.git.matheus.bernardino%40usp.br In-Reply-To: ``` From: Jonathan Nieder The following patch will fix a bug in do_git_config_sequence, which makes it ignore worktree-specific configurations on submodules when the_repository represents the superproject. To do so, the function will need access to the 'struct repository' instance of the submodule. But it currently only receives the 'git_dir' and 'commondir' paths through 'struct config_options'. So change the struct to hold a repository pointer instead of the two strings, and adjust its users. Signed-off-by: Jonathan Nieder Signed-off-by: Matheus Tavares --- Hi, Jonathan. I just made a small change in this patch, in read_early_repo(): when running some test cases in t0001, I noticed that `the_early_repo.settings.initialized` was 0 even though the repo was populated. So I added a flag to track the repo state for the later cleanup. builtin/config.c | 6 ++---- config.c | 35 +++++++++++++++++++++-------------- config.h | 4 ++-- 3 files changed, 25 insertions(+), 20 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 5e39f61885..ca4caedf33 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -699,10 +699,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) config_options.respect_includes = !given_config_source.file; else config_options.respect_includes = respect_includes_opt; - if (!nongit) { - config_options.commondir = get_git_common_dir(); - config_options.git_dir = get_git_dir(); - } + if (!nongit) + config_options.repo = the_repository; if (end_nul) { term = '\0'; diff --git a/config.c b/config.c index 2bdff4457b..97f3022c92 100644 --- a/config.c +++ b/config.c @@ -222,8 +222,8 @@ static int include_by_gitdir(const struct config_options *opts, const char *git_dir; int already_tried_absolute = 0; - if (opts->git_dir) - git_dir = opts->git_dir; + if (opts->repo && opts->repo->gitdir) + git_dir = opts->repo->gitdir; else goto done; @@ -1720,10 +1720,10 @@ static int do_git_config_sequence(const struct config_options *opts, char *repo_config; enum config_scope prev_parsing_scope = current_parsing_scope; - if (opts->commondir) - repo_config = mkpathdup("%s/config", opts->commondir); - else if (opts->git_dir) - BUG("git_dir without commondir"); + if (opts->repo && opts->repo->commondir) + repo_config = mkpathdup("%s/config", opts->repo->commondir); + else if (opts->repo && opts->repo->gitdir) + BUG("gitdir without commondir"); else repo_config = NULL; @@ -1824,27 +1824,35 @@ void read_early_config(config_fn_t cb, void *data) struct config_options opts = {0}; struct strbuf commondir = STRBUF_INIT; struct strbuf gitdir = STRBUF_INIT; + struct repository the_early_repo = {0}; + int early_repo_initialized = 0; opts.respect_includes = 1; if (have_git_dir()) { - opts.commondir = get_git_common_dir(); - opts.git_dir = get_git_dir(); + opts.repo = the_repository; /* * When setup_git_directory() was not yet asked to discover the * GIT_DIR, we ask discover_git_directory() to figure out whether there * is any repository config we should use (but unlike - * setup_git_directory_gently(), no global state is changed, most + * setup_git_directory_gently(), no global state is changed; most * notably, the current working directory is still the same after the * call). + * + * NEEDSWORK: There is some duplicate work between + * discover_git_directory and repo_init. Update to use a variant of + * repo_init that does its own repository discovery once available. */ - } else if (!discover_git_directory(&commondir, &gitdir)) { - opts.commondir = commondir.buf; - opts.git_dir = gitdir.buf; + } else if (!discover_git_directory(&commondir, &gitdir) && + !repo_init(&the_early_repo, gitdir.buf, NULL)) { + opts.repo = &the_early_repo; + early_repo_initialized = 1; } config_with_options(cb, data, NULL, &opts); + if (early_repo_initialized) + repo_clear(&the_early_repo); strbuf_release(&commondir); strbuf_release(&gitdir); } @@ -2097,8 +2105,7 @@ static void repo_read_config(struct repository *repo) struct config_options opts = { 0 }; opts.respect_includes = 1; - opts.commondir = repo->commondir; - opts.git_dir = repo->gitdir; + opts.repo = repo; if (!repo->config) repo->config = xcalloc(1, sizeof(struct config_set)); diff --git a/config.h b/config.h index 91cdfbfb41..e56293fb29 100644 --- a/config.h +++ b/config.h @@ -21,6 +21,7 @@ */ struct object_id; +struct repository; /* git_config_parse_key() returns these negated: */ #define CONFIG_INVALID_KEY 1 @@ -87,8 +88,7 @@ struct config_options { unsigned int ignore_worktree : 1; unsigned int ignore_cmdline : 1; unsigned int system_gently : 1; - const char *commondir; - const char *git_dir; + struct repository *repo; config_parser_event_fn_t event_fn; void *event_fn_data; enum config_error_action { -- 2.28.0 ``` ## Matheus Tavares, 2020-09-10 17:21 Subject: [PATCH v6 1/9] doc: grep: unify info on configuration variables Message-ID: <70c9a4e7415f532c2aa6fdd460a927b5e677cd97.1599758167.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/70c9a4e7415f532c2aa6fdd460a927b5e677cd97.1599758167.git.matheus.bernardino%40usp.br In-Reply-To: ``` Explanations about the configuration variables for git-grep are duplicated in "Documentation/git-grep.txt" and "Documentation/config/grep.txt", which can make maintenance difficult. The first also contains a definition not present in the latter (grep.fullName). To avoid problems like this, let's unify the information in the second file and include it in the first. Signed-off-by: Matheus Tavares --- Documentation/config/grep.txt | 10 ++++++++-- Documentation/git-grep.txt | 36 ++++++----------------------------- 2 files changed, 14 insertions(+), 32 deletions(-) diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt index 44abe45a7c..dd51db38e1 100644 --- a/Documentation/config/grep.txt +++ b/Documentation/config/grep.txt @@ -16,8 +16,14 @@ grep.extendedRegexp:: other than 'default'. grep.threads:: - Number of grep worker threads to use. - See `grep.threads` in linkgit:git-grep[1] for more information. + Number of grep worker threads to use. See `--threads` +ifndef::git-grep[] + in linkgit:git-grep[1] +endif::git-grep[] + for more information. + +grep.fullName:: + If set to true, enable `--full-name` option by default. grep.fallbackToNoIndex:: If set to true, fall back to git grep --no-index if git grep diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt index a7f9bc99ea..9bdf807584 100644 --- a/Documentation/git-grep.txt +++ b/Documentation/git-grep.txt @@ -41,34 +41,8 @@ characters. An empty string as search expression matches all lines. CONFIGURATION ------------- -grep.lineNumber:: - If set to true, enable `-n` option by default. - -grep.column:: - If set to true, enable the `--column` option by default. - -grep.patternType:: - Set the default matching behavior. Using a value of 'basic', 'extended', - 'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`, - `--fixed-strings`, or `--perl-regexp` option accordingly, while the - value 'default' will return to the default matching behavior. - -grep.extendedRegexp:: - If set to true, enable `--extended-regexp` option by default. This - option is ignored when the `grep.patternType` option is set to a value - other than 'default'. - -grep.threads:: - Number of grep worker threads to use. If unset (or set to 0), Git will - use as many threads as the number of logical cores available. - -grep.fullName:: - If set to true, enable `--full-name` option by default. - -grep.fallbackToNoIndex:: - If set to true, fall back to git grep --no-index if git grep - is executed outside of a git repository. Defaults to false. - +:git-grep: 1 +include::config/grep.txt[] OPTIONS ------- @@ -269,8 +243,10 @@ providing this option will cause it to die. found. --threads :: - Number of grep worker threads to use. - See `grep.threads` in 'CONFIGURATION' for more information. + Number of grep worker threads to use. If not provided (or set to + 0), Git will use as many worker threads as the number of logical + cores available. The default value can also be set with the + `grep.threads` configuration. -f :: Read patterns from , one per line. -- 2.28.0 ``` ## Matheus Tavares, 2020-09-10 17:21 Subject: [PATCH v6 3/9] t/helper/test-config: be consistent with exit codes Message-ID: <45d13744b7849ec485f76e0a67e972fdd01ba21a.1599758167.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/45d13744b7849ec485f76e0a67e972fdd01ba21a.1599758167.git.matheus.bernardino%40usp.br In-Reply-To: ``` The test-config helper can return at least three different exit codes to reflect the status of the requested operation. And these codes are checked in some of the tests. But there is an inconsistent place in the helper where an usage error returns the same code as a "value not found" error. Let's fix that and, while we are here, document the meaning of each exit code in the file's header. Signed-off-by: Matheus Tavares --- t/helper/test-config.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/t/helper/test-config.c b/t/helper/test-config.c index a6e936721f..9e9d50099a 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -30,6 +30,11 @@ * iterate -> iterate over all values using git_config(), and print some * data for each * + * Exit codes: + * 0: success + * 1: value not found for the given config key + * 2: config file path given as argument is inaccessible or doesn't exist + * * Examples: * * To print the value with highest priority for key "foo.bAr Baz.rock": @@ -80,10 +85,10 @@ int cmd__config(int argc, const char **argv) git_configset_init(&cs); - if (argc < 2) { - fprintf(stderr, "Please, provide a command name on the command-line\n"); - goto exit1; - } else if (argc == 3 && !strcmp(argv[1], "get_value")) { + if (argc < 2) + die("Please, provide a command name on the command-line"); + + if (argc == 3 && !strcmp(argv[1], "get_value")) { if (!git_config_get_value(argv[2], &v)) { if (!v) printf("(NULL)\n"); -- 2.28.0 ``` ## Matheus Tavares, 2021-02-09 21:33 Subject: [PATCH v7] grep: honor sparse-checkout on working tree searches Message-ID: <5f3f7ac77039d41d1692ceae4b0c5df3bb45b74a.1612901326.git.matheus.bernardino@usp.br> URL: https://gitlist.dev/e/5f3f7ac77039d41d1692ceae4b0c5df3bb45b74a.1612901326.git.matheus.bernardino%40usp.br In-Reply-To: ``` On a sparse checked out repository, `git grep` (without --cached) ends up searching the cache when an entry matches the search pathspec and has the SKIP_WORKTREE bit set. This is confusing both because the sparse paths are not expected to be in a working tree search (as they are not checked out), and because the output mixes working tree and cache results without distinguishing them. (Note that grep also resorts to the cache on working tree searches that include --assume-unchanged paths. But the whole point in that case is to assume that the contents of the index entry and the file are the same. This does not apply to the case of sparse paths, where the file isn't even expected to be present.) Fix that by teaching grep to honor the sparse-checkout rules for working tree searches. If the user wants to grep paths outside the current sparse-checkout definition, they may either update the sparsity rules to materialize the files, or use --cached to search all blobs registered in the index. Note: it might also be interesting to add a configuration option that allow users to search paths that are present despite having the SKIP_WORKTREE bit set, and/or to restrict searches in the index and past revisions too. These ideas are left as future improvements to avoid conflicting with other sparse-checkout topics currently in flight. Suggested-by: Elijah Newren Signed-off-by: Matheus Tavares --- This new version includes only the bug fix for the working tree grep, as discussed in [1]. I think there are a couple other patches that could be extracted from the previous v6 [2] and sent as standalone topics, without the risk of conflicting with the sparse-index work. E.g. the unification of the git-grep.txt and config/grep.txt doc files. I'll look into that tomorrow. [1]: https://lore.kernel.org/git/CABPp-BHwNoVnooqDFPAsZxBT9aR5Dwk5D9sDRCvYSb8akxAJgA@mail.gmail.com/ [2]: https://lore.kernel.org/git/cover.1599758167.git.matheus.bernardino@usp.br/ builtin/grep.c | 7 +- t/t7011-skip-worktree-reading.sh | 9 -- t/t7817-grep-sparse-checkout.sh | 174 +++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 11 deletions(-) create mode 100755 t/t7817-grep-sparse-checkout.sh diff --git a/builtin/grep.c b/builtin/grep.c index ca259af441..bff214c755 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -508,6 +508,10 @@ static int grep_cache(struct grep_opt *opt, for (nr = 0; nr < repo->index->cache_nr; nr++) { const struct cache_entry *ce = repo->index->cache[nr]; + + if (!cached && ce_skip_worktree(ce)) + continue; + strbuf_setlen(&name, name_base_len); strbuf_addstr(&name, ce->name); @@ -520,8 +524,7 @@ static int grep_cache(struct grep_opt *opt, * cache entry are identical, even if worktree file has * been modified, so use cache version instead */ - if (cached || (ce->ce_flags & CE_VALID) || - ce_skip_worktree(ce)) { + if (cached || (ce->ce_flags & CE_VALID)) { if (ce_stage(ce) || ce_intent_to_add(ce)) continue; hit |= grep_oid(opt, &ce->oid, name.buf, diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh index 37525cae3a..26852586ac 100755 --- a/t/t7011-skip-worktree-reading.sh +++ b/t/t7011-skip-worktree-reading.sh @@ -109,15 +109,6 @@ test_expect_success 'ls-files --modified' ' test -z "$(git ls-files -m)" ' -test_expect_success 'grep with skip-worktree file' ' - git update-index --no-skip-worktree 1 && - echo test > 1 && - git update-index 1 && - git update-index --skip-worktree 1 && - rm 1 && - test "$(git grep --no-ext-grep test)" = "1:test" -' - echo ":000000 100644 $ZERO_OID $EMPTY_BLOB A 1" > expected test_expect_success 'diff-index does not examine skip-worktree absent entries' ' setup_absent && diff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh new file mode 100755 index 0000000000..590b99bbb6 --- /dev/null +++ b/t/t7817-grep-sparse-checkout.sh @@ -0,0 +1,174 @@ +#!/bin/sh + +test_description='grep in sparse checkout + +This test creates a repo with the following structure: + +. +|-- a +|-- b +|-- dir +| `-- c +|-- sub +| |-- A +| | `-- a +| `-- B +| `-- b +`-- sub2 + `-- a + +Where the outer repository has non-cone mode sparsity patterns, sub is a +submodule with cone mode sparsity patterns and sub2 is a submodule that is +excluded by the superproject sparsity patterns. The resulting sparse checkout +should leave the following structure in the working tree: + +. +|-- a +|-- sub +| `-- B +| `-- b +`-- sub2 + `-- a + +But note that sub2 should have the SKIP_WORKTREE bit set. +' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo "text" >a && + echo "text" >b && + mkdir dir && + echo "text" >dir/c && + + git init sub && + ( + cd sub && + mkdir A B && + echo "text" >A/a && + echo "text" >B/b && + git add A B && + git commit -m sub && + git sparse-checkout init --cone && + git sparse-checkout set B + ) && + + git init sub2 && + ( + cd sub2 && + echo "text" >a && + git add a && + git commit -m sub2 + ) && + + git submodule add ./sub && + git submodule add ./sub2 && + git add a b dir && + git commit -m super && + git sparse-checkout init --no-cone && + git sparse-checkout set "/*" "!b" "!/*/" "sub" && + + git tag -am tag-to-commit tag-to-commit HEAD && + tree=$(git rev-parse HEAD^{tree}) && + git tag -am tag-to-tree tag-to-tree $tree && + + test_path_is_missing b && + test_path_is_missing dir && + test_path_is_missing sub/A && + test_path_is_file a && + test_path_is_file sub/B/b && + test_path_is_file sub2/a && + git branch -m main +' + +# The test below covers a special case: the sparsity patterns exclude '/b' and +# sparse checkout is enabled, but the path exists in the working tree (e.g. +# manually created after `git sparse-checkout init`). git grep should skip it. +test_expect_success 'working tree grep honors sparse checkout' ' + cat >expect <<-EOF && + a:text + EOF + test_when_finished "rm -f b" && + echo "new-text" >b && + git grep "text" >actual && + test_cmp expect actual +' + +test_expect_success 'grep searches unmerged file despite not matching sparsity patterns' ' + cat >expect <<-EOF && + b:modified-b-in-branchX + b:modified-b-in-branchY + EOF + test_when_finished "test_might_fail git merge --abort && \ + git checkout main && git sparse-checkout init" && + + git sparse-checkout disable && + git checkout -b branchY main && + test_commit modified-b-in-branchY b && + git checkout -b branchX main && + test_commit modified-b-in-branchX b && + + git sparse-checkout init && + test_path_is_missing b && + test_must_fail git merge branchY && + git grep "modified-b" >actual && + test_cmp expect actual +' + +test_expect_success 'grep --cached searches entries with the SKIP_WORKTREE bit' ' + cat >expect <<-EOF && + a:text + b:text + dir/c:text + EOF + git grep --cached "text" >actual && + test_cmp expect actual +' + +# Note that sub2/ is present in the worktree but it is excluded by the sparsity +# patterns, so grep should not recurse into it. +test_expect_success 'grep --recurse-submodules honors sparse checkout in submodule' ' + cat >expect <<-EOF && + a:text + sub/B/b:text + EOF + git grep --recurse-submodules "text" >actual && + test_cmp expect actual +' + +test_expect_success 'grep --recurse-submodules --cached searches entries with the SKIP_WORKTREE bit' ' + cat >expect <<-EOF && + a:text + b:text + dir/c:text + sub/A/a:text + sub/B/b:text + sub2/a:text + EOF + git grep --recurse-submodules --cached "text" >actual && + test_cmp expect actual +' + +test_expect_success 'working tree grep does not search the index with CE_VALID and SKIP_WORKTREE' ' + cat >expect <<-EOF && + a:text + EOF + test_when_finished "git update-index --no-assume-unchanged b" && + git update-index --assume-unchanged b && + git grep text >actual && + test_cmp expect actual +' + +test_expect_success 'grep --cached searches index entries with both CE_VALID and SKIP_WORKTREE' ' + cat >expect <<-EOF && + a:text + b:text + dir/c:text + EOF + test_when_finished "git update-index --no-assume-unchanged b" && + git update-index --assume-unchanged b && + git grep --cached text >actual && + test_cmp expect actual +' + +test_done -- 2.29.2 ``` ## Junio C Hamano, 2021-02-09 23:23 Subject: Re: [PATCH v7] grep: honor sparse-checkout on working tree searches Message-ID: URL: https://gitlist.dev/e/xmqqtuqkygfb.fsf%40gitster.c.googlers.com In-Reply-To: <5f3f7ac77039d41d1692ceae4b0c5df3bb45b74a.1612901326.git.matheus.bernardino@usp.br> ``` Matheus Tavares writes: > This new version includes only the bug fix for the working tree grep, as > discussed in [1]. I think there are a couple other patches that could be > extracted from the previous v6 [2] and sent as standalone topics, > without the risk of conflicting with the sparse-index work. E.g. the > unification of the git-grep.txt and config/grep.txt doc files. I'll look > into that tomorrow. As mt/rm-sparse-checkout depends on the v6 that you are ejecting with this patch, I'll stop merging that topic to 'seen' for now. Thanks for keeping an eye on this one. ``` ## Elijah Newren, 2021-02-10 06:12 Subject: Re: [PATCH v7] grep: honor sparse-checkout on working tree searches Message-ID: URL: https://gitlist.dev/e/CABPp-BFMk7XX-qkXC_C6vh_x7OPUS%2BDfu-6SFWvm2WVA14zLSg%40mail.gmail.com In-Reply-To: ``` On Tue, Feb 9, 2021 at 3:23 PM Junio C Hamano wrote: > > Matheus Tavares writes: > > > This new version includes only the bug fix for the working tree grep, as > > discussed in [1]. I think there are a couple other patches that could be > > extracted from the previous v6 [2] and sent as standalone topics, > > without the risk of conflicting with the sparse-index work. E.g. the > > unification of the git-grep.txt and config/grep.txt doc files. I'll look > > into that tomorrow. > > As mt/rm-sparse-checkout depends on the v6 that you are ejecting > with this patch, I'll stop merging that topic to 'seen' for now. > > Thanks for keeping an eye on this one. Yeah, Matheus and I have been talking about both topics off-list a bit; I hadn't yet responded to his final revision of a newer mt/rm-sparse-checkout but he has a good replacement for that topic now that he'll send to the list soon. Anyway, for the patch submitted here you can add a Reviewed-by: Elijah Newren ```