{"thread":{"id":"54177","subject":"[PATCH v5 1/8] doc: grep: unify info on configuration variables","startedAt":"2020-09-02T06:18:02Z","lastAt":"2021-02-10T06:22:21Z","messageCount":30,"participants":["Matheus Tavares","Eric Sunshine","Matheus Tavares Bernardino","Jonathan Nieder","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":5,"patchTotal":8},"messages":[{"id":"404898","messageId":"70c9a4e7415f532c2aa6fdd460a927b5e677cd97.1599026986.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599026986.git.matheus.bernardino@usp.br","subject":"[PATCH v5 1/8] doc: grep: unify info on configuration variables","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-02T06:17:30Z","receivedAt":"2020-09-02T06:18:02Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Explanations about the configuration variables for git-grep are\nduplicated in \"Documentation/git-grep.txt\" and\n\"Documentation/config/grep.txt\", which can make maintenance difficult.\nThe first also contains a definition not present in the latter\n(grep.fullName). To avoid problems like this, let's unify the\ninformation in the second file and include it in the first.\n\nSigned-off-by: Matheus Tavares \n---\n Documentation/config/grep.txt | 10 ++++++++--\n Documentation/git-grep.txt | 36 ++++++-----------------------------\n 2 files changed, 14 insertions(+), 32 deletions(-)\n\ndiff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt\nindex 44abe45a7c..dd51db38e1 100644\n--- a/Documentation/config/grep.txt\n+++ b/Documentation/config/grep.txt\n@@ -16,8 +16,14 @@ grep.extendedRegexp::\n \tother than 'default'.\n \n grep.threads::\n-\tNumber of grep worker threads to use.\n-\tSee `grep.threads` in linkgit:git-grep[1] for more information.\n+\tNumber of grep worker threads to use. See `--threads`\n+ifndef::git-grep[]\n+\tin linkgit:git-grep[1]\n+endif::git-grep[]\n+\tfor more information.\n+\n+grep.fullName::\n+\tIf set to true, enable `--full-name` option by default.\n \n grep.fallbackToNoIndex::\n \tIf set to true, fall back to git grep --no-index if git grep\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex a7f9bc99ea..9bdf807584 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -41,34 +41,8 @@ characters. An empty string as search expression matches all lines.\n CONFIGURATION\n -------------\n \n-grep.lineNumber::\n-\tIf set to true, enable `-n` option by default.\n-\n-grep.column::\n-\tIf set to true, enable the `--column` option by default.\n-\n-grep.patternType::\n-\tSet the default matching behavior. Using a value of 'basic', 'extended',\n-\t'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,\n-\t`--fixed-strings`, or `--perl-regexp` option accordingly, while the\n-\tvalue 'default' will return to the default matching behavior.\n-\n-grep.extendedRegexp::\n-\tIf set to true, enable `--extended-regexp` option by default. This\n-\toption is ignored when the `grep.patternType` option is set to a value\n-\tother than 'default'.\n-\n-grep.threads::\n-\tNumber of grep worker threads to use. If unset (or set to 0), Git will\n-\tuse as many threads as the number of logical cores available.\n-\n-grep.fullName::\n-\tIf set to true, enable `--full-name` option by default.\n-\n-grep.fallbackToNoIndex::\n-\tIf set to true, fall back to git grep --no-index if git grep\n-\tis executed outside of a git repository. Defaults to false.\n-\n+:git-grep: 1\n+include::config/grep.txt[]\n \n OPTIONS\n -------\n@@ -269,8 +243,10 @@ providing this option will cause it to die.\n \tfound.\n \n --threads ::\n-\tNumber of grep worker threads to use.\n-\tSee `grep.threads` in 'CONFIGURATION' for more information.\n+\tNumber of grep worker threads to use. If not provided (or set to\n+\t0), Git will use as many worker threads as the number of logical\n+\tcores available. The default value can also be set with the\n+\t`grep.threads` configuration.\n \n -f ::\n \tRead patterns from , one per line.\n-- \n2.28.0\n\n"},{"id":"404899","messageId":"cover.1599026986.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1591974940.git.matheus.bernardino@usp.br","subject":"[PATCH v5 0/8] grep: honor sparse checkout and add option to ignore it","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-02T06:17:29Z","receivedAt":"2020-09-02T06:18:02Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"This series makes git-grep restrict its output to the sparsity patterns when\nrequested by the user. A new global option is added to control this behavior\nin grep and hopefully more commands in the future. There are also a\ncouple fixes in t/helper/test-config and in a test that uses it.\n\nChanges since v4:\n\n- Rebased on top of master to use repo_config_get_string_tmp(), added in\n jk/leakfix, in t/helper/test-config (patch 6).\n\n- Added patch 2, to make sure a test that relies on test-config checks its\n output in addition to the exit code, to avoid false positives.\n\n- Split patch \"t/helper/test-config: return exit codes consistently\" into\n three separated ones, as these are in fact three non-related changes:\n\t\"t/helper/test-config: unify exit labels\"\n\t\"t/helper/test-config: check argc before accessing argv\"\n\t\"t/helper/test-config: be consistent with exit codes\"\n\n- Removed TC_USAGE_ERROR in favor of calling die(). Also removed the\n test_config_exit_code enum.\n\n- On \"config: correctly read worktree configs in submodules\":\n * Improved commit message to focus on the problem instead of the\n implementation and remove section about the grep example.\n * Made use of the parse_options API\n * Allocated subrepo struct in the stack instead of malloc()'ing.\n\nMatheus Tavares (8):\n doc: grep: unify info on configuration variables\n t1308-config-set: avoid false positives when using test-config\n t/helper/test-config: be consistent with exit codes\n t/helper/test-config: check argc before accessing argv\n t/helper/test-config: unify exit labels\n config: correctly read worktree configs in submodules\n grep: honor sparse checkout patterns\n config: add setting to ignore sparsity patterns in some cmds\n\n Documentation/config.txt | 2 +\n Documentation/config/grep.txt | 18 +-\n Documentation/config/sparse.txt | 20 ++\n Documentation/git-grep.txt | 36 +--\n Documentation/git.txt | 4 +\n Makefile | 1 +\n builtin/grep.c | 134 ++++++++++-\n config.c | 21 +-\n contrib/completion/git-completion.bash | 2 +\n git.c | 5 +\n sparse-checkout.c | 18 ++\n sparse-checkout.h | 11 +\n t/helper/test-config.c | 126 ++++++----\n t/t1308-config-set.sh | 8 +-\n t/t2404-worktree-config.sh | 16 ++\n t/t7011-skip-worktree-reading.sh | 9 -\n t/t7817-grep-sparse-checkout.sh | 321 +++++++++++++++++++++++++\n t/t9902-completion.sh | 4 +-\n 18 files changed, 652 insertions(+), 104 deletions(-)\n create mode 100644 Documentation/config/sparse.txt\n create mode 100644 sparse-checkout.c\n create mode 100644 sparse-checkout.h\n create mode 100755 t/t7817-grep-sparse-checkout.sh\n\nRange-diff against v4:\n1: fc47a96bfa = 1: 70c9a4e741 doc: grep: unify info on configuration variables\n-: ---------- > 2: f53782f14c t1308-config-set: avoid false positives when using test-config\n-: ---------- > 3: 85e1588d6c t/helper/test-config: be consistent with exit codes\n-: ---------- > 4: 0750191342 t/helper/test-config: check argc before accessing argv\n2: 874aab36dd ! 5: 56535b0e36 t/helper/test-config: return exit codes consistently\n @@ Metadata\n Author: Matheus Tavares \n \n ## Commit message ##\n - t/helper/test-config: return exit codes consistently\n + t/helper/test-config: unify exit labels\n \n - The test-config helper may exit with a variety of at least four\n - different codes, to reflect the status of the requested operations.\n - These codes are sometimes checked in the tests, but not all of the codes\n - are returned consistently by the helper: 1 will usually refer to a\n - \"value not found\", but usage errors can also return 1 or 128. Moreover,\n - 128 is also expected on errors within the configset functions. These\n - inconsistent uses of the exit codes can lead to false positives in the\n - tests. Although all tests which expect errors and check the helper's\n - exit code currently also check the output, it's still better to\n - standardize the exit codes and avoid future problems in new tests.\n - While we are here, let's also check that we have the expected argc for\n - configset_get_value and configset_get_value_multi, before trying to use\n - argv.\n -\n - Note: this change is implemented with the unification of the exit\n - labels. This might seem unnecessary, for now, but it will benefit the\n - next patch, which will increase the cleanup section.\n + test-config's main function has three different exit labels, all of\n + which have to perform the same cleanup code before returning. Unify the\n + labels in preparation for a future patch which will increase the cleanup\n + section.\n \n Signed-off-by: Matheus Tavares \n \n ## t/helper/test-config.c ##\n -@@\n - * iterate -> iterate over all values using git_config(), and print some\n - * data for each\n - *\n -+ * Exit codes:\n -+ * 0: success\n -+ * 1: value not found for the given config key\n -+ * 2: config file path given as argument is inaccessible or doesn't exist\n -+ * 129: test-config usage error\n -+ *\n -+ * Note: tests may also expect 128 for die() calls in the config machinery.\n -+ *\n - * Examples:\n - *\n - * To print the value with highest priority for key \"foo.bAr Baz.rock\":\n @@ t/helper/test-config.c: static int early_config_cb(const char *var, const char *value, void *vdata)\n \treturn 0;\n }\n \n -+enum test_config_exit_code {\n -+\tTC_SUCCESS = 0,\n -+\tTC_VALUE_NOT_FOUND = 1,\n -+\tTC_CONFIG_FILE_ERROR = 2,\n -+\tTC_USAGE_ERROR = 129,\n -+};\n ++#define TC_VALUE_NOT_FOUND 1\n ++#define TC_CONFIG_FILE_ERROR 2\n +\n int cmd__config(int argc, const char **argv)\n {\n - \tint i, val;\n +-\tint i, val;\n ++\tint i, val, ret = 0;\n \tconst char *v;\n \tconst struct string_list *strptr;\n \tstruct config_set cs;\n -+\tenum test_config_exit_code ret = TC_SUCCESS;\n \n \tif (argc == 3 && !strcmp(argv[1], \"read_early_config\")) {\n \t\tread_early_config(early_config_cb, (void *)argv[2]);\n -\t\treturn 0;\n -+\t\treturn TC_SUCCESS;\n ++\t\treturn ret;\n \t}\n \n \tsetup_git_directory();\n - \n - \tgit_configset_init(&cs);\n - \n --\tif (argc < 2) {\n --\t\tfprintf(stderr, \"Please, provide a command name on the command-line\\n\");\n --\t\tgoto exit1;\n --\t} else if (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n -+\tif (argc < 2)\n -+\t\tgoto print_usage_error;\n -+\n -+\tif (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n - \t\tif (!git_config_get_value(argv[2], &v)) {\n - \t\t\tif (!v)\n +@@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n \t\t\t\tprintf(\"(NULL)\\n\");\n \t\t\telse\n \t\t\t\tprintf(\"%s\\n\", v);\n @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n -\t\t\tgoto exit1;\n +\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n --\t} else if (!strcmp(argv[1], \"configset_get_value\")) {\n -+\t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value\")) {\n + \t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n -\t\t\tgoto exit1;\n +\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n --\t} else if (!strcmp(argv[1], \"configset_get_value_multi\")) {\n -+\t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value_multi\")) {\n + \t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value_multi\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n \t\tgit_config(iterate_cb, NULL);\n -\t\tgoto exit0;\n +\t} else {\n -+print_usage_error:\n -+\t\tfprintf(stderr, \"Invalid syntax. Usage: test-tool config [args]\\n\");\n -+\t\tret = TC_USAGE_ERROR;\n ++\t\tdie(\"%s: Please check the syntax and the function name\", argv[0]);\n \t}\n \n -\tdie(\"%s: Please check the syntax and the function name\", argv[0]);\n3: c5093099f3 < -: ---------- t/helper/test-config: facilitate addition of new cli options\n4: b510de0de0 ! 6: 3e02e1bd24 config: correctly read worktree configs in submodules\n @@ Metadata\n ## Commit message ##\n config: correctly read worktree configs in submodules\n \n - One of the steps in do_git_config_sequence() is to load the\n - worktree-specific config file. Although the function receives a git_dir\n - string, it relies on git_pathdup(), which uses the_repository->git_dir,\n - to make the path to the file. Furthermore, it also checks that\n - extensions.worktreeConfig is set through the\n - repository_format_worktree_config variable, which refers to\n - the_repository only. Thus, when a submodule has worktree-specific\n - settings, a command executed in the superproject that recurses into the\n - submodule won't find the said settings.\n + The config machinery is not able to read worktree configs from a\n + submodule in a process where the_repository represents the superproject.\n + Furthermore, when extensions.worktreeConfig is set on the superproject,\n + querying for a worktree config in a submodule will, instead, return\n + the value set at the superproject.\n \n - This will be especially important in the next patch: git-grep will learn\n - to honor sparse checkouts and, when running with --recurse-submodules,\n - the submodule's sparse checkout settings must be loaded. As these\n - settings are stored in the config.worktree file, they would be ignored\n - without this patch. So let's fix this by reading the right\n - config.worktree file and extensions.worktreeConfig setting, based on the\n - git_dir and commondir paths given to do_git_config_sequence(). Also\n - add a test to avoid any regressions.\n + The problem resides in do_git_config_sequence(). Although the function\n + receives a git_dir string, it uses the_repository->git_dir when making\n + the path to the worktree config file. And when checking if\n + extensions.worktreeConfig is set, it uses the global\n + repository_format_worktree_config variable, which refers to\n + the_repository only. So let's fix this by using the git_dir given to the\n + function and reading the extension value from the right place. Also add\n + a test to avoid any regressions.\n \n Signed-off-by: Matheus Tavares \n \n @@ t/helper/test-config.c\n #include \"config.h\"\n #include \"string-list.h\"\n +#include \"submodule-config.h\"\n ++#include \"parse-options.h\"\n \n /*\n * This program exposes the C API of the configuration mechanism\n @@ t/helper/test-config.c\n *\n * get_value -> prints the value with highest priority for the entered key\n *\n -@@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n +@@ t/helper/test-config.c: static int early_config_cb(const char *var, const char *value, void *vdata)\n + #define TC_VALUE_NOT_FOUND 1\n + #define TC_CONFIG_FILE_ERROR 2\n + \n ++static const char *test_config_usage[] = {\n ++\t\"test-tool config [--submodule=] []\",\n ++\tNULL\n ++};\n ++\n + int cmd__config(int argc, const char **argv)\n + {\n + \tint i, val, ret = 0;\n + \tconst char *v;\n \tconst struct string_list *strptr;\n - \tstruct config_set cs = { .hash_initialized = 0 };\n - \tenum test_config_exit_code ret = TC_SUCCESS;\n -+\tstruct repository *repo = the_repository;\n + \tstruct config_set cs;\n ++\tstruct repository subrepo, *repo = the_repository;\n +\tconst char *subrepo_path = NULL;\n - \n - \targc--; /* skip over \"config\" */\n - \targv++;\n -@@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n - \tif (argc == 0)\n - \t\tgoto print_usage_error;\n - \n -+\tif (skip_prefix(*argv, \"--submodule=\", &subrepo_path)) {\n -+\t\targc--;\n -+\t\targv++;\n -+\t\tif (argc == 0)\n -+\t\t\tgoto print_usage_error;\n -+\t}\n +\n - \tif (argc == 2 && !strcmp(argv[0], \"read_early_config\")) {\n -+\t\tif (subrepo_path) {\n -+\t\t\tfprintf(stderr, \"Cannot use --submodule with read_early_config\\n\");\n -+\t\t\treturn TC_USAGE_ERROR;\n -+\t\t}\n - \t\tread_early_config(early_config_cb, (void *)argv[1]);\n - \t\treturn TC_SUCCESS;\n ++\tstruct option options[] = {\n ++\t\tOPT_STRING(0, \"submodule\", &subrepo_path, \"path\",\n ++\t\t\t \"run on the submodule at \"),\n ++\t\tOPT_END()\n ++\t};\n ++\n ++\targc = parse_options(argc, argv, NULL, options, test_config_usage,\n ++\t\t\t PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_STOP_AT_NON_OPTION);\n ++\tif (argc < 2)\n ++\t\tdie(\"Please, provide a command name on the command-line\");\n + \n + \tif (argc == 3 && !strcmp(argv[1], \"read_early_config\")) {\n ++\t\tif (subrepo_path)\n ++\t\t\tdie(\"cannot use --submodule with read_early_config\");\n + \t\tread_early_config(early_config_cb, (void *)argv[2]);\n + \t\treturn ret;\n \t}\n @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n - \tsetup_git_directory();\n + \n \tgit_configset_init(&cs);\n \n +-\tif (argc < 2)\n +-\t\tdie(\"Please, provide a command name on the command-line\");\n +\tif (subrepo_path) {\n +\t\tconst struct submodule *sub;\n -+\t\tstruct repository *subrepo = xcalloc(1, sizeof(*repo));\n +\n +\t\tsub = submodule_from_path(the_repository, &null_oid, subrepo_path);\n -+\t\tif (!sub || repo_submodule_init(subrepo, the_repository, sub)) {\n -+\t\t\tfprintf(stderr, \"Invalid argument to --submodule: '%s'\\n\",\n -+\t\t\t\tsubrepo_path);\n -+\t\t\tfree(subrepo);\n -+\t\t\tret = TC_USAGE_ERROR;\n -+\t\t\tgoto out;\n -+\t\t}\n -+\t\trepo = subrepo;\n -+\t}\n ++\t\tif (!sub || repo_submodule_init(&subrepo, the_repository, sub))\n ++\t\t\tdie(\"invalid argument to --submodule: '%s'\", subrepo_path);\n +\n - \tif (argc == 2 && !strcmp(argv[0], \"get_value\")) {\n --\t\tif (!git_config_get_value(argv[1], &v)) {\n -+\t\tif (!repo_config_get_value(repo, argv[1], &v)) {\n ++\t\trepo = &subrepo;\n ++\t}\n + \n + \tif (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n +-\t\tif (!git_config_get_value(argv[2], &v)) {\n ++\t\tif (!repo_config_get_value(repo, argv[2], &v)) {\n \t\t\tif (!v)\n \t\t\t\tprintf(\"(NULL)\\n\");\n \t\t\telse\n @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n - \t} else if (argc == 2 && !strcmp(argv[0], \"get_value_multi\")) {\n --\t\tstrptr = git_config_get_value_multi(argv[1]);\n -+\t\tstrptr = repo_config_get_value_multi(repo, argv[1]);\n + \t} else if (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n +-\t\tstrptr = git_config_get_value_multi(argv[2]);\n ++\t\tstrptr = repo_config_get_value_multi(repo, argv[2]);\n \t\tif (strptr) {\n \t\t\tfor (i = 0; i < strptr->nr; i++) {\n \t\t\t\tv = strptr->items[i].string;\n @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n - \t} else if (argc == 2 && !strcmp(argv[0], \"get_int\")) {\n --\t\tif (!git_config_get_int(argv[1], &val)) {\n -+\t\tif (!repo_config_get_int(repo, argv[1], &val)) {\n + \t} else if (argc == 3 && !strcmp(argv[1], \"get_int\")) {\n +-\t\tif (!git_config_get_int(argv[2], &val)) {\n ++\t\tif (!repo_config_get_int(repo, argv[2], &val)) {\n \t\t\tprintf(\"%d\\n\", val);\n \t\t} else {\n - \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[1]);\n + \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n - \t} else if (argc == 2 && !strcmp(argv[0], \"get_bool\")) {\n --\t\tif (!git_config_get_bool(argv[1], &val)) {\n -+\t\tif (!repo_config_get_bool(repo, argv[1], &val)) {\n + \t} else if (argc == 3 && !strcmp(argv[1], \"get_bool\")) {\n +-\t\tif (!git_config_get_bool(argv[2], &val)) {\n ++\t\tif (!repo_config_get_bool(repo, argv[2], &val)) {\n \t\t\tprintf(\"%d\\n\", val);\n \t\t} else {\n +\n - \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[1]);\n + \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n - \t} else if (argc == 2 && !strcmp(argv[0], \"get_string\")) {\n --\t\tif (!git_config_get_string_tmp(argv[1], &v)) {\n -+\t\tif (!repo_config_get_string_tmp(repo, argv[1], &v)) {\n + \t} else if (argc == 3 && !strcmp(argv[1], \"get_string\")) {\n +-\t\tif (!git_config_get_string_tmp(argv[2], &v)) {\n ++\t\tif (!repo_config_get_string_tmp(repo, argv[2], &v)) {\n \t\t\tprintf(\"%s\\n\", v);\n \t\t} else {\n - \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[1]);\n + \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n - \t} else if (argc >= 2 && !strcmp(argv[0], \"configset_get_value\")) {\n -+\t\tif (subrepo_path) {\n -+\t\t\tfprintf(stderr, \"Cannot use --submodule with configset_get_value\\n\");\n -+\t\t\tret = TC_USAGE_ERROR;\n -+\t\t\tgoto out;\n -+\t\t}\n - \t\tfor (i = 2; i < argc; i++) {\n + \t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value\")) {\n ++\t\tif (subrepo_path)\n ++\t\t\tdie(\"cannot use --submodule with configset_get_value\");\n ++\n + \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n - \t} else if (argc >= 2 && !strcmp(argv[0], \"configset_get_value_multi\")) {\n -+\t\tif (subrepo_path) {\n -+\t\t\tfprintf(stderr, \"Cannot use --submodule with configset_get_value_multi\\n\");\n -+\t\t\tret = TC_USAGE_ERROR;\n -+\t\t\tgoto out;\n -+\t\t}\n - \t\tfor (i = 2; i < argc; i++) {\n + \t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value_multi\")) {\n ++\t\tif (subrepo_path)\n ++\t\t\tdie(\"cannot use --submodule with configset_get_value_multi\");\n ++\n + \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n - \t} else if (!strcmp(argv[0], \"iterate\")) {\n + \t} else if (!strcmp(argv[1], \"iterate\")) {\n -\t\tgit_config(iterate_cb, NULL);\n +\t\trepo_config(repo, iterate_cb, NULL);\n \t} else {\n - print_usage_error:\n --\t\tfprintf(stderr, \"Invalid syntax. Usage: test-tool config [args]\\n\");\n -+\t\tfprintf(stderr, \"Invalid syntax. Usage: test-tool config\"\n -+\t\t\t\t\" [--submodule=] [args]\\n\");\n - \t\tret = TC_USAGE_ERROR;\n + \t\tdie(\"%s: Please check the syntax and the function name\", argv[0]);\n \t}\n \n out:\n \tgit_configset_clear(&cs);\n -+\tif (repo != the_repository) {\n ++\tif (repo != the_repository)\n +\t\trepo_clear(repo);\n -+\t\tfree(repo);\n -+\t}\n \treturn ret;\n }\n \n5: 6d9720abf5 = 7: 902556a7b6 grep: honor sparse checkout patterns\n6: affb931d35 = 8: 70e7d7b90c config: add setting to ignore sparsity patterns in some cmds\n-- \n2.28.0\n\n"},{"id":"404900","messageId":"f53782f14c5f53da5d5537b369a810a94f9ce184.1599026986.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599026986.git.matheus.bernardino@usp.br","subject":"[PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-02T06:17:31Z","receivedAt":"2020-09-02T06:18:09Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"One test in t1308 expects test-config to fail with exit code 128 due to\na parsing error in the config machinery. But test-config might also exit\nwith 128 for any other reason that leads it to call die(). Therefore the\ntest can potentially succeed for the wrong reason. To avoid false\npositives, let's check test-config's output, in addition to the exit\ncode, and make sure that the cause of the error is the one we expect in\nthis test.\n\nMoreover, the test was using the auxiliary function check_config which\noptionally takes a string to compare the test-config stdout against.\nBecause this string is optional, there is a risk that future callers may\nalso check only the exit code and not the output. To avoid that, make\nthe string parameter of this function mandatory.\n\nSigned-off-by: Matheus Tavares \n---\n t/t1308-config-set.sh | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 3a527e3a84..cff17120dc 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -14,10 +14,7 @@ check_config () {\n \t\texpect_code=0\n \tfi &&\n \top=$1 key=$2 && shift && shift &&\n-\tif test $# != 0\n-\tthen\n-\t\tprintf \"%s\\n\" \"$@\"\n-\tfi >expect &&\n+\tprintf \"%s\\n\" \"$@\" >expect &&\n \ttest_expect_code $expect_code test-tool config \"$op\" \"$key\" >actual &&\n \ttest_cmp expect actual\n }\n@@ -130,7 +127,8 @@ test_expect_success 'check line error when NULL string is queried' '\n '\n \n test_expect_success 'find integer if value is non parse-able' '\n-\tcheck_config expect_code 128 get_int lamb.head\n+\ttest_expect_code 128 test-tool config get_int lamb.head 2>result &&\n+\ttest_i18ngrep \"fatal: bad numeric config value '\\'none\\'' for '\\'lamb.head\\''\" result\n '\n \n test_expect_success 'find bool value for the entered key' '\n-- \n2.28.0\n\n"},{"id":"404901","messageId":"85e1588d6c5431c32fe0262671df1d247d98b83e.1599026986.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599026986.git.matheus.bernardino@usp.br","subject":"[PATCH v5 3/8] t/helper/test-config: be consistent with exit codes","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-02T06:17:32Z","receivedAt":"2020-09-02T06:18:12Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The test-config helper can return at least three different exit codes to\nreflect the status of the requested operation. And these codes are\nchecked in some of the tests. But there is an inconsistent place in the\nhelper where an usage error returns the same code as a \"value not found\"\nerror. Let's fix that and, while we are here, document the meaning of\neach exit code in the file's header.\n\nSigned-off-by: Matheus Tavares \n---\n t/helper/test-config.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex a6e936721f..9e9d50099a 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -30,6 +30,11 @@\n * iterate -> iterate over all values using git_config(), and print some\n * data for each\n *\n+ * Exit codes:\n+ * 0: success\n+ * 1: value not found for the given config key\n+ * 2: config file path given as argument is inaccessible or doesn't exist\n+ *\n * Examples:\n *\n * To print the value with highest priority for key \"foo.bAr Baz.rock\":\n@@ -80,10 +85,10 @@ int cmd__config(int argc, const char **argv)\n \n \tgit_configset_init(&cs);\n \n-\tif (argc < 2) {\n-\t\tfprintf(stderr, \"Please, provide a command name on the command-line\\n\");\n-\t\tgoto exit1;\n-\t} else if (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n+\tif (argc < 2)\n+\t\tdie(\"Please, provide a command name on the command-line\");\n+\n+\tif (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n \t\tif (!git_config_get_value(argv[2], &v)) {\n \t\t\tif (!v)\n \t\t\t\tprintf(\"(NULL)\\n\");\n-- \n2.28.0\n\n"},{"id":"404902","messageId":"0750191342754bcca398c6fdad522616b0f3fbc3.1599026986.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599026986.git.matheus.bernardino@usp.br","subject":"[PATCH v5 4/8] t/helper/test-config: check argc before accessing argv","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-02T06:17:33Z","receivedAt":"2020-09-02T06:18:19Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Check that we have the expected argc in 'configset_get_value' and\n'configset_get_value_multi' before trying to access argv elements.\n\nSigned-off-by: Matheus Tavares \n---\n t/helper/test-config.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 9e9d50099a..26d9c2ac4c 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -138,7 +138,7 @@ int cmd__config(int argc, const char **argv)\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tgoto exit1;\n \t\t}\n-\t} else if (!strcmp(argv[1], \"configset_get_value\")) {\n+\t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n@@ -156,7 +156,7 @@ int cmd__config(int argc, const char **argv)\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tgoto exit1;\n \t\t}\n-\t} else if (!strcmp(argv[1], \"configset_get_value_multi\")) {\n+\t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value_multi\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n-- \n2.28.0\n\n"},{"id":"404903","messageId":"56535b0e36e94dc73aa570f4f3c0466305c6432f.1599026986.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599026986.git.matheus.bernardino@usp.br","subject":"[PATCH v5 5/8] t/helper/test-config: unify exit labels","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-02T06:17:34Z","receivedAt":"2020-09-02T06:18:22Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"test-config's main function has three different exit labels, all of\nwhich have to perform the same cleanup code before returning. Unify the\nlabels in preparation for the next patch which will increase the cleanup\nsection.\n\nSigned-off-by: Matheus Tavares \n---\n t/helper/test-config.c | 51 +++++++++++++++++-------------------------\n 1 file changed, 20 insertions(+), 31 deletions(-)\n\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 26d9c2ac4c..8fe43e9775 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -69,16 +69,19 @@ static int early_config_cb(const char *var, const char *value, void *vdata)\n \treturn 0;\n }\n \n+#define TC_VALUE_NOT_FOUND 1\n+#define TC_CONFIG_FILE_ERROR 2\n+\n int cmd__config(int argc, const char **argv)\n {\n-\tint i, val;\n+\tint i, val, ret = 0;\n \tconst char *v;\n \tconst struct string_list *strptr;\n \tstruct config_set cs;\n \n \tif (argc == 3 && !strcmp(argv[1], \"read_early_config\")) {\n \t\tread_early_config(early_config_cb, (void *)argv[2]);\n-\t\treturn 0;\n+\t\treturn ret;\n \t}\n \n \tsetup_git_directory();\n@@ -94,10 +97,9 @@ int cmd__config(int argc, const char **argv)\n \t\t\t\tprintf(\"(NULL)\\n\");\n \t\t\telse\n \t\t\t\tprintf(\"%s\\n\", v);\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n \t\tstrptr = git_config_get_value_multi(argv[2]);\n@@ -109,41 +111,38 @@ int cmd__config(int argc, const char **argv)\n \t\t\t\telse\n \t\t\t\t\tprintf(\"%s\\n\", v);\n \t\t\t}\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_int\")) {\n \t\tif (!git_config_get_int(argv[2], &val)) {\n \t\t\tprintf(\"%d\\n\", val);\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_bool\")) {\n \t\tif (!git_config_get_bool(argv[2], &val)) {\n \t\t\tprintf(\"%d\\n\", val);\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_string\")) {\n \t\tif (!git_config_get_string_tmp(argv[2], &v)) {\n \t\t\tprintf(\"%s\\n\", v);\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n \t\t\t\tfprintf(stderr, \"Error (%d) reading configuration file %s.\\n\", err, argv[i]);\n-\t\t\t\tgoto exit2;\n+\t\t\t\tret = TC_CONFIG_FILE_ERROR;\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t}\n \t\tif (!git_configset_get_value(&cs, argv[2], &v)) {\n@@ -151,17 +150,17 @@ int cmd__config(int argc, const char **argv)\n \t\t\t\tprintf(\"(NULL)\\n\");\n \t\t\telse\n \t\t\t\tprintf(\"%s\\n\", v);\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value_multi\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n \t\t\t\tfprintf(stderr, \"Error (%d) reading configuration file %s.\\n\", err, argv[i]);\n-\t\t\t\tgoto exit2;\n+\t\t\t\tret = TC_CONFIG_FILE_ERROR;\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t}\n \t\tstrptr = git_configset_get_value_multi(&cs, argv[2]);\n@@ -173,27 +172,17 @@ int cmd__config(int argc, const char **argv)\n \t\t\t\telse\n \t\t\t\t\tprintf(\"%s\\n\", v);\n \t\t\t}\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (!strcmp(argv[1], \"iterate\")) {\n \t\tgit_config(iterate_cb, NULL);\n-\t\tgoto exit0;\n+\t} else {\n+\t\tdie(\"%s: Please check the syntax and the function name\", argv[0]);\n \t}\n \n-\tdie(\"%s: Please check the syntax and the function name\", argv[0]);\n-\n-exit0:\n-\tgit_configset_clear(&cs);\n-\treturn 0;\n-\n-exit1:\n-\tgit_configset_clear(&cs);\n-\treturn 1;\n-\n-exit2:\n+out:\n \tgit_configset_clear(&cs);\n-\treturn 2;\n+\treturn ret;\n }\n-- \n2.28.0\n\n"},{"id":"404904","messageId":"3e02e1bd248438e0b435a19d857432edcaa15a2c.1599026986.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599026986.git.matheus.bernardino@usp.br","subject":"[PATCH v5 6/8] config: correctly read worktree configs in submodules","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-02T06:17:35Z","receivedAt":"2020-09-02T06:18:25Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The config machinery is not able to read worktree configs from a\nsubmodule in a process where the_repository represents the superproject.\nFurthermore, when extensions.worktreeConfig is set on the superproject,\nquerying for a worktree config in a submodule will, instead, return\nthe value set at the superproject.\n\nThe problem resides in do_git_config_sequence(). Although the function\nreceives a git_dir string, it uses the_repository->git_dir when making\nthe path to the worktree config file. And when checking if\nextensions.worktreeConfig is set, it uses the global\nrepository_format_worktree_config variable, which refers to\nthe_repository only. So let's fix this by using the git_dir given to the\nfunction and reading the extension value from the right place. Also add\na test to avoid any regressions.\n\nSigned-off-by: Matheus Tavares \n---\n config.c | 21 ++++++++++---\n t/helper/test-config.c | 62 ++++++++++++++++++++++++++++++++------\n t/t2404-worktree-config.sh | 16 ++++++++++\n 3 files changed, 85 insertions(+), 14 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 2bdff4457b..e1e7fab6dc 100644\n--- a/config.c\n+++ b/config.c\n@@ -1747,11 +1747,22 @@ static int do_git_config_sequence(const struct config_options *opts,\n \t\tret += git_config_from_file(fn, repo_config, data);\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_WORKTREE;\n-\tif (!opts->ignore_worktree && repository_format_worktree_config) {\n-\t\tchar *path = git_pathdup(\"config.worktree\");\n-\t\tif (!access_or_die(path, R_OK, 0))\n-\t\t\tret += git_config_from_file(fn, path, data);\n-\t\tfree(path);\n+\tif (!opts->ignore_worktree && repo_config && opts->git_dir) {\n+\t\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tread_repository_format(&repo_fmt, repo_config);\n+\n+\t\tif (!verify_repository_format(&repo_fmt, &buf) &&\n+\t\t repo_fmt.worktree_config) {\n+\t\t\tchar *path = mkpathdup(\"%s/config.worktree\", opts->git_dir);\n+\t\t\tif (!access_or_die(path, R_OK, 0))\n+\t\t\t\tret += git_config_from_file(fn, path, data);\n+\t\t\tfree(path);\n+\t\t}\n+\n+\t\tstrbuf_release(&buf);\n+\t\tclear_repository_format(&repo_fmt);\n \t}\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_COMMAND;\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 8fe43e9775..2924c09c21 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -2,12 +2,20 @@\n #include \"cache.h\"\n #include \"config.h\"\n #include \"string-list.h\"\n+#include \"submodule-config.h\"\n+#include \"parse-options.h\"\n \n /*\n * This program exposes the C API of the configuration mechanism\n * as a set of simple commands in order to facilitate testing.\n *\n- * Reads stdin and prints result of command to stdout:\n+ * Usage: test-tool config [--submodule=] []\n+ *\n+ * If --submodule= is given, will operate on the submodule at the\n+ * given . This option is not valid for the commands: read_early_config,\n+ * configset_get_value and configset_get_value_multi.\n+ *\n+ * Possible cmds are:\n *\n * get_value -> prints the value with highest priority for the entered key\n *\n@@ -72,14 +80,34 @@ static int early_config_cb(const char *var, const char *value, void *vdata)\n #define TC_VALUE_NOT_FOUND 1\n #define TC_CONFIG_FILE_ERROR 2\n \n+static const char *test_config_usage[] = {\n+\t\"test-tool config [--submodule=] []\",\n+\tNULL\n+};\n+\n int cmd__config(int argc, const char **argv)\n {\n \tint i, val, ret = 0;\n \tconst char *v;\n \tconst struct string_list *strptr;\n \tstruct config_set cs;\n+\tstruct repository subrepo, *repo = the_repository;\n+\tconst char *subrepo_path = NULL;\n+\n+\tstruct option options[] = {\n+\t\tOPT_STRING(0, \"submodule\", &subrepo_path, \"path\",\n+\t\t\t \"run on the submodule at \"),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, NULL, options, test_config_usage,\n+\t\t\t PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_STOP_AT_NON_OPTION);\n+\tif (argc < 2)\n+\t\tdie(\"Please, provide a command name on the command-line\");\n \n \tif (argc == 3 && !strcmp(argv[1], \"read_early_config\")) {\n+\t\tif (subrepo_path)\n+\t\t\tdie(\"cannot use --submodule with read_early_config\");\n \t\tread_early_config(early_config_cb, (void *)argv[2]);\n \t\treturn ret;\n \t}\n@@ -88,11 +116,18 @@ int cmd__config(int argc, const char **argv)\n \n \tgit_configset_init(&cs);\n \n-\tif (argc < 2)\n-\t\tdie(\"Please, provide a command name on the command-line\");\n+\tif (subrepo_path) {\n+\t\tconst struct submodule *sub;\n+\n+\t\tsub = submodule_from_path(the_repository, &null_oid, subrepo_path);\n+\t\tif (!sub || repo_submodule_init(&subrepo, the_repository, sub))\n+\t\t\tdie(\"invalid argument to --submodule: '%s'\", subrepo_path);\n+\n+\t\trepo = &subrepo;\n+\t}\n \n \tif (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n-\t\tif (!git_config_get_value(argv[2], &v)) {\n+\t\tif (!repo_config_get_value(repo, argv[2], &v)) {\n \t\t\tif (!v)\n \t\t\t\tprintf(\"(NULL)\\n\");\n \t\t\telse\n@@ -102,7 +137,7 @@ int cmd__config(int argc, const char **argv)\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n-\t\tstrptr = git_config_get_value_multi(argv[2]);\n+\t\tstrptr = repo_config_get_value_multi(repo, argv[2]);\n \t\tif (strptr) {\n \t\t\tfor (i = 0; i < strptr->nr; i++) {\n \t\t\t\tv = strptr->items[i].string;\n@@ -116,27 +151,31 @@ int cmd__config(int argc, const char **argv)\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_int\")) {\n-\t\tif (!git_config_get_int(argv[2], &val)) {\n+\t\tif (!repo_config_get_int(repo, argv[2], &val)) {\n \t\t\tprintf(\"%d\\n\", val);\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_bool\")) {\n-\t\tif (!git_config_get_bool(argv[2], &val)) {\n+\t\tif (!repo_config_get_bool(repo, argv[2], &val)) {\n \t\t\tprintf(\"%d\\n\", val);\n \t\t} else {\n+\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_string\")) {\n-\t\tif (!git_config_get_string_tmp(argv[2], &v)) {\n+\t\tif (!repo_config_get_string_tmp(repo, argv[2], &v)) {\n \t\t\tprintf(\"%s\\n\", v);\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value\")) {\n+\t\tif (subrepo_path)\n+\t\t\tdie(\"cannot use --submodule with configset_get_value\");\n+\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n@@ -155,6 +194,9 @@ int cmd__config(int argc, const char **argv)\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value_multi\")) {\n+\t\tif (subrepo_path)\n+\t\t\tdie(\"cannot use --submodule with configset_get_value_multi\");\n+\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n@@ -177,12 +219,14 @@ int cmd__config(int argc, const char **argv)\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (!strcmp(argv[1], \"iterate\")) {\n-\t\tgit_config(iterate_cb, NULL);\n+\t\trepo_config(repo, iterate_cb, NULL);\n \t} else {\n \t\tdie(\"%s: Please check the syntax and the function name\", argv[0]);\n \t}\n \n out:\n \tgit_configset_clear(&cs);\n+\tif (repo != the_repository)\n+\t\trepo_clear(repo);\n \treturn ret;\n }\ndiff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh\nindex 9536d10919..1e32c93735 100755\n--- a/t/t2404-worktree-config.sh\n+++ b/t/t2404-worktree-config.sh\n@@ -78,4 +78,20 @@ test_expect_success 'config.worktree no longer read without extension' '\n \ttest_cmp_config -C wt2 shared this.is\n '\n \n+test_expect_success 'correctly read config.worktree from submodules' '\n+\ttest_unconfig extensions.worktreeConfig &&\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\ttest_commit A &&\n+\t\tgit config extensions.worktreeConfig true &&\n+\t\tgit config --worktree wtconfig.sub test-value\n+\t) &&\n+\tgit submodule add ./sub &&\n+\tgit commit -m \"add sub\" &&\n+\techo test-value >expect &&\n+\ttest-tool config --submodule=sub get_value wtconfig.sub >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.28.0\n\n"},{"id":"404905","messageId":"902556a7b60974539ea2ce390bfd8f161d998270.1599026986.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599026986.git.matheus.bernardino@usp.br","subject":"[PATCH v5 7/8] grep: honor sparse checkout patterns","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-02T06:17:36Z","receivedAt":"2020-09-02T06:18:27Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"One of the main uses for a sparse checkout is to allow users to focus on\nthe subset of files in a repository in which they are interested. But\ngit-grep currently ignores the sparsity patterns and reports all matches\nfound outside this subset, which kind of goes in the opposite direction.\nThere are some use cases for ignoring the sparsity patterns and the next\ncommit will add an option to obtain this behavior, but here we start by\nmaking grep honor the sparsity boundaries in every case where this is\nrelevant:\n\n- git grep in worktree\n- git grep --cached\n- git grep $REVISION\n\nFor the worktree and cached cases, we iterate over paths without the\nSKIP_WORKTREE bit set, and limit our searches to these paths. For the\n$REVISION case, we limit the paths we search to those that match the\nsparsity patterns. (We do not check the SKIP_WORKTREE bit for the\n$REVISION case, because $REVISION may contain paths that do not exist in\nHEAD and thus for which we have no SKIP_WORKTREE bit to consult. The\nsparsity patterns tell us how the SKIP_WORKTREE bit would be set if we\nwere to check out $REVISION, so we consult those. Also, we don't use the\nsparsity patterns with the worktree or cached cases, both because we\nhave a bit we can check directly and more efficiently, and because\nunmerged entries from a merge or a rebase could cause more files to\ntemporarily be present than the sparsity patterns would normally\nselect.)\n\nNote that there is a special case here: `git grep $TREE`. In this case,\nwe cannot know whether $TREE corresponds to the root of the repository\nor some sub-tree, and thus there is no way for us to know which sparsity\npatterns, if any, apply. So the $TREE case will not use sparsity\npatterns or any SKIP_WORKTREE bits and will instead always search all\nfiles within the $TREE.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 125 ++++++++++++++++++--\n t/t7011-skip-worktree-reading.sh | 9 --\n t/t7817-grep-sparse-checkout.sh | 195 +++++++++++++++++++++++++++++++\n 3 files changed, 312 insertions(+), 17 deletions(-)\n create mode 100755 t/t7817-grep-sparse-checkout.sh\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex f58979bc3f..a32815de0a 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -410,7 +410,7 @@ static int grep_cache(struct grep_opt *opt,\n \t\t const struct pathspec *pathspec, int cached);\n static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t struct tree_desc *tree, struct strbuf *base, int tn_len,\n-\t\t int check_attr);\n+\t\t int is_root_tree);\n \n static int grep_submodule(struct grep_opt *opt,\n \t\t\t const struct pathspec *pathspec,\n@@ -508,6 +508,10 @@ static int grep_cache(struct grep_opt *opt,\n \n \tfor (nr = 0; nr < repo->index->cache_nr; nr++) {\n \t\tconst struct cache_entry *ce = repo->index->cache[nr];\n+\n+\t\tif (ce_skip_worktree(ce))\n+\t\t\tcontinue;\n+\n \t\tstrbuf_setlen(&name, name_base_len);\n \t\tstrbuf_addstr(&name, ce->name);\n \n@@ -520,8 +524,7 @@ static int grep_cache(struct grep_opt *opt,\n \t\t\t * cache entry are identical, even if worktree file has\n \t\t\t * been modified, so use cache version instead\n \t\t\t */\n-\t\t\tif (cached || (ce->ce_flags & CE_VALID) ||\n-\t\t\t ce_skip_worktree(ce)) {\n+\t\t\tif (cached || (ce->ce_flags & CE_VALID)) {\n \t\t\t\tif (ce_stage(ce) || ce_intent_to_add(ce))\n \t\t\t\t\tcontinue;\n \t\t\t\thit |= grep_oid(opt, &ce->oid, name.buf,\n@@ -552,9 +555,76 @@ static int grep_cache(struct grep_opt *opt,\n \treturn hit;\n }\n \n-static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n-\t\t struct tree_desc *tree, struct strbuf *base, int tn_len,\n-\t\t int check_attr)\n+static struct pattern_list *get_sparsity_patterns(struct repository *repo)\n+{\n+\tstruct pattern_list *patterns;\n+\tchar *sparse_file;\n+\tint sparse_config, cone_config;\n+\n+\tif (repo_config_get_bool(repo, \"core.sparsecheckout\", &sparse_config) ||\n+\t !sparse_config) {\n+\t\treturn NULL;\n+\t}\n+\n+\tsparse_file = repo_git_path(repo, \"info/sparse-checkout\");\n+\tpatterns = xcalloc(1, sizeof(*patterns));\n+\n+\tif (repo_config_get_bool(repo, \"core.sparsecheckoutcone\", &cone_config))\n+\t\tcone_config = 0;\n+\tpatterns->use_cone_patterns = cone_config;\n+\n+\tif (add_patterns_from_file_to_list(sparse_file, \"\", 0, patterns, NULL)) {\n+\t\tif (file_exists(sparse_file)) {\n+\t\t\twarning(_(\"failed to load sparse-checkout file: '%s'\"),\n+\t\t\t\tsparse_file);\n+\t\t}\n+\t\tfree(sparse_file);\n+\t\tfree(patterns);\n+\t\treturn NULL;\n+\t}\n+\n+\tfree(sparse_file);\n+\treturn patterns;\n+}\n+\n+static int path_in_sparse_checkout(struct strbuf *path, int prefix_len,\n+\t\t\t\t unsigned int entry_mode,\n+\t\t\t\t struct index_state *istate,\n+\t\t\t\t struct pattern_list *sparsity,\n+\t\t\t\t enum pattern_match_result parent_match,\n+\t\t\t\t enum pattern_match_result *match)\n+{\n+\tint dtype = DT_UNKNOWN;\n+\tint is_dir = S_ISDIR(entry_mode);\n+\n+\tif (parent_match == MATCHED_RECURSIVE) {\n+\t\t*match = parent_match;\n+\t\treturn 1;\n+\t}\n+\n+\tif (is_dir && !is_dir_sep(path->buf[path->len - 1]))\n+\t\tstrbuf_addch(path, '/');\n+\n+\t*match = path_matches_pattern_list(path->buf, path->len,\n+\t\t\t\t\t path->buf + prefix_len, &dtype,\n+\t\t\t\t\t sparsity, istate);\n+\tif (*match == UNDECIDED)\n+\t\t*match = parent_match;\n+\n+\tif (is_dir)\n+\t\tstrbuf_trim_trailing_dir_sep(path);\n+\n+\tif (*match == NOT_MATCHED &&\n+\t\t(!is_dir || (is_dir && sparsity->use_cone_patterns)))\n+\t return 0;\n+\n+\treturn 1;\n+}\n+\n+static int do_grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n+\t\t\tstruct tree_desc *tree, struct strbuf *base, int tn_len,\n+\t\t\tint check_attr, struct pattern_list *sparsity,\n+\t\t\tenum pattern_match_result default_sparsity_match)\n {\n \tstruct repository *repo = opt->repo;\n \tint hit = 0;\n@@ -570,6 +640,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \n \twhile (tree_entry(tree, &entry)) {\n \t\tint te_len = tree_entry_len(&entry);\n+\t\tenum pattern_match_result sparsity_match = 0;\n \n \t\tif (match != all_entries_interesting) {\n \t\t\tstrbuf_addstr(&name, base->buf + tn_len);\n@@ -586,6 +657,19 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \n \t\tstrbuf_add(base, entry.path, te_len);\n \n+\t\tif (sparsity) {\n+\t\t\tstruct strbuf path = STRBUF_INIT;\n+\t\t\tstrbuf_addstr(&path, base->buf + tn_len);\n+\n+\t\t\tif (!path_in_sparse_checkout(&path, old_baselen - tn_len,\n+\t\t\t\t\t\t entry.mode, repo->index,\n+\t\t\t\t\t\t sparsity, default_sparsity_match,\n+\t\t\t\t\t\t &sparsity_match)) {\n+\t\t\t\tstrbuf_setlen(base, old_baselen);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n \t\tif (S_ISREG(entry.mode)) {\n \t\t\thit |= grep_oid(opt, &entry.oid, base->buf, tn_len,\n \t\t\t\t\t check_attr ? base->buf + tn_len : NULL);\n@@ -602,8 +686,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \n \t\t\tstrbuf_addch(base, '/');\n \t\t\tinit_tree_desc(&sub, data, size);\n-\t\t\thit |= grep_tree(opt, pathspec, &sub, base, tn_len,\n-\t\t\t\t\t check_attr);\n+\t\t\thit |= do_grep_tree(opt, pathspec, &sub, base, tn_len,\n+\t\t\t\t\t check_attr, sparsity, sparsity_match);\n \t\t\tfree(data);\n \t\t} else if (recurse_submodules && S_ISGITLINK(entry.mode)) {\n \t\t\thit |= grep_submodule(opt, pathspec, &entry.oid,\n@@ -621,6 +705,31 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \treturn hit;\n }\n \n+/*\n+ * Note: sparsity patterns and paths' attributes will only be considered if\n+ * is_root_tree has true value. (Otherwise, we cannot properly perform pattern\n+ * matching on paths.)\n+ */\n+static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n+\t\t struct tree_desc *tree, struct strbuf *base, int tn_len,\n+\t\t int is_root_tree)\n+{\n+\tstruct pattern_list *patterns = NULL;\n+\tint ret;\n+\n+\tif (is_root_tree)\n+\t\tpatterns = get_sparsity_patterns(opt->repo);\n+\n+\tret = do_grep_tree(opt, pathspec, tree, base, tn_len, is_root_tree,\n+\t\t\t patterns, 0);\n+\n+\tif (patterns) {\n+\t\tclear_pattern_list(patterns);\n+\t\tfree(patterns);\n+\t}\n+\treturn ret;\n+}\n+\n static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t struct object *obj, const char *name, const char *path)\n {\ndiff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh\nindex 37525cae3a..26852586ac 100755\n--- a/t/t7011-skip-worktree-reading.sh\n+++ b/t/t7011-skip-worktree-reading.sh\n@@ -109,15 +109,6 @@ test_expect_success 'ls-files --modified' '\n \ttest -z \"$(git ls-files -m)\"\n '\n \n-test_expect_success 'grep with skip-worktree file' '\n-\tgit update-index --no-skip-worktree 1 &&\n-\techo test > 1 &&\n-\tgit update-index 1 &&\n-\tgit update-index --skip-worktree 1 &&\n-\trm 1 &&\n-\ttest \"$(git grep --no-ext-grep test)\" = \"1:test\"\n-'\n-\n echo \":000000 100644 $ZERO_OID $EMPTY_BLOB A\t1\" > expected\n test_expect_success 'diff-index does not examine skip-worktree absent entries' '\n \tsetup_absent &&\ndiff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh\nnew file mode 100755\nindex 0000000000..b3109e3479\n--- /dev/null\n+++ b/t/t7817-grep-sparse-checkout.sh\n@@ -0,0 +1,195 @@\n+#!/bin/sh\n+\n+test_description='grep in sparse checkout\n+\n+This test creates a repo with the following structure:\n+\n+.\n+|-- a\n+|-- b\n+|-- dir\n+| `-- c\n+|-- sub\n+| |-- A\n+| | `-- a\n+| `-- B\n+| `-- b\n+`-- sub2\n+ `-- a\n+\n+Where the outer repository has non-cone mode sparsity patterns, sub is a\n+submodule with cone mode sparsity patterns and sub2 is a submodule that is\n+excluded by the superproject sparsity patterns. The resulting sparse checkout\n+should leave the following structure in the working tree:\n+\n+.\n+|-- a\n+|-- sub\n+| `-- B\n+| `-- b\n+`-- sub2\n+ `-- a\n+\n+But note that sub2 should have the SKIP_WORKTREE bit set.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo \"text\" >a &&\n+\techo \"text\" >b &&\n+\tmkdir dir &&\n+\techo \"text\" >dir/c &&\n+\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tmkdir A B &&\n+\t\techo \"text\" >A/a &&\n+\t\techo \"text\" >B/b &&\n+\t\tgit add A B &&\n+\t\tgit commit -m sub &&\n+\t\tgit sparse-checkout init --cone &&\n+\t\tgit sparse-checkout set B\n+\t) &&\n+\n+\tgit init sub2 &&\n+\t(\n+\t\tcd sub2 &&\n+\t\techo \"text\" >a &&\n+\t\tgit add a &&\n+\t\tgit commit -m sub2\n+\t) &&\n+\n+\tgit submodule add ./sub &&\n+\tgit submodule add ./sub2 &&\n+\tgit add a b dir &&\n+\tgit commit -m super &&\n+\tgit sparse-checkout init --no-cone &&\n+\tgit sparse-checkout set \"/*\" \"!b\" \"!/*/\" \"sub\" &&\n+\n+\tgit tag -am tag-to-commit tag-to-commit HEAD &&\n+\ttree=$(git rev-parse HEAD^{tree}) &&\n+\tgit tag -am tag-to-tree tag-to-tree $tree &&\n+\n+\ttest_path_is_missing b &&\n+\ttest_path_is_missing dir &&\n+\ttest_path_is_missing sub/A &&\n+\ttest_path_is_file a &&\n+\ttest_path_is_file sub/B/b &&\n+\ttest_path_is_file sub2/a\n+'\n+\n+# The test below checks a special case: the sparsity patterns exclude '/b'\n+# and sparse checkout is enabled, but the path exists in the working tree (e.g.\n+# manually created after `git sparse-checkout init`). In this case, grep should\n+# skip it.\n+test_expect_success 'grep in working tree should honor sparse checkout' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tEOF\n+\techo \"new-text\" >b &&\n+\ttest_when_finished \"rm b\" &&\n+\tgit grep \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep unmerged file despite not matching sparsity patterns' '\n+\tcat >expect <<-EOF &&\n+\tb:modified-b-in-branchX\n+\tb:modified-b-in-branchY\n+\tEOF\n+\ttest_when_finished \"test_might_fail git merge --abort && \\\n+\t\t\t git checkout master\" &&\n+\n+\tgit sparse-checkout disable &&\n+\tgit checkout -b branchY master &&\n+\ttest_commit modified-b-in-branchY b &&\n+\tgit checkout -b branchX master &&\n+\ttest_commit modified-b-in-branchX b &&\n+\n+\tgit sparse-checkout init &&\n+\ttest_path_is_missing b &&\n+\ttest_must_fail git merge branchY &&\n+\tgit grep \"modified-b\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --cached should honor sparse checkout' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tEOF\n+\tgit grep --cached \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep should honor sparse checkout' '\n+\tcommit=$(git rev-parse HEAD) &&\n+\tcat >expect_commit <<-EOF &&\n+\t$commit:a:text\n+\tEOF\n+\tcat >expect_tag-to-commit <<-EOF &&\n+\ttag-to-commit:a:text\n+\tEOF\n+\tgit grep \"text\" $commit >actual_commit &&\n+\ttest_cmp expect_commit actual_commit &&\n+\tgit grep \"text\" tag-to-commit >actual_tag-to-commit &&\n+\ttest_cmp expect_tag-to-commit actual_tag-to-commit\n+'\n+\n+test_expect_success 'grep should ignore sparsity patterns' '\n+\tcommit=$(git rev-parse HEAD) &&\n+\ttree=$(git rev-parse HEAD^{tree}) &&\n+\tcat >expect_tree <<-EOF &&\n+\t$tree:a:text\n+\t$tree:b:text\n+\t$tree:dir/c:text\n+\tEOF\n+\tcat >expect_tag-to-tree <<-EOF &&\n+\ttag-to-tree:a:text\n+\ttag-to-tree:b:text\n+\ttag-to-tree:dir/c:text\n+\tEOF\n+\tgit grep \"text\" $tree >actual_tree &&\n+\ttest_cmp expect_tree actual_tree &&\n+\tgit grep \"text\" tag-to-tree >actual_tag-to-tree &&\n+\ttest_cmp expect_tag-to-tree actual_tag-to-tree\n+'\n+\n+# Note that sub2/ is present in the worktree but it is excluded by the sparsity\n+# patterns, so grep should not recurse into it.\n+test_expect_success 'grep --recurse-submodules should honor sparse checkout in submodule' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tsub/B/b:text\n+\tEOF\n+\tgit grep --recurse-submodules \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --recurse-submodules --cached should honor sparse checkout in submodule' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tsub/B/b:text\n+\tEOF\n+\tgit grep --recurse-submodules --cached \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --recurse-submodules should honor sparse checkout in submodule' '\n+\tcommit=$(git rev-parse HEAD) &&\n+\tcat >expect_commit <<-EOF &&\n+\t$commit:a:text\n+\t$commit:sub/B/b:text\n+\tEOF\n+\tcat >expect_tag-to-commit <<-EOF &&\n+\ttag-to-commit:a:text\n+\ttag-to-commit:sub/B/b:text\n+\tEOF\n+\tgit grep --recurse-submodules \"text\" $commit >actual_commit &&\n+\ttest_cmp expect_commit actual_commit &&\n+\tgit grep --recurse-submodules \"text\" tag-to-commit >actual_tag-to-commit &&\n+\ttest_cmp expect_tag-to-commit actual_tag-to-commit\n+'\n+\n+test_done\n-- \n2.28.0\n\n"},{"id":"404906","messageId":"70e7d7b90c22452d018acf6fe2a1825813b6448e.1599026986.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599026986.git.matheus.bernardino@usp.br","subject":"[PATCH v5 8/8] config: add setting to ignore sparsity patterns in some cmds","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-02T06:17:37Z","receivedAt":"2020-09-02T06:18:43Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"When sparse checkout is enabled, some users expect the output of certain\ncommands (such as grep, diff, and log) to be also restricted within the\nsparsity patterns. This would allow them to effectively work only on the\nsubset of files in which they are interested; and allow some commands to\npossibly perform better, by not considering uninteresting paths. For\nthis reason, we taught grep to honor the sparsity patterns, in the\nprevious patch. But, on the other hand, allowing grep and the other\ncommands mentioned to optionally ignore the patterns also make for some\ninteresting use cases. E.g. using grep to search for a function\ndocumentation that resides outside the sparse checkout.\n\nIn any case, there is no current way for users to configure the behavior\nthey want for these commands. Aiming to provide this flexibility, let's\nintroduce the sparse.restrictCmds setting (and the analogous\n--[no]-restrict-to-sparse-paths global option). The default value is\ntrue. For now, grep is the only one affected by this setting, but the\ngoal is to have support for more commands, in the future.\n\nHelped-by: Elijah Newren \nSigned-off-by: Matheus Tavares \n---\n Documentation/config.txt | 2 +\n Documentation/config/grep.txt | 8 ++\n Documentation/config/sparse.txt | 20 ++++\n Documentation/git.txt | 4 +\n Makefile | 1 +\n builtin/grep.c | 13 ++-\n contrib/completion/git-completion.bash | 2 +\n git.c | 5 +\n sparse-checkout.c | 18 ++++\n sparse-checkout.h | 11 +++\n t/t7817-grep-sparse-checkout.sh | 132 ++++++++++++++++++++++++-\n t/t9902-completion.sh | 4 +-\n 12 files changed, 214 insertions(+), 6 deletions(-)\n create mode 100644 Documentation/config/sparse.txt\n create mode 100644 sparse-checkout.c\n create mode 100644 sparse-checkout.h\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3042d80978..3b6e0901b8 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -438,6 +438,8 @@ include::config/sequencer.txt[]\n \n include::config/showbranch.txt[]\n \n+include::config/sparse.txt[]\n+\n include::config/splitindex.txt[]\n \n include::config/ssh.txt[]\ndiff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt\nindex dd51db38e1..a3275ab4b7 100644\n--- a/Documentation/config/grep.txt\n+++ b/Documentation/config/grep.txt\n@@ -28,3 +28,11 @@ grep.fullName::\n grep.fallbackToNoIndex::\n \tIf set to true, fall back to git grep --no-index if git grep\n \tis executed outside of a git repository. Defaults to false.\n+\n+ifdef::git-grep[]\n+sparse.restrictCmds::\n+\tSee base definition in linkgit:git-config[1]. grep honors\n+\tsparse.restrictCmds by limiting searches to the sparsity paths in three\n+\tcases: when searching the working tree, when searching the index with\n+\t--cached, and when searching a specified commit.\n+endif::git-grep[]\ndiff --git a/Documentation/config/sparse.txt b/Documentation/config/sparse.txt\nnew file mode 100644\nindex 0000000000..494761526e\n--- /dev/null\n+++ b/Documentation/config/sparse.txt\n@@ -0,0 +1,20 @@\n+sparse.restrictCmds::\n+\tOnly meaningful in conjunction with core.sparseCheckout. This option\n+\textends sparse checkouts (which limit which paths are written to the\n+\tworking tree), so that output and operations are also limited to the\n+\tsparsity paths where possible and implemented. The purpose of this\n+\toption is to (1) focus output for the user on the portion of the\n+\trepository that is of interest to them, and (2) enable potentially\n+\tdramatic performance improvements, especially in conjunction with\n+\tpartial clones.\n++\n+When this option is true (default), some git commands may limit their behavior\n+to the paths specified by the sparsity patterns, or to the intersection of\n+those paths and any (like `*.c`) that the user might also specify on the\n+command line. When false, the affected commands will work on full trees,\n+ignoring the sparsity patterns. For now, only git-grep honors this setting.\n++\n+Note: commands which export, integrity check, or create history will always\n+operate on full trees (e.g. fast-export, format-patch, fsck, commit, etc.),\n+unaffected by any sparsity patterns. Also, writing commands such as\n+sparse-checkout and read-tree will not be affected by this configuration.\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 2fc92586b5..d857509573 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -180,6 +180,10 @@ If you just want to run git as if it was started in `` then use\n \tDo not perform optional operations that require locks. This is\n \tequivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`.\n \n+--[no-]restrict-to-sparse-paths::\n+\tOverrides the sparse.restrictCmds configuration (see\n+\tlinkgit:git-config[1]) for this execution.\n+\n --list-cmds=group[,group...]::\n \tList commands by group. This is an internal/experimental\n \toption and may change or be removed in the future. Supported\ndiff --git a/Makefile b/Makefile\nindex 65f8cfb236..778c9e499e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -982,6 +982,7 @@ LIB_OBJS += sha1-name.o\n LIB_OBJS += shallow.o\n LIB_OBJS += sideband.o\n LIB_OBJS += sigchain.o\n+LIB_OBJS += sparse-checkout.o\n LIB_OBJS += split-index.o\n LIB_OBJS += stable-qsort.o\n LIB_OBJS += strbuf.o\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex a32815de0a..3fa364e91c 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -25,6 +25,7 @@\n #include \"submodule-config.h\"\n #include \"object-store.h\"\n #include \"packfile.h\"\n+#include \"sparse-checkout.h\"\n \n static char const * const grep_usage[] = {\n \tN_(\"git grep [] [-e] [...] [[--] ...]\"),\n@@ -498,6 +499,7 @@ static int grep_cache(struct grep_opt *opt,\n \tint nr;\n \tstruct strbuf name = STRBUF_INIT;\n \tint name_base_len = 0;\n+\tint sparse_paths_only =\trestrict_to_sparse_paths(repo);\n \tif (repo->submodule_prefix) {\n \t\tname_base_len = strlen(repo->submodule_prefix);\n \t\tstrbuf_addstr(&name, repo->submodule_prefix);\n@@ -509,7 +511,7 @@ static int grep_cache(struct grep_opt *opt,\n \tfor (nr = 0; nr < repo->index->cache_nr; nr++) {\n \t\tconst struct cache_entry *ce = repo->index->cache[nr];\n \n-\t\tif (ce_skip_worktree(ce))\n+\t\tif (sparse_paths_only && ce_skip_worktree(ce))\n \t\t\tcontinue;\n \n \t\tstrbuf_setlen(&name, name_base_len);\n@@ -715,9 +717,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t int is_root_tree)\n {\n \tstruct pattern_list *patterns = NULL;\n+\tint sparse_paths_only = restrict_to_sparse_paths(opt->repo);\n \tint ret;\n \n-\tif (is_root_tree)\n+\tif (is_root_tree && sparse_paths_only)\n \t\tpatterns = get_sparsity_patterns(opt->repo);\n \n \tret = do_grep_tree(opt, pathspec, tree, base, tn_len, is_root_tree,\n@@ -1258,6 +1261,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \n \tif (!use_index || untracked) {\n \t\tint use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;\n+\n+\t\tif (opt_restrict_to_sparse_paths >= 0) {\n+\t\t\tdie(_(\"--[no-]restrict-to-sparse-paths is incompatible\"\n+\t\t\t\t \" with --no-index and --untracked\"));\n+\t\t}\n+\n \t\thit = grep_directory(&opt, &pathspec, use_exclude, use_index);\n \t} else if (0 <= opt_exclude) {\n \t\tdie(_(\"--[no-]exclude-standard cannot be used for tracked contents\"));\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9147fba3d5..de12766a70 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3402,6 +3402,8 @@ __git_main ()\n \t\t\t--namespace=\n \t\t\t--no-replace-objects\n \t\t\t--help\n+\t\t\t--restrict-to-sparse-paths\n+\t\t\t--no-restrict-to-sparse-paths\n \t\t\t\"\n \t\t\t;;\n \t\t*)\ndiff --git a/git.c b/git.c\nindex 8bd1d7551d..81206b424c 100644\n--- a/git.c\n+++ b/git.c\n@@ -5,6 +5,7 @@\n #include \"run-command.h\"\n #include \"alias.h\"\n #include \"shallow.h\"\n+#include \"sparse-checkout.h\"\n \n #define RUN_SETUP\t\t(1<<0)\n #define RUN_SETUP_GENTLY\t(1<<1)\n@@ -311,6 +312,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t} else {\n \t\t\t\texit(list_cmds(cmd));\n \t\t\t}\n+\t\t} else if (!strcmp(cmd, \"--restrict-to-sparse-paths\")) {\n+\t\t\topt_restrict_to_sparse_paths = 1;\n+\t\t} else if (!strcmp(cmd, \"--no-restrict-to-sparse-paths\")) {\n+\t\t\topt_restrict_to_sparse_paths = 0;\n \t\t} else {\n \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n \t\t\tusage(git_usage_string);\ndiff --git a/sparse-checkout.c b/sparse-checkout.c\nnew file mode 100644\nindex 0000000000..96c5ed5446\n--- /dev/null\n+++ b/sparse-checkout.c\n@@ -0,0 +1,18 @@\n+#include \"cache.h\"\n+#include \"config.h\"\n+#include \"sparse-checkout.h\"\n+\n+int opt_restrict_to_sparse_paths = -1;\n+\n+int restrict_to_sparse_paths(struct repository *repo)\n+{\n+\tint ret;\n+\n+\tif (opt_restrict_to_sparse_paths >= 0)\n+\t\treturn opt_restrict_to_sparse_paths;\n+\n+\tif (repo_config_get_bool(repo, \"sparse.restrictcmds\", &ret))\n+\t\tret = 1;\n+\n+\treturn ret;\n+}\ndiff --git a/sparse-checkout.h b/sparse-checkout.h\nnew file mode 100644\nindex 0000000000..a4805e443a\n--- /dev/null\n+++ b/sparse-checkout.h\n@@ -0,0 +1,11 @@\n+#ifndef SPARSE_CHECKOUT_H\n+#define SPARSE_CHECKOUT_H\n+\n+struct repository;\n+\n+extern int opt_restrict_to_sparse_paths;\n+\n+/* Whether or not cmds should restrict behavior on sparse paths, in this repo */\n+int restrict_to_sparse_paths(struct repository *repo);\n+\n+#endif /* SPARSE_CHECKOUT_H */\ndiff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh\nindex b3109e3479..f93a4f71d1 100755\n--- a/t/t7817-grep-sparse-checkout.sh\n+++ b/t/t7817-grep-sparse-checkout.sh\n@@ -80,10 +80,10 @@ test_expect_success 'setup' '\n \ttest_path_is_file sub2/a\n '\n \n-# The test below checks a special case: the sparsity patterns exclude '/b'\n+# The two tests below check a special case: the sparsity patterns exclude '/b'\n # and sparse checkout is enabled, but the path exists in the working tree (e.g.\n # manually created after `git sparse-checkout init`). In this case, grep should\n-# skip it.\n+# skip the file by default, but not with --no-restrict-to-sparse-paths.\n test_expect_success 'grep in working tree should honor sparse checkout' '\n \tcat >expect <<-EOF &&\n \ta:text\n@@ -93,6 +93,16 @@ test_expect_success 'grep in working tree should honor sparse checkout' '\n \tgit grep \"text\" >actual &&\n \ttest_cmp expect actual\n '\n+test_expect_success 'grep w/ --no-restrict-to-sparse-paths for sparsely excluded but present paths' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tb:new-text\n+\tEOF\n+\techo \"new-text\" >b &&\n+\ttest_when_finished \"rm b\" &&\n+\tgit --no-restrict-to-sparse-paths grep \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n \n test_expect_success 'grep unmerged file despite not matching sparsity patterns' '\n \tcat >expect <<-EOF &&\n@@ -157,7 +167,7 @@ test_expect_success 'grep should ignore sparsity patterns' '\n '\n \n # Note that sub2/ is present in the worktree but it is excluded by the sparsity\n-# patterns, so grep should not recurse into it.\n+# patterns, so grep should only recurse into it with --no-restrict-to-sparse-paths.\n test_expect_success 'grep --recurse-submodules should honor sparse checkout in submodule' '\n \tcat >expect <<-EOF &&\n \ta:text\n@@ -166,6 +176,15 @@ test_expect_success 'grep --recurse-submodules should honor sparse checkout in s\n \tgit grep --recurse-submodules \"text\" >actual &&\n \ttest_cmp expect actual\n '\n+test_expect_success 'grep --recurse-submodules should search in excluded submodules w/ --no-restrict-to-sparse-paths' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tsub/B/b:text\n+\tsub2/a:text\n+\tEOF\n+\tgit --no-restrict-to-sparse-paths grep --recurse-submodules \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n \n test_expect_success 'grep --recurse-submodules --cached should honor sparse checkout in submodule' '\n \tcat >expect <<-EOF &&\n@@ -192,4 +211,111 @@ test_expect_success 'grep --recurse-submodules should honor sparse\n \ttest_cmp expect_tag-to-commit actual_tag-to-commit\n '\n \n+for cmd in 'git --no-restrict-to-sparse-paths grep' \\\n+\t 'git -c sparse.restrictCmds=false grep' \\\n+\t 'git -c sparse.restrictCmds=true --no-restrict-to-sparse-paths grep'\n+do\n+\n+\ttest_expect_success \"$cmd --cached should ignore sparsity patterns\" '\n+\t\tcat >expect <<-EOF &&\n+\t\ta:text\n+\t\tb:text\n+\t\tdir/c:text\n+\t\tEOF\n+\t\t$cmd --cached \"text\" >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd should ignore sparsity patterns\" '\n+\t\tcommit=$(git rev-parse HEAD) &&\n+\t\tcat >expect_commit <<-EOF &&\n+\t\t$commit:a:text\n+\t\t$commit:b:text\n+\t\t$commit:dir/c:text\n+\t\tEOF\n+\t\tcat >expect_tag-to-commit <<-EOF &&\n+\t\ttag-to-commit:a:text\n+\t\ttag-to-commit:b:text\n+\t\ttag-to-commit:dir/c:text\n+\t\tEOF\n+\t\t$cmd \"text\" $commit >actual_commit &&\n+\t\ttest_cmp expect_commit actual_commit &&\n+\t\t$cmd \"text\" tag-to-commit >actual_tag-to-commit &&\n+\t\ttest_cmp expect_tag-to-commit actual_tag-to-commit\n+\t'\n+done\n+\n+test_expect_success 'grep --recurse-submodules --cached w/ --no-restrict-to-sparse-paths' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tb:text\n+\tdir/c:text\n+\tsub/A/a:text\n+\tsub/B/b:text\n+\tsub2/a:text\n+\tEOF\n+\tgit --no-restrict-to-sparse-paths grep --recurse-submodules --cached \\\n+\t\t\"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --recurse-submodules w/ --no-restrict-to-sparse-paths' '\n+\tcommit=$(git rev-parse HEAD) &&\n+\tcat >expect_commit <<-EOF &&\n+\t$commit:a:text\n+\t$commit:b:text\n+\t$commit:dir/c:text\n+\t$commit:sub/A/a:text\n+\t$commit:sub/B/b:text\n+\t$commit:sub2/a:text\n+\tEOF\n+\tcat >expect_tag-to-commit <<-EOF &&\n+\ttag-to-commit:a:text\n+\ttag-to-commit:b:text\n+\ttag-to-commit:dir/c:text\n+\ttag-to-commit:sub/A/a:text\n+\ttag-to-commit:sub/B/b:text\n+\ttag-to-commit:sub2/a:text\n+\tEOF\n+\tgit --no-restrict-to-sparse-paths grep --recurse-submodules \"text\" \\\n+\t\t$commit >actual_commit &&\n+\ttest_cmp expect_commit actual_commit &&\n+\tgit --no-restrict-to-sparse-paths grep --recurse-submodules \"text\" \\\n+\t\ttag-to-commit >actual_tag-to-commit &&\n+\ttest_cmp expect_tag-to-commit actual_tag-to-commit\n+'\n+\n+test_expect_success 'should respect the sparse.restrictCmds values from submodules' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tsub/A/a:text\n+\tsub/B/b:text\n+\tEOF\n+\ttest_config -C sub sparse.restrictCmds false &&\n+\tgit grep --cached --recurse-submodules \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'should propagate --[no]-restrict-to-sparse-paths to submodules' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tb:text\n+\tdir/c:text\n+\tsub/A/a:text\n+\tsub/B/b:text\n+\tsub2/a:text\n+\tEOF\n+\ttest_config -C sub sparse.restrictCmds true &&\n+\tgit --no-restrict-to-sparse-paths grep --cached --recurse-submodules \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+for opt in '--untracked' '--no-index'\n+do\n+\ttest_expect_success \"--[no]-restrict-to-sparse-paths and $opt are incompatible\" \"\n+\t\ttest_must_fail git --restrict-to-sparse-paths grep $opt . 2>actual &&\n+\t\ttest_i18ngrep 'restrict-to-sparse-paths is incompatible with' actual\n+\t\"\n+done\n+\n test_done\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 8425b9a531..a8c2ac9d70 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1928,6 +1928,8 @@ test_expect_success 'double dash \"git\" itself' '\n \t--namespace=\n \t--no-replace-objects Z\n \t--help Z\n+\t--restrict-to-sparse-paths Z\n+\t--no-restrict-to-sparse-paths Z\n \tEOF\n '\n \n@@ -1970,7 +1972,7 @@ test_expect_success 'general options' '\n \ttest_completion \"git --nam\" \"--namespace=\" &&\n \ttest_completion \"git --bar\" \"--bare \" &&\n \ttest_completion \"git --inf\" \"--info-path \" &&\n-\ttest_completion \"git --no-r\" \"--no-replace-objects \"\n+\ttest_completion \"git --no-rep\" \"--no-replace-objects \"\n '\n \n test_expect_success 'general options plus command' '\n-- \n2.28.0\n\n"},{"id":"404907","messageId":"CAPig+cSLjMabeLgkg9N7+ZJ1jotbpJx0FAnjkpNSt0Lf+Q0wNQ@mail.gmail.com","threadId":"54177","inReplyTo":"f53782f14c5f53da5d5537b369a810a94f9ce184.1599026986.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-02T06:57:17Z","receivedAt":"2020-09-02T06:57:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares\n wrote:\n> One test in t1308 expects test-config to fail with exit code 128 due to\n> a parsing error in the config machinery. But test-config might also exit\n> with 128 for any other reason that leads it to call die(). Therefore the\n> test can potentially succeed for the wrong reason. To avoid false\n> positives, let's check test-config's output, in addition to the exit\n> code, and make sure that the cause of the error is the one we expect in\n> this test.\n>\n> Moreover, the test was using the auxiliary function check_config which\n> optionally takes a string to compare the test-config stdout against.\n> Because this string is optional, there is a risk that future callers may\n> also check only the exit code and not the output. To avoid that, make\n> the string parameter of this function mandatory.\n>\n> Signed-off-by: Matheus Tavares \n> ---\n> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\n> @@ -14,10 +14,7 @@ check_config () {\n> expect_code=0\n> fi &&\n> op=$1 key=$2 && shift && shift &&\n> - if test $# != 0\n> - then\n> - printf \"%s\\n\" \"$@\"\n> - fi >expect &&\n> + printf \"%s\\n\" \"$@\" >expect &&\n\nThis change in behavior is quite subtle. With the original code,\n\"expect\" will be entirely empty if no argument is provided, whereas\nwith the revised code, \"expect\" will contain a single newline. This\ncould be improved by making the argument genuinely mandatory as stated\nin the commit message. Perhaps something like this:\n\n if test $# -eq 0\n then\n BUG \"check_config 'value' argument missing\"\n fi &&\n printf \"%s\\n\" \"$@\" >expect &&\n\n> @@ -130,7 +127,8 @@ test_expect_success 'check line error when NULL string is queried' '\n> test_expect_success 'find integer if value is non parse-able' '\n> - check_config expect_code 128 get_int lamb.head\n> + test_expect_code 128 test-tool config get_int lamb.head 2>result &&\n> + test_i18ngrep \"fatal: bad numeric config value '\\'none\\'' for '\\'lamb.head\\''\" result\n> '\n\nThe complex '\\'quoting\\'' magic leaves and re-enters the single-quote\ncontext of the test body and makes it difficult to reason about. Since\nthis is a pattern argument to grep, a simpler alternative would be:\n\n test_i18ngrep \"fatal: bad numeric config value .none. for\n.lamb.head.\" result\n\nAside from that, do I understand correctly that all other callers\nwhich expect a non-zero exit code will find the error message on\nstdout, but this case will find it on stderr? That makes one wonder\nif, rather than dropping use of check_config() here, instead\ncheck_config() should be enhanced to accept an additional option, such\nas 'stderr' which causes it to check stderr rather than stdout\n(similar to how 'expect_code' allows the caller to override the\nexpected exit code). But perhaps that would be overengineered if this\ncase is not expected to come up again as more callers are added in the\nfuture?\n"},{"id":"404908","messageId":"CAPig+cT7bO97kWgQCftetR_4i_eTU4FsGejkdkY+cbd5K78oLA@mail.gmail.com","threadId":"54177","inReplyTo":"0750191342754bcca398c6fdad522616b0f3fbc3.1599026986.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v5 4/8] t/helper/test-config: check argc before accessing argv","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-02T07:18:38Z","receivedAt":"2020-09-02T07:18:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares\n wrote:\n> Check that we have the expected argc in 'configset_get_value' and\n> 'configset_get_value_multi' before trying to access argv elements.\n>\n> Signed-off-by: Matheus Tavares \n> ---\n> diff --git a/t/helper/test-config.c b/t/helper/test-config.c\n> @@ -138,7 +138,7 @@ int cmd__config(int argc, const char **argv)\n> - } else if (!strcmp(argv[1], \"configset_get_value\")) {\n> + } else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value\")) {\n> for (i = 3; i < argc; i++) {\n> @@ -156,7 +156,7 @@ int cmd__config(int argc, const char **argv)\n> printf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n\nThis is certainly a bug fix since it was accessing argv[2] without\nchecking that that element was even present, but the more significant\noutcome of this change is that it now correctly diagnoses when these\ntwo commands are called with the wrong number of arguments (just like\nall the other commands -- except \"iterate\" -- diagnose incorrect\nnumber of arguments). It might make sense, therefore, for the commit\nmessage to focus on that improvement and mention the out-of-bounds\narray access fix as a side-effect. However, that itself is not worth a\nre-roll.\n"},{"id":"404909","messageId":"CAPig+cSZFOXuLBKBLrNkZ=dTmQUrp-QiUb7wYGoza-YmEFiM9A@mail.gmail.com","threadId":"54177","inReplyTo":"56535b0e36e94dc73aa570f4f3c0466305c6432f.1599026986.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v5 5/8] t/helper/test-config: unify exit labels","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-02T07:30:29Z","receivedAt":"2020-09-02T07:30:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares\n wrote:\n> test-config's main function has three different exit labels, all of\n> which have to perform the same cleanup code before returning. Unify the\n> labels in preparation for the next patch which will increase the cleanup\n> section.\n>\n> Signed-off-by: Matheus Tavares \n> ---\n> diff --git a/t/helper/test-config.c b/t/helper/test-config.c\n> @@ -69,16 +69,19 @@ static int early_config_cb(const char *var, const char *value, void *vdata)\n> +#define TC_VALUE_NOT_FOUND 1\n> +#define TC_CONFIG_FILE_ERROR 2\n> +\n> int cmd__config(int argc, const char **argv)\n> {\n> + int i, val, ret = 0;\n>\n> if (argc == 3 && !strcmp(argv[1], \"read_early_config\")) {\n> read_early_config(early_config_cb, (void *)argv[2]);\n> - return 0;\n> + return ret;\n> }\n\nThis one change feels both fragile and increases cognitive load, thus\ndoes not seem desirable. It feels fragile because someone could come\nalong and insert code above this conditional which assigns some value\nother than 0 to 'ret' (not realizing that this conditional wants to\nreturn 0), thus breaking it. It increases cognitive load because it\nforces the reader to scan all the code leading up to this point to\ndetermine what value this conditional really wants to return.\n\nNevertheless, this is a minor objection, not necessarily worth a re-roll.\n\n> @@ -94,10 +97,9 @@ int cmd__config(int argc, const char **argv)\n> printf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n> - goto exit1;\n> + ret = TC_VALUE_NOT_FOUND;\n\nThis sort of change, on the other hand, does not increase cognitive\nload because it's quite obvious what return value this conditional\nwants to return (because it's assigning it explicitly).\n"},{"id":"404918","messageId":"CAHd-oW5gk16=Tmhi_e43vVCufM9=zd8jtZJ_EFJVLmC4dJ06=w@mail.gmail.com","threadId":"54177","inReplyTo":"CAPig+cSLjMabeLgkg9N7+ZJ1jotbpJx0FAnjkpNSt0Lf+Q0wNQ@mail.gmail.com","subject":"Re: [PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-02T16:16:06Z","receivedAt":"2020-09-02T16:16:52Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Wed, Sep 2, 2020 at 3:57 AM Eric Sunshine wrote:\n>\n> On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares\n> wrote:\n> > One test in t1308 expects test-config to fail with exit code 128 due to\n> > a parsing error in the config machinery. But test-config might also exit\n> > with 128 for any other reason that leads it to call die(). Therefore the\n> > test can potentially succeed for the wrong reason. To avoid false\n> > positives, let's check test-config's output, in addition to the exit\n> > code, and make sure that the cause of the error is the one we expect in\n> > this test.\n> >\n> > Moreover, the test was using the auxiliary function check_config which\n> > optionally takes a string to compare the test-config stdout against.\n> > Because this string is optional, there is a risk that future callers may\n> > also check only the exit code and not the output. To avoid that, make\n> > the string parameter of this function mandatory.\n> >\n> > Signed-off-by: Matheus Tavares \n> > ---\n> > diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\n> > @@ -14,10 +14,7 @@ check_config () {\n> > expect_code=0\n> > fi &&\n> > op=$1 key=$2 && shift && shift &&\n> > - if test $# != 0\n> > - then\n> > - printf \"%s\\n\" \"$@\"\n> > - fi >expect &&\n> > + printf \"%s\\n\" \"$@\" >expect &&\n>\n> This change in behavior is quite subtle. With the original code,\n> \"expect\" will be entirely empty if no argument is provided, whereas\n> with the revised code, \"expect\" will contain a single newline. This\n> could be improved by making the argument genuinely mandatory as stated\n> in the commit message. Perhaps something like this:\n>\n> if test $# -eq 0\n> then\n> BUG \"check_config 'value' argument missing\"\n> fi &&\n> printf \"%s\\n\" \"$@\" >expect &&\n\nThanks for catching this. I will add the check.\n\n> > @@ -130,7 +127,8 @@ test_expect_success 'check line error when NULL string is queried' '\n> > test_expect_success 'find integer if value is non parse-able' '\n> > - check_config expect_code 128 get_int lamb.head\n> > + test_expect_code 128 test-tool config get_int lamb.head 2>result &&\n> > + test_i18ngrep \"fatal: bad numeric config value '\\'none\\'' for '\\'lamb.head\\''\" result\n> > '\n>exit\n> The complex '\\'quoting\\'' magic leaves and re-enters the single-quote\n> context of the test body and makes it difficult to reason about. Since\n> this is a pattern argument to grep, a simpler alternative would be:\n>\n> test_i18ngrep \"fatal: bad numeric config value .none. for\n> .lamb.head.\" result\n\nWill do, thanks.\n\n> Aside from that, do I understand correctly that all other callers\n> which expect a non-zero exit code will find the error message on\n> stdout, but this case will find it on stderr?\n\nRight. This happens because, for a \"value not found\" error,\ntest-config will exit with code 1 and print to stdout. This is the\nonly case where it exits with a non-zero code and prints to stdout\ninstead of stderr.\n\nWith that said, I'm wondering now whether we should change the\nfunction's signature from:\n\n`check_config [expect_code ] `\n\nto:\n\n`check_config `\n`check_config expect_not_found `\n\nThe second form would then automatically expect exit code 1 and check\nstdout for the message 'Value not found for \"\"'. With this we\ncan avoid wrong uses of check_config to check an arbitrary error code\nwithout also checking stderr.\n\n> That makes one wonder\n> if, rather than dropping use of check_config() here, instead\n> check_config() should be enhanced to accept an additional option, such\n> as 'stderr' which causes it to check stderr rather than stdout\n> (similar to how 'expect_code' allows the caller to override the\n> expected exit code). But perhaps that would be overengineered if this\n> case is not expected to come up again as more callers are added in the\n> future?\n\nThat's an interesting idea. However, because some callers may want to\nuse test_i18ngrep instead of test_cmp, I think the required logic\nwould become too complex.\n"},{"id":"404920","messageId":"CAPig+cSdAp=gaJ01MSxhxc+j=+OFkiPK7e6c960RZwUU22sKhQ@mail.gmail.com","threadId":"54177","inReplyTo":"CAHd-oW5gk16=Tmhi_e43vVCufM9=zd8jtZJ_EFJVLmC4dJ06=w@mail.gmail.com","subject":"Re: [PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-02T16:38:00Z","receivedAt":"2020-09-02T16:38:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 2, 2020 at 12:16 PM Matheus Tavares Bernardino\n wrote:\n> With that said, I'm wondering now whether we should change the\n> function's signature from:\n>\n> `check_config [expect_code ] `\n>\n> to:\n>\n> `check_config `\n> `check_config expect_not_found `\n>\n> The second form would then automatically expect exit code 1 and check\n> stdout for the message 'Value not found for \"\"'. With this we\n> can avoid wrong uses of check_config to check an arbitrary error code\n> without also checking stderr.\n\nYes, that seems more straightforward. In fact, at this point, you\ncould just have two distinct functions and eliminate the ugly\ncomplexity of the existing check_config() implementation. Perhaps\nsomething like this (typed in email):\n\n check_config () {\n test_tool config \"$1\" \"$2\" >actual &&\n shift && shift &&\n printf \"%s\\n\" \"$@\" >expect &&\n test_cmp expect actual\n }\n\n check_not_found () {\n test_expect_code 1 test_tool config \"$1\" \"$2\" >actual &&\n echo \"Value not found for \\\"$2\\\"\" >expect &&\n test_cmp expect actual\n }\n"},{"id":"404936","messageId":"20200902201523.GA3941032@google.com","threadId":"54177","inReplyTo":"3e02e1bd248438e0b435a19d857432edcaa15a2c.1599026986.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v5 6/8] config: correctly read worktree configs in submodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-09-02T20:15:23Z","receivedAt":"2020-09-02T20:15:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Matheus Tavares wrote:\n\n> The config machinery is not able to read worktree configs from a\n> submodule in a process where the_repository represents the superproject.\n\n... where the_repository represents the superproject and\nextensions.worktreeConfig is not set there, right?\n\n> Furthermore, when extensions.worktreeConfig is set on the superproject,\n> querying for a worktree config in a submodule will, instead, return\n> the value set at the superproject.\n>\n> The problem resides in do_git_config_sequence(). Although the function\n> receives a git_dir string, it uses the_repository->git_dir when making\n\nThis part of the commit message seems to be rephrasing what the patch\nsays; for that kind of thing, it seems better to let the patch speak\nfor itself. Can we describe what is happening at a higher level (in\nother words the intent instead of the details of how that is\nmanifested in code)? For example,\n\n The relevant code is in do_git_config_sequence. Although it is designed\n to act on an arbitrary repository, specified by the passed-in git_dir\n string, it accidentally depends on the_repository in two places:\n\n - it reads the global variable `repository_format_worktree_config`,\n which is set based on the content of the_repository, to determine\n whether extensions.worktreeConfig is set\n\n - it uses the git_pathdup helper to find the config.worktree file,\n instead of making a path using the passed-in git_dir falue\n\n Sever these dependencies.\n\n[...]\n> --- a/config.c\n> +++ b/config.c\n> @@ -1747,11 +1747,22 @@ static int do_git_config_sequence(const struct config_options *opts,\n> \t\tret += git_config_from_file(fn, repo_config, data);\n> \n> \tcurrent_parsing_scope = CONFIG_SCOPE_WORKTREE;\n> -\tif (!opts->ignore_worktree && repository_format_worktree_config) {\n> +\tif (!opts->ignore_worktree && repo_config && opts->git_dir) {\n\nrepo_config is non-NULL if and only if commondir is non-NULL and\ncommondir is always non-NUlL if git_dir is non-NULL (as checked higher\nin the function), right? I think that means this condition could be\nwritten more simply as\n\n\tif (!opts->ignore_worktree && opts->git_dir) {\n\nwhich I think should be easier for the reader to understand.\n\n> +\t\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\t\tread_repository_format(&repo_fmt, repo_config);\n> +\n> +\t\tif (!verify_repository_format(&repo_fmt, &buf) &&\n> +\t\t repo_fmt.worktree_config) {\n\nIn the common case where we are acting on the_repository, this add\nextra complexity and slows the routine down.\n\nWould passing in the 'struct repository *' to allow distinguishing\nthat case help? Something like this:\n\ndiff --git i/builtin/config.c w/builtin/config.c\nindex 5e39f618854..ca4caedf33a 100644\n--- i/builtin/config.c\n+++ w/builtin/config.c\n@@ -699,10 +699,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tconfig_options.respect_includes = !given_config_source.file;\n \telse\n \t\tconfig_options.respect_includes = respect_includes_opt;\n-\tif (!nongit) {\n-\t\tconfig_options.commondir = get_git_common_dir();\n-\t\tconfig_options.git_dir = get_git_dir();\n-\t}\n+\tif (!nongit)\n+\t\tconfig_options.repo = the_repository;\n \n \tif (end_nul) {\n \t\tterm = '\\0';\ndiff --git i/config.c w/config.c\nindex 2bdff4457be..70a1dd0ad3f 100644\n--- i/config.c\n+++ w/config.c\n@@ -222,8 +222,8 @@ static int include_by_gitdir(const struct config_options *opts,\n \tconst char *git_dir;\n \tint already_tried_absolute = 0;\n \n-\tif (opts->git_dir)\n-\t\tgit_dir = opts->git_dir;\n+\tif (opts->repo && opts->repo->gitdir)\n+\t\tgit_dir = opts->repo->gitdir;\n \telse\n \t\tgoto done;\n \n@@ -1720,10 +1720,10 @@ static int do_git_config_sequence(const struct config_options *opts,\n \tchar *repo_config;\n \tenum config_scope prev_parsing_scope = current_parsing_scope;\n \n-\tif (opts->commondir)\n-\t\trepo_config = mkpathdup(\"%s/config\", opts->commondir);\n-\telse if (opts->git_dir)\n-\t\tBUG(\"git_dir without commondir\");\n+\tif (opts->repo && opts->repo->commondir)\n+\t\trepo_config = mkpathdup(\"%s/config\", opts->repo->commondir);\n+\telse if (opts->repo && opts->repo->gitdir)\n+\t\tBUG(\"gitdir without commondir\");\n \telse\n \t\trepo_config = NULL;\n \n@@ -1824,27 +1824,33 @@ void read_early_config(config_fn_t cb, void *data)\n \tstruct config_options opts = {0};\n \tstruct strbuf commondir = STRBUF_INIT;\n \tstruct strbuf gitdir = STRBUF_INIT;\n+\tstruct repository the_early_repo = {0};\n \n \topts.respect_includes = 1;\n \n \tif (have_git_dir()) {\n-\t\topts.commondir = get_git_common_dir();\n-\t\topts.git_dir = get_git_dir();\n+\t\topts.repo = the_repository;\n \t/*\n \t * When setup_git_directory() was not yet asked to discover the\n \t * GIT_DIR, we ask discover_git_directory() to figure out whether there\n \t * is any repository config we should use (but unlike\n-\t * setup_git_directory_gently(), no global state is changed, most\n+\t * setup_git_directory_gently(), no global state is changed; most\n \t * notably, the current working directory is still the same after the\n \t * call).\n+\t *\n+\t * NEEDSWORK: There is some duplicate work between\n+\t * discover_git_directory and repo_init. Update to use a variant of\n+\t * repo_init that does its own repository discovery once available.\n \t */\n \t} else if (!discover_git_directory(&commondir, &gitdir)) {\n-\t\topts.commondir = commondir.buf;\n-\t\topts.git_dir = gitdir.buf;\n+\t\trepo_init(&the_early_repo, gitdir.buf, NULL);\n+\t\topts.repo = &the_early_repo;\n \t}\n \n \tconfig_with_options(cb, data, NULL, &opts);\n \n+\tif (the_early_repo.settings.initialized)\n+\t\trepo_clear(&the_early_repo);\n \tstrbuf_release(&commondir);\n \tstrbuf_release(&gitdir);\n }\n@@ -2097,8 +2103,7 @@ static void repo_read_config(struct repository *repo)\n \tstruct config_options opts = { 0 };\n \n \topts.respect_includes = 1;\n-\topts.commondir = repo->commondir;\n-\topts.git_dir = repo->gitdir;\n+\topts.repo = repo;\n \n \tif (!repo->config)\n \t\trepo->config = xcalloc(1, sizeof(struct config_set));\ndiff --git i/config.h w/config.h\nindex 91cdfbfb414..e56293fb29f 100644\n--- i/config.h\n+++ w/config.h\n@@ -21,6 +21,7 @@\n */\n \n struct object_id;\n+struct repository;\n \n /* git_config_parse_key() returns these negated: */\n #define CONFIG_INVALID_KEY 1\n@@ -87,8 +88,7 @@ struct config_options {\n \tunsigned int ignore_worktree : 1;\n \tunsigned int ignore_cmdline : 1;\n \tunsigned int system_gently : 1;\n-\tconst char *commondir;\n-\tconst char *git_dir;\n+\tstruct repository *repo;\n \tconfig_parser_event_fn_t event_fn;\n \tvoid *event_fn_data;\n \tenum config_error_action {\n==== >8 ====\n\n[...]\n> --- a/t/helper/test-config.c\n> +++ b/t/helper/test-config.c\n[...]\n> @@ -72,14 +80,34 @@ static int early_config_cb(const char *var, const char *value, void *vdata)\n> #define TC_VALUE_NOT_FOUND 1\n> #define TC_CONFIG_FILE_ERROR 2\n> \n> +static const char *test_config_usage[] = {\n> +\t\"test-tool config [--submodule=] []\",\n> +\tNULL\n> +};\n> +\n> int cmd__config(int argc, const char **argv)\n> {\n> \tint i, val, ret = 0;\n> \tconst char *v;\n> \tconst struct string_list *strptr;\n> \tstruct config_set cs;\n> +\tstruct repository subrepo, *repo = the_repository;\n> +\tconst char *subrepo_path = NULL;\n> +\n> +\tstruct option options[] = {\n> +\t\tOPT_STRING(0, \"submodule\", &subrepo_path, \"path\",\n> +\t\t\t \"run on the submodule at \"),\n> +\t\tOPT_END()\n> +\t};\n\nNice.\n\n> +\n> +\targc = parse_options(argc, argv, NULL, options, test_config_usage,\n> +\t\t\t PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_STOP_AT_NON_OPTION);\n> +\tif (argc < 2)\n> +\t\tdie(\"Please, provide a command name on the command-line\");\n\noptional nit: can use usage_with_options here. It produces a better\nerror message than any other I can think of (all I can think of are\nthings like \"need a \").\n\nThis is from existing code, but the use of parse_options opens up the\npossibility of taking advantage of the parse-options generated message. :)\n\n[...]\n> --- a/t/t2404-worktree-config.sh\n> +++ b/t/t2404-worktree-config.sh\n> @@ -78,4 +78,20 @@ test_expect_success 'config.worktree no longer read without extension' '\n> \ttest_cmp_config -c wt2 shared this.is\n> '\n> \n> +test_expect_success 'correctly read config.worktree from submodules' '\n> +\ttest_unconfig extensions.worktreeconfig &&\n> +\tgit init sub &&\n> +\t(\n> +\t\tcd sub &&\n> +\t\ttest_commit a &&\n> +\t\tgit config extensions.worktreeconfig true &&\n> +\t\tgit config --worktree wtconfig.sub test-value\n> +\t) &&\n> +\tgit submodule add ./sub &&\n> +\tgit commit -m \"add sub\" &&\n> +\techo test-value >expect &&\n> +\ttest-tool config --submodule=sub get_value wtconfig.sub >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nLovely.\n\nSummary: I like the direction this change goes in.\n\nI think we can do it without repeating repository format discovery in\nthe the_repository case and without duplicating repository format\ndiscovery code in the submodule case. If it proves too fussy, then a\nNEEDSWORK comment would be helpful to help the reader see what is\ngoing on.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"405246","messageId":"CAHd-oW6jCiaXdsKnhde0iBDWXpZyh4cf5j_8zW7x=W3BaZ9Kbg@mail.gmail.com","threadId":"54177","inReplyTo":"20200902201523.GA3941032@google.com","subject":"Re: [PATCH v5 6/8] config: correctly read worktree configs in submodules","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-09T13:04:23Z","receivedAt":"2020-09-09T14:20:29Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Jonathan\n\nSorry for the late reply, last week was quite busy.\n\nOn Wed, Sep 2, 2020 at 5:15 PM Jonathan Nieder wrote:\n>\n> Matheus Tavares wrote:\n>\n> > The config machinery is not able to read worktree configs from a\n> > submodule in a process where the_repository represents the superproject.\n>\n> ... where the_repository represents the superproject and\n> extensions.worktreeConfig is not set there, right?\n>\n> > Furthermore, when extensions.worktreeConfig is set on the superproject,\n> > querying for a worktree config in a submodule will, instead, return\n> > the value set at the superproject.\n> >\n> > The problem resides in do_git_config_sequence(). Although the function\n> > receives a git_dir string, it uses the_repository->git_dir when making\n>\n> This part of the commit message seems to be rephrasing what the patch\n> says; for that kind of thing, it seems better to let the patch speak\n> for itself. Can we describe what is happening at a higher level (in\n> other words the intent instead of the details of how that is\n> manifested in code)? For example,\n>\n> The relevant code is in do_git_config_sequence. Although it is designed\n> to act on an arbitrary repository, specified by the passed-in git_dir\n> string, it accidentally depends on the_repository in two places:\n>\n> - it reads the global variable `repository_format_worktree_config`,\n> which is set based on the content of the_repository, to determine\n> whether extensions.worktreeConfig is set\n>\n> - it uses the git_pathdup helper to find the config.worktree file,\n> instead of making a path using the passed-in git_dir falue\n>\n> Sever these dependencies.\n\nYeah, much better, thanks! :)\n\n> [...]\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -1747,11 +1747,22 @@ static int do_git_config_sequence(const struct config_options *opts,\n> > ret += git_config_from_file(fn, repo_config, data);\n> >\n> > current_parsing_scope = CONFIG_SCOPE_WORKTREE;\n> > - if (!opts->ignore_worktree && repository_format_worktree_config) {\n> > + if (!opts->ignore_worktree && repo_config && opts->git_dir) {\n>\n> repo_config is non-NULL if and only if commondir is non-NULL and\n> commondir is always non-NUlL if git_dir is non-NULL (as checked higher\n> in the function), right? I think that means this condition could be\n> written more simply as\n>\n> if (!opts->ignore_worktree && opts->git_dir) {\n>\n> which I think should be easier for the reader to understand.\n\nNice, thanks.\n\n> > + struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n> > + struct strbuf buf = STRBUF_INIT;\n> > +\n> > + read_repository_format(&repo_fmt, repo_config);\n> > +\n> > + if (!verify_repository_format(&repo_fmt, &buf) &&\n> > + repo_fmt.worktree_config) {\n>\n> In the common case where we are acting on the_repository, this add\n> extra complexity and slows the routine down.\n>\n> Would passing in the 'struct repository *' to allow distinguishing\n> that case help? Something like this:\n>\n> diff --git i/builtin/config.c w/builtin/config.c\n> index 5e39f618854..ca4caedf33a 100644\n> --- i/builtin/config.c\n> +++ w/builtin/config.c\n> @@ -699,10 +699,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n> config_options.respect_includes = !given_config_source.file;\n> else\n> config_options.respect_includes = respect_includes_opt;\n> - if (!nongit) {\n> - config_options.commondir = get_git_common_dir();\n> - config_options.git_dir = get_git_dir();\n> - }\n> + if (!nongit)\n> + config_options.repo = the_repository;\n>\n> if (end_nul) {\n> term = '\\0';\n> diff --git i/config.c w/config.c\n> index 2bdff4457be..70a1dd0ad3f 100644\n> --- i/config.c\n> +++ w/config.c\n> @@ -222,8 +222,8 @@ static int include_by_gitdir(const struct config_options *opts,\n> const char *git_dir;\n> int already_tried_absolute = 0;\n>\n> - if (opts->git_dir)\n> - git_dir = opts->git_dir;\n> + if (opts->repo && opts->repo->gitdir)\n> + git_dir = opts->repo->gitdir;\n> else\n> goto done;\n>\n> @@ -1720,10 +1720,10 @@ static int do_git_config_sequence(const struct config_options *opts,\n> char *repo_config;\n> enum config_scope prev_parsing_scope = current_parsing_scope;\n>\n> - if (opts->commondir)\n> - repo_config = mkpathdup(\"%s/config\", opts->commondir);\n> - else if (opts->git_dir)\n> - BUG(\"git_dir without commondir\");\n> + if (opts->repo && opts->repo->commondir)\n> + repo_config = mkpathdup(\"%s/config\", opts->repo->commondir);\n> + else if (opts->repo && opts->repo->gitdir)\n> + BUG(\"gitdir without commondir\");\n> else\n> repo_config = NULL;\n>\n> @@ -1824,27 +1824,33 @@ void read_early_config(config_fn_t cb, void *data)\n> struct config_options opts = {0};\n> struct strbuf commondir = STRBUF_INIT;\n> struct strbuf gitdir = STRBUF_INIT;\n> + struct repository the_early_repo = {0};\n>\n> opts.respect_includes = 1;\n>\n> if (have_git_dir()) {\n> - opts.commondir = get_git_common_dir();\n> - opts.git_dir = get_git_dir();\n> + opts.repo = the_repository;\n\nI'm not very familiar with the code in setup.c so I apologize for the\nnoob question: have_git_dir() returns `startup_info->have_repository\n|| the_repository->gitdir`; so is it possible that it returns true but\nthe_repository->gitdir is not initialized yet? If so, should we also\ncheck the_repository->gitdir here (before assigning opts.repo), and\ncall BUG() when it is NULL, like get_git_dir() does?\n\nHmm, nevertheless, I see that you already check `opts.repo &&\nopts.repo->gitdir` before trying to use it in\ndo_git_config_sequence(). So it should already cover this case, right?\n\n> /*\n> * When setup_git_directory() was not yet asked to discover the\n> * GIT_DIR, we ask discover_git_directory() to figure out whether there\n> * is any repository config we should use (but unlike\n> - * setup_git_directory_gently(), no global state is changed, most\n> + * setup_git_directory_gently(), no global state is changed; most\n> * notably, the current working directory is still the same after the\n> * call).\n> + *\n> + * NEEDSWORK: There is some duplicate work between\n> + * discover_git_directory and repo_init. Update to use a variant of\n> + * repo_init that does its own repository discovery once available.\n> */\n> } else if (!discover_git_directory(&commondir, &gitdir)) {\n> - opts.commondir = commondir.buf;\n> - opts.git_dir = gitdir.buf;\n> + repo_init(&the_early_repo, gitdir.buf, NULL);\n> + opts.repo = &the_early_repo;\n> }\n>\n> config_with_options(cb, data, NULL, &opts);\n>\n> + if (the_early_repo.settings.initialized)\n> + repo_clear(&the_early_repo);\n>\n> strbuf_release(&commondir);\n> strbuf_release(&gitdir);\n> }\n> @@ -2097,8 +2103,7 @@ static void repo_read_config(struct repository *repo)\n> struct config_options opts = { 0 };\n>\n> opts.respect_includes = 1;\n> - opts.commondir = repo->commondir;\n> - opts.git_dir = repo->gitdir;\n> + opts.repo = repo;\n>\n> if (!repo->config)\n> repo->config = xcalloc(1, sizeof(struct config_set));\n> diff --git i/config.h w/config.h\n> index 91cdfbfb414..e56293fb29f 100644\n> --- i/config.h\n> +++ w/config.h\n> @@ -21,6 +21,7 @@\n> */\n>\n> struct object_id;\n> +struct repository;\n>\n> /* git_config_parse_key() returns these negated: */\n> #define CONFIG_INVALID_KEY 1\n> @@ -87,8 +88,7 @@ struct config_options {\n> unsigned int ignore_worktree : 1;\n> unsigned int ignore_cmdline : 1;\n> unsigned int system_gently : 1;\n> - const char *commondir;\n> - const char *git_dir;\n> + struct repository *repo;\n> config_parser_event_fn_t event_fn;\n> void *event_fn_data;\n> enum config_error_action {\n> ==== >8 ====\n\nThanks a lot for this :) I was thinking of adding it as a preparatory\npatch before the fix itself. May I have your S-o-B as the author?\n\nBest,\nMatheus\n"},{"id":"405303","messageId":"20200909233247.GA667601@google.com","threadId":"54177","inReplyTo":"CAHd-oW6jCiaXdsKnhde0iBDWXpZyh4cf5j_8zW7x=W3BaZ9Kbg@mail.gmail.com","subject":"Re: [PATCH v5 6/8] config: correctly read worktree configs in submodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-09-09T23:32:47Z","receivedAt":"2020-09-10T02:47:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMatheus Tavares Bernardino wrote:\n\n> Sorry for the late reply, last week was quite busy.\n\nNo problem. It's an unusual time for everyone.\n\n[...]\n> On Wed, Sep 2, 2020 at 5:15 PM Jonathan Nieder wrote:\n\n>> @@ -1824,27 +1824,33 @@ void read_early_config(config_fn_t cb, void *data)\n>> struct config_options opts = {0};\n>> struct strbuf commondir = STRBUF_INIT;\n>> struct strbuf gitdir = STRBUF_INIT;\n>> + struct repository the_early_repo = {0};\n>>\n>> opts.respect_includes = 1;\n>>\n>> if (have_git_dir()) {\n>> - opts.commondir = get_git_common_dir();\n>> - opts.git_dir = get_git_dir();\n>> + opts.repo = the_repository;\n>\n> I'm not very familiar with the code in setup.c so I apologize for the\n> noob question: have_git_dir() returns `startup_info->have_repository\n> || the_repository->gitdir`; so is it possible that it returns true but\n> the_repository->gitdir is not initialized yet? If so, should we also\n> check the_repository->gitdir here (before assigning opts.repo), and\n> call BUG() when it is NULL, like get_git_dir() does?\n>\n> Hmm, nevertheless, I see that you already check `opts.repo &&\n> opts.repo->gitdir` before trying to use it in\n> do_git_config_sequence(). So it should already cover this case, right?\n\nRight --- the main point is that a BUG() call represents \"this can't\nhappen\", or in other words, it's an assertion failure. As a matter of\ndefensive coding functions like get_git_dir() guard against such cases\nto make debugging a little easier and exploitation a little more\ndifficult when the impossible happens.\n\n[...]\n> Thanks a lot for this :) I was thinking of adding it as a preparatory\n> patch before the fix itself. May I have your S-o-B as the author?\n\nSure!\n\nSigned-off-by: Jonathan Nieder \n\nThanks,\nJonathan\n"},{"id":"405336","messageId":"cover.1599758167.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599026986.git.matheus.bernardino@usp.br","subject":"[PATCH v6 0/9] grep: honor sparse checkout and add option to ignore it","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-10T17:21:19Z","receivedAt":"2020-09-10T17:22:55Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"This series makes git-grep restrict its output to the sparsity patterns when\nrequested by the user. A new global option is added to control this behavior\nin grep and hopefully more commands in the future. There is also a fix in\nconfig.c, to correctly read worktree-specific settings on submodules.\n\nChanges since v5:\n\nPatch 2:\n- Avoid complex '\\'quoting\\'' by using '.', as the string is going to\n be used in a grep search.\n- Split the check_config auxiliary function into two, to remove some\n unecessary logic and future-proof them against wrong usages of\n test-config.\n\nPatch 4: reword commit message to focus on correctly diagnosing missing\n arguments.\n\nPatch 5: don't replace explicit `return 0` with `return ret`.\n\nAdded patch 6, passing a `struct repository` to do_git_config_sequence().\n\nPatch 6:\n- Removed global `repository_format_worktree_config` and cached\n extensions.worktreeConfig value in `struct repository`, to avoid repeating\n repository format discovery.\n- Improved commit message as suggested by Jonathan.\n- Use usage_with_options()\n\n\nJonathan Nieder (1):\n config: make do_git_config_sequence receive a 'struct repository'\n\nMatheus Tavares (8):\n doc: grep: unify info on configuration variables\n t1308-config-set: avoid false positives when using test-config\n t/helper/test-config: be consistent with exit codes\n t/helper/test-config: diagnose missing arguments\n t/helper/test-config: unify exit labels\n config: correctly read worktree configs in submodules\n grep: honor sparse checkout patterns\n config: add setting to ignore sparsity patterns in some cmds\n\n Documentation/config.txt | 2 +\n Documentation/config/grep.txt | 18 +-\n Documentation/config/sparse.txt | 20 ++\n Documentation/git-grep.txt | 36 +--\n Documentation/git.txt | 4 +\n Makefile | 1 +\n builtin/config.c | 8 +-\n builtin/grep.c | 134 ++++++++++-\n cache.h | 1 -\n config.c | 47 ++--\n config.h | 4 +-\n contrib/completion/git-completion.bash | 2 +\n environment.c | 1 -\n git.c | 5 +\n repository.c | 1 +\n repository.h | 1 +\n setup.c | 4 +-\n sparse-checkout.c | 18 ++\n sparse-checkout.h | 11 +\n t/helper/test-config.c | 124 ++++++----\n t/t1308-config-set.sh | 28 +--\n t/t2404-worktree-config.sh | 16 ++\n t/t7011-skip-worktree-reading.sh | 9 -\n t/t7817-grep-sparse-checkout.sh | 321 +++++++++++++++++++++++++\n t/t9902-completion.sh | 4 +-\n 25 files changed, 682 insertions(+), 138 deletions(-)\n create mode 100644 Documentation/config/sparse.txt\n create mode 100644 sparse-checkout.c\n create mode 100644 sparse-checkout.h\n create mode 100755 t/t7817-grep-sparse-checkout.sh\n\nRange-diff against v5:\n 1: 70c9a4e741 = 1: 70c9a4e741 doc: grep: unify info on configuration variables\n 2: f53782f14c < -: ---------- t1308-config-set: avoid false positives when using test-config\n -: ---------- > 2: 3c2d722152 t1308-config-set: avoid false positives when using test-config\n 3: 85e1588d6c = 3: 45d13744b7 t/helper/test-config: be consistent with exit codes\n 4: 0750191342 ! 4: 51656e43c3 t/helper/test-config: check argc before accessing argv\n @@ Metadata\n Author: Matheus Tavares \n \n ## Commit message ##\n - t/helper/test-config: check argc before accessing argv\n + t/helper/test-config: diagnose missing arguments\n \n - Check that we have the expected argc in 'configset_get_value' and\n - 'configset_get_value_multi' before trying to access argv elements.\n + test-config verifies that the correct number of arguments was given for\n + all of its commands except for 'configset_get_value' and\n + 'configset_get_value_multi'. Add the check to these two, so that we\n + properly report missing arguments and prevent out-of-bounds access to\n + argv[].\n \n Signed-off-by: Matheus Tavares \n \n 5: 56535b0e36 ! 5: 924d2e8ceb t/helper/test-config: unify exit labels\n @@ t/helper/test-config.c: static int early_config_cb(const char *var, const char *\n \tconst char *v;\n \tconst struct string_list *strptr;\n \tstruct config_set cs;\n - \n - \tif (argc == 3 && !strcmp(argv[1], \"read_early_config\")) {\n - \t\tread_early_config(early_config_cb, (void *)argv[2]);\n --\t\treturn 0;\n -+\t\treturn ret;\n - \t}\n - \n - \tsetup_git_directory();\n @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n \t\t\t\tprintf(\"(NULL)\\n\");\n \t\t\telse\n -: ---------- > 6: f5c0fc3336 config: make do_git_config_sequence receive a 'struct repository'\n 6: 3e02e1bd24 ! 7: 3a28b8e608 config: correctly read worktree configs in submodules\n @@ Commit message\n config: correctly read worktree configs in submodules\n \n The config machinery is not able to read worktree configs from a\n - submodule in a process where the_repository represents the superproject.\n - Furthermore, when extensions.worktreeConfig is set on the superproject,\n - querying for a worktree config in a submodule will, instead, return\n - the value set at the superproject.\n -\n - The problem resides in do_git_config_sequence(). Although the function\n - receives a git_dir string, it uses the_repository->git_dir when making\n - the path to the worktree config file. And when checking if\n - extensions.worktreeConfig is set, it uses the global\n - repository_format_worktree_config variable, which refers to\n - the_repository only. So let's fix this by using the git_dir given to the\n - function and reading the extension value from the right place. Also add\n - a test to avoid any regressions.\n + submodule in a process where the_repository represents the superproject\n + and extensions.worktreeConfig is not set there. Furthermore, when\n + extensions.worktreeConfig is set on the superproject, querying for a\n + worktree config in a submodule will, instead, return the value set at\n + the superproject.\n +\n + The relevant code is in do_git_config_sequence(). Although it is\n + designed to act on an arbitrary repository, specified in the passed-in\n + `struct config_options`, it accidentally depends on the_repository in\n + two places:\n +\n + - it reads the global variable `repository_format_worktree_config`,\n + which is set based on the content of the_repository, to determine\n + whether extensions.worktreeConfig is set.\n +\n + - it uses the git_pathdup() helper to find the config.worktree file,\n + instead of making a path using the passed-in repository.\n +\n + Sever these dependencies and add a regression test. Also, to avoid\n + future misuses of `repository_format_worktree_config` like this one,\n + remove this global variable and store the config value on\n + `struct repository` itself.\n \n Signed-off-by: Matheus Tavares \n \n + ## builtin/config.c ##\n +@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)\n + \t\tgiven_config_source.scope = CONFIG_SCOPE_LOCAL;\n + \t} else if (use_worktree_config) {\n + \t\tstruct worktree **worktrees = get_worktrees();\n +-\t\tif (repository_format_worktree_config)\n ++\t\tif (!nongit && the_repository->worktree_config_extension)\n + \t\t\tgiven_config_source.file = git_pathdup(\"config.worktree\");\n + \t\telse if (worktrees[0] && worktrees[1])\n + \t\t\tdie(_(\"--worktree cannot be used with multiple \"\n +\n + ## cache.h ##\n +@@ cache.h: extern int grafts_replace_parents;\n + #define GIT_REPO_VERSION 0\n + #define GIT_REPO_VERSION_READ 1\n + extern int repository_format_precious_objects;\n +-extern int repository_format_worktree_config;\n + \n + /*\n + * You _have_ to initialize a `struct repository_format` using\n +\n ## config.c ##\n @@ config.c: static int do_git_config_sequence(const struct config_options *opts,\n \t\tret += git_config_from_file(fn, repo_config, data);\n @@ config.c: static int do_git_config_sequence(const struct config_options *opts,\n -\t\tif (!access_or_die(path, R_OK, 0))\n -\t\t\tret += git_config_from_file(fn, path, data);\n -\t\tfree(path);\n -+\tif (!opts->ignore_worktree && repo_config && opts->git_dir) {\n -+\t\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n -+\t\tstruct strbuf buf = STRBUF_INIT;\n -+\n -+\t\tread_repository_format(&repo_fmt, repo_config);\n -+\n -+\t\tif (!verify_repository_format(&repo_fmt, &buf) &&\n -+\t\t repo_fmt.worktree_config) {\n -+\t\t\tchar *path = mkpathdup(\"%s/config.worktree\", opts->git_dir);\n -+\t\t\tif (!access_or_die(path, R_OK, 0))\n -+\t\t\t\tret += git_config_from_file(fn, path, data);\n -+\t\t\tfree(path);\n -+\t\t}\n -+\n -+\t\tstrbuf_release(&buf);\n -+\t\tclear_repository_format(&repo_fmt);\n ++\tif (!opts->ignore_worktree && opts->repo && opts->repo->gitdir &&\n ++\t opts->repo->worktree_config_extension) {\n ++\t\tstruct strbuf path = STRBUF_INIT;\n ++\t\tstrbuf_repo_git_path(&path, opts->repo, \"config.worktree\");\n ++\t\tif (!access_or_die(path.buf, R_OK, 0))\n ++\t\t\tret += git_config_from_file(fn, path.buf, data);\n ++\t\tstrbuf_release(&path);\n \t}\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_COMMAND;\n \n + ## environment.c ##\n +@@ environment.c: int warn_ambiguous_refs = 1;\n + int warn_on_object_refname_ambiguity = 1;\n + int ref_paranoia = -1;\n + int repository_format_precious_objects;\n +-int repository_format_worktree_config;\n + const char *git_commit_encoding;\n + const char *git_log_output_encoding;\n + char *apply_default_whitespace;\n +\n + ## repository.c ##\n +@@ repository.c: int repo_init(struct repository *repo,\n + \t\tgoto error;\n + \n + \trepo_set_hash_algo(repo, format.hash_algo);\n ++\trepo->worktree_config_extension = format.worktree_config;\n + \n + \tif (worktree)\n + \t\trepo_set_worktree(repo, worktree);\n +\n + ## repository.h ##\n +@@ repository.h: struct repository {\n + \n + \t/* Indicate if a repository has a different 'commondir' from 'gitdir' */\n + \tunsigned different_commondir:1;\n ++\tunsigned worktree_config_extension:1;\n + };\n + \n + extern struct repository *the_repository;\n +\n + ## setup.c ##\n +@@ setup.c: static int check_repository_format_gently(const char *gitdir, struct repository_\n + \n + \trepository_format_precious_objects = candidate->precious_objects;\n + \tset_repository_format_partial_clone(candidate->partial_clone);\n +-\trepository_format_worktree_config = candidate->worktree_config;\n ++\tthe_repository->worktree_config_extension = candidate->worktree_config;\n + \tstring_list_clear(&candidate->unknown_extensions, 0);\n + \tstring_list_clear(&candidate->v1_only_extensions, 0);\n + \n +-\tif (repository_format_worktree_config) {\n ++\tif (the_repository->worktree_config_extension) {\n + \t\t/*\n + \t\t * pick up core.bare and core.worktree from per-worktree\n + \t\t * config if present\n +\n ## t/helper/test-config.c ##\n @@\n #include \"cache.h\"\n @@ t/helper/test-config.c: static int early_config_cb(const char *var, const char *\n +\targc = parse_options(argc, argv, NULL, options, test_config_usage,\n +\t\t\t PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_STOP_AT_NON_OPTION);\n +\tif (argc < 2)\n -+\t\tdie(\"Please, provide a command name on the command-line\");\n ++\t\tusage_with_options(test_config_usage, options);\n \n \tif (argc == 3 && !strcmp(argv[1], \"read_early_config\")) {\n +\t\tif (subrepo_path)\n +\t\t\tdie(\"cannot use --submodule with read_early_config\");\n \t\tread_early_config(early_config_cb, (void *)argv[2]);\n - \t\treturn ret;\n + \t\treturn 0;\n \t}\n @@ t/helper/test-config.c: int cmd__config(int argc, const char **argv)\n \n 7: 902556a7b6 = 8: 2fc889c9c2 grep: honor sparse checkout patterns\n 8: 70e7d7b90c = 9: 92bc5351cf config: add setting to ignore sparsity patterns in some cmds\n-- \n2.28.0\n\n"},{"id":"405337","messageId":"3c2d722152575d8a4f7aa01d958823a66c5ba26e.1599758167.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599758167.git.matheus.bernardino@usp.br","subject":"[PATCH v6 2/9] t1308-config-set: avoid false positives when using test-config","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-10T17:21:21Z","receivedAt":"2020-09-10T17:23:18Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"One test in t1308 expects test-config to fail with exit code 128 due to\na parsing error in the config machinery. But test-config might also exit\nwith 128 for any other reason that leads it to call die(). Therefore the\ntest can potentially succeed for the wrong reason. To avoid false\npositives, let's check test-config's stderr, in addition to the exit\ncode, and make sure that the cause of the error is the one we expect in\nthis test.\n\nMoreover, the test was using the auxiliary function check_config, which\noptionally takes a number to compare with test-config's exit code, and a\nstring to compare with its stdout. Because the function does not check\nstderr, it can induce improper uses, like the one corrected in this\npatch. To avoid this, remove the optional expect_code parameter,\ndisallowing tests that expect an error from test-config to use this\nhelper function. There is one error, though, which is printed to\nstdout despite returning a non-zero code: \"value not found\" (exit code\n1). For this one, let's add another function which properly checks\nstdout and the code.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Matheus Tavares \n---\n t/t1308-config-set.sh | 28 +++++++++++++---------------\n 1 file changed, 13 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 3a527e3a84..66c6363080 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -7,18 +7,15 @@ test_description='Test git config-set API in different settings'\n # 'check_config get_* section.key value' verifies that the entry for\n # section.key is 'value'\n check_config () {\n-\tif test \"$1\" = expect_code\n-\tthen\n-\t\texpect_code=\"$2\" && shift && shift\n-\telse\n-\t\texpect_code=0\n-\tfi &&\n-\top=$1 key=$2 && shift && shift &&\n-\tif test $# != 0\n-\tthen\n-\t\tprintf \"%s\\n\" \"$@\"\n-\tfi >expect &&\n-\ttest_expect_code $expect_code test-tool config \"$op\" \"$key\" >actual &&\n+\ttest-tool config \"$1\" \"$2\" >actual &&\n+\tshift && shift &&\n+\tprintf \"%s\\n\" \"$@\" >expect &&\n+\ttest_cmp expect actual\n+}\n+\n+check_not_found () {\n+\ttest_expect_code 1 test-tool config \"$1\" \"$2\" >actual &&\n+\techo \"Value not found for \\\"$2\\\"\" >expect &&\n \ttest_cmp expect actual\n }\n \n@@ -108,7 +105,7 @@ test_expect_success 'key with case insensitive section header & variable' '\n '\n \n test_expect_success 'find value with misspelled key' '\n-\tcheck_config expect_code 1 get_value \"my.fOo Bar.hi\" \"Value not found for \\\"my.fOo Bar.hi\\\"\"\n+\tcheck_not_found get_value \"my.fOo Bar.hi\"\n '\n \n test_expect_success 'find value with the highest priority' '\n@@ -121,7 +118,7 @@ test_expect_success 'find integer value for a key' '\n \n test_expect_success 'find string value for a key' '\n \tcheck_config get_string case.baz hask &&\n-\tcheck_config expect_code 1 get_string case.ba \"Value not found for \\\"case.ba\\\"\"\n+\tcheck_not_found get_string case.ba\n '\n \n test_expect_success 'check line error when NULL string is queried' '\n@@ -130,7 +127,8 @@ test_expect_success 'check line error when NULL string is queried' '\n '\n \n test_expect_success 'find integer if value is non parse-able' '\n-\tcheck_config expect_code 128 get_int lamb.head\n+\ttest_expect_code 128 test-tool config get_int lamb.head 2>result &&\n+\ttest_i18ngrep \"fatal: bad numeric config value .none. for .lamb\\.head.\" result\n '\n \n test_expect_success 'find bool value for the entered key' '\n-- \n2.28.0\n\n"},{"id":"405338","messageId":"51656e43c35e5b83a004dcd7ed34499a0ebed137.1599758167.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599758167.git.matheus.bernardino@usp.br","subject":"[PATCH v6 4/9] t/helper/test-config: diagnose missing arguments","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-10T17:21:23Z","receivedAt":"2020-09-10T17:23:25Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"test-config verifies that the correct number of arguments was given for\nall of its commands except for 'configset_get_value' and\n'configset_get_value_multi'. Add the check to these two, so that we\nproperly report missing arguments and prevent out-of-bounds access to\nargv[].\n\nSigned-off-by: Matheus Tavares \n---\n t/helper/test-config.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 9e9d50099a..26d9c2ac4c 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -138,7 +138,7 @@ int cmd__config(int argc, const char **argv)\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tgoto exit1;\n \t\t}\n-\t} else if (!strcmp(argv[1], \"configset_get_value\")) {\n+\t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n@@ -156,7 +156,7 @@ int cmd__config(int argc, const char **argv)\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tgoto exit1;\n \t\t}\n-\t} else if (!strcmp(argv[1], \"configset_get_value_multi\")) {\n+\t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value_multi\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n-- \n2.28.0\n\n"},{"id":"405339","messageId":"3a28b8e608c727d0e184fadf34ba4113933517c5.1599758167.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599758167.git.matheus.bernardino@usp.br","subject":"[PATCH v6 7/9] config: correctly read worktree configs in submodules","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-10T17:21:26Z","receivedAt":"2020-09-10T17:23:33Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The config machinery is not able to read worktree configs from a\nsubmodule in a process where the_repository represents the superproject\nand extensions.worktreeConfig is not set there. Furthermore, when\nextensions.worktreeConfig is set on the superproject, querying for a\nworktree config in a submodule will, instead, return the value set at\nthe superproject.\n\nThe relevant code is in do_git_config_sequence(). Although it is\ndesigned to act on an arbitrary repository, specified in the passed-in\n`struct config_options`, it accidentally depends on the_repository in\ntwo places:\n\n- it reads the global variable `repository_format_worktree_config`,\n which is set based on the content of the_repository, to determine\n whether extensions.worktreeConfig is set.\n\n- it uses the git_pathdup() helper to find the config.worktree file,\n instead of making a path using the passed-in repository.\n\nSever these dependencies and add a regression test. Also, to avoid\nfuture misuses of `repository_format_worktree_config` like this one,\nremove this global variable and store the config value on\n`struct repository` itself.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/config.c | 2 +-\n cache.h | 1 -\n config.c | 12 +++++---\n environment.c | 1 -\n repository.c | 1 +\n repository.h | 1 +\n setup.c | 4 +--\n t/helper/test-config.c | 62 ++++++++++++++++++++++++++++++++------\n t/t2404-worktree-config.sh | 16 ++++++++++\n 9 files changed, 81 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex ca4caedf33..586faad359 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -673,7 +673,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tgiven_config_source.scope = CONFIG_SCOPE_LOCAL;\n \t} else if (use_worktree_config) {\n \t\tstruct worktree **worktrees = get_worktrees();\n-\t\tif (repository_format_worktree_config)\n+\t\tif (!nongit && the_repository->worktree_config_extension)\n \t\t\tgiven_config_source.file = git_pathdup(\"config.worktree\");\n \t\telse if (worktrees[0] && worktrees[1])\n \t\t\tdie(_(\"--worktree cannot be used with multiple \"\ndiff --git a/cache.h b/cache.h\nindex 4cad61ffa4..4cdce7b68f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1029,7 +1029,6 @@ extern int grafts_replace_parents;\n #define GIT_REPO_VERSION 0\n #define GIT_REPO_VERSION_READ 1\n extern int repository_format_precious_objects;\n-extern int repository_format_worktree_config;\n \n /*\n * You _have_ to initialize a `struct repository_format` using\ndiff --git a/config.c b/config.c\nindex 97f3022c92..38c177d1e5 100644\n--- a/config.c\n+++ b/config.c\n@@ -1747,11 +1747,13 @@ static int do_git_config_sequence(const struct config_options *opts,\n \t\tret += git_config_from_file(fn, repo_config, data);\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_WORKTREE;\n-\tif (!opts->ignore_worktree && repository_format_worktree_config) {\n-\t\tchar *path = git_pathdup(\"config.worktree\");\n-\t\tif (!access_or_die(path, R_OK, 0))\n-\t\t\tret += git_config_from_file(fn, path, data);\n-\t\tfree(path);\n+\tif (!opts->ignore_worktree && opts->repo && opts->repo->gitdir &&\n+\t opts->repo->worktree_config_extension) {\n+\t\tstruct strbuf path = STRBUF_INIT;\n+\t\tstrbuf_repo_git_path(&path, opts->repo, \"config.worktree\");\n+\t\tif (!access_or_die(path.buf, R_OK, 0))\n+\t\t\tret += git_config_from_file(fn, path.buf, data);\n+\t\tstrbuf_release(&path);\n \t}\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_COMMAND;\ndiff --git a/environment.c b/environment.c\nindex bb518c61cd..5bfb8cf9c2 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -32,7 +32,6 @@ int warn_ambiguous_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n int ref_paranoia = -1;\n int repository_format_precious_objects;\n-int repository_format_worktree_config;\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\n char *apply_default_whitespace;\ndiff --git a/repository.c b/repository.c\nindex a4174ddb06..8d18d9d3f2 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -170,6 +170,7 @@ int repo_init(struct repository *repo,\n \t\tgoto error;\n \n \trepo_set_hash_algo(repo, format.hash_algo);\n+\trepo->worktree_config_extension = format.worktree_config;\n \n \tif (worktree)\n \t\trepo_set_worktree(repo, worktree);\ndiff --git a/repository.h b/repository.h\nindex 3c1f7d54bd..3d060bc3e6 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -136,6 +136,7 @@ struct repository {\n \n \t/* Indicate if a repository has a different 'commondir' from 'gitdir' */\n \tunsigned different_commondir:1;\n+\tunsigned worktree_config_extension:1;\n };\n \n extern struct repository *the_repository;\ndiff --git a/setup.c b/setup.c\nindex c04cd25a30..73cd42dbbe 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -567,11 +567,11 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \n \trepository_format_precious_objects = candidate->precious_objects;\n \tset_repository_format_partial_clone(candidate->partial_clone);\n-\trepository_format_worktree_config = candidate->worktree_config;\n+\tthe_repository->worktree_config_extension = candidate->worktree_config;\n \tstring_list_clear(&candidate->unknown_extensions, 0);\n \tstring_list_clear(&candidate->v1_only_extensions, 0);\n \n-\tif (repository_format_worktree_config) {\n+\tif (the_repository->worktree_config_extension) {\n \t\t/*\n \t\t * pick up core.bare and core.worktree from per-worktree\n \t\t * config if present\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 06c61d91e1..87488aab6b 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -2,12 +2,20 @@\n #include \"cache.h\"\n #include \"config.h\"\n #include \"string-list.h\"\n+#include \"submodule-config.h\"\n+#include \"parse-options.h\"\n \n /*\n * This program exposes the C API of the configuration mechanism\n * as a set of simple commands in order to facilitate testing.\n *\n- * Reads stdin and prints result of command to stdout:\n+ * Usage: test-tool config [--submodule=] []\n+ *\n+ * If --submodule= is given, will operate on the submodule at the\n+ * given . This option is not valid for the commands: read_early_config,\n+ * configset_get_value and configset_get_value_multi.\n+ *\n+ * Possible cmds are:\n *\n * get_value -> prints the value with highest priority for the entered key\n *\n@@ -72,14 +80,34 @@ static int early_config_cb(const char *var, const char *value, void *vdata)\n #define TC_VALUE_NOT_FOUND 1\n #define TC_CONFIG_FILE_ERROR 2\n \n+static const char *test_config_usage[] = {\n+\t\"test-tool config [--submodule=] []\",\n+\tNULL\n+};\n+\n int cmd__config(int argc, const char **argv)\n {\n \tint i, val, ret = 0;\n \tconst char *v;\n \tconst struct string_list *strptr;\n \tstruct config_set cs;\n+\tstruct repository subrepo, *repo = the_repository;\n+\tconst char *subrepo_path = NULL;\n+\n+\tstruct option options[] = {\n+\t\tOPT_STRING(0, \"submodule\", &subrepo_path, \"path\",\n+\t\t\t \"run on the submodule at \"),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, NULL, options, test_config_usage,\n+\t\t\t PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_STOP_AT_NON_OPTION);\n+\tif (argc < 2)\n+\t\tusage_with_options(test_config_usage, options);\n \n \tif (argc == 3 && !strcmp(argv[1], \"read_early_config\")) {\n+\t\tif (subrepo_path)\n+\t\t\tdie(\"cannot use --submodule with read_early_config\");\n \t\tread_early_config(early_config_cb, (void *)argv[2]);\n \t\treturn 0;\n \t}\n@@ -88,11 +116,18 @@ int cmd__config(int argc, const char **argv)\n \n \tgit_configset_init(&cs);\n \n-\tif (argc < 2)\n-\t\tdie(\"Please, provide a command name on the command-line\");\n+\tif (subrepo_path) {\n+\t\tconst struct submodule *sub;\n+\n+\t\tsub = submodule_from_path(the_repository, &null_oid, subrepo_path);\n+\t\tif (!sub || repo_submodule_init(&subrepo, the_repository, sub))\n+\t\t\tdie(\"invalid argument to --submodule: '%s'\", subrepo_path);\n+\n+\t\trepo = &subrepo;\n+\t}\n \n \tif (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n-\t\tif (!git_config_get_value(argv[2], &v)) {\n+\t\tif (!repo_config_get_value(repo, argv[2], &v)) {\n \t\t\tif (!v)\n \t\t\t\tprintf(\"(NULL)\\n\");\n \t\t\telse\n@@ -102,7 +137,7 @@ int cmd__config(int argc, const char **argv)\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n-\t\tstrptr = git_config_get_value_multi(argv[2]);\n+\t\tstrptr = repo_config_get_value_multi(repo, argv[2]);\n \t\tif (strptr) {\n \t\t\tfor (i = 0; i < strptr->nr; i++) {\n \t\t\t\tv = strptr->items[i].string;\n@@ -116,27 +151,31 @@ int cmd__config(int argc, const char **argv)\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_int\")) {\n-\t\tif (!git_config_get_int(argv[2], &val)) {\n+\t\tif (!repo_config_get_int(repo, argv[2], &val)) {\n \t\t\tprintf(\"%d\\n\", val);\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_bool\")) {\n-\t\tif (!git_config_get_bool(argv[2], &val)) {\n+\t\tif (!repo_config_get_bool(repo, argv[2], &val)) {\n \t\t\tprintf(\"%d\\n\", val);\n \t\t} else {\n+\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_string\")) {\n-\t\tif (!git_config_get_string_tmp(argv[2], &v)) {\n+\t\tif (!repo_config_get_string_tmp(repo, argv[2], &v)) {\n \t\t\tprintf(\"%s\\n\", v);\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value\")) {\n+\t\tif (subrepo_path)\n+\t\t\tdie(\"cannot use --submodule with configset_get_value\");\n+\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n@@ -155,6 +194,9 @@ int cmd__config(int argc, const char **argv)\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value_multi\")) {\n+\t\tif (subrepo_path)\n+\t\t\tdie(\"cannot use --submodule with configset_get_value_multi\");\n+\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n@@ -177,12 +219,14 @@ int cmd__config(int argc, const char **argv)\n \t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (!strcmp(argv[1], \"iterate\")) {\n-\t\tgit_config(iterate_cb, NULL);\n+\t\trepo_config(repo, iterate_cb, NULL);\n \t} else {\n \t\tdie(\"%s: Please check the syntax and the function name\", argv[0]);\n \t}\n \n out:\n \tgit_configset_clear(&cs);\n+\tif (repo != the_repository)\n+\t\trepo_clear(repo);\n \treturn ret;\n }\ndiff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh\nindex 9536d10919..1e32c93735 100755\n--- a/t/t2404-worktree-config.sh\n+++ b/t/t2404-worktree-config.sh\n@@ -78,4 +78,20 @@ test_expect_success 'config.worktree no longer read without extension' '\n \ttest_cmp_config -C wt2 shared this.is\n '\n \n+test_expect_success 'correctly read config.worktree from submodules' '\n+\ttest_unconfig extensions.worktreeConfig &&\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\ttest_commit A &&\n+\t\tgit config extensions.worktreeConfig true &&\n+\t\tgit config --worktree wtconfig.sub test-value\n+\t) &&\n+\tgit submodule add ./sub &&\n+\tgit commit -m \"add sub\" &&\n+\techo test-value >expect &&\n+\ttest-tool config --submodule=sub get_value wtconfig.sub >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.28.0\n\n"},{"id":"405340","messageId":"924d2e8ceb8b2131a13bafa356c5dc98eccae806.1599758167.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599758167.git.matheus.bernardino@usp.br","subject":"[PATCH v6 5/9] t/helper/test-config: unify exit labels","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-10T17:21:24Z","receivedAt":"2020-09-10T17:23:40Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"test-config's main function has three different exit labels, all of\nwhich have to perform the same cleanup code before returning. Unify the\nlabels in preparation for a future patch which will increase the cleanup\nsection.\n\nSigned-off-by: Matheus Tavares \n---\n t/helper/test-config.c | 49 ++++++++++++++++--------------------------\n 1 file changed, 19 insertions(+), 30 deletions(-)\n\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 26d9c2ac4c..06c61d91e1 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -69,9 +69,12 @@ static int early_config_cb(const char *var, const char *value, void *vdata)\n \treturn 0;\n }\n \n+#define TC_VALUE_NOT_FOUND 1\n+#define TC_CONFIG_FILE_ERROR 2\n+\n int cmd__config(int argc, const char **argv)\n {\n-\tint i, val;\n+\tint i, val, ret = 0;\n \tconst char *v;\n \tconst struct string_list *strptr;\n \tstruct config_set cs;\n@@ -94,10 +97,9 @@ int cmd__config(int argc, const char **argv)\n \t\t\t\tprintf(\"(NULL)\\n\");\n \t\t\telse\n \t\t\t\tprintf(\"%s\\n\", v);\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n \t\tstrptr = git_config_get_value_multi(argv[2]);\n@@ -109,41 +111,38 @@ int cmd__config(int argc, const char **argv)\n \t\t\t\telse\n \t\t\t\t\tprintf(\"%s\\n\", v);\n \t\t\t}\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_int\")) {\n \t\tif (!git_config_get_int(argv[2], &val)) {\n \t\t\tprintf(\"%d\\n\", val);\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_bool\")) {\n \t\tif (!git_config_get_bool(argv[2], &val)) {\n \t\t\tprintf(\"%d\\n\", val);\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_string\")) {\n \t\tif (!git_config_get_string_tmp(argv[2], &v)) {\n \t\t\tprintf(\"%s\\n\", v);\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n \t\t\t\tfprintf(stderr, \"Error (%d) reading configuration file %s.\\n\", err, argv[i]);\n-\t\t\t\tgoto exit2;\n+\t\t\t\tret = TC_CONFIG_FILE_ERROR;\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t}\n \t\tif (!git_configset_get_value(&cs, argv[2], &v)) {\n@@ -151,17 +150,17 @@ int cmd__config(int argc, const char **argv)\n \t\t\t\tprintf(\"(NULL)\\n\");\n \t\t\telse\n \t\t\t\tprintf(\"%s\\n\", v);\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (argc >= 3 && !strcmp(argv[1], \"configset_get_value_multi\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n \t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n \t\t\t\tfprintf(stderr, \"Error (%d) reading configuration file %s.\\n\", err, argv[i]);\n-\t\t\t\tgoto exit2;\n+\t\t\t\tret = TC_CONFIG_FILE_ERROR;\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t}\n \t\tstrptr = git_configset_get_value_multi(&cs, argv[2]);\n@@ -173,27 +172,17 @@ int cmd__config(int argc, const char **argv)\n \t\t\t\telse\n \t\t\t\t\tprintf(\"%s\\n\", v);\n \t\t\t}\n-\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\tgoto exit1;\n+\t\t\tret = TC_VALUE_NOT_FOUND;\n \t\t}\n \t} else if (!strcmp(argv[1], \"iterate\")) {\n \t\tgit_config(iterate_cb, NULL);\n-\t\tgoto exit0;\n+\t} else {\n+\t\tdie(\"%s: Please check the syntax and the function name\", argv[0]);\n \t}\n \n-\tdie(\"%s: Please check the syntax and the function name\", argv[0]);\n-\n-exit0:\n-\tgit_configset_clear(&cs);\n-\treturn 0;\n-\n-exit1:\n-\tgit_configset_clear(&cs);\n-\treturn 1;\n-\n-exit2:\n+out:\n \tgit_configset_clear(&cs);\n-\treturn 2;\n+\treturn ret;\n }\n-- \n2.28.0\n\n"},{"id":"405341","messageId":"2fc889c9c264fc10d878f31bd89cc44e79982516.1599758167.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599758167.git.matheus.bernardino@usp.br","subject":"[PATCH v6 8/9] grep: honor sparse checkout patterns","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-10T17:21:27Z","receivedAt":"2020-09-10T17:24:01Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"One of the main uses for a sparse checkout is to allow users to focus on\nthe subset of files in a repository in which they are interested. But\ngit-grep currently ignores the sparsity patterns and reports all matches\nfound outside this subset, which kind of goes in the opposite direction.\nThere are some use cases for ignoring the sparsity patterns and the next\ncommit will add an option to obtain this behavior, but here we start by\nmaking grep honor the sparsity boundaries in every case where this is\nrelevant:\n\n- git grep in worktree\n- git grep --cached\n- git grep $REVISION\n\nFor the worktree and cached cases, we iterate over paths without the\nSKIP_WORKTREE bit set, and limit our searches to these paths. For the\n$REVISION case, we limit the paths we search to those that match the\nsparsity patterns. (We do not check the SKIP_WORKTREE bit for the\n$REVISION case, because $REVISION may contain paths that do not exist in\nHEAD and thus for which we have no SKIP_WORKTREE bit to consult. The\nsparsity patterns tell us how the SKIP_WORKTREE bit would be set if we\nwere to check out $REVISION, so we consult those. Also, we don't use the\nsparsity patterns with the worktree or cached cases, both because we\nhave a bit we can check directly and more efficiently, and because\nunmerged entries from a merge or a rebase could cause more files to\ntemporarily be present than the sparsity patterns would normally\nselect.)\n\nNote that there is a special case here: `git grep $TREE`. In this case,\nwe cannot know whether $TREE corresponds to the root of the repository\nor some sub-tree, and thus there is no way for us to know which sparsity\npatterns, if any, apply. So the $TREE case will not use sparsity\npatterns or any SKIP_WORKTREE bits and will instead always search all\nfiles within the $TREE.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 125 ++++++++++++++++++--\n t/t7011-skip-worktree-reading.sh | 9 --\n t/t7817-grep-sparse-checkout.sh | 195 +++++++++++++++++++++++++++++++\n 3 files changed, 312 insertions(+), 17 deletions(-)\n create mode 100755 t/t7817-grep-sparse-checkout.sh\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex f58979bc3f..a32815de0a 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -410,7 +410,7 @@ static int grep_cache(struct grep_opt *opt,\n \t\t const struct pathspec *pathspec, int cached);\n static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t struct tree_desc *tree, struct strbuf *base, int tn_len,\n-\t\t int check_attr);\n+\t\t int is_root_tree);\n \n static int grep_submodule(struct grep_opt *opt,\n \t\t\t const struct pathspec *pathspec,\n@@ -508,6 +508,10 @@ static int grep_cache(struct grep_opt *opt,\n \n \tfor (nr = 0; nr < repo->index->cache_nr; nr++) {\n \t\tconst struct cache_entry *ce = repo->index->cache[nr];\n+\n+\t\tif (ce_skip_worktree(ce))\n+\t\t\tcontinue;\n+\n \t\tstrbuf_setlen(&name, name_base_len);\n \t\tstrbuf_addstr(&name, ce->name);\n \n@@ -520,8 +524,7 @@ static int grep_cache(struct grep_opt *opt,\n \t\t\t * cache entry are identical, even if worktree file has\n \t\t\t * been modified, so use cache version instead\n \t\t\t */\n-\t\t\tif (cached || (ce->ce_flags & CE_VALID) ||\n-\t\t\t ce_skip_worktree(ce)) {\n+\t\t\tif (cached || (ce->ce_flags & CE_VALID)) {\n \t\t\t\tif (ce_stage(ce) || ce_intent_to_add(ce))\n \t\t\t\t\tcontinue;\n \t\t\t\thit |= grep_oid(opt, &ce->oid, name.buf,\n@@ -552,9 +555,76 @@ static int grep_cache(struct grep_opt *opt,\n \treturn hit;\n }\n \n-static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n-\t\t struct tree_desc *tree, struct strbuf *base, int tn_len,\n-\t\t int check_attr)\n+static struct pattern_list *get_sparsity_patterns(struct repository *repo)\n+{\n+\tstruct pattern_list *patterns;\n+\tchar *sparse_file;\n+\tint sparse_config, cone_config;\n+\n+\tif (repo_config_get_bool(repo, \"core.sparsecheckout\", &sparse_config) ||\n+\t !sparse_config) {\n+\t\treturn NULL;\n+\t}\n+\n+\tsparse_file = repo_git_path(repo, \"info/sparse-checkout\");\n+\tpatterns = xcalloc(1, sizeof(*patterns));\n+\n+\tif (repo_config_get_bool(repo, \"core.sparsecheckoutcone\", &cone_config))\n+\t\tcone_config = 0;\n+\tpatterns->use_cone_patterns = cone_config;\n+\n+\tif (add_patterns_from_file_to_list(sparse_file, \"\", 0, patterns, NULL)) {\n+\t\tif (file_exists(sparse_file)) {\n+\t\t\twarning(_(\"failed to load sparse-checkout file: '%s'\"),\n+\t\t\t\tsparse_file);\n+\t\t}\n+\t\tfree(sparse_file);\n+\t\tfree(patterns);\n+\t\treturn NULL;\n+\t}\n+\n+\tfree(sparse_file);\n+\treturn patterns;\n+}\n+\n+static int path_in_sparse_checkout(struct strbuf *path, int prefix_len,\n+\t\t\t\t unsigned int entry_mode,\n+\t\t\t\t struct index_state *istate,\n+\t\t\t\t struct pattern_list *sparsity,\n+\t\t\t\t enum pattern_match_result parent_match,\n+\t\t\t\t enum pattern_match_result *match)\n+{\n+\tint dtype = DT_UNKNOWN;\n+\tint is_dir = S_ISDIR(entry_mode);\n+\n+\tif (parent_match == MATCHED_RECURSIVE) {\n+\t\t*match = parent_match;\n+\t\treturn 1;\n+\t}\n+\n+\tif (is_dir && !is_dir_sep(path->buf[path->len - 1]))\n+\t\tstrbuf_addch(path, '/');\n+\n+\t*match = path_matches_pattern_list(path->buf, path->len,\n+\t\t\t\t\t path->buf + prefix_len, &dtype,\n+\t\t\t\t\t sparsity, istate);\n+\tif (*match == UNDECIDED)\n+\t\t*match = parent_match;\n+\n+\tif (is_dir)\n+\t\tstrbuf_trim_trailing_dir_sep(path);\n+\n+\tif (*match == NOT_MATCHED &&\n+\t\t(!is_dir || (is_dir && sparsity->use_cone_patterns)))\n+\t return 0;\n+\n+\treturn 1;\n+}\n+\n+static int do_grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n+\t\t\tstruct tree_desc *tree, struct strbuf *base, int tn_len,\n+\t\t\tint check_attr, struct pattern_list *sparsity,\n+\t\t\tenum pattern_match_result default_sparsity_match)\n {\n \tstruct repository *repo = opt->repo;\n \tint hit = 0;\n@@ -570,6 +640,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \n \twhile (tree_entry(tree, &entry)) {\n \t\tint te_len = tree_entry_len(&entry);\n+\t\tenum pattern_match_result sparsity_match = 0;\n \n \t\tif (match != all_entries_interesting) {\n \t\t\tstrbuf_addstr(&name, base->buf + tn_len);\n@@ -586,6 +657,19 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \n \t\tstrbuf_add(base, entry.path, te_len);\n \n+\t\tif (sparsity) {\n+\t\t\tstruct strbuf path = STRBUF_INIT;\n+\t\t\tstrbuf_addstr(&path, base->buf + tn_len);\n+\n+\t\t\tif (!path_in_sparse_checkout(&path, old_baselen - tn_len,\n+\t\t\t\t\t\t entry.mode, repo->index,\n+\t\t\t\t\t\t sparsity, default_sparsity_match,\n+\t\t\t\t\t\t &sparsity_match)) {\n+\t\t\t\tstrbuf_setlen(base, old_baselen);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n \t\tif (S_ISREG(entry.mode)) {\n \t\t\thit |= grep_oid(opt, &entry.oid, base->buf, tn_len,\n \t\t\t\t\t check_attr ? base->buf + tn_len : NULL);\n@@ -602,8 +686,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \n \t\t\tstrbuf_addch(base, '/');\n \t\t\tinit_tree_desc(&sub, data, size);\n-\t\t\thit |= grep_tree(opt, pathspec, &sub, base, tn_len,\n-\t\t\t\t\t check_attr);\n+\t\t\thit |= do_grep_tree(opt, pathspec, &sub, base, tn_len,\n+\t\t\t\t\t check_attr, sparsity, sparsity_match);\n \t\t\tfree(data);\n \t\t} else if (recurse_submodules && S_ISGITLINK(entry.mode)) {\n \t\t\thit |= grep_submodule(opt, pathspec, &entry.oid,\n@@ -621,6 +705,31 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \treturn hit;\n }\n \n+/*\n+ * Note: sparsity patterns and paths' attributes will only be considered if\n+ * is_root_tree has true value. (Otherwise, we cannot properly perform pattern\n+ * matching on paths.)\n+ */\n+static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n+\t\t struct tree_desc *tree, struct strbuf *base, int tn_len,\n+\t\t int is_root_tree)\n+{\n+\tstruct pattern_list *patterns = NULL;\n+\tint ret;\n+\n+\tif (is_root_tree)\n+\t\tpatterns = get_sparsity_patterns(opt->repo);\n+\n+\tret = do_grep_tree(opt, pathspec, tree, base, tn_len, is_root_tree,\n+\t\t\t patterns, 0);\n+\n+\tif (patterns) {\n+\t\tclear_pattern_list(patterns);\n+\t\tfree(patterns);\n+\t}\n+\treturn ret;\n+}\n+\n static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t struct object *obj, const char *name, const char *path)\n {\ndiff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh\nindex 37525cae3a..26852586ac 100755\n--- a/t/t7011-skip-worktree-reading.sh\n+++ b/t/t7011-skip-worktree-reading.sh\n@@ -109,15 +109,6 @@ test_expect_success 'ls-files --modified' '\n \ttest -z \"$(git ls-files -m)\"\n '\n \n-test_expect_success 'grep with skip-worktree file' '\n-\tgit update-index --no-skip-worktree 1 &&\n-\techo test > 1 &&\n-\tgit update-index 1 &&\n-\tgit update-index --skip-worktree 1 &&\n-\trm 1 &&\n-\ttest \"$(git grep --no-ext-grep test)\" = \"1:test\"\n-'\n-\n echo \":000000 100644 $ZERO_OID $EMPTY_BLOB A\t1\" > expected\n test_expect_success 'diff-index does not examine skip-worktree absent entries' '\n \tsetup_absent &&\ndiff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh\nnew file mode 100755\nindex 0000000000..b3109e3479\n--- /dev/null\n+++ b/t/t7817-grep-sparse-checkout.sh\n@@ -0,0 +1,195 @@\n+#!/bin/sh\n+\n+test_description='grep in sparse checkout\n+\n+This test creates a repo with the following structure:\n+\n+.\n+|-- a\n+|-- b\n+|-- dir\n+| `-- c\n+|-- sub\n+| |-- A\n+| | `-- a\n+| `-- B\n+| `-- b\n+`-- sub2\n+ `-- a\n+\n+Where the outer repository has non-cone mode sparsity patterns, sub is a\n+submodule with cone mode sparsity patterns and sub2 is a submodule that is\n+excluded by the superproject sparsity patterns. The resulting sparse checkout\n+should leave the following structure in the working tree:\n+\n+.\n+|-- a\n+|-- sub\n+| `-- B\n+| `-- b\n+`-- sub2\n+ `-- a\n+\n+But note that sub2 should have the SKIP_WORKTREE bit set.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo \"text\" >a &&\n+\techo \"text\" >b &&\n+\tmkdir dir &&\n+\techo \"text\" >dir/c &&\n+\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tmkdir A B &&\n+\t\techo \"text\" >A/a &&\n+\t\techo \"text\" >B/b &&\n+\t\tgit add A B &&\n+\t\tgit commit -m sub &&\n+\t\tgit sparse-checkout init --cone &&\n+\t\tgit sparse-checkout set B\n+\t) &&\n+\n+\tgit init sub2 &&\n+\t(\n+\t\tcd sub2 &&\n+\t\techo \"text\" >a &&\n+\t\tgit add a &&\n+\t\tgit commit -m sub2\n+\t) &&\n+\n+\tgit submodule add ./sub &&\n+\tgit submodule add ./sub2 &&\n+\tgit add a b dir &&\n+\tgit commit -m super &&\n+\tgit sparse-checkout init --no-cone &&\n+\tgit sparse-checkout set \"/*\" \"!b\" \"!/*/\" \"sub\" &&\n+\n+\tgit tag -am tag-to-commit tag-to-commit HEAD &&\n+\ttree=$(git rev-parse HEAD^{tree}) &&\n+\tgit tag -am tag-to-tree tag-to-tree $tree &&\n+\n+\ttest_path_is_missing b &&\n+\ttest_path_is_missing dir &&\n+\ttest_path_is_missing sub/A &&\n+\ttest_path_is_file a &&\n+\ttest_path_is_file sub/B/b &&\n+\ttest_path_is_file sub2/a\n+'\n+\n+# The test below checks a special case: the sparsity patterns exclude '/b'\n+# and sparse checkout is enabled, but the path exists in the working tree (e.g.\n+# manually created after `git sparse-checkout init`). In this case, grep should\n+# skip it.\n+test_expect_success 'grep in working tree should honor sparse checkout' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tEOF\n+\techo \"new-text\" >b &&\n+\ttest_when_finished \"rm b\" &&\n+\tgit grep \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep unmerged file despite not matching sparsity patterns' '\n+\tcat >expect <<-EOF &&\n+\tb:modified-b-in-branchX\n+\tb:modified-b-in-branchY\n+\tEOF\n+\ttest_when_finished \"test_might_fail git merge --abort && \\\n+\t\t\t git checkout master\" &&\n+\n+\tgit sparse-checkout disable &&\n+\tgit checkout -b branchY master &&\n+\ttest_commit modified-b-in-branchY b &&\n+\tgit checkout -b branchX master &&\n+\ttest_commit modified-b-in-branchX b &&\n+\n+\tgit sparse-checkout init &&\n+\ttest_path_is_missing b &&\n+\ttest_must_fail git merge branchY &&\n+\tgit grep \"modified-b\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --cached should honor sparse checkout' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tEOF\n+\tgit grep --cached \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep should honor sparse checkout' '\n+\tcommit=$(git rev-parse HEAD) &&\n+\tcat >expect_commit <<-EOF &&\n+\t$commit:a:text\n+\tEOF\n+\tcat >expect_tag-to-commit <<-EOF &&\n+\ttag-to-commit:a:text\n+\tEOF\n+\tgit grep \"text\" $commit >actual_commit &&\n+\ttest_cmp expect_commit actual_commit &&\n+\tgit grep \"text\" tag-to-commit >actual_tag-to-commit &&\n+\ttest_cmp expect_tag-to-commit actual_tag-to-commit\n+'\n+\n+test_expect_success 'grep should ignore sparsity patterns' '\n+\tcommit=$(git rev-parse HEAD) &&\n+\ttree=$(git rev-parse HEAD^{tree}) &&\n+\tcat >expect_tree <<-EOF &&\n+\t$tree:a:text\n+\t$tree:b:text\n+\t$tree:dir/c:text\n+\tEOF\n+\tcat >expect_tag-to-tree <<-EOF &&\n+\ttag-to-tree:a:text\n+\ttag-to-tree:b:text\n+\ttag-to-tree:dir/c:text\n+\tEOF\n+\tgit grep \"text\" $tree >actual_tree &&\n+\ttest_cmp expect_tree actual_tree &&\n+\tgit grep \"text\" tag-to-tree >actual_tag-to-tree &&\n+\ttest_cmp expect_tag-to-tree actual_tag-to-tree\n+'\n+\n+# Note that sub2/ is present in the worktree but it is excluded by the sparsity\n+# patterns, so grep should not recurse into it.\n+test_expect_success 'grep --recurse-submodules should honor sparse checkout in submodule' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tsub/B/b:text\n+\tEOF\n+\tgit grep --recurse-submodules \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --recurse-submodules --cached should honor sparse checkout in submodule' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tsub/B/b:text\n+\tEOF\n+\tgit grep --recurse-submodules --cached \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --recurse-submodules should honor sparse checkout in submodule' '\n+\tcommit=$(git rev-parse HEAD) &&\n+\tcat >expect_commit <<-EOF &&\n+\t$commit:a:text\n+\t$commit:sub/B/b:text\n+\tEOF\n+\tcat >expect_tag-to-commit <<-EOF &&\n+\ttag-to-commit:a:text\n+\ttag-to-commit:sub/B/b:text\n+\tEOF\n+\tgit grep --recurse-submodules \"text\" $commit >actual_commit &&\n+\ttest_cmp expect_commit actual_commit &&\n+\tgit grep --recurse-submodules \"text\" tag-to-commit >actual_tag-to-commit &&\n+\ttest_cmp expect_tag-to-commit actual_tag-to-commit\n+'\n+\n+test_done\n-- \n2.28.0\n\n"},{"id":"405342","messageId":"92bc5351cf5fdbf7435ba4d99530d6ce02f957c4.1599758167.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599758167.git.matheus.bernardino@usp.br","subject":"[PATCH v6 9/9] config: add setting to ignore sparsity patterns in some cmds","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-10T17:21:28Z","receivedAt":"2020-09-10T17:24:09Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"When sparse checkout is enabled, some users expect the output of certain\ncommands (such as grep, diff, and log) to be also restricted within the\nsparsity patterns. This would allow them to effectively work only on the\nsubset of files in which they are interested; and allow some commands to\npossibly perform better, by not considering uninteresting paths. For\nthis reason, we taught grep to honor the sparsity patterns, in the\nprevious patch. But, on the other hand, allowing grep and the other\ncommands mentioned to optionally ignore the patterns also make for some\ninteresting use cases. E.g. using grep to search for a function\ndocumentation that resides outside the sparse checkout.\n\nIn any case, there is no current way for users to configure the behavior\nthey want for these commands. Aiming to provide this flexibility, let's\nintroduce the sparse.restrictCmds setting (and the analogous\n--[no]-restrict-to-sparse-paths global option). The default value is\ntrue. For now, grep is the only one affected by this setting, but the\ngoal is to have support for more commands, in the future.\n\nHelped-by: Elijah Newren \nSigned-off-by: Matheus Tavares \n---\n Documentation/config.txt | 2 +\n Documentation/config/grep.txt | 8 ++\n Documentation/config/sparse.txt | 20 ++++\n Documentation/git.txt | 4 +\n Makefile | 1 +\n builtin/grep.c | 13 ++-\n contrib/completion/git-completion.bash | 2 +\n git.c | 5 +\n sparse-checkout.c | 18 ++++\n sparse-checkout.h | 11 +++\n t/t7817-grep-sparse-checkout.sh | 132 ++++++++++++++++++++++++-\n t/t9902-completion.sh | 4 +-\n 12 files changed, 214 insertions(+), 6 deletions(-)\n create mode 100644 Documentation/config/sparse.txt\n create mode 100644 sparse-checkout.c\n create mode 100644 sparse-checkout.h\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3042d80978..3b6e0901b8 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -438,6 +438,8 @@ include::config/sequencer.txt[]\n \n include::config/showbranch.txt[]\n \n+include::config/sparse.txt[]\n+\n include::config/splitindex.txt[]\n \n include::config/ssh.txt[]\ndiff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt\nindex dd51db38e1..a3275ab4b7 100644\n--- a/Documentation/config/grep.txt\n+++ b/Documentation/config/grep.txt\n@@ -28,3 +28,11 @@ grep.fullName::\n grep.fallbackToNoIndex::\n \tIf set to true, fall back to git grep --no-index if git grep\n \tis executed outside of a git repository. Defaults to false.\n+\n+ifdef::git-grep[]\n+sparse.restrictCmds::\n+\tSee base definition in linkgit:git-config[1]. grep honors\n+\tsparse.restrictCmds by limiting searches to the sparsity paths in three\n+\tcases: when searching the working tree, when searching the index with\n+\t--cached, and when searching a specified commit.\n+endif::git-grep[]\ndiff --git a/Documentation/config/sparse.txt b/Documentation/config/sparse.txt\nnew file mode 100644\nindex 0000000000..494761526e\n--- /dev/null\n+++ b/Documentation/config/sparse.txt\n@@ -0,0 +1,20 @@\n+sparse.restrictCmds::\n+\tOnly meaningful in conjunction with core.sparseCheckout. This option\n+\textends sparse checkouts (which limit which paths are written to the\n+\tworking tree), so that output and operations are also limited to the\n+\tsparsity paths where possible and implemented. The purpose of this\n+\toption is to (1) focus output for the user on the portion of the\n+\trepository that is of interest to them, and (2) enable potentially\n+\tdramatic performance improvements, especially in conjunction with\n+\tpartial clones.\n++\n+When this option is true (default), some git commands may limit their behavior\n+to the paths specified by the sparsity patterns, or to the intersection of\n+those paths and any (like `*.c`) that the user might also specify on the\n+command line. When false, the affected commands will work on full trees,\n+ignoring the sparsity patterns. For now, only git-grep honors this setting.\n++\n+Note: commands which export, integrity check, or create history will always\n+operate on full trees (e.g. fast-export, format-patch, fsck, commit, etc.),\n+unaffected by any sparsity patterns. Also, writing commands such as\n+sparse-checkout and read-tree will not be affected by this configuration.\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 2fc92586b5..d857509573 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -180,6 +180,10 @@ If you just want to run git as if it was started in `` then use\n \tDo not perform optional operations that require locks. This is\n \tequivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`.\n \n+--[no-]restrict-to-sparse-paths::\n+\tOverrides the sparse.restrictCmds configuration (see\n+\tlinkgit:git-config[1]) for this execution.\n+\n --list-cmds=group[,group...]::\n \tList commands by group. This is an internal/experimental\n \toption and may change or be removed in the future. Supported\ndiff --git a/Makefile b/Makefile\nindex 65f8cfb236..778c9e499e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -982,6 +982,7 @@ LIB_OBJS += sha1-name.o\n LIB_OBJS += shallow.o\n LIB_OBJS += sideband.o\n LIB_OBJS += sigchain.o\n+LIB_OBJS += sparse-checkout.o\n LIB_OBJS += split-index.o\n LIB_OBJS += stable-qsort.o\n LIB_OBJS += strbuf.o\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex a32815de0a..3fa364e91c 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -25,6 +25,7 @@\n #include \"submodule-config.h\"\n #include \"object-store.h\"\n #include \"packfile.h\"\n+#include \"sparse-checkout.h\"\n \n static char const * const grep_usage[] = {\n \tN_(\"git grep [] [-e] [...] [[--] ...]\"),\n@@ -498,6 +499,7 @@ static int grep_cache(struct grep_opt *opt,\n \tint nr;\n \tstruct strbuf name = STRBUF_INIT;\n \tint name_base_len = 0;\n+\tint sparse_paths_only =\trestrict_to_sparse_paths(repo);\n \tif (repo->submodule_prefix) {\n \t\tname_base_len = strlen(repo->submodule_prefix);\n \t\tstrbuf_addstr(&name, repo->submodule_prefix);\n@@ -509,7 +511,7 @@ static int grep_cache(struct grep_opt *opt,\n \tfor (nr = 0; nr < repo->index->cache_nr; nr++) {\n \t\tconst struct cache_entry *ce = repo->index->cache[nr];\n \n-\t\tif (ce_skip_worktree(ce))\n+\t\tif (sparse_paths_only && ce_skip_worktree(ce))\n \t\t\tcontinue;\n \n \t\tstrbuf_setlen(&name, name_base_len);\n@@ -715,9 +717,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t int is_root_tree)\n {\n \tstruct pattern_list *patterns = NULL;\n+\tint sparse_paths_only = restrict_to_sparse_paths(opt->repo);\n \tint ret;\n \n-\tif (is_root_tree)\n+\tif (is_root_tree && sparse_paths_only)\n \t\tpatterns = get_sparsity_patterns(opt->repo);\n \n \tret = do_grep_tree(opt, pathspec, tree, base, tn_len, is_root_tree,\n@@ -1258,6 +1261,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \n \tif (!use_index || untracked) {\n \t\tint use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;\n+\n+\t\tif (opt_restrict_to_sparse_paths >= 0) {\n+\t\t\tdie(_(\"--[no-]restrict-to-sparse-paths is incompatible\"\n+\t\t\t\t \" with --no-index and --untracked\"));\n+\t\t}\n+\n \t\thit = grep_directory(&opt, &pathspec, use_exclude, use_index);\n \t} else if (0 <= opt_exclude) {\n \t\tdie(_(\"--[no-]exclude-standard cannot be used for tracked contents\"));\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9147fba3d5..de12766a70 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3402,6 +3402,8 @@ __git_main ()\n \t\t\t--namespace=\n \t\t\t--no-replace-objects\n \t\t\t--help\n+\t\t\t--restrict-to-sparse-paths\n+\t\t\t--no-restrict-to-sparse-paths\n \t\t\t\"\n \t\t\t;;\n \t\t*)\ndiff --git a/git.c b/git.c\nindex 8bd1d7551d..81206b424c 100644\n--- a/git.c\n+++ b/git.c\n@@ -5,6 +5,7 @@\n #include \"run-command.h\"\n #include \"alias.h\"\n #include \"shallow.h\"\n+#include \"sparse-checkout.h\"\n \n #define RUN_SETUP\t\t(1<<0)\n #define RUN_SETUP_GENTLY\t(1<<1)\n@@ -311,6 +312,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t} else {\n \t\t\t\texit(list_cmds(cmd));\n \t\t\t}\n+\t\t} else if (!strcmp(cmd, \"--restrict-to-sparse-paths\")) {\n+\t\t\topt_restrict_to_sparse_paths = 1;\n+\t\t} else if (!strcmp(cmd, \"--no-restrict-to-sparse-paths\")) {\n+\t\t\topt_restrict_to_sparse_paths = 0;\n \t\t} else {\n \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n \t\t\tusage(git_usage_string);\ndiff --git a/sparse-checkout.c b/sparse-checkout.c\nnew file mode 100644\nindex 0000000000..96c5ed5446\n--- /dev/null\n+++ b/sparse-checkout.c\n@@ -0,0 +1,18 @@\n+#include \"cache.h\"\n+#include \"config.h\"\n+#include \"sparse-checkout.h\"\n+\n+int opt_restrict_to_sparse_paths = -1;\n+\n+int restrict_to_sparse_paths(struct repository *repo)\n+{\n+\tint ret;\n+\n+\tif (opt_restrict_to_sparse_paths >= 0)\n+\t\treturn opt_restrict_to_sparse_paths;\n+\n+\tif (repo_config_get_bool(repo, \"sparse.restrictcmds\", &ret))\n+\t\tret = 1;\n+\n+\treturn ret;\n+}\ndiff --git a/sparse-checkout.h b/sparse-checkout.h\nnew file mode 100644\nindex 0000000000..a4805e443a\n--- /dev/null\n+++ b/sparse-checkout.h\n@@ -0,0 +1,11 @@\n+#ifndef SPARSE_CHECKOUT_H\n+#define SPARSE_CHECKOUT_H\n+\n+struct repository;\n+\n+extern int opt_restrict_to_sparse_paths;\n+\n+/* Whether or not cmds should restrict behavior on sparse paths, in this repo */\n+int restrict_to_sparse_paths(struct repository *repo);\n+\n+#endif /* SPARSE_CHECKOUT_H */\ndiff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh\nindex b3109e3479..f93a4f71d1 100755\n--- a/t/t7817-grep-sparse-checkout.sh\n+++ b/t/t7817-grep-sparse-checkout.sh\n@@ -80,10 +80,10 @@ test_expect_success 'setup' '\n \ttest_path_is_file sub2/a\n '\n \n-# The test below checks a special case: the sparsity patterns exclude '/b'\n+# The two tests below check a special case: the sparsity patterns exclude '/b'\n # and sparse checkout is enabled, but the path exists in the working tree (e.g.\n # manually created after `git sparse-checkout init`). In this case, grep should\n-# skip it.\n+# skip the file by default, but not with --no-restrict-to-sparse-paths.\n test_expect_success 'grep in working tree should honor sparse checkout' '\n \tcat >expect <<-EOF &&\n \ta:text\n@@ -93,6 +93,16 @@ test_expect_success 'grep in working tree should honor sparse checkout' '\n \tgit grep \"text\" >actual &&\n \ttest_cmp expect actual\n '\n+test_expect_success 'grep w/ --no-restrict-to-sparse-paths for sparsely excluded but present paths' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tb:new-text\n+\tEOF\n+\techo \"new-text\" >b &&\n+\ttest_when_finished \"rm b\" &&\n+\tgit --no-restrict-to-sparse-paths grep \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n \n test_expect_success 'grep unmerged file despite not matching sparsity patterns' '\n \tcat >expect <<-EOF &&\n@@ -157,7 +167,7 @@ test_expect_success 'grep should ignore sparsity patterns' '\n '\n \n # Note that sub2/ is present in the worktree but it is excluded by the sparsity\n-# patterns, so grep should not recurse into it.\n+# patterns, so grep should only recurse into it with --no-restrict-to-sparse-paths.\n test_expect_success 'grep --recurse-submodules should honor sparse checkout in submodule' '\n \tcat >expect <<-EOF &&\n \ta:text\n@@ -166,6 +176,15 @@ test_expect_success 'grep --recurse-submodules should honor sparse checkout in s\n \tgit grep --recurse-submodules \"text\" >actual &&\n \ttest_cmp expect actual\n '\n+test_expect_success 'grep --recurse-submodules should search in excluded submodules w/ --no-restrict-to-sparse-paths' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tsub/B/b:text\n+\tsub2/a:text\n+\tEOF\n+\tgit --no-restrict-to-sparse-paths grep --recurse-submodules \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n \n test_expect_success 'grep --recurse-submodules --cached should honor sparse checkout in submodule' '\n \tcat >expect <<-EOF &&\n@@ -192,4 +211,111 @@ test_expect_success 'grep --recurse-submodules should honor sparse\n \ttest_cmp expect_tag-to-commit actual_tag-to-commit\n '\n \n+for cmd in 'git --no-restrict-to-sparse-paths grep' \\\n+\t 'git -c sparse.restrictCmds=false grep' \\\n+\t 'git -c sparse.restrictCmds=true --no-restrict-to-sparse-paths grep'\n+do\n+\n+\ttest_expect_success \"$cmd --cached should ignore sparsity patterns\" '\n+\t\tcat >expect <<-EOF &&\n+\t\ta:text\n+\t\tb:text\n+\t\tdir/c:text\n+\t\tEOF\n+\t\t$cmd --cached \"text\" >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd should ignore sparsity patterns\" '\n+\t\tcommit=$(git rev-parse HEAD) &&\n+\t\tcat >expect_commit <<-EOF &&\n+\t\t$commit:a:text\n+\t\t$commit:b:text\n+\t\t$commit:dir/c:text\n+\t\tEOF\n+\t\tcat >expect_tag-to-commit <<-EOF &&\n+\t\ttag-to-commit:a:text\n+\t\ttag-to-commit:b:text\n+\t\ttag-to-commit:dir/c:text\n+\t\tEOF\n+\t\t$cmd \"text\" $commit >actual_commit &&\n+\t\ttest_cmp expect_commit actual_commit &&\n+\t\t$cmd \"text\" tag-to-commit >actual_tag-to-commit &&\n+\t\ttest_cmp expect_tag-to-commit actual_tag-to-commit\n+\t'\n+done\n+\n+test_expect_success 'grep --recurse-submodules --cached w/ --no-restrict-to-sparse-paths' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tb:text\n+\tdir/c:text\n+\tsub/A/a:text\n+\tsub/B/b:text\n+\tsub2/a:text\n+\tEOF\n+\tgit --no-restrict-to-sparse-paths grep --recurse-submodules --cached \\\n+\t\t\"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --recurse-submodules w/ --no-restrict-to-sparse-paths' '\n+\tcommit=$(git rev-parse HEAD) &&\n+\tcat >expect_commit <<-EOF &&\n+\t$commit:a:text\n+\t$commit:b:text\n+\t$commit:dir/c:text\n+\t$commit:sub/A/a:text\n+\t$commit:sub/B/b:text\n+\t$commit:sub2/a:text\n+\tEOF\n+\tcat >expect_tag-to-commit <<-EOF &&\n+\ttag-to-commit:a:text\n+\ttag-to-commit:b:text\n+\ttag-to-commit:dir/c:text\n+\ttag-to-commit:sub/A/a:text\n+\ttag-to-commit:sub/B/b:text\n+\ttag-to-commit:sub2/a:text\n+\tEOF\n+\tgit --no-restrict-to-sparse-paths grep --recurse-submodules \"text\" \\\n+\t\t$commit >actual_commit &&\n+\ttest_cmp expect_commit actual_commit &&\n+\tgit --no-restrict-to-sparse-paths grep --recurse-submodules \"text\" \\\n+\t\ttag-to-commit >actual_tag-to-commit &&\n+\ttest_cmp expect_tag-to-commit actual_tag-to-commit\n+'\n+\n+test_expect_success 'should respect the sparse.restrictCmds values from submodules' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tsub/A/a:text\n+\tsub/B/b:text\n+\tEOF\n+\ttest_config -C sub sparse.restrictCmds false &&\n+\tgit grep --cached --recurse-submodules \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'should propagate --[no]-restrict-to-sparse-paths to submodules' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tb:text\n+\tdir/c:text\n+\tsub/A/a:text\n+\tsub/B/b:text\n+\tsub2/a:text\n+\tEOF\n+\ttest_config -C sub sparse.restrictCmds true &&\n+\tgit --no-restrict-to-sparse-paths grep --cached --recurse-submodules \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+for opt in '--untracked' '--no-index'\n+do\n+\ttest_expect_success \"--[no]-restrict-to-sparse-paths and $opt are incompatible\" \"\n+\t\ttest_must_fail git --restrict-to-sparse-paths grep $opt . 2>actual &&\n+\t\ttest_i18ngrep 'restrict-to-sparse-paths is incompatible with' actual\n+\t\"\n+done\n+\n test_done\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 8425b9a531..a8c2ac9d70 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1928,6 +1928,8 @@ test_expect_success 'double dash \"git\" itself' '\n \t--namespace=\n \t--no-replace-objects Z\n \t--help Z\n+\t--restrict-to-sparse-paths Z\n+\t--no-restrict-to-sparse-paths Z\n \tEOF\n '\n \n@@ -1970,7 +1972,7 @@ test_expect_success 'general options' '\n \ttest_completion \"git --nam\" \"--namespace=\" &&\n \ttest_completion \"git --bar\" \"--bare \" &&\n \ttest_completion \"git --inf\" \"--info-path \" &&\n-\ttest_completion \"git --no-r\" \"--no-replace-objects \"\n+\ttest_completion \"git --no-rep\" \"--no-replace-objects \"\n '\n \n test_expect_success 'general options plus command' '\n-- \n2.28.0\n\n"},{"id":"405343","messageId":"f5c0fc33365414987ff1cbeb8d38f11ab99b3004.1599758167.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599758167.git.matheus.bernardino@usp.br","subject":"[PATCH v6 6/9] config: make do_git_config_sequence receive a 'struct repository'","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-10T17:21:25Z","receivedAt":"2020-09-10T17:24:42Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"From: Jonathan Nieder \n\nThe following patch will fix a bug in do_git_config_sequence, which\nmakes it ignore worktree-specific configurations on submodules when\nthe_repository represents the superproject. To do so, the function will\nneed access to the 'struct repository' instance of the submodule. But it\ncurrently only receives the 'git_dir' and 'commondir' paths through\n'struct config_options'. So change the struct to hold a repository\npointer instead of the two strings, and adjust its users.\n\nSigned-off-by: Jonathan Nieder \nSigned-off-by: Matheus Tavares \n---\n\nHi, Jonathan. I just made a small change in this patch, in\nread_early_repo(): when running some test cases in t0001, I noticed that\n`the_early_repo.settings.initialized` was 0 even though the repo was\npopulated. So I added a flag to track the repo state for the later\ncleanup.\n\n builtin/config.c | 6 ++----\n config.c | 35 +++++++++++++++++++++--------------\n config.h | 4 ++--\n 3 files changed, 25 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 5e39f61885..ca4caedf33 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -699,10 +699,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tconfig_options.respect_includes = !given_config_source.file;\n \telse\n \t\tconfig_options.respect_includes = respect_includes_opt;\n-\tif (!nongit) {\n-\t\tconfig_options.commondir = get_git_common_dir();\n-\t\tconfig_options.git_dir = get_git_dir();\n-\t}\n+\tif (!nongit)\n+\t\tconfig_options.repo = the_repository;\n \n \tif (end_nul) {\n \t\tterm = '\\0';\ndiff --git a/config.c b/config.c\nindex 2bdff4457b..97f3022c92 100644\n--- a/config.c\n+++ b/config.c\n@@ -222,8 +222,8 @@ static int include_by_gitdir(const struct config_options *opts,\n \tconst char *git_dir;\n \tint already_tried_absolute = 0;\n \n-\tif (opts->git_dir)\n-\t\tgit_dir = opts->git_dir;\n+\tif (opts->repo && opts->repo->gitdir)\n+\t\tgit_dir = opts->repo->gitdir;\n \telse\n \t\tgoto done;\n \n@@ -1720,10 +1720,10 @@ static int do_git_config_sequence(const struct config_options *opts,\n \tchar *repo_config;\n \tenum config_scope prev_parsing_scope = current_parsing_scope;\n \n-\tif (opts->commondir)\n-\t\trepo_config = mkpathdup(\"%s/config\", opts->commondir);\n-\telse if (opts->git_dir)\n-\t\tBUG(\"git_dir without commondir\");\n+\tif (opts->repo && opts->repo->commondir)\n+\t\trepo_config = mkpathdup(\"%s/config\", opts->repo->commondir);\n+\telse if (opts->repo && opts->repo->gitdir)\n+\t\tBUG(\"gitdir without commondir\");\n \telse\n \t\trepo_config = NULL;\n \n@@ -1824,27 +1824,35 @@ void read_early_config(config_fn_t cb, void *data)\n \tstruct config_options opts = {0};\n \tstruct strbuf commondir = STRBUF_INIT;\n \tstruct strbuf gitdir = STRBUF_INIT;\n+\tstruct repository the_early_repo = {0};\n+\tint early_repo_initialized = 0;\n \n \topts.respect_includes = 1;\n \n \tif (have_git_dir()) {\n-\t\topts.commondir = get_git_common_dir();\n-\t\topts.git_dir = get_git_dir();\n+\t\topts.repo = the_repository;\n \t/*\n \t * When setup_git_directory() was not yet asked to discover the\n \t * GIT_DIR, we ask discover_git_directory() to figure out whether there\n \t * is any repository config we should use (but unlike\n-\t * setup_git_directory_gently(), no global state is changed, most\n+\t * setup_git_directory_gently(), no global state is changed; most\n \t * notably, the current working directory is still the same after the\n \t * call).\n+\t *\n+\t * NEEDSWORK: There is some duplicate work between\n+\t * discover_git_directory and repo_init. Update to use a variant of\n+\t * repo_init that does its own repository discovery once available.\n \t */\n-\t} else if (!discover_git_directory(&commondir, &gitdir)) {\n-\t\topts.commondir = commondir.buf;\n-\t\topts.git_dir = gitdir.buf;\n+\t} else if (!discover_git_directory(&commondir, &gitdir) &&\n+\t\t !repo_init(&the_early_repo, gitdir.buf, NULL)) {\n+\t\topts.repo = &the_early_repo;\n+\t\tearly_repo_initialized = 1;\n \t}\n \n \tconfig_with_options(cb, data, NULL, &opts);\n \n+\tif (early_repo_initialized)\n+\t\trepo_clear(&the_early_repo);\n \tstrbuf_release(&commondir);\n \tstrbuf_release(&gitdir);\n }\n@@ -2097,8 +2105,7 @@ static void repo_read_config(struct repository *repo)\n \tstruct config_options opts = { 0 };\n \n \topts.respect_includes = 1;\n-\topts.commondir = repo->commondir;\n-\topts.git_dir = repo->gitdir;\n+\topts.repo = repo;\n \n \tif (!repo->config)\n \t\trepo->config = xcalloc(1, sizeof(struct config_set));\ndiff --git a/config.h b/config.h\nindex 91cdfbfb41..e56293fb29 100644\n--- a/config.h\n+++ b/config.h\n@@ -21,6 +21,7 @@\n */\n \n struct object_id;\n+struct repository;\n \n /* git_config_parse_key() returns these negated: */\n #define CONFIG_INVALID_KEY 1\n@@ -87,8 +88,7 @@ struct config_options {\n \tunsigned int ignore_worktree : 1;\n \tunsigned int ignore_cmdline : 1;\n \tunsigned int system_gently : 1;\n-\tconst char *commondir;\n-\tconst char *git_dir;\n+\tstruct repository *repo;\n \tconfig_parser_event_fn_t event_fn;\n \tvoid *event_fn_data;\n \tenum config_error_action {\n-- \n2.28.0\n\n"},{"id":"405344","messageId":"70c9a4e7415f532c2aa6fdd460a927b5e677cd97.1599758167.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599758167.git.matheus.bernardino@usp.br","subject":"[PATCH v6 1/9] doc: grep: unify info on configuration variables","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-10T17:21:20Z","receivedAt":"2020-09-10T17:42:51Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Explanations about the configuration variables for git-grep are\nduplicated in \"Documentation/git-grep.txt\" and\n\"Documentation/config/grep.txt\", which can make maintenance difficult.\nThe first also contains a definition not present in the latter\n(grep.fullName). To avoid problems like this, let's unify the\ninformation in the second file and include it in the first.\n\nSigned-off-by: Matheus Tavares \n---\n Documentation/config/grep.txt | 10 ++++++++--\n Documentation/git-grep.txt | 36 ++++++-----------------------------\n 2 files changed, 14 insertions(+), 32 deletions(-)\n\ndiff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt\nindex 44abe45a7c..dd51db38e1 100644\n--- a/Documentation/config/grep.txt\n+++ b/Documentation/config/grep.txt\n@@ -16,8 +16,14 @@ grep.extendedRegexp::\n \tother than 'default'.\n \n grep.threads::\n-\tNumber of grep worker threads to use.\n-\tSee `grep.threads` in linkgit:git-grep[1] for more information.\n+\tNumber of grep worker threads to use. See `--threads`\n+ifndef::git-grep[]\n+\tin linkgit:git-grep[1]\n+endif::git-grep[]\n+\tfor more information.\n+\n+grep.fullName::\n+\tIf set to true, enable `--full-name` option by default.\n \n grep.fallbackToNoIndex::\n \tIf set to true, fall back to git grep --no-index if git grep\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex a7f9bc99ea..9bdf807584 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -41,34 +41,8 @@ characters. An empty string as search expression matches all lines.\n CONFIGURATION\n -------------\n \n-grep.lineNumber::\n-\tIf set to true, enable `-n` option by default.\n-\n-grep.column::\n-\tIf set to true, enable the `--column` option by default.\n-\n-grep.patternType::\n-\tSet the default matching behavior. Using a value of 'basic', 'extended',\n-\t'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,\n-\t`--fixed-strings`, or `--perl-regexp` option accordingly, while the\n-\tvalue 'default' will return to the default matching behavior.\n-\n-grep.extendedRegexp::\n-\tIf set to true, enable `--extended-regexp` option by default. This\n-\toption is ignored when the `grep.patternType` option is set to a value\n-\tother than 'default'.\n-\n-grep.threads::\n-\tNumber of grep worker threads to use. If unset (or set to 0), Git will\n-\tuse as many threads as the number of logical cores available.\n-\n-grep.fullName::\n-\tIf set to true, enable `--full-name` option by default.\n-\n-grep.fallbackToNoIndex::\n-\tIf set to true, fall back to git grep --no-index if git grep\n-\tis executed outside of a git repository. Defaults to false.\n-\n+:git-grep: 1\n+include::config/grep.txt[]\n \n OPTIONS\n -------\n@@ -269,8 +243,10 @@ providing this option will cause it to die.\n \tfound.\n \n --threads ::\n-\tNumber of grep worker threads to use.\n-\tSee `grep.threads` in 'CONFIGURATION' for more information.\n+\tNumber of grep worker threads to use. If not provided (or set to\n+\t0), Git will use as many worker threads as the number of logical\n+\tcores available. The default value can also be set with the\n+\t`grep.threads` configuration.\n \n -f ::\n \tRead patterns from , one per line.\n-- \n2.28.0\n\n"},{"id":"405345","messageId":"45d13744b7849ec485f76e0a67e972fdd01ba21a.1599758167.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599758167.git.matheus.bernardino@usp.br","subject":"[PATCH v6 3/9] t/helper/test-config: be consistent with exit codes","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-10T17:21:22Z","receivedAt":"2020-09-10T17:52:07Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The test-config helper can return at least three different exit codes to\nreflect the status of the requested operation. And these codes are\nchecked in some of the tests. But there is an inconsistent place in the\nhelper where an usage error returns the same code as a \"value not found\"\nerror. Let's fix that and, while we are here, document the meaning of\neach exit code in the file's header.\n\nSigned-off-by: Matheus Tavares \n---\n t/helper/test-config.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex a6e936721f..9e9d50099a 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -30,6 +30,11 @@\n * iterate -> iterate over all values using git_config(), and print some\n * data for each\n *\n+ * Exit codes:\n+ * 0: success\n+ * 1: value not found for the given config key\n+ * 2: config file path given as argument is inaccessible or doesn't exist\n+ *\n * Examples:\n *\n * To print the value with highest priority for key \"foo.bAr Baz.rock\":\n@@ -80,10 +85,10 @@ int cmd__config(int argc, const char **argv)\n \n \tgit_configset_init(&cs);\n \n-\tif (argc < 2) {\n-\t\tfprintf(stderr, \"Please, provide a command name on the command-line\\n\");\n-\t\tgoto exit1;\n-\t} else if (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n+\tif (argc < 2)\n+\t\tdie(\"Please, provide a command name on the command-line\");\n+\n+\tif (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n \t\tif (!git_config_get_value(argv[2], &v)) {\n \t\t\tif (!v)\n \t\t\t\tprintf(\"(NULL)\\n\");\n-- \n2.28.0\n\n"},{"id":"416576","messageId":"5f3f7ac77039d41d1692ceae4b0c5df3bb45b74a.1612901326.git.matheus.bernardino@usp.br","threadId":"54177","inReplyTo":"cover.1599758167.git.matheus.bernardino@usp.br","subject":"[PATCH v7] grep: honor sparse-checkout on working tree searches","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-09T21:33:30Z","receivedAt":"2021-02-09T22:59:13Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On a sparse checked out repository, `git grep` (without --cached) ends\nup searching the cache when an entry matches the search pathspec and has\nthe SKIP_WORKTREE bit set. This is confusing both because the sparse\npaths are not expected to be in a working tree search (as they are not\nchecked out), and because the output mixes working tree and cache\nresults without distinguishing them. (Note that grep also resorts to the\ncache on working tree searches that include --assume-unchanged paths.\nBut the whole point in that case is to assume that the contents of the\nindex entry and the file are the same. This does not apply to the case\nof sparse paths, where the file isn't even expected to be present.)\n\nFix that by teaching grep to honor the sparse-checkout rules for working\ntree searches. If the user wants to grep paths outside the current\nsparse-checkout definition, they may either update the sparsity rules to\nmaterialize the files, or use --cached to search all blobs registered in\nthe index.\n\nNote: it might also be interesting to add a configuration option that\nallow users to search paths that are present despite having the\nSKIP_WORKTREE bit set, and/or to restrict searches in the index and past\nrevisions too. These ideas are left as future improvements to avoid\nconflicting with other sparse-checkout topics currently in flight.\n\nSuggested-by: Elijah Newren \nSigned-off-by: Matheus Tavares \n---\n\nThis new version includes only the bug fix for the working tree grep, as\ndiscussed in [1]. I think there are a couple other patches that could be\nextracted from the previous v6 [2] and sent as standalone topics,\nwithout the risk of conflicting with the sparse-index work. E.g. the\nunification of the git-grep.txt and config/grep.txt doc files. I'll look\ninto that tomorrow.\n\n[1]: https://lore.kernel.org/git/CABPp-BHwNoVnooqDFPAsZxBT9aR5Dwk5D9sDRCvYSb8akxAJgA@mail.gmail.com/\n[2]: https://lore.kernel.org/git/cover.1599758167.git.matheus.bernardino@usp.br/\n\n builtin/grep.c | 7 +-\n t/t7011-skip-worktree-reading.sh | 9 --\n t/t7817-grep-sparse-checkout.sh | 174 +++++++++++++++++++++++++++++++\n 3 files changed, 179 insertions(+), 11 deletions(-)\n create mode 100755 t/t7817-grep-sparse-checkout.sh\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex ca259af441..bff214c755 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -508,6 +508,10 @@ static int grep_cache(struct grep_opt *opt,\n \n \tfor (nr = 0; nr < repo->index->cache_nr; nr++) {\n \t\tconst struct cache_entry *ce = repo->index->cache[nr];\n+\n+\t\tif (!cached && ce_skip_worktree(ce))\n+\t\t\tcontinue;\n+\n \t\tstrbuf_setlen(&name, name_base_len);\n \t\tstrbuf_addstr(&name, ce->name);\n \n@@ -520,8 +524,7 @@ static int grep_cache(struct grep_opt *opt,\n \t\t\t * cache entry are identical, even if worktree file has\n \t\t\t * been modified, so use cache version instead\n \t\t\t */\n-\t\t\tif (cached || (ce->ce_flags & CE_VALID) ||\n-\t\t\t ce_skip_worktree(ce)) {\n+\t\t\tif (cached || (ce->ce_flags & CE_VALID)) {\n \t\t\t\tif (ce_stage(ce) || ce_intent_to_add(ce))\n \t\t\t\t\tcontinue;\n \t\t\t\thit |= grep_oid(opt, &ce->oid, name.buf,\ndiff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh\nindex 37525cae3a..26852586ac 100755\n--- a/t/t7011-skip-worktree-reading.sh\n+++ b/t/t7011-skip-worktree-reading.sh\n@@ -109,15 +109,6 @@ test_expect_success 'ls-files --modified' '\n \ttest -z \"$(git ls-files -m)\"\n '\n \n-test_expect_success 'grep with skip-worktree file' '\n-\tgit update-index --no-skip-worktree 1 &&\n-\techo test > 1 &&\n-\tgit update-index 1 &&\n-\tgit update-index --skip-worktree 1 &&\n-\trm 1 &&\n-\ttest \"$(git grep --no-ext-grep test)\" = \"1:test\"\n-'\n-\n echo \":000000 100644 $ZERO_OID $EMPTY_BLOB A\t1\" > expected\n test_expect_success 'diff-index does not examine skip-worktree absent entries' '\n \tsetup_absent &&\ndiff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh\nnew file mode 100755\nindex 0000000000..590b99bbb6\n--- /dev/null\n+++ b/t/t7817-grep-sparse-checkout.sh\n@@ -0,0 +1,174 @@\n+#!/bin/sh\n+\n+test_description='grep in sparse checkout\n+\n+This test creates a repo with the following structure:\n+\n+.\n+|-- a\n+|-- b\n+|-- dir\n+| `-- c\n+|-- sub\n+| |-- A\n+| | `-- a\n+| `-- B\n+| `-- b\n+`-- sub2\n+ `-- a\n+\n+Where the outer repository has non-cone mode sparsity patterns, sub is a\n+submodule with cone mode sparsity patterns and sub2 is a submodule that is\n+excluded by the superproject sparsity patterns. The resulting sparse checkout\n+should leave the following structure in the working tree:\n+\n+.\n+|-- a\n+|-- sub\n+| `-- B\n+| `-- b\n+`-- sub2\n+ `-- a\n+\n+But note that sub2 should have the SKIP_WORKTREE bit set.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo \"text\" >a &&\n+\techo \"text\" >b &&\n+\tmkdir dir &&\n+\techo \"text\" >dir/c &&\n+\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tmkdir A B &&\n+\t\techo \"text\" >A/a &&\n+\t\techo \"text\" >B/b &&\n+\t\tgit add A B &&\n+\t\tgit commit -m sub &&\n+\t\tgit sparse-checkout init --cone &&\n+\t\tgit sparse-checkout set B\n+\t) &&\n+\n+\tgit init sub2 &&\n+\t(\n+\t\tcd sub2 &&\n+\t\techo \"text\" >a &&\n+\t\tgit add a &&\n+\t\tgit commit -m sub2\n+\t) &&\n+\n+\tgit submodule add ./sub &&\n+\tgit submodule add ./sub2 &&\n+\tgit add a b dir &&\n+\tgit commit -m super &&\n+\tgit sparse-checkout init --no-cone &&\n+\tgit sparse-checkout set \"/*\" \"!b\" \"!/*/\" \"sub\" &&\n+\n+\tgit tag -am tag-to-commit tag-to-commit HEAD &&\n+\ttree=$(git rev-parse HEAD^{tree}) &&\n+\tgit tag -am tag-to-tree tag-to-tree $tree &&\n+\n+\ttest_path_is_missing b &&\n+\ttest_path_is_missing dir &&\n+\ttest_path_is_missing sub/A &&\n+\ttest_path_is_file a &&\n+\ttest_path_is_file sub/B/b &&\n+\ttest_path_is_file sub2/a &&\n+\tgit branch -m main\n+'\n+\n+# The test below covers a special case: the sparsity patterns exclude '/b' and\n+# sparse checkout is enabled, but the path exists in the working tree (e.g.\n+# manually created after `git sparse-checkout init`). git grep should skip it.\n+test_expect_success 'working tree grep honors sparse checkout' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tEOF\n+\ttest_when_finished \"rm -f b\" &&\n+\techo \"new-text\" >b &&\n+\tgit grep \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep searches unmerged file despite not matching sparsity patterns' '\n+\tcat >expect <<-EOF &&\n+\tb:modified-b-in-branchX\n+\tb:modified-b-in-branchY\n+\tEOF\n+\ttest_when_finished \"test_might_fail git merge --abort && \\\n+\t\t\t git checkout main && git sparse-checkout init\" &&\n+\n+\tgit sparse-checkout disable &&\n+\tgit checkout -b branchY main &&\n+\ttest_commit modified-b-in-branchY b &&\n+\tgit checkout -b branchX main &&\n+\ttest_commit modified-b-in-branchX b &&\n+\n+\tgit sparse-checkout init &&\n+\ttest_path_is_missing b &&\n+\ttest_must_fail git merge branchY &&\n+\tgit grep \"modified-b\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --cached searches entries with the SKIP_WORKTREE bit' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tb:text\n+\tdir/c:text\n+\tEOF\n+\tgit grep --cached \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# Note that sub2/ is present in the worktree but it is excluded by the sparsity\n+# patterns, so grep should not recurse into it.\n+test_expect_success 'grep --recurse-submodules honors sparse checkout in submodule' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tsub/B/b:text\n+\tEOF\n+\tgit grep --recurse-submodules \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --recurse-submodules --cached searches entries with the SKIP_WORKTREE bit' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tb:text\n+\tdir/c:text\n+\tsub/A/a:text\n+\tsub/B/b:text\n+\tsub2/a:text\n+\tEOF\n+\tgit grep --recurse-submodules --cached \"text\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'working tree grep does not search the index with CE_VALID and SKIP_WORKTREE' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tEOF\n+\ttest_when_finished \"git update-index --no-assume-unchanged b\" &&\n+\tgit update-index --assume-unchanged b &&\n+\tgit grep text >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --cached searches index entries with both CE_VALID and SKIP_WORKTREE' '\n+\tcat >expect <<-EOF &&\n+\ta:text\n+\tb:text\n+\tdir/c:text\n+\tEOF\n+\ttest_when_finished \"git update-index --no-assume-unchanged b\" &&\n+\tgit update-index --assume-unchanged b &&\n+\tgit grep --cached text >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.29.2\n\n"},{"id":"416591","messageId":"xmqqtuqkygfb.fsf@gitster.c.googlers.com","threadId":"54177","inReplyTo":"5f3f7ac77039d41d1692ceae4b0c5df3bb45b74a.1612901326.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v7] grep: honor sparse-checkout on working tree searches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-09T23:23:04Z","receivedAt":"2021-02-10T00:11:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares writes:\n\n> This new version includes only the bug fix for the working tree grep, as\n> discussed in [1]. I think there are a couple other patches that could be\n> extracted from the previous v6 [2] and sent as standalone topics,\n> without the risk of conflicting with the sparse-index work. E.g. the\n> unification of the git-grep.txt and config/grep.txt doc files. I'll look\n> into that tomorrow.\n\nAs mt/rm-sparse-checkout depends on the v6 that you are ejecting\nwith this patch, I'll stop merging that topic to 'seen' for now.\n\nThanks for keeping an eye on this one.\n\n"},{"id":"416603","messageId":"CABPp-BFMk7XX-qkXC_C6vh_x7OPUS+Dfu-6SFWvm2WVA14zLSg@mail.gmail.com","threadId":"54177","inReplyTo":"xmqqtuqkygfb.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v7] grep: honor sparse-checkout on working tree searches","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-02-10T06:12:08Z","receivedAt":"2021-02-10T06:22:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Feb 9, 2021 at 3:23 PM Junio C Hamano wrote:\n>\n> Matheus Tavares writes:\n>\n> > This new version includes only the bug fix for the working tree grep, as\n> > discussed in [1]. I think there are a couple other patches that could be\n> > extracted from the previous v6 [2] and sent as standalone topics,\n> > without the risk of conflicting with the sparse-index work. E.g. the\n> > unification of the git-grep.txt and config/grep.txt doc files. I'll look\n> > into that tomorrow.\n>\n> As mt/rm-sparse-checkout depends on the v6 that you are ejecting\n> with this patch, I'll stop merging that topic to 'seen' for now.\n>\n> Thanks for keeping an eye on this one.\n\nYeah, Matheus and I have been talking about both topics off-list a\nbit; I hadn't yet responded to his final revision of a newer\nmt/rm-sparse-checkout but he has a good replacement for that topic now\nthat he'll send to the list soon.\n\nAnyway, for the patch submitted here you can add a\n\nReviewed-by: Elijah Newren \n"}]}