{"thread":{"id":"46147","subject":"[PATCH v2 0/8] Avoid problem where git_dir is set after alias expansion","startedAt":"2017-06-08T19:53:39Z","lastAt":"2017-06-14T10:03:26Z","messageCount":32,"participants":["Johannes Schindelin","Brandon Williams","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":8},"messages":[{"id":"321765","messageId":"cover.1496951503.git.johannes.schindelin@gmx.de","threadId":"46147","inReplyTo":null,"subject":"[PATCH v2 0/8] Avoid problem where git_dir is set after alias expansion","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T19:53:26Z","receivedAt":"2017-06-08T19:53:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When expanding an alias in a subdirectory, we setup the git_dir\n(gently), read the config, and then restore the \"env\" (e.g. the current\nworking directory) so that the command specified by the alias can run\ncorrectly.\n\nWhat we failed to reset was the git_dir, meaning that in the most common\ncase, it was now pointing to a .git/ directory *in the subdirectory*.\n\nThis problem was identified in the GVFS fork, where a pre-command hook\nwas introduced to allow pre-fetching missing blobs.\n\nAn early quick fix in the GVFS fork simply built on top of the\nsave_env_before_alias() hack, introducing another hack that saves the\ngit_dir and restores it after an alias is expanded:\n\n\thttps://github.com/Microsoft/git/commit/2d859ba3b\n\nThat is very hacky, though, and it is much better (although much more\ninvolved, too) to fix this \"properly\", i.e. by replacing the ugly\nsave/restore logic by simply using the early config code path.\n\nHowever, aliases are strange beasts.\n\nWhen an alias refers to a single Git command (originally the sole\nintention of aliases), the current working directory is restored to what\nit had been before expanding the alias.\n\nBut when an alias starts with an exclamation point, i.e. referring to a\ncommand-line to be interpreted by the shell, the current working\ndirectory is no longer in the subdirectory but instead in the worktree's\ntop-level directory.\n\nThis is even true for worktrees added by `git worktree add`.\n\nBut when we are inside the .git/ directory, the current working\ndirectory is *restored* to the subdirectory inside the .git/ directory.\n\nIn short, the logic is a bit complicated what is the expected current\nworking directory after expanding an alias and before actually running\nit.\n\nThat is why this patch series had to expand the signature of the early\nconfig machinery to return the additional information for aliases'\nbenefit.\n\nChanges since v1:\n\n- 1/8's commit message clarifies why the other early return in\n discover_git_directory() does not need an equivalent resetting of\n git_dir.\n\n- 3/8's commit message fixes awkward language (thanks, Brandon!).\n\n- the `worktree_dir` variables/parameters have been renamed to\n `cdup_dir` to clarify that they only get populated if the search for\n the .git/ directory determined that the current working directory\n is a subdirectory of the directory containing .git/.\n\n\nJohannes Schindelin (8):\n discover_git_directory(): avoid setting invalid git_dir\n config: report correct line number upon error\n help: use early config when autocorrecting aliases\n read_early_config(): optionally return the worktree's top-level\n directory\n t1308: relax the test verifying that empty alias values are disallowed\n t7006: demonstrate a problem with aliases in subdirectories\n alias_lookup(): optionally return top-level directory\n Use the early config machinery to expand aliases\n\n alias.c | 33 +++++++++++++++++++++-------\n builtin/help.c | 2 +-\n cache.h | 7 +++---\n config.c | 7 +++---\n git.c | 59 ++++++--------------------------------------------\n help.c | 2 +-\n pager.c | 4 ++--\n setup.c | 13 +++++++++--\n t/helper/test-config.c | 2 +-\n t/t1308-config-set.sh | 4 +++-\n t/t7006-pager.sh | 11 ++++++++++\n 11 files changed, 70 insertions(+), 74 deletions(-)\n\n\nbase-commit: 8d1b10321b20bd2a73a5b561cfc3cf2e8051b70b\nPublished-As: https://github.com/dscho/git/releases/tag/alias-early-config-v2\nFetch-It-Via: git fetch https://github.com/dscho/git alias-early-config-v2\n\nInterdiff vs v1:\n diff --git a/alias.c b/alias.c\n index c1b87154944..b7c4a4f0217 100644\n --- a/alias.c\n +++ b/alias.c\n @@ -16,13 +16,13 @@ static int config_alias_cb(const char *key, const char *value, void *d)\n \treturn 0;\n }\n \n -char *alias_lookup(const char *alias, struct strbuf *worktree_dir)\n +char *alias_lookup(const char *alias, struct strbuf *cdup_dir)\n {\n \tstruct config_alias_data data = { STRBUF_INIT, NULL };\n \n \tstrbuf_addf(&data.key, \"alias.%s\", alias);\n \tif (git_config_key_is_valid(data.key.buf))\n -\t\tread_early_config(config_alias_cb, &data, worktree_dir);\n +\t\tread_early_config(config_alias_cb, &data, cdup_dir);\n \tstrbuf_release(&data.key);\n \n \treturn data.v;\n diff --git a/cache.h b/cache.h\n index 77ce8e8c80b..65f2e5bf04c 100644\n --- a/cache.h\n +++ b/cache.h\n @@ -530,7 +530,7 @@ extern void setup_work_tree(void);\n * appended to gitdir. The return value is either NULL if no repository was\n * found, or pointing to the path inside gitdir's buffer.\n */\n -extern const char *discover_git_directory(struct strbuf *gitdir, struct strbuf *worktree_dir);\n +extern const char *discover_git_directory(struct strbuf *gitdir, struct strbuf *cdup_dir);\n extern const char *setup_git_directory_gently(int *);\n extern const char *setup_git_directory(void);\n extern char *prefix_path(const char *prefix, int len, const char *path);\n @@ -1914,7 +1914,7 @@ extern int git_config_from_blob_sha1(config_fn_t fn, const char *name,\n extern void git_config_push_parameter(const char *text);\n extern int git_config_from_parameters(config_fn_t fn, void *data);\n extern void read_early_config(config_fn_t cb, void *data,\n -\t\t\t struct strbuf *worktree_dir);\n +\t\t\t struct strbuf *cdup_dir);\n extern void git_config(config_fn_t fn, void *);\n extern int git_config_with_options(config_fn_t fn, void *,\n \t\t\t\t struct git_config_source *config_source,\n @@ -2189,7 +2189,7 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);\n /* ls-files */\n void overlay_tree_on_cache(const char *tree_name, const char *prefix);\n \n -char *alias_lookup(const char *alias, struct strbuf *worktree_dir);\n +char *alias_lookup(const char *alias, struct strbuf *cdup_dir);\n int split_cmdline(char *cmdline, const char ***argv);\n /* Takes a negative value returned by split_cmdline */\n const char *split_cmdline_strerror(int cmdline_errno);\n diff --git a/config.c b/config.c\n index 3d78c11fc00..5aec6e4c87c 100644\n --- a/config.c\n +++ b/config.c\n @@ -1651,7 +1651,7 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n \t}\n }\n \n -void read_early_config(config_fn_t cb, void *data, struct strbuf *worktree_dir)\n +void read_early_config(config_fn_t cb, void *data, struct strbuf *cdup_dir)\n {\n \tstruct config_options opts = {0};\n \tstruct strbuf buf = STRBUF_INIT;\n @@ -1668,7 +1668,7 @@ void read_early_config(config_fn_t cb, void *data, struct strbuf *worktree_dir)\n \t * notably, the current working directory is still the same after the\n \t * call).\n \t */\n -\telse if (discover_git_directory(&buf, worktree_dir))\n +\telse if (discover_git_directory(&buf, cdup_dir))\n \t\topts.git_dir = buf.buf;\n \n \tgit_config_with_options(cb, data, NULL, &opts);\n diff --git a/git.c b/git.c\n index c82cd455948..6511595d467 100644\n --- a/git.c\n +++ b/git.c\n @@ -201,7 +201,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \n static int handle_alias(int *argcp, const char ***argv)\n {\n -\tstruct strbuf worktree_dir = STRBUF_INIT;\n +\tstruct strbuf cdup_dir = STRBUF_INIT;\n \tint envchanged = 0, ret = 0, saved_errno = errno;\n \tint count, option_count;\n \tconst char **new_argv;\n @@ -209,12 +209,12 @@ static int handle_alias(int *argcp, const char ***argv)\n \tchar *alias_string;\n \n \talias_command = (*argv)[0];\n -\talias_string = alias_lookup(alias_command, &worktree_dir);\n +\talias_string = alias_lookup(alias_command, &cdup_dir);\n \tif (alias_string) {\n \t\tif (alias_string[0] == '!') {\n \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n \n -\t\t\tif (worktree_dir.len)\n +\t\t\tif (cdup_dir.len)\n \t\t\t\tsetup_git_directory();\n \n \t\t\tcommit_pager_choice();\n @@ -224,14 +224,14 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\targv_array_pushv(&child.args, (*argv) + 1);\n \n \t\t\tret = run_command(&child);\n -\t\t\tstrbuf_release(&worktree_dir);\n +\t\t\tstrbuf_release(&cdup_dir);\n \t\t\tif (ret >= 0) /* normal exit */\n \t\t\t\texit(ret);\n \n \t\t\tdie_errno(\"While expanding alias '%s': '%s'\",\n \t\t\t alias_command, alias_string + 1);\n \t\t}\n -\t\tstrbuf_release(&worktree_dir);\n +\t\tstrbuf_release(&cdup_dir);\n \t\tcount = split_cmdline(alias_string, &new_argv);\n \t\tif (count < 0)\n \t\t\tdie(\"Bad alias.%s string: %s\", alias_command,\n diff --git a/setup.c b/setup.c\n index 771822fd0ca..3f6fc1577b7 100644\n --- a/setup.c\n +++ b/setup.c\n @@ -946,10 +946,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n }\n \n const char *discover_git_directory(struct strbuf *gitdir,\n -\t\t\t\t struct strbuf *worktree_dir)\n +\t\t\t\t struct strbuf *cdup_dir)\n {\n \tstruct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;\n -\tsize_t gitdir_offset = gitdir->len, cwd_len, worktree_dir_offset;\n +\tsize_t gitdir_offset = gitdir->len, cwd_len, cdup_dir_offset;\n \tstruct repository_format candidate;\n \n \tif (strbuf_getcwd(&dir))\n @@ -974,9 +974,9 @@ const char *discover_git_directory(struct strbuf *gitdir,\n \t\tstrbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);\n \t}\n \n -\tif (worktree_dir) {\n -\t\tworktree_dir_offset = worktree_dir->len;\n -\t\tstrbuf_addbuf(worktree_dir, &dir);\n +\tif (cdup_dir) {\n +\t\tcdup_dir_offset = cdup_dir->len;\n +\t\tstrbuf_addbuf(cdup_dir, &dir);\n \t}\n \n \tstrbuf_reset(&dir);\n @@ -989,8 +989,8 @@ const char *discover_git_directory(struct strbuf *gitdir,\n \t\t\tgitdir->buf + gitdir_offset, err.buf);\n \t\tstrbuf_release(&err);\n \t\tstrbuf_setlen(gitdir, gitdir_offset);\n -\t\tif (worktree_dir)\n -\t\t\tstrbuf_setlen(worktree_dir, worktree_dir_offset);\n +\t\tif (cdup_dir)\n +\t\t\tstrbuf_setlen(cdup_dir, cdup_dir_offset);\n \t\treturn NULL;\n \t}\n \n-- \n2.13.0.windows.1.460.g13f583bedb5\n\n"},{"id":"321766","messageId":"291e8b643990aee04654b34c6f953387c3c030d4.1496951503.git.johannes.schindelin@gmx.de","threadId":"46147","inReplyTo":"cover.1496951503.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/8] config: report correct line number upon error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T19:53:35Z","receivedAt":"2017-06-08T19:53:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When get_value() parses a key/value pair, it is possible that the line\nnumber is decreased (because the \\n has been consumed already) before the\nkey/value pair is passed to the callback function, to allow for the\ncorrect line to be attributed in case of an error.\n\nHowever, when git_parse_source() asks get_value() to parse the key/value\npair, the error reporting is performed *after* get_value() returns.\n\nWhich means that we have to be careful not to increase the line number\nin get_value() after the callback function returned an error.\n\nSigned-off-by: Johannes Schindelin \n---\n config.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 146cb3452ad..9b88531a70d 100644\n--- a/config.c\n+++ b/config.c\n@@ -604,7 +604,8 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n \t */\n \tcf->linenr--;\n \tret = fn(name->buf, value, data);\n-\tcf->linenr++;\n+\tif (!ret)\n+\t\tcf->linenr++;\n \treturn ret;\n }\n \n-- \n2.13.0.windows.1.460.g13f583bedb5\n\n\n"},{"id":"321767","messageId":"0cef47383c0dd2287ffda2a6eaf1f430ed3ee726.1496951503.git.johannes.schindelin@gmx.de","threadId":"46147","inReplyTo":"cover.1496951503.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 4/8] read_early_config(): optionally return the worktree's top-level directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T19:53:41Z","receivedAt":"2017-06-08T19:53:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"So far, when we invoked the early config code path, we implicitly\ndetermined the top-level directory of the worktree while discovering the\n.git/ directory.\n\nAnd then we simply forgot that information.\n\nHowever, when we expand aliases, we very much need that information, as\naliases expanding to shell commands, i.e. whose value starts with an\nexclamation point, have to be executed in the top-level directory of the\nworktree. There are exceptions abound, not only with non-shell aliases\n(which are supposed to be executed in the original working directory\ninstead), but also when being started inside the .git/ directory or in a\nworktree created via `git worktree add`.\n\nIn preparation for allowing the alias machinery to make use of the early\nconfig machinery, let's add an optional strbuf parameter to the\nread_early_config() function; if not NULL, the path of said top-level\ndirectory is appended to the strbuf. As a special case, nothing is\nappended if setup_git_directory() would have restored the original\nworking directory before returning.\n\nSigned-off-by: Johannes Schindelin \n---\n cache.h | 5 +++--\n config.c | 4 ++--\n help.c | 2 +-\n pager.c | 4 ++--\n setup.c | 12 ++++++++++--\n t/helper/test-config.c | 2 +-\n 6 files changed, 19 insertions(+), 10 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4d92aae0e81..f769ef1779c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -530,7 +530,7 @@ extern void setup_work_tree(void);\n * appended to gitdir. The return value is either NULL if no repository was\n * found, or pointing to the path inside gitdir's buffer.\n */\n-extern const char *discover_git_directory(struct strbuf *gitdir);\n+extern const char *discover_git_directory(struct strbuf *gitdir, struct strbuf *cdup_dir);\n extern const char *setup_git_directory_gently(int *);\n extern const char *setup_git_directory(void);\n extern char *prefix_path(const char *prefix, int len, const char *path);\n@@ -1913,7 +1913,8 @@ extern int git_config_from_blob_sha1(config_fn_t fn, const char *name,\n \t\t\t\t const unsigned char *sha1, void *data);\n extern void git_config_push_parameter(const char *text);\n extern int git_config_from_parameters(config_fn_t fn, void *data);\n-extern void read_early_config(config_fn_t cb, void *data);\n+extern void read_early_config(config_fn_t cb, void *data,\n+\t\t\t struct strbuf *cdup_dir);\n extern void git_config(config_fn_t fn, void *);\n extern int git_config_with_options(config_fn_t fn, void *,\n \t\t\t\t struct git_config_source *config_source,\ndiff --git a/config.c b/config.c\nindex 9b88531a70d..5aec6e4c87c 100644\n--- a/config.c\n+++ b/config.c\n@@ -1651,7 +1651,7 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n \t}\n }\n \n-void read_early_config(config_fn_t cb, void *data)\n+void read_early_config(config_fn_t cb, void *data, struct strbuf *cdup_dir)\n {\n \tstruct config_options opts = {0};\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -1668,7 +1668,7 @@ void read_early_config(config_fn_t cb, void *data)\n \t * notably, the current working directory is still the same after the\n \t * call).\n \t */\n-\telse if (discover_git_directory(&buf))\n+\telse if (discover_git_directory(&buf, cdup_dir))\n \t\topts.git_dir = buf.buf;\n \n \tgit_config_with_options(cb, data, NULL, &opts);\ndiff --git a/help.c b/help.c\nindex b44c55ec2da..f78747e8413 100644\n--- a/help.c\n+++ b/help.c\n@@ -289,7 +289,7 @@ const char *help_unknown_cmd(const char *cmd)\n \tmemset(&other_cmds, 0, sizeof(other_cmds));\n \tmemset(&aliases, 0, sizeof(aliases));\n \n-\tread_early_config(git_unknown_cmd_config, NULL);\n+\tread_early_config(git_unknown_cmd_config, NULL, NULL);\n \n \tload_command_list(\"git-\", &main_cmds, &other_cmds);\n \ndiff --git a/pager.c b/pager.c\nindex c113d898a4a..857cf5ecb32 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -53,7 +53,7 @@ const char *git_pager(int stdout_is_tty)\n \tpager = getenv(\"GIT_PAGER\");\n \tif (!pager) {\n \t\tif (!pager_program)\n-\t\t\tread_early_config(core_pager_config, NULL);\n+\t\t\tread_early_config(core_pager_config, NULL, NULL);\n \t\tpager = pager_program;\n \t}\n \tif (!pager)\n@@ -214,7 +214,7 @@ int check_pager_config(const char *cmd)\n \tdata.want = -1;\n \tdata.value = NULL;\n \n-\tread_early_config(pager_command_config, &data);\n+\tread_early_config(pager_command_config, &data, NULL);\n \n \tif (data.value)\n \t\tpager_program = data.value;\ndiff --git a/setup.c b/setup.c\nindex 2435186e448..3f6fc1577b7 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -945,10 +945,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t}\n }\n \n-const char *discover_git_directory(struct strbuf *gitdir)\n+const char *discover_git_directory(struct strbuf *gitdir,\n+\t\t\t\t struct strbuf *cdup_dir)\n {\n \tstruct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;\n-\tsize_t gitdir_offset = gitdir->len, cwd_len;\n+\tsize_t gitdir_offset = gitdir->len, cwd_len, cdup_dir_offset;\n \tstruct repository_format candidate;\n \n \tif (strbuf_getcwd(&dir))\n@@ -973,6 +974,11 @@ const char *discover_git_directory(struct strbuf *gitdir)\n \t\tstrbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);\n \t}\n \n+\tif (cdup_dir) {\n+\t\tcdup_dir_offset = cdup_dir->len;\n+\t\tstrbuf_addbuf(cdup_dir, &dir);\n+\t}\n+\n \tstrbuf_reset(&dir);\n \tstrbuf_addf(&dir, \"%s/config\", gitdir->buf + gitdir_offset);\n \tread_repository_format(&candidate, dir.buf);\n@@ -983,6 +989,8 @@ const char *discover_git_directory(struct strbuf *gitdir)\n \t\t\tgitdir->buf + gitdir_offset, err.buf);\n \t\tstrbuf_release(&err);\n \t\tstrbuf_setlen(gitdir, gitdir_offset);\n+\t\tif (cdup_dir)\n+\t\t\tstrbuf_setlen(cdup_dir, cdup_dir_offset);\n \t\treturn NULL;\n \t}\n \ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 8e3ed6a76cb..5e1e78f8fa2 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -84,7 +84,7 @@ int cmd_main(int argc, const char **argv)\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\tread_early_config(early_config_cb, (void *)argv[2], NULL);\n \t\treturn 0;\n \t}\n \n-- \n2.13.0.windows.1.460.g13f583bedb5\n\n\n"},{"id":"321768","messageId":"effd58a564e54ccd3d695d91bc9c4f32255b8ab7.1496951503.git.johannes.schindelin@gmx.de","threadId":"46147","inReplyTo":"cover.1496951503.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/8] discover_git_directory(): avoid setting invalid git_dir","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T19:53:32Z","receivedAt":"2017-06-08T19:53:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When discovering a .git/ directory, we take pains to ensure that its\nrepository format version matches Git's expectations, and we return NULL\notherwise.\n\nHowever, we still appended the invalid path to the strbuf passed as\nargument.\n\nLet's just reset the strbuf to the state before we appended the .git/\ndirectory that was eventually rejected.\n\nThere is another early return path in that function, when\nsetup_git_directory_gently_1() returns GIT_DIR_NONE or an error. In that\ncase, the gitdir parameter has not been touched, therefore there is no\nneed for an equivalent change in that code path.\n\nSigned-off-by: Johannes Schindelin \n---\n setup.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/setup.c b/setup.c\nindex e3f7699a902..2435186e448 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -982,6 +982,7 @@ const char *discover_git_directory(struct strbuf *gitdir)\n \t\twarning(\"ignoring git dir '%s': %s\",\n \t\t\tgitdir->buf + gitdir_offset, err.buf);\n \t\tstrbuf_release(&err);\n+\t\tstrbuf_setlen(gitdir, gitdir_offset);\n \t\treturn NULL;\n \t}\n \n-- \n2.13.0.windows.1.460.g13f583bedb5\n\n\n"},{"id":"321769","messageId":"2be3d75cd3341c1386f97c77cc63bec89c1f6a47.1496951503.git.johannes.schindelin@gmx.de","threadId":"46147","inReplyTo":"cover.1496951503.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 3/8] help: use early config when autocorrecting aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T19:53:38Z","receivedAt":"2017-06-08T19:53:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Git has this feature which suggests similar commands (including aliases)\nin case the user specified an unknown command.\n\nThis feature currently relies on a side effect of the way we expand\naliases right now: when a command is not a builtin, we use the regular\nconfig machinery (meaning: discovering the .git/ directory and\ninitializing global state such as the config cache) to see whether the\ncommand refers to an alias.\n\nHowever, we will change the way aliases are expanded in the next\ncommits, to use the early config instead. That means that the\nautocorrect feature can no longer discover the available aliases by\nlooking at the config cache (because it has not yet been initialized).\n\nSo let's just use the early config machinery instead.\n\nThis is slightly less performant than the previous way, as the early\nconfig is used *twice*: once to see whether the command refers to an\nalias, and then to see what aliases are most similar. However, this is\nhardly a performance-critical code path, so performance is less important\nhere.\n\nSigned-off-by: Johannes Schindelin \n---\n help.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/help.c b/help.c\nindex db7f3d79a01..b44c55ec2da 100644\n--- a/help.c\n+++ b/help.c\n@@ -289,7 +289,7 @@ const char *help_unknown_cmd(const char *cmd)\n \tmemset(&other_cmds, 0, sizeof(other_cmds));\n \tmemset(&aliases, 0, sizeof(aliases));\n \n-\tgit_config(git_unknown_cmd_config, NULL);\n+\tread_early_config(git_unknown_cmd_config, NULL);\n \n \tload_command_list(\"git-\", &main_cmds, &other_cmds);\n \n-- \n2.13.0.windows.1.460.g13f583bedb5\n\n\n"},{"id":"321770","messageId":"b9e212d508e9103037da2070cc0aaed530ea7473.1496951503.git.johannes.schindelin@gmx.de","threadId":"46147","inReplyTo":"cover.1496951503.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 5/8] t1308: relax the test verifying that empty alias values are disallowed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T19:53:44Z","receivedAt":"2017-06-08T19:53:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We are about to change the way aliases are expanded, to use the early\nconfig machinery.\n\nThis machinery reports errors in a slightly different manner than the\ncached config machinery.\n\nLet's not get hung up by the precise wording of the message mentioning\nthe lin number. It is really sufficient to verify that all the relevant\ninformation is given to the user.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t1308-config-set.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex ff50960ccae..69a0aa56d6d 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -215,7 +215,9 @@ test_expect_success 'check line errors for malformed values' '\n \t\tbr\n \tEOF\n \ttest_expect_code 128 git br 2>result &&\n-\ttest_i18ngrep \"fatal: .*alias\\.br.*\\.git/config.*line 2\" result\n+\ttest_i18ngrep \"missing value for .alias\\.br\" result &&\n+\ttest_i18ngrep \"fatal: .*\\.git/config\" result &&\n+\ttest_i18ngrep \"fatal: .*line 2\" result\n '\n \n test_expect_success 'error on modifying repo config without repo' '\n-- \n2.13.0.windows.1.460.g13f583bedb5\n\n\n"},{"id":"321771","messageId":"d20265ec04b02a58d9537813e85b5c842436ee22.1496951503.git.johannes.schindelin@gmx.de","threadId":"46147","inReplyTo":"cover.1496951503.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 6/8] t7006: demonstrate a problem with aliases in subdirectories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T19:53:47Z","receivedAt":"2017-06-08T19:54:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When expanding aliases, the git_dir is set during the alias expansion\n(by virtue of running setup_git_directory_gently()).\n\nThis git_dir may be relative to the current working directory, and\nindeed often is simply \".git/\".\n\nWhen the alias expands to a shell command, we restore the original\nworking directory, though, yet we do not reset git_dir.\n\nAs a consequence, subsequent read_early_config() runs will mistake the\ngit_dir to be populated properly and not find the correct config.\n\nDemonstrate this problem by adding a test case.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t7006-pager.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 4f3794d415e..83881ec3a0c 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -391,6 +391,17 @@ test_expect_success TTY 'core.pager in repo config works and retains cwd' '\n \t)\n '\n \n+test_expect_failure TTY 'core.pager is found via alias in subdirectory' '\n+\tsane_unset GIT_PAGER &&\n+\ttest_config core.pager \"cat >via-alias\" &&\n+\t(\n+\t\tcd sub &&\n+\t\trm -f via-alias &&\n+\t\ttest_terminal git -c alias.r=\"-p rev-parse\" r HEAD &&\n+\t\ttest_path_is_file via-alias\n+\t)\n+'\n+\n test_doesnt_paginate expect_failure test_must_fail 'git -p nonsense'\n \n test_pager_choices 'git shortlog'\n-- \n2.13.0.windows.1.460.g13f583bedb5\n\n\n"},{"id":"321772","messageId":"af40ce088de03130a5edfae187bb8274b5a9d3a9.1496951503.git.johannes.schindelin@gmx.de","threadId":"46147","inReplyTo":"cover.1496951503.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 8/8] Use the early config machinery to expand aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T19:53:53Z","receivedAt":"2017-06-08T19:54:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We already taught alias_lookup() to use the early config if the .git/\ndirectory was not yet discovered, of course, however, since we called\nsetup_git_directory_gently() before expanding the alias (if any), we\nonly used the early config code path only if outside of any Git-managed\ndirectory.\n\nWith this commit, we finally switch over to really using the early\nconfig code path.\n\nRather than just chdir()ing into the indicated directory in case of an\nalias expanding to a shell command, we simply set up the .git/ directory\nso that e.g. GIT_PREFIX is set as expected.\n\nThis change also fixes a known issue where Git tried to read the pager\nconfig from an incorrect path in a subdirectory of a Git worktree if an\nalias expanded to a shell command.\n\nSigned-off-by: Johannes Schindelin \n---\n git.c | 59 +++++++-------------------------------------------------\n t/t7006-pager.sh | 2 +-\n 2 files changed, 8 insertions(+), 53 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 4163beaead4..6511595d467 100644\n--- a/git.c\n+++ b/git.c\n@@ -16,50 +16,6 @@ const char git_more_info_string[] =\n \t \"to read about a specific subcommand or concept.\");\n \n static int use_pager = -1;\n-static char *orig_cwd;\n-static const char *env_names[] = {\n-\tGIT_DIR_ENVIRONMENT,\n-\tGIT_WORK_TREE_ENVIRONMENT,\n-\tGIT_IMPLICIT_WORK_TREE_ENVIRONMENT,\n-\tGIT_PREFIX_ENVIRONMENT\n-};\n-static char *orig_env[4];\n-static int save_restore_env_balance;\n-\n-static void save_env_before_alias(void)\n-{\n-\tint i;\n-\n-\tassert(save_restore_env_balance == 0);\n-\tsave_restore_env_balance = 1;\n-\torig_cwd = xgetcwd();\n-\tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n-\t\torig_env[i] = getenv(env_names[i]);\n-\t\torig_env[i] = xstrdup_or_null(orig_env[i]);\n-\t}\n-}\n-\n-static void restore_env(int external_alias)\n-{\n-\tint i;\n-\n-\tassert(save_restore_env_balance == 1);\n-\tsave_restore_env_balance = 0;\n-\tif (!external_alias && orig_cwd && chdir(orig_cwd))\n-\t\tdie_errno(\"could not move to %s\", orig_cwd);\n-\tfree(orig_cwd);\n-\tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n-\t\tif (external_alias &&\n-\t\t !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))\n-\t\t\tcontinue;\n-\t\tif (orig_env[i]) {\n-\t\t\tsetenv(env_names[i], orig_env[i], 1);\n-\t\t\tfree(orig_env[i]);\n-\t\t} else {\n-\t\t\tunsetenv(env_names[i]);\n-\t\t}\n-\t}\n-}\n \n static void commit_pager_choice(void) {\n \tswitch (use_pager) {\n@@ -245,36 +201,37 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \n static int handle_alias(int *argcp, const char ***argv)\n {\n+\tstruct strbuf cdup_dir = STRBUF_INIT;\n \tint envchanged = 0, ret = 0, saved_errno = errno;\n \tint count, option_count;\n \tconst char **new_argv;\n \tconst char *alias_command;\n \tchar *alias_string;\n-\tint unused_nongit;\n-\n-\tsave_env_before_alias();\n-\tsetup_git_directory_gently(&unused_nongit);\n \n \talias_command = (*argv)[0];\n-\talias_string = alias_lookup(alias_command, NULL);\n+\talias_string = alias_lookup(alias_command, &cdup_dir);\n \tif (alias_string) {\n \t\tif (alias_string[0] == '!') {\n \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n \n+\t\t\tif (cdup_dir.len)\n+\t\t\t\tsetup_git_directory();\n+\n \t\t\tcommit_pager_choice();\n-\t\t\trestore_env(1);\n \n \t\t\tchild.use_shell = 1;\n \t\t\targv_array_push(&child.args, alias_string + 1);\n \t\t\targv_array_pushv(&child.args, (*argv) + 1);\n \n \t\t\tret = run_command(&child);\n+\t\t\tstrbuf_release(&cdup_dir);\n \t\t\tif (ret >= 0) /* normal exit */\n \t\t\t\texit(ret);\n \n \t\t\tdie_errno(\"While expanding alias '%s': '%s'\",\n \t\t\t alias_command, alias_string + 1);\n \t\t}\n+\t\tstrbuf_release(&cdup_dir);\n \t\tcount = split_cmdline(alias_string, &new_argv);\n \t\tif (count < 0)\n \t\t\tdie(\"Bad alias.%s string: %s\", alias_command,\n@@ -308,8 +265,6 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\tret = 1;\n \t}\n \n-\trestore_env(0);\n-\n \terrno = saved_errno;\n \n \treturn ret;\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 83881ec3a0c..20b4d83c281 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -391,7 +391,7 @@ test_expect_success TTY 'core.pager in repo config works and retains cwd' '\n \t)\n '\n \n-test_expect_failure TTY 'core.pager is found via alias in subdirectory' '\n+test_expect_success TTY 'core.pager is found via alias in subdirectory' '\n \tsane_unset GIT_PAGER &&\n \ttest_config core.pager \"cat >via-alias\" &&\n \t(\n-- \n2.13.0.windows.1.460.g13f583bedb5\n"},{"id":"321773","messageId":"aa6601bbfa50c728b7820e0a2991da76288509a5.1496951503.git.johannes.schindelin@gmx.de","threadId":"46147","inReplyTo":"cover.1496951503.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 7/8] alias_lookup(): optionally return top-level directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T19:53:50Z","receivedAt":"2017-06-08T19:54:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When an alias expands to a shell command, and when we are inside a\nregular worktree's subdirectory (i.e. not inside the .git/ directory nor\nin a worktree initialized via `git worktree add`), and only then, the\ncurrent working directory is switched to the top-level directory of the\nworktree before running the expanded alias.\n\nWhile this behavior is somewhat confusing, it is well-established and\nneeds to be preserved, even with the upcoming change to use the early\nconfig machinery to expand aliases.\n\nThat means that alias_lookup() needs to determine the working directory\nto switch to, in case the alias turns out to expand to a shall command.\n\nSigned-off-by: Johannes Schindelin \n---\n alias.c | 33 +++++++++++++++++++++++++--------\n builtin/help.c | 2 +-\n cache.h | 2 +-\n git.c | 2 +-\n 4 files changed, 28 insertions(+), 11 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex 3b90397a99d..b7c4a4f0217 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -1,14 +1,31 @@\n #include \"cache.h\"\n \n-char *alias_lookup(const char *alias)\n+struct config_alias_data\n {\n-\tchar *v = NULL;\n-\tstruct strbuf key = STRBUF_INIT;\n-\tstrbuf_addf(&key, \"alias.%s\", alias);\n-\tif (git_config_key_is_valid(key.buf))\n-\t\tgit_config_get_string(key.buf, &v);\n-\tstrbuf_release(&key);\n-\treturn v;\n+\tstruct strbuf key;\n+\tchar *v;\n+};\n+\n+static int config_alias_cb(const char *key, const char *value, void *d)\n+{\n+\tstruct config_alias_data *data = d;\n+\n+\tif (!strcmp(key, data->key.buf))\n+\t\treturn git_config_string((const char **)&data->v, key, value);\n+\n+\treturn 0;\n+}\n+\n+char *alias_lookup(const char *alias, struct strbuf *cdup_dir)\n+{\n+\tstruct config_alias_data data = { STRBUF_INIT, NULL };\n+\n+\tstrbuf_addf(&data.key, \"alias.%s\", alias);\n+\tif (git_config_key_is_valid(data.key.buf))\n+\t\tread_early_config(config_alias_cb, &data, cdup_dir);\n+\tstrbuf_release(&data.key);\n+\n+\treturn data.v;\n }\n \n #define SPLIT_CMDLINE_BAD_ENDING 1\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 49f7a07f85d..6f208ff1ab3 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -435,7 +435,7 @@ static const char *check_git_cmd(const char* cmd)\n \tif (is_git_command(cmd))\n \t\treturn cmd;\n \n-\talias = alias_lookup(cmd);\n+\talias = alias_lookup(cmd, NULL);\n \tif (alias) {\n \t\tprintf_ln(_(\"`git %s' is aliased to `%s'\"), cmd, alias);\n \t\tfree(alias);\ndiff --git a/cache.h b/cache.h\nindex f769ef1779c..65f2e5bf04c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -2189,7 +2189,7 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);\n /* ls-files */\n void overlay_tree_on_cache(const char *tree_name, const char *prefix);\n \n-char *alias_lookup(const char *alias);\n+char *alias_lookup(const char *alias, struct strbuf *cdup_dir);\n int split_cmdline(char *cmdline, const char ***argv);\n /* Takes a negative value returned by split_cmdline */\n const char *split_cmdline_strerror(int cmdline_errno);\ndiff --git a/git.c b/git.c\nindex 8ff44f081d4..4163beaead4 100644\n--- a/git.c\n+++ b/git.c\n@@ -256,7 +256,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \tsetup_git_directory_gently(&unused_nongit);\n \n \talias_command = (*argv)[0];\n-\talias_string = alias_lookup(alias_command);\n+\talias_string = alias_lookup(alias_command, NULL);\n \tif (alias_string) {\n \t\tif (alias_string[0] == '!') {\n \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n-- \n2.13.0.windows.1.460.g13f583bedb5\n\n\n"},{"id":"321776","messageId":"20170608223237.GB121539@google.com","threadId":"46147","inReplyTo":"cover.1496951503.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 0/8] Avoid problem where git_dir is set after alias expansion","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-08T22:32:37Z","receivedAt":"2017-06-08T22:32:45Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/08, Johannes Schindelin wrote:\n> \n> Changes since v1:\n> \n> - 1/8's commit message clarifies why the other early return in\n> discover_git_directory() does not need an equivalent resetting of\n> git_dir.\n> \n> - 3/8's commit message fixes awkward language (thanks, Brandon!).\n> \n> - the `worktree_dir` variables/parameters have been renamed to\n> `cdup_dir` to clarify that they only get populated if the search for\n> the .git/ directory determined that the current working directory\n> is a subdirectory of the directory containing .git/.\n> \n> \n\nv2 addresses some of the comments I had with v1, so everything looks\ngood to me.\n\n> Johannes Schindelin (8):\n> discover_git_directory(): avoid setting invalid git_dir\n> config: report correct line number upon error\n> help: use early config when autocorrecting aliases\n> read_early_config(): optionally return the worktree's top-level\n> directory\n> t1308: relax the test verifying that empty alias values are disallowed\n> t7006: demonstrate a problem with aliases in subdirectories\n> alias_lookup(): optionally return top-level directory\n> Use the early config machinery to expand aliases\n> \n> alias.c | 33 +++++++++++++++++++++-------\n> builtin/help.c | 2 +-\n> cache.h | 7 +++---\n> config.c | 7 +++---\n> git.c | 59 ++++++--------------------------------------------\n> help.c | 2 +-\n> pager.c | 4 ++--\n> setup.c | 13 +++++++++--\n> t/helper/test-config.c | 2 +-\n> t/t1308-config-set.sh | 4 +++-\n> t/t7006-pager.sh | 11 ++++++++++\n> 11 files changed, 70 insertions(+), 74 deletions(-)\n\n-- \nBrandon Williams\n"},{"id":"321876","messageId":"xmqq37b84g9t.fsf@gitster.mtv.corp.google.com","threadId":"46147","inReplyTo":"b9e212d508e9103037da2070cc0aaed530ea7473.1496951503.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 5/8] t1308: relax the test verifying that empty alias values are disallowed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-10T04:29:18Z","receivedAt":"2017-06-10T04:29:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> We are about to change the way aliases are expanded, to use the early\n> config machinery.\n>\n> This machinery reports errors in a slightly different manner than the\n> cached config machinery.\n>\n> Let's not get hung up by the precise wording of the message mentioning\n> the lin number. It is really sufficient to verify that all the relevant\n> information is given to the user.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> t/t1308-config-set.sh | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\n> index ff50960ccae..69a0aa56d6d 100755\n> --- a/t/t1308-config-set.sh\n> +++ b/t/t1308-config-set.sh\n> @@ -215,7 +215,9 @@ test_expect_success 'check line errors for malformed values' '\n> \t\tbr\n> \tEOF\n> \ttest_expect_code 128 git br 2>result &&\n> -\ttest_i18ngrep \"fatal: .*alias\\.br.*\\.git/config.*line 2\" result\n> +\ttest_i18ngrep \"missing value for .alias\\.br\" result &&\n> +\ttest_i18ngrep \"fatal: .*\\.git/config\" result &&\n> +\ttest_i18ngrep \"fatal: .*line 2\" result\n\nJust judging from the looks of these, it appears that the updated\nmessages are more descriptive than the original. Am I mistaken?\n\nIf so I think the log message undersells the changes made by the\nseries by sounding as if you are saying \"we are too lazy to bother\ngiving the same message, so here is a workaround\".\n\nIn any case, s/lin number/line number/ is needed ;-)\n"},{"id":"321895","messageId":"20170610084015.ej62tkw7gb26z4no@sigill.intra.peff.net","threadId":"46147","inReplyTo":"effd58a564e54ccd3d695d91bc9c4f32255b8ab7.1496951503.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 1/8] discover_git_directory(): avoid setting invalid git_dir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-10T08:40:16Z","receivedAt":"2017-06-10T08:40:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 08, 2017 at 09:53:32PM +0200, Johannes Schindelin wrote:\n\n> When discovering a .git/ directory, we take pains to ensure that its\n> repository format version matches Git's expectations, and we return NULL\n> otherwise.\n> \n> However, we still appended the invalid path to the strbuf passed as\n> argument.\n> \n> Let's just reset the strbuf to the state before we appended the .git/\n> directory that was eventually rejected.\n> \n> There is another early return path in that function, when\n> setup_git_directory_gently_1() returns GIT_DIR_NONE or an error. In that\n> case, the gitdir parameter has not been touched, therefore there is no\n> need for an equivalent change in that code path.\n\nAfter reading this, I thought at first this was a user-visible (albeit\nobscure) bug where something like:\n\n git init\n git init bogus\n cd bogus\n git config core.repositoryformatversion 5\n\nwould mean that early-config barfs on seeing our bogus \".git\", but that\nscrews up the discovery going back to the real repository root. But\nthat's not the case, because we always stop walking anyway when we see\nsuch a bogus git dir.\n\nSo it's just a cleanup (though obviously a sensible one).\n\n-Peff\n"},{"id":"321899","messageId":"20170610090424.zibu4sgd2lr7ghfk@sigill.intra.peff.net","threadId":"46147","inReplyTo":"291e8b643990aee04654b34c6f953387c3c030d4.1496951503.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 2/8] config: report correct line number upon error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-10T09:04:24Z","receivedAt":"2017-06-10T09:04:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 08, 2017 at 09:53:35PM +0200, Johannes Schindelin wrote:\n\n> When get_value() parses a key/value pair, it is possible that the line\n> number is decreased (because the \\n has been consumed already) before the\n> key/value pair is passed to the callback function, to allow for the\n> correct line to be attributed in case of an error.\n> \n> However, when git_parse_source() asks get_value() to parse the key/value\n> pair, the error reporting is performed *after* get_value() returns.\n> \n> Which means that we have to be careful not to increase the line number\n> in get_value() after the callback function returned an error.\n\nGood catch. Would we want a test here, like:\n\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 13b7851f7..4cad8366a 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -703,6 +703,12 @@ test_expect_success 'invalid unit' '\n \ttest_i18ngrep \"bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit\" actual\n '\n \n+test_expect_success 'invalid path' '\n+\techo \"[bool]var\" >invalid &&\n+\ttest_must_fail git config -f invalid --path bool.var 2>actual &&\n+\ttest_i18ngrep \"line 1\" actual\n+'\n+\n test_expect_success 'invalid stdin config' '\n \techo \"[broken\" | test_must_fail git config --list --file - >output 2>&1 &&\n \ttest_i18ngrep \"bad config line 1 in standard input\" output\n\nwhich currently reports \"line 2\" instead of line 1.\n\nI wondered if the same bug could be found earlier (e.g,. from\nparse_value() when it sees an unpaired quote), but it looks like we do a\ncf->linenr-- rollback there already.\n\n> diff --git a/config.c b/config.c\n> index 146cb3452ad..9b88531a70d 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -604,7 +604,8 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n> \t */\n> \tcf->linenr--;\n> \tret = fn(name->buf, value, data);\n> -\tcf->linenr++;\n> +\tif (!ret)\n> +\t\tcf->linenr++;\n> \treturn ret;\n> }\n\nI think this should be \"if (ret < 0)\". The caller only considers it an\nerror if get_value() returns a negative number. As you have it here, I\nthink a config callback which returned a positive number would end up\nwith nonsense line numbers.\n\n-Peff\n"},{"id":"321900","messageId":"20170610090536.zqsiqi72ffpqt2ue@sigill.intra.peff.net","threadId":"46147","inReplyTo":"2be3d75cd3341c1386f97c77cc63bec89c1f6a47.1496951503.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 3/8] help: use early config when autocorrecting aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-10T09:05:36Z","receivedAt":"2017-06-10T09:05:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 08, 2017 at 09:53:38PM +0200, Johannes Schindelin wrote:\n\n> Git has this feature which suggests similar commands (including aliases)\n> in case the user specified an unknown command.\n> \n> This feature currently relies on a side effect of the way we expand\n> aliases right now: when a command is not a builtin, we use the regular\n> config machinery (meaning: discovering the .git/ directory and\n> initializing global state such as the config cache) to see whether the\n> command refers to an alias.\n> \n> However, we will change the way aliases are expanded in the next\n> commits, to use the early config instead. That means that the\n> autocorrect feature can no longer discover the available aliases by\n> looking at the config cache (because it has not yet been initialized).\n> \n> So let's just use the early config machinery instead.\n> \n> This is slightly less performant than the previous way, as the early\n> config is used *twice*: once to see whether the command refers to an\n> alias, and then to see what aliases are most similar. However, this is\n> hardly a performance-critical code path, so performance is less important\n> here.\n\nGood explanation, and the patch looks obviously correct.\n\n-Peff\n"},{"id":"321903","messageId":"20170610094423.kscoqrljxjki3np4@sigill.intra.peff.net","threadId":"46147","inReplyTo":"0cef47383c0dd2287ffda2a6eaf1f430ed3ee726.1496951503.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 4/8] read_early_config(): optionally return the worktree's top-level directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-10T09:44:24Z","receivedAt":"2017-06-10T09:44:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 08, 2017 at 09:53:41PM +0200, Johannes Schindelin wrote:\n\n> So far, when we invoked the early config code path, we implicitly\n> determined the top-level directory of the worktree while discovering the\n> .git/ directory.\n> \n> And then we simply forgot that information.\n> \n> However, when we expand aliases, we very much need that information, as\n> aliases expanding to shell commands, i.e. whose value starts with an\n> exclamation point, have to be executed in the top-level directory of the\n> worktree. There are exceptions abound, not only with non-shell aliases\n> (which are supposed to be executed in the original working directory\n> instead), but also when being started inside the .git/ directory or in a\n> worktree created via `git worktree add`.\n\nI understand why you wrote it this way, but it really feels backwards.\nWe're calling a function that's about config, and oh by the way\nsometimes it tells us about which git directory we're in (and sometimes\nnot because it didn't actually run discover_git_directory anyway).\n\nIt would make a lot more sense to me if we told read_early_config() \"oh\nby the way, I discovered the git directory already, here it is\".\n\nThe same applies to the later patch to pass the information through\nalias_lookup().\n\nTaking a step back, this is really just an optimization, right? The\ncleanest thing would be for the alias code, right before launching a\nshell alias, to discover_git_directory(). But you don't want to do that\nbecause it's too expensive?\n\nIf so, my questions would be:\n\n 1. Is it actually that expensive, or is this premature optimization?\n We are, after all, about to fork and execute a shell. Have we\n measured?\n\n 2. Can we cache the results of discovery_git_directory() between\n calls? That would not only fix your issue here, but it would\n optimize the calls we make when we call read_early_config() for\n multiple items (e.g., both pager and alias config).\n\n The only trick is making sure our previous value is still valid.\n I suspect it would work to just ignore this, as we only chdir when\n doing setup_git_directory(), and that should generally take\n precedence over discover_git_directory() being called at all. But\n for extra safety, we should be able to key it to the cwd, like:\n\n strbuf_getcwd(&now_cwd);\n if (!strbuf_cmp(&cached_cwd, &now_cwd) {\n strbuf_addbuf(cdup_dir, &cached_cdup_dir));\n return xstrdup(cached_gitdir);\n }\n strbuf_swap(&cached_cwd, &now_cwd);\n strbuf_release(&now_cwd);\n ... rest of function, but store result in cached_gitdir ...\n\n-Peff\n"},{"id":"321904","messageId":"20170610100730.j62kh2hayt6zknr5@sigill.intra.peff.net","threadId":"46147","inReplyTo":"af40ce088de03130a5edfae187bb8274b5a9d3a9.1496951503.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 8/8] Use the early config machinery to expand aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-10T10:07:30Z","receivedAt":"2017-06-10T10:07:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 08, 2017 at 09:53:53PM +0200, Johannes Schindelin wrote:\n\n> @@ -245,36 +201,37 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> \n> static int handle_alias(int *argcp, const char ***argv)\n> {\n> +\tstruct strbuf cdup_dir = STRBUF_INIT;\n> \tint envchanged = 0, ret = 0, saved_errno = errno;\n> \tint count, option_count;\n> \tconst char **new_argv;\n> \tconst char *alias_command;\n> \tchar *alias_string;\n> -\tint unused_nongit;\n> -\n> -\tsave_env_before_alias();\n> -\tsetup_git_directory_gently(&unused_nongit);\n> \n> \talias_command = (*argv)[0];\n> -\talias_string = alias_lookup(alias_command, NULL);\n> +\talias_string = alias_lookup(alias_command, &cdup_dir);\n> \tif (alias_string) {\n> \t\tif (alias_string[0] == '!') {\n> \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n> \n> +\t\t\tif (cdup_dir.len)\n> +\t\t\t\tsetup_git_directory();\n> +\n\nI'm really confused here. We went to all the trouble to record the\ncdup_dir as part of the alias lookup so that we could make sure to\nchdir() to it. But we don't seem to ever look at its value. We just use\nit as a proxy for \"did we find a git-dir\". And if we did, we go to all\nthe trouble to re-discover it via setup_git_directory().\n\nI understand why you must use setup_git_directory() and not just a\nchdir(), because the latter sets up variables like GIT_PREFIX and\nGIT_DIR that the shell alias may be depending on.\n\nBut couldn't we just unconditionally do:\n\n setup_git_directory_gently();\n\nhere to move into the top-level if there is one, without caring about\ncdup_dir at all?\n\nThat should be equally correct. It does do an extra discovery when we\nalready know that there isn't a gitdir at all. But my earlier comments\nabout optimizing apply (measure, and possibly cache). And in the common\ncase that we are in a repo, it's exactly the same, since the setup call\nhere would re-discover from scratch (though again, if we care, caching\ncould solve that).\n\n-Peff\n"},{"id":"321905","messageId":"20170610101057.yeemax73c2t6truz@sigill.intra.peff.net","threadId":"46147","inReplyTo":"20170610100730.j62kh2hayt6zknr5@sigill.intra.peff.net","subject":"Re: [PATCH v2 8/8] Use the early config machinery to expand aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-10T10:10:57Z","receivedAt":"2017-06-10T10:11:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 10, 2017 at 06:07:30AM -0400, Jeff King wrote:\n\n> But couldn't we just unconditionally do:\n> \n> setup_git_directory_gently();\n> \n> here to move into the top-level if there is one, without caring about\n> cdup_dir at all?\n\nIOW, drop your patch 4, and then squash patches 7 and 8 into this:\n\ndiff --git a/alias.c b/alias.c\nindex 3b90397a9..6bdc93630 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -1,14 +1,31 @@\n #include \"cache.h\"\n \n+struct config_alias_data\n+{\n+\tstruct strbuf key;\n+\tchar *v;\n+};\n+\n+static int config_alias_cb(const char *key, const char *value, void *d)\n+{\n+\tstruct config_alias_data *data = d;\n+\n+\tif (!strcmp(key, data->key.buf))\n+\t\treturn git_config_string((const char **)&data->v, key, value);\n+\n+\treturn 0;\n+}\n+\n char *alias_lookup(const char *alias)\n {\n-\tchar *v = NULL;\n-\tstruct strbuf key = STRBUF_INIT;\n-\tstrbuf_addf(&key, \"alias.%s\", alias);\n-\tif (git_config_key_is_valid(key.buf))\n-\t\tgit_config_get_string(key.buf, &v);\n-\tstrbuf_release(&key);\n-\treturn v;\n+\tstruct config_alias_data data = { STRBUF_INIT, NULL };\n+\n+\tstrbuf_addf(&data.key, \"alias.%s\", alias);\n+\tif (git_config_key_is_valid(data.key.buf))\n+\t\tread_early_config(config_alias_cb, &data);\n+\tstrbuf_release(&data.key);\n+\n+\treturn data.v;\n }\n \n #define SPLIT_CMDLINE_BAD_ENDING 1\ndiff --git a/git.c b/git.c\nindex 8ff44f081..4e556fa9a 100644\n--- a/git.c\n+++ b/git.c\n@@ -16,50 +16,6 @@ const char git_more_info_string[] =\n \t \"to read about a specific subcommand or concept.\");\n \n static int use_pager = -1;\n-static char *orig_cwd;\n-static const char *env_names[] = {\n-\tGIT_DIR_ENVIRONMENT,\n-\tGIT_WORK_TREE_ENVIRONMENT,\n-\tGIT_IMPLICIT_WORK_TREE_ENVIRONMENT,\n-\tGIT_PREFIX_ENVIRONMENT\n-};\n-static char *orig_env[4];\n-static int save_restore_env_balance;\n-\n-static void save_env_before_alias(void)\n-{\n-\tint i;\n-\n-\tassert(save_restore_env_balance == 0);\n-\tsave_restore_env_balance = 1;\n-\torig_cwd = xgetcwd();\n-\tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n-\t\torig_env[i] = getenv(env_names[i]);\n-\t\torig_env[i] = xstrdup_or_null(orig_env[i]);\n-\t}\n-}\n-\n-static void restore_env(int external_alias)\n-{\n-\tint i;\n-\n-\tassert(save_restore_env_balance == 1);\n-\tsave_restore_env_balance = 0;\n-\tif (!external_alias && orig_cwd && chdir(orig_cwd))\n-\t\tdie_errno(\"could not move to %s\", orig_cwd);\n-\tfree(orig_cwd);\n-\tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n-\t\tif (external_alias &&\n-\t\t !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))\n-\t\t\tcontinue;\n-\t\tif (orig_env[i]) {\n-\t\t\tsetenv(env_names[i], orig_env[i], 1);\n-\t\t\tfree(orig_env[i]);\n-\t\t} else {\n-\t\t\tunsetenv(env_names[i]);\n-\t\t}\n-\t}\n-}\n \n static void commit_pager_choice(void) {\n \tswitch (use_pager) {\n@@ -250,19 +206,17 @@ static int handle_alias(int *argcp, const char ***argv)\n \tconst char **new_argv;\n \tconst char *alias_command;\n \tchar *alias_string;\n-\tint unused_nongit;\n-\n-\tsave_env_before_alias();\n-\tsetup_git_directory_gently(&unused_nongit);\n \n \talias_command = (*argv)[0];\n \talias_string = alias_lookup(alias_command);\n \tif (alias_string) {\n \t\tif (alias_string[0] == '!') {\n+\t\t\tint nongit_ok;\n \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n \n+\t\t\tsetup_git_directory_gently(&nongit_ok);\n+\n \t\t\tcommit_pager_choice();\n-\t\t\trestore_env(1);\n \n \t\t\tchild.use_shell = 1;\n \t\t\targv_array_push(&child.args, alias_string + 1);\n@@ -308,8 +262,6 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\tret = 1;\n \t}\n \n-\trestore_env(0);\n-\n \terrno = saved_errno;\n \n \treturn ret;\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 83881ec3a..20b4d83c2 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -391,7 +391,7 @@ test_expect_success TTY 'core.pager in repo config works and retains cwd' '\n \t)\n '\n \n-test_expect_failure TTY 'core.pager is found via alias in subdirectory' '\n+test_expect_success TTY 'core.pager is found via alias in subdirectory' '\n \tsane_unset GIT_PAGER &&\n \ttest_config core.pager \"cat >via-alias\" &&\n \t(\n"},{"id":"321907","messageId":"20170610101813.n26wmxcueicecptz@sigill.intra.peff.net","threadId":"46147","inReplyTo":"aa6601bbfa50c728b7820e0a2991da76288509a5.1496951503.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-10T10:18:13Z","receivedAt":"2017-06-10T10:18:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 08, 2017 at 09:53:50PM +0200, Johannes Schindelin wrote:\n\n> -char *alias_lookup(const char *alias)\n> [...]\n> {\n> -\tchar *v = NULL;\n> -\tstruct strbuf key = STRBUF_INIT;\n> -\tstrbuf_addf(&key, \"alias.%s\", alias);\n> -\tif (git_config_key_is_valid(key.buf))\n> -\t\tgit_config_get_string(key.buf, &v);\n> -\tstrbuf_release(&key);\n> -\treturn v;\n> +\tstruct strbuf key;\n> +\tchar *v;\n> +};\n> [...]\n> +char *alias_lookup(const char *alias, struct strbuf *cdup_dir)\n> +{\n> +\tstruct config_alias_data data = { STRBUF_INIT, NULL };\n> +\n> +\tstrbuf_addf(&data.key, \"alias.%s\", alias);\n> +\tif (git_config_key_is_valid(data.key.buf))\n> +\t\tread_early_config(config_alias_cb, &data, cdup_dir);\n> +\tstrbuf_release(&data.key);\n> +\n> +\treturn data.v;\n> }\n\nTwo optional cleanups here:\n\n 1. We don't need to call config_key_is_valid when using a callback. We\n only needed that to prevent the configset machinery from issuing a\n warning. It does save us reading the config entirely when the\n program name is syntactically invalid as an alias, but that's a\n pretty rare case.\n\n 2. Now that we're not using the configset machinery, we don't need to\n have the alias name as a full string. Instead of using the strbuf,\n we could just pass the \"alias\" string itself and do:\n\n if (skip_prefix(var, \"alias.\", &key) && !strcmp(key, data->key))\n\n in the callback.\n\nI don't think either is a big deal, though.\n\n-Peff\n"},{"id":"321989","messageId":"xmqq1sqpndhg.fsf@gitster.mtv.corp.google.com","threadId":"46147","inReplyTo":"20170610101057.yeemax73c2t6truz@sigill.intra.peff.net","subject":"Re: [PATCH v2 8/8] Use the early config machinery to expand aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-12T20:43:39Z","receivedAt":"2017-06-12T20:43:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Jun 10, 2017 at 06:07:30AM -0400, Jeff King wrote:\n>\n>> But couldn't we just unconditionally do:\n>> \n>> setup_git_directory_gently();\n>> \n>> here to move into the top-level if there is one, without caring about\n>> cdup_dir at all?\n>\n> IOW, drop your patch 4, and then squash patches 7 and 8 into this:\n\nThat looks a lot simpler.\n\n> diff --git a/alias.c b/alias.c\n> index 3b90397a9..6bdc93630 100644\n> --- a/alias.c\n> +++ b/alias.c\n> @@ -1,14 +1,31 @@\n> #include \"cache.h\"\n> \n> +struct config_alias_data\n> +{\n> +\tstruct strbuf key;\n> +\tchar *v;\n> +};\n> +\n> +static int config_alias_cb(const char *key, const char *value, void *d)\n> +{\n> +\tstruct config_alias_data *data = d;\n> +\n> +\tif (!strcmp(key, data->key.buf))\n> +\t\treturn git_config_string((const char **)&data->v, key, value);\n> +\n> +\treturn 0;\n> +}\n> +\n> char *alias_lookup(const char *alias)\n> {\n> -\tchar *v = NULL;\n> -\tstruct strbuf key = STRBUF_INIT;\n> -\tstrbuf_addf(&key, \"alias.%s\", alias);\n> -\tif (git_config_key_is_valid(key.buf))\n> -\t\tgit_config_get_string(key.buf, &v);\n> -\tstrbuf_release(&key);\n> -\treturn v;\n> +\tstruct config_alias_data data = { STRBUF_INIT, NULL };\n> +\n> +\tstrbuf_addf(&data.key, \"alias.%s\", alias);\n> +\tif (git_config_key_is_valid(data.key.buf))\n> +\t\tread_early_config(config_alias_cb, &data);\n> +\tstrbuf_release(&data.key);\n> +\n> +\treturn data.v;\n> }\n> \n> #define SPLIT_CMDLINE_BAD_ENDING 1\n> diff --git a/git.c b/git.c\n> index 8ff44f081..4e556fa9a 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -16,50 +16,6 @@ const char git_more_info_string[] =\n> \t \"to read about a specific subcommand or concept.\");\n> \n> static int use_pager = -1;\n> -static char *orig_cwd;\n> -static const char *env_names[] = {\n> -\tGIT_DIR_ENVIRONMENT,\n> -\tGIT_WORK_TREE_ENVIRONMENT,\n> -\tGIT_IMPLICIT_WORK_TREE_ENVIRONMENT,\n> -\tGIT_PREFIX_ENVIRONMENT\n> -};\n> -static char *orig_env[4];\n> -static int save_restore_env_balance;\n> -\n> -static void save_env_before_alias(void)\n> -{\n> -\tint i;\n> -\n> -\tassert(save_restore_env_balance == 0);\n> -\tsave_restore_env_balance = 1;\n> -\torig_cwd = xgetcwd();\n> -\tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n> -\t\torig_env[i] = getenv(env_names[i]);\n> -\t\torig_env[i] = xstrdup_or_null(orig_env[i]);\n> -\t}\n> -}\n> -\n> -static void restore_env(int external_alias)\n> -{\n> -\tint i;\n> -\n> -\tassert(save_restore_env_balance == 1);\n> -\tsave_restore_env_balance = 0;\n> -\tif (!external_alias && orig_cwd && chdir(orig_cwd))\n> -\t\tdie_errno(\"could not move to %s\", orig_cwd);\n> -\tfree(orig_cwd);\n> -\tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n> -\t\tif (external_alias &&\n> -\t\t !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))\n> -\t\t\tcontinue;\n> -\t\tif (orig_env[i]) {\n> -\t\t\tsetenv(env_names[i], orig_env[i], 1);\n> -\t\t\tfree(orig_env[i]);\n> -\t\t} else {\n> -\t\t\tunsetenv(env_names[i]);\n> -\t\t}\n> -\t}\n> -}\n> \n> static void commit_pager_choice(void) {\n> \tswitch (use_pager) {\n> @@ -250,19 +206,17 @@ static int handle_alias(int *argcp, const char ***argv)\n> \tconst char **new_argv;\n> \tconst char *alias_command;\n> \tchar *alias_string;\n> -\tint unused_nongit;\n> -\n> -\tsave_env_before_alias();\n> -\tsetup_git_directory_gently(&unused_nongit);\n> \n> \talias_command = (*argv)[0];\n> \talias_string = alias_lookup(alias_command);\n> \tif (alias_string) {\n> \t\tif (alias_string[0] == '!') {\n> +\t\t\tint nongit_ok;\n> \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n> \n> +\t\t\tsetup_git_directory_gently(&nongit_ok);\n> +\n> \t\t\tcommit_pager_choice();\n> -\t\t\trestore_env(1);\n> \n> \t\t\tchild.use_shell = 1;\n> \t\t\targv_array_push(&child.args, alias_string + 1);\n> @@ -308,8 +262,6 @@ static int handle_alias(int *argcp, const char ***argv)\n> \t\tret = 1;\n> \t}\n> \n> -\trestore_env(0);\n> -\n> \terrno = saved_errno;\n> \n> \treturn ret;\n> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n> index 83881ec3a..20b4d83c2 100755\n> --- a/t/t7006-pager.sh\n> +++ b/t/t7006-pager.sh\n> @@ -391,7 +391,7 @@ test_expect_success TTY 'core.pager in repo config works and retains cwd' '\n> \t)\n> '\n> \n> -test_expect_failure TTY 'core.pager is found via alias in subdirectory' '\n> +test_expect_success TTY 'core.pager is found via alias in subdirectory' '\n> \tsane_unset GIT_PAGER &&\n> \ttest_config core.pager \"cat >via-alias\" &&\n> \t(\n"},{"id":"322067","messageId":"alpine.DEB.2.21.1.1706131244150.171564@virtualbox","threadId":"46147","inReplyTo":"xmqq37b84g9t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 5/8] t1308: relax the test verifying that empty alias values are disallowed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-13T10:50:16Z","receivedAt":"2017-06-13T10:50:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sat, 10 Jun 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > We are about to change the way aliases are expanded, to use the early\n> > config machinery.\n> >\n> > This machinery reports errors in a slightly different manner than the\n> > cached config machinery.\n> >\n> > Let's not get hung up by the precise wording of the message mentioning\n> > the lin number. It is really sufficient to verify that all the relevant\n> > information is given to the user.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > t/t1308-config-set.sh | 4 +++-\n> > 1 file changed, 3 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\n> > index ff50960ccae..69a0aa56d6d 100755\n> > --- a/t/t1308-config-set.sh\n> > +++ b/t/t1308-config-set.sh\n> > @@ -215,7 +215,9 @@ test_expect_success 'check line errors for malformed values' '\n> > \t\tbr\n> > \tEOF\n> > \ttest_expect_code 128 git br 2>result &&\n> > -\ttest_i18ngrep \"fatal: .*alias\\.br.*\\.git/config.*line 2\" result\n> > +\ttest_i18ngrep \"missing value for .alias\\.br\" result &&\n> > +\ttest_i18ngrep \"fatal: .*\\.git/config\" result &&\n> > +\ttest_i18ngrep \"fatal: .*line 2\" result\n> \n> Just judging from the looks of these, it appears that the updated\n> messages are more descriptive than the original. Am I mistaken?\n\nSadly, I do not think so. It is just different, not better. Maybe less\nredundant... See for yourself:\n\nBefore:\n\n\terror: missing value for 'alias.br'\n\tfatal: bad config variable 'alias.br' in file '.git/config' at line 2\n\nAfter:\n\n\terror: missing value for 'alias.br'\n\tfatal: bad config line 2 in file .git/config\n\n> If so I think the log message undersells the changes made by the\n> series by sounding as if you are saying \"we are too lazy to bother\n> giving the same message, so here is a workaround\".\n\nI fear this is not underselling anything for an improvement.\n\nThe real fix would indeed be (as mentioned by Brandon elsewhere) to unify\nthe code paths between the cached and the non-cached config machinery, so\nas to provide the exact same error message in this case.\n\nIt is annoying that I lack the time to make that happen. My official\nexcuse, though, is that that fix is outside the purpose of this patch\nseries.\n\n> In any case, s/lin number/line number/ is needed ;-)\n\nTru. ;-)\n\nCia,\nDsch\n"},{"id":"322068","messageId":"alpine.DEB.2.21.1.1706131256400.171564@virtualbox","threadId":"46147","inReplyTo":"20170610090424.zibu4sgd2lr7ghfk@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/8] config: report correct line number upon error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-13T11:02:49Z","receivedAt":"2017-06-13T11:03:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sat, 10 Jun 2017, Jeff King wrote:\n\n> On Thu, Jun 08, 2017 at 09:53:35PM +0200, Johannes Schindelin wrote:\n> \n> > When get_value() parses a key/value pair, it is possible that the line\n> > number is decreased (because the \\n has been consumed already) before the\n> > key/value pair is passed to the callback function, to allow for the\n> > correct line to be attributed in case of an error.\n> > \n> > However, when git_parse_source() asks get_value() to parse the key/value\n> > pair, the error reporting is performed *after* get_value() returns.\n> > \n> > Which means that we have to be careful not to increase the line number\n> > in get_value() after the callback function returned an error.\n> \n> Good catch. Would we want a test here, like:\n> \n> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> index 13b7851f7..4cad8366a 100755\n> --- a/t/t1300-repo-config.sh\n> +++ b/t/t1300-repo-config.sh\n> @@ -703,6 +703,12 @@ test_expect_success 'invalid unit' '\n> \ttest_i18ngrep \"bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit\" actual\n> '\n> \n> +test_expect_success 'invalid path' '\n> +\techo \"[bool]var\" >invalid &&\n> +\ttest_must_fail git config -f invalid --path bool.var 2>actual &&\n> +\ttest_i18ngrep \"line 1\" actual\n> +'\n> +\n> test_expect_success 'invalid stdin config' '\n> \techo \"[broken\" | test_must_fail git config --list --file - >output 2>&1 &&\n> \ttest_i18ngrep \"bad config line 1 in standard input\" output\n> \n> which currently reports \"line 2\" instead of line 1.\n\nMmmmkay.\n\nI am always reluctant to add *even more* stuff to the test suite, in\nparticular since my patch series implicitly changes t1308 to test for this\nvery thing.\n\nBut I guess my fight against the test suite taking longer and longer and\nlonger is a fight I must lose, as I am pretty alone in that endeavor, and\nwould have have to fight fellow developers. So I guess it is time to give\nup.\n\nI added the test case you suggested, with a title that I find a bit more\ndescriptive.\n\n> I wondered if the same bug could be found earlier (e.g,. from\n> parse_value() when it sees an unpaired quote), but it looks like we do a\n> cf->linenr-- rollback there already.\n\nRight.\n\nThe underlying bug, of course, is that we use a different machinery to\nhandle cached vs non-cached config, but that is a different story.\n\n> > diff --git a/config.c b/config.c\n> > index 146cb3452ad..9b88531a70d 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -604,7 +604,8 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n> > \t */\n> > \tcf->linenr--;\n> > \tret = fn(name->buf, value, data);\n> > -\tcf->linenr++;\n> > +\tif (!ret)\n> > +\t\tcf->linenr++;\n> > \treturn ret;\n> > }\n> \n> I think this should be \"if (ret < 0)\". The caller only considers it an\n> error if get_value() returns a negative number. As you have it here, I\n> think a config callback which returned a positive number would end up\n> with nonsense line numbers.\n\nI think you are half-correct: it should be `if (ret >= 0)` (the linenr\nneeds to be modified back in case of success, not in case of failure, in\ncase of failure there will be some reporting going on that needs the same\nline number as `fn()` had seen).\n\nCiao,\nDscho\n"},{"id":"322069","messageId":"alpine.DEB.2.21.1.1706131319130.171564@virtualbox","threadId":"46147","inReplyTo":"20170610101813.n26wmxcueicecptz@sigill.intra.peff.net","subject":"Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-13T11:21:28Z","receivedAt":"2017-06-13T11:22:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sat, 10 Jun 2017, Jeff King wrote:\n\n> On Thu, Jun 08, 2017 at 09:53:50PM +0200, Johannes Schindelin wrote:\n> \n> > -char *alias_lookup(const char *alias)\n> > [...]\n> > {\n> > -\tchar *v = NULL;\n> > -\tstruct strbuf key = STRBUF_INIT;\n> > -\tstrbuf_addf(&key, \"alias.%s\", alias);\n> > -\tif (git_config_key_is_valid(key.buf))\n> > -\t\tgit_config_get_string(key.buf, &v);\n> > -\tstrbuf_release(&key);\n> > -\treturn v;\n> > +\tstruct strbuf key;\n> > +\tchar *v;\n> > +};\n> > [...]\n> > +char *alias_lookup(const char *alias, struct strbuf *cdup_dir)\n> > +{\n> > +\tstruct config_alias_data data = { STRBUF_INIT, NULL };\n> > +\n> > +\tstrbuf_addf(&data.key, \"alias.%s\", alias);\n> > +\tif (git_config_key_is_valid(data.key.buf))\n> > +\t\tread_early_config(config_alias_cb, &data, cdup_dir);\n> > +\tstrbuf_release(&data.key);\n> > +\n> > +\treturn data.v;\n> > }\n> \n> Two optional cleanups here:\n> \n> 1. We don't need to call config_key_is_valid when using a callback. We\n> only needed that to prevent the configset machinery from issuing a\n> warning. It does save us reading the config entirely when the\n> program name is syntactically invalid as an alias, but that's a\n> pretty rare case.\n\nIt may be a pretty rare case, or it may not be. I do not want to think\nhard about this, so I just wanted to keep that test.\n\nBut since you suggested it, I will simply blame all the fallout (if any)\non you.\n\n;-)\n\n> 2. Now that we're not using the configset machinery, we don't need to\n> have the alias name as a full string. Instead of using the strbuf,\n> we could just pass the \"alias\" string itself and do:\n> \n> if (skip_prefix(var, \"alias.\", &key) && !strcmp(key, data->key))\n> \n> in the callback.\n\nAs you probably guessed, I had tried that first and then figured that if\nI needed to keep the config_key_is_valid() test anyway, I could just as\nwell keep the strbuf around for later use.\n\nWill change the code,\nDscho\n"},{"id":"322070","messageId":"alpine.DEB.2.21.1.1706131323330.171564@virtualbox","threadId":"46147","inReplyTo":"20170610100730.j62kh2hayt6zknr5@sigill.intra.peff.net","subject":"Re: [PATCH v2 8/8] Use the early config machinery to expand aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-13T11:25:53Z","receivedAt":"2017-06-13T11:26:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sat, 10 Jun 2017, Jeff King wrote:\n\n> On Thu, Jun 08, 2017 at 09:53:53PM +0200, Johannes Schindelin wrote:\n> \n> > @@ -245,36 +201,37 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> > \n> > static int handle_alias(int *argcp, const char ***argv)\n> > {\n> > +\tstruct strbuf cdup_dir = STRBUF_INIT;\n> > \tint envchanged = 0, ret = 0, saved_errno = errno;\n> > \tint count, option_count;\n> > \tconst char **new_argv;\n> > \tconst char *alias_command;\n> > \tchar *alias_string;\n> > -\tint unused_nongit;\n> > -\n> > -\tsave_env_before_alias();\n> > -\tsetup_git_directory_gently(&unused_nongit);\n> > \n> > \talias_command = (*argv)[0];\n> > -\talias_string = alias_lookup(alias_command, NULL);\n> > +\talias_string = alias_lookup(alias_command, &cdup_dir);\n> > \tif (alias_string) {\n> > \t\tif (alias_string[0] == '!') {\n> > \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n> > \n> > +\t\t\tif (cdup_dir.len)\n> > +\t\t\t\tsetup_git_directory();\n> > +\n> \n> I'm really confused here. We went to all the trouble to record the\n> cdup_dir as part of the alias lookup so that we could make sure to\n> chdir() to it.\n\nRight, that was my first idea.\n\nBut then the test suite taught me that we set at least GIT_PREFIX and that\ntests break (or, in other words, promises Git made earlier) if those\nvariables are not set.\n\nSo as a quick fix, I simply called setup_git_directory() instead of\nchdir(cdup_dir.buf).\n\n> I understand why you must use setup_git_directory() and not just a\n> chdir(), because the latter sets up variables like GIT_PREFIX and\n> GIT_DIR that the shell alias may be depending on.\n\nExactly.\n\n> But couldn't we just unconditionally do:\n> \n> setup_git_directory_gently();\n\nBut of course we can do that! Why on earth did I not think of that...\n\nWill change the code accordingly.\n\nCiao,\nDscho\n"},{"id":"322071","messageId":"20170613112849.vvgmwnxlrfztyf5x@sigill.intra.peff.net","threadId":"46147","inReplyTo":"alpine.DEB.2.21.1.1706131256400.171564@virtualbox","subject":"Re: [PATCH v2 2/8] config: report correct line number upon error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-13T11:28:50Z","receivedAt":"2017-06-13T11:28:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 13, 2017 at 01:02:49PM +0200, Johannes Schindelin wrote:\n\n> > +test_expect_success 'invalid path' '\n> > +\techo \"[bool]var\" >invalid &&\n> > +\ttest_must_fail git config -f invalid --path bool.var 2>actual &&\n> > +\ttest_i18ngrep \"line 1\" actual\n> > +'\n> > +\n> > test_expect_success 'invalid stdin config' '\n> > \techo \"[broken\" | test_must_fail git config --list --file - >output 2>&1 &&\n> > \ttest_i18ngrep \"bad config line 1 in standard input\" output\n> > \n> > which currently reports \"line 2\" instead of line 1.\n> \n> Mmmmkay.\n> \n> I am always reluctant to add *even more* stuff to the test suite, in\n> particular since my patch series implicitly changes t1308 to test for this\n> very thing.\n\nIf there's another test that covers this, I'm happy to use that. I just\ndidn't notice it.\n\n> > > +\tif (!ret)\n> > > +\t\tcf->linenr++;\n> > > \treturn ret;\n> > > }\n> > \n> > I think this should be \"if (ret < 0)\". The caller only considers it an\n> > error if get_value() returns a negative number. As you have it here, I\n> > think a config callback which returned a positive number would end up\n> > with nonsense line numbers.\n> \n> I think you are half-correct: it should be `if (ret >= 0)` (the linenr\n> needs to be modified back in case of success, not in case of failure, in\n> case of failure there will be some reporting going on that needs the same\n> line number as `fn()` had seen).\n\nOops, right.\n\n-Peff\n"},{"id":"322072","messageId":"20170613112940.ulkjfchkg6k7xsc5@sigill.intra.peff.net","threadId":"46147","inReplyTo":"alpine.DEB.2.21.1.1706131319130.171564@virtualbox","subject":"Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-13T11:29:41Z","receivedAt":"2017-06-13T11:29:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 13, 2017 at 01:21:28PM +0200, Johannes Schindelin wrote:\n\n> > Two optional cleanups here:\n> > \n> > 1. We don't need to call config_key_is_valid when using a callback. We\n> > only needed that to prevent the configset machinery from issuing a\n> > warning. It does save us reading the config entirely when the\n> > program name is syntactically invalid as an alias, but that's a\n> > pretty rare case.\n> \n> It may be a pretty rare case, or it may not be. I do not want to think\n> hard about this, so I just wanted to keep that test.\n> \n> But since you suggested it, I will simply blame all the fallout (if any)\n> on you.\n> \n> ;-)\n\nI accept all the blame. :)\n\n(And I was the person who added the is_valid check here, so I can take\nboth blame and credit for the whole thing).\n\n-Peff\n"},{"id":"322073","messageId":"20170613113015.6l35nyyu3vbtjss7@sigill.intra.peff.net","threadId":"46147","inReplyTo":"alpine.DEB.2.21.1.1706131323330.171564@virtualbox","subject":"Re: [PATCH v2 8/8] Use the early config machinery to expand aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-13T11:30:16Z","receivedAt":"2017-06-13T11:30:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 13, 2017 at 01:25:53PM +0200, Johannes Schindelin wrote:\n\n> > But couldn't we just unconditionally do:\n> > \n> > setup_git_directory_gently();\n> \n> But of course we can do that! Why on earth did I not think of that...\n> \n> Will change the code accordingly.\n\nThanks. I was really worried you were going to come back with some\nsubtle point that I missed. But I'm happy we will be able to take the\nsimpler route.\n\n-Peff\n"},{"id":"322075","messageId":"alpine.DEB.2.21.1.1706131339320.171564@virtualbox","threadId":"46147","inReplyTo":"alpine.DEB.2.21.1.1706131319130.171564@virtualbox","subject":"Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-13T11:42:02Z","receivedAt":"2017-06-13T11:42:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 13 Jun 2017, Johannes Schindelin wrote:\n\n> On Sat, 10 Jun 2017, Jeff King wrote:\n> \n> > On Thu, Jun 08, 2017 at 09:53:50PM +0200, Johannes Schindelin wrote:\n> > \n> > > -char *alias_lookup(const char *alias)\n> > > [...]\n> > > {\n> > > -\tchar *v = NULL;\n> > > -\tstruct strbuf key = STRBUF_INIT;\n> > > -\tstrbuf_addf(&key, \"alias.%s\", alias);\n> > > -\tif (git_config_key_is_valid(key.buf))\n> > > -\t\tgit_config_get_string(key.buf, &v);\n> > > -\tstrbuf_release(&key);\n> > > -\treturn v;\n> > > +\tstruct strbuf key;\n> > > +\tchar *v;\n> > > +};\n> > > [...]\n> > > +char *alias_lookup(const char *alias, struct strbuf *cdup_dir)\n> > > +{\n> > > +\tstruct config_alias_data data = { STRBUF_INIT, NULL };\n> > > +\n> > > +\tstrbuf_addf(&data.key, \"alias.%s\", alias);\n> > > +\tif (git_config_key_is_valid(data.key.buf))\n> > > +\t\tread_early_config(config_alias_cb, &data, cdup_dir);\n> > > +\tstrbuf_release(&data.key);\n> > > +\n> > > +\treturn data.v;\n> > > }\n> > \n> > Two optional cleanups here:\n> > \n> > 1. We don't need to call config_key_is_valid when using a callback. We\n> > only needed that to prevent the configset machinery from issuing a\n> > warning. It does save us reading the config entirely when the\n> > program name is syntactically invalid as an alias, but that's a\n> > pretty rare case.\n> \n> It may be a pretty rare case, or it may not be. I do not want to think\n> hard about this, so I just wanted to keep that test.\n> \n> But since you suggested it, I will simply blame all the fallout (if any)\n> on you.\n> \n> ;-)\n> \n> > 2. Now that we're not using the configset machinery, we don't need to\n> > have the alias name as a full string. Instead of using the strbuf,\n> > we could just pass the \"alias\" string itself and do:\n> > \n> > if (skip_prefix(var, \"alias.\", &key) && !strcmp(key, data->key))\n> > \n> > in the callback.\n> \n> As you probably guessed, I had tried that first and then figured that if\n> I needed to keep the config_key_is_valid() test anyway, I could just as\n> well keep the strbuf around for later use.\n> \n> Will change the code,\n\nAlas, I won't change the code after all.\n\nIt is really tempting to avoid the extra strbuf, but then the error\nmessage would change from\n\n\terror: missing value for 'alias.br'\n\nto\n\n\terror: missing value for 'br'\n\nwhich is of course no good at all.\n\nAnd since I already have to keep that strbuf, I'll simply keep the\nconfig_key_is_valid() guard, too (because why not).\n\nCiao,\nDscho\n"},{"id":"322076","messageId":"20170613114254.oo6r6oqwtchvjuim@sigill.intra.peff.net","threadId":"46147","inReplyTo":"alpine.DEB.2.21.1.1706131339320.171564@virtualbox","subject":"Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-13T11:42:54Z","receivedAt":"2017-06-13T11:43:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 13, 2017 at 01:42:02PM +0200, Johannes Schindelin wrote:\n\n> > As you probably guessed, I had tried that first and then figured that if\n> > I needed to keep the config_key_is_valid() test anyway, I could just as\n> > well keep the strbuf around for later use.\n> > \n> > Will change the code,\n> \n> Alas, I won't change the code after all.\n> \n> It is really tempting to avoid the extra strbuf, but then the error\n> message would change from\n> \n> \terror: missing value for 'alias.br'\n> \n> to\n> \n> \terror: missing value for 'br'\n> \n> which is of course no good at all.\n> \n> And since I already have to keep that strbuf, I'll simply keep the\n> config_key_is_valid() guard, too (because why not).\n\nOof, yeah, that is definitely worse. I'm fine with keeping both parts.\n\n-Peff\n"},{"id":"322085","messageId":"xmqq8tkw9h7p.fsf@gitster.mtv.corp.google.com","threadId":"46147","inReplyTo":"alpine.DEB.2.21.1.1706131244150.171564@virtualbox","subject":"Re: [PATCH v2 5/8] t1308: relax the test verifying that empty alias values are disallowed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-13T12:59:06Z","receivedAt":"2017-06-13T12:59:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Sadly, I do not think so. It is just different, not better. Maybe less\n> redundant... See for yourself:\n\nYup, I noticed and was referring to this \"less redundant\" as an\nimprovement, actually.\n\n> The real fix would indeed be (as mentioned by Brandon elsewhere) to unify\n> the code paths between the cached and the non-cached config machinery, so\n> as to provide the exact same error message in this case.\n\nYeah, the unifying of the messages would be a good addition in the\nmid term but I tend to agree that it can be done after this series\nlands.\n\nThanks for clarification.\n"},{"id":"322100","messageId":"xmqqefun96y6.fsf@gitster.mtv.corp.google.com","threadId":"46147","inReplyTo":"20170613114254.oo6r6oqwtchvjuim@sigill.intra.peff.net","subject":"Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-13T16:40:49Z","receivedAt":"2017-06-13T16:40:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jun 13, 2017 at 01:42:02PM +0200, Johannes Schindelin wrote:\n>\n>> > As you probably guessed, I had tried that first and then figured that if\n>> > I needed to keep the config_key_is_valid() test anyway, I could just as\n>> > well keep the strbuf around for later use.\n>> > \n>> > Will change the code,\n>> \n>> Alas, I won't change the code after all.\n>> \n>> It is really tempting to avoid the extra strbuf, but then the error\n>> message would change from\n>> \n>> \terror: missing value for 'alias.br'\n>> \n>> to\n>> \n>> \terror: missing value for 'br'\n>> \n>> which is of course no good at all.\n>> \n>> And since I already have to keep that strbuf, I'll simply keep the\n>> config_key_is_valid() guard, too (because why not).\n>\n> Oof, yeah, that is definitely worse. I'm fine with keeping both parts.\n\nWhen you replace Dscho's \"compare 'var' with 'alias.br' that is in\nstrbuf naively with the \"skip-prefix and compare with br\" without\nchanging anything else, i.e.\n\n if (skip_prefix(var, \"alias.\", &key) && !strcmp(key, data->key))\n\treturn git_config_string((const char **)&data->v, key, value);\n\nit would cause the \"br\" to be fed to git_config_string() and result\nin problem reported for \"br\", not \"alias.br\". \n\nBut this can be trivially fixed by passing \"var\" instead of \"key\" to\ngit_config_string(), no? Am I mistaken?\n\n"},{"id":"322191","messageId":"20170614060220.pjb26zqgbtbtpxpy@sigill.intra.peff.net","threadId":"46147","inReplyTo":"xmqqefun96y6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T06:02:20Z","receivedAt":"2017-06-14T06:02:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 13, 2017 at 09:40:49AM -0700, Junio C Hamano wrote:\n\n> >> It is really tempting to avoid the extra strbuf, but then the error\n> >> message would change from\n> >> \n> >> \terror: missing value for 'alias.br'\n> >> \n> >> to\n> >> \n> >> \terror: missing value for 'br'\n> >> \n> >> which is of course no good at all.\n> >> \n> >> And since I already have to keep that strbuf, I'll simply keep the\n> >> config_key_is_valid() guard, too (because why not).\n> >\n> > Oof, yeah, that is definitely worse. I'm fine with keeping both parts.\n> \n> When you replace Dscho's \"compare 'var' with 'alias.br' that is in\n> strbuf naively with the \"skip-prefix and compare with br\" without\n> changing anything else, i.e.\n> \n> if (skip_prefix(var, \"alias.\", &key) && !strcmp(key, data->key))\n> \treturn git_config_string((const char **)&data->v, key, value);\n> \n> it would cause the \"br\" to be fed to git_config_string() and result\n> in problem reported for \"br\", not \"alias.br\". \n> \n> But this can be trivially fixed by passing \"var\" instead of \"key\" to\n> git_config_string(), no? Am I mistaken?\n\nRight, I wasn't thinking. That is a perfectly good solution, and matches\nthe usual mechanism in other config callbacks.\n\n-Peff\n"},{"id":"322217","messageId":"alpine.DEB.2.21.1.1706141158380.171564@virtualbox","threadId":"46147","inReplyTo":"xmqqefun96y6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T10:03:07Z","receivedAt":"2017-06-14T10:03:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 13 Jun 2017, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Tue, Jun 13, 2017 at 01:42:02PM +0200, Johannes Schindelin wrote:\n> >\n> >> > As you probably guessed, I had tried that first and then figured that if\n> >> > I needed to keep the config_key_is_valid() test anyway, I could just as\n> >> > well keep the strbuf around for later use.\n> >> > \n> >> > Will change the code,\n> >> \n> >> Alas, I won't change the code after all.\n> >> \n> >> It is really tempting to avoid the extra strbuf, but then the error\n> >> message would change from\n> >> \n> >> \terror: missing value for 'alias.br'\n> >> \n> >> to\n> >> \n> >> \terror: missing value for 'br'\n> >> \n> >> which is of course no good at all.\n> >> \n> >> And since I already have to keep that strbuf, I'll simply keep the\n> >> config_key_is_valid() guard, too (because why not).\n> >\n> > Oof, yeah, that is definitely worse. I'm fine with keeping both parts.\n> \n> When you replace Dscho's \"compare 'var' with 'alias.br' that is in\n> strbuf naively with the \"skip-prefix and compare with br\" without\n> changing anything else, i.e.\n> \n> if (skip_prefix(var, \"alias.\", &key) && !strcmp(key, data->key))\n> \treturn git_config_string((const char **)&data->v, key, value);\n> \n> it would cause the \"br\" to be fed to git_config_string() and result\n> in problem reported for \"br\", not \"alias.br\". \n> \n> But this can be trivially fixed by passing \"var\" instead of \"key\" to\n> git_config_string(), no? Am I mistaken?\n\nOh my. You are right! The `git_config_string()` function uses the `key`\nparameter only for reporting, not for anything else.\n\nSo all I had to do was to introduce another variable so that the `key`\npointer would not advance beyond the \"alias.\".\n\nWill fix,\nDscho\n"}]}