{"thread":{"id":"46133","subject":"[PATCH 1/9] discover_git_directory(): avoid setting invalid git_dir","startedAt":"2017-06-07T16:06:23Z","lastAt":"2017-06-10T01:33:48Z","messageCount":34,"participants":["Johannes Schindelin","Brandon Williams","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"321679","messageId":"300d32d72b67f8c12b6cc91c36a5735a77f5ed89.1496851544.git.johannes.schindelin@gmx.de","threadId":"46133","inReplyTo":"cover.1496851544.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/9] discover_git_directory(): avoid setting invalid git_dir","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-07T16:06:06Z","receivedAt":"2017-06-07T16:06:23Z","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\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":"321680","messageId":"078fd144ddefa22892861488a7d3f48eb8d5b775.1496851544.git.johannes.schindelin@gmx.de","threadId":"46133","inReplyTo":"cover.1496851544.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/9] config: report correct line number upon error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-07T16:06:09Z","receivedAt":"2017-06-07T16:06:27Z","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":"321681","messageId":"cover.1496851544.git.johannes.schindelin@gmx.de","threadId":"46133","inReplyTo":null,"subject":"[PATCH 0/9] Avoid problem where git_dir is set after alias expansion","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-07T16:06:01Z","receivedAt":"2017-06-07T16:06:29Z","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\n\nJohannes Schindelin (9):\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 TODO:\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-v1\nFetch-It-Via: git fetch https://github.com/dscho/git alias-early-config-v1\n-- \n2.13.0.windows.1.460.g13f583bedb5\n\n"},{"id":"321682","messageId":"d7b77cbd688b7536aebee49df652af594d2a1293.1496851544.git.johannes.schindelin@gmx.de","threadId":"46133","inReplyTo":"cover.1496851544.git.johannes.schindelin@gmx.de","subject":"[PATCH 3/9] help: use early config when autocorrecting aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-07T16:06:12Z","receivedAt":"2017-06-07T16:06:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Git has this feature where suggests similar commands (including aliases)\nin case that 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":"321683","messageId":"6d339785a007c2805bdc17c4e24da713a5996703.1496851544.git.johannes.schindelin@gmx.de","threadId":"46133","inReplyTo":"cover.1496851544.git.johannes.schindelin@gmx.de","subject":"[PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-07T16:06:16Z","receivedAt":"2017-06-07T16:07:04Z","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..a8bbddf5955 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 *worktree_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 *worktree_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..3d78c11fc00 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 *worktree_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, worktree_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..771822fd0ca 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 *worktree_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, worktree_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 (worktree_dir) {\n+\t\tworktree_dir_offset = worktree_dir->len;\n+\t\tstrbuf_addbuf(worktree_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 (worktree_dir)\n+\t\t\tstrbuf_setlen(worktree_dir, worktree_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":"321684","messageId":"a5bf8684025908e395a9ff664539ba8b3b83e1a3.1496851544.git.johannes.schindelin@gmx.de","threadId":"46133","inReplyTo":"cover.1496851544.git.johannes.schindelin@gmx.de","subject":"[PATCH 7/9] alias_lookup(): optionally return top-level directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-07T16:06:55Z","receivedAt":"2017-06-07T16:07:06Z","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..c1b87154944 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 *worktree_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+\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 a8bbddf5955..77ce8e8c80b 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 *worktree_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":"321685","messageId":"e56eaa53b87052c670a4d431cc2cefde4088a200.1496851544.git.johannes.schindelin@gmx.de","threadId":"46133","inReplyTo":"cover.1496851544.git.johannes.schindelin@gmx.de","subject":"[PATCH 6/9] t7006: demonstrate a problem with aliases in subdirectories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-07T16:06:52Z","receivedAt":"2017-06-07T16:07:07Z","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":"321686","messageId":"3325827e66b1a9e693f81e8aa4cae44f41dfdc25.1496851544.git.johannes.schindelin@gmx.de","threadId":"46133","inReplyTo":"cover.1496851544.git.johannes.schindelin@gmx.de","subject":"[PATCH 5/9] t1308: relax the test verifying that empty alias values are disallowed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-07T16:06:47Z","receivedAt":"2017-06-07T16:07:20Z","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":"321687","messageId":"3b916e1f5055486795c784d55704b4720598f1a9.1496851544.git.johannes.schindelin@gmx.de","threadId":"46133","inReplyTo":"cover.1496851544.git.johannes.schindelin@gmx.de","subject":"[PATCH 8/9] Use the early config machinery to expand aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-07T16:07:03Z","receivedAt":"2017-06-07T16:07:27Z","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..c82cd455948 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 worktree_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, &worktree_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\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(&worktree_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\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":"321688","messageId":"alpine.DEB.2.21.1.1706071808040.171564@virtualbox","threadId":"46133","inReplyTo":"cover.1496851544.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/9] Avoid problem where git_dir is set after alias expansion","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-07T16:09:04Z","receivedAt":"2017-06-07T16:09:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 7 Jun 2017, Johannes Schindelin wrote:\n\n> Johannes Schindelin (9):\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> TODO:\n\nThe 9/9 is obviously an empty commit that I used to track things still to\ndo. Please ignore that \"TODO:\" entry and rest assured that there is no 9/9\nin this first iteration of the patch series.\n\nCiao,\nJohannes\n"},{"id":"321694","messageId":"20170607174529.GB110638@google.com","threadId":"46133","inReplyTo":"300d32d72b67f8c12b6cc91c36a5735a77f5ed89.1496851544.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/9] discover_git_directory(): avoid setting invalid git_dir","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-07T17:45:29Z","receivedAt":"2017-06-07T17:45:41Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/07, Johannes Schindelin wrote:\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> Signed-off-by: Johannes Schindelin \n\nSeems sane. This way the strbuf is in the same state it was in before\ncalling this function (upon a failure that is).\n\n> ---\n> setup.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/setup.c b/setup.c\n> index 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\nThere is another part of this function that returns NULL (which isn't\nshown by this diff) after performing 'setup_git_dir_gently_1', do we\nneed to worry about anything that 'setup_git_dir_gently_1' has\npotentially appended to 'gitdir' upon 'setup_git_dir_gently_1' failing?\n\n> \t}\n> \n> -- \n> 2.13.0.windows.1.460.g13f583bedb5\n> \n> \n\n-- \nBrandon Williams\n"},{"id":"321695","messageId":"20170607175122.GC110638@google.com","threadId":"46133","inReplyTo":"d7b77cbd688b7536aebee49df652af594d2a1293.1496851544.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 3/9] help: use early config when autocorrecting aliases","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-07T17:51:22Z","receivedAt":"2017-06-07T17:51:34Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/07, Johannes Schindelin wrote:\n> Git has this feature where suggests similar commands (including aliases)\n\nnit: s/where/which/\n\n> in case that the user specified an unknown command.\n\ns/that//\n\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\nAgreed, and it is more important to be correct than performant.\n\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> help.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/help.c b/help.c\n> index 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> -- \n> 2.13.0.windows.1.460.g13f583bedb5\n> \n> \n\n-- \nBrandon Williams\n"},{"id":"321697","messageId":"20170607181333.GD110638@google.com","threadId":"46133","inReplyTo":"6d339785a007c2805bdc17c4e24da713a5996703.1496851544.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-07T18:13:33Z","receivedAt":"2017-06-07T18:13:57Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/07, Johannes Schindelin wrote:\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> \n> In preparation for allowing the alias machinery to make use of the early\n> config machinery, let's add an optional strbuf parameter to the\n> read_early_config() function; if not NULL, the path of said top-level\n> directory is appended to the strbuf. As a special case, nothing is\n> appended if setup_git_directory() would have restored the original\n> working directory before returning.\n> \n> Signed-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> \n> diff --git a/cache.h b/cache.h\n> index 4d92aae0e81..a8bbddf5955 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 *worktree_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 *worktree_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> diff --git a/config.c b/config.c\n> index 9b88531a70d..3d78c11fc00 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 *worktree_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, worktree_dir))\n> \t\topts.git_dir = buf.buf;\n\nIt feels kind of weird to get back worktree info after calling\nread_early_config but I understand why you need to get it. One thing to\nconsider is the 'if' statement not shown here since you aren't adding any\nworktree information if that branch is taken. Maybe we can drop the\nfirst if statement all together as 'read_early_config' is used before\nsetup is run and it should really only be triggered when setup has been\nrun.\n\n> \n> \tgit_config_with_options(cb, data, NULL, &opts);\n> diff --git a/help.c b/help.c\n> index 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> \n> diff --git a/pager.c b/pager.c\n> index 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;\n> diff --git a/setup.c b/setup.c\n> index 2435186e448..771822fd0ca 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 *worktree_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, worktree_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 (worktree_dir) {\n> +\t\tworktree_dir_offset = worktree_dir->len;\n> +\t\tstrbuf_addbuf(worktree_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 (worktree_dir)\n> +\t\t\tstrbuf_setlen(worktree_dir, worktree_dir_offset);\n> \t\treturn NULL;\n> \t}\n> \n> diff --git a/t/helper/test-config.c b/t/helper/test-config.c\n> index 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> -- \n> 2.13.0.windows.1.460.g13f583bedb5\n> \n> \n\n-- \nBrandon Williams\n"},{"id":"321698","messageId":"20170607181553.GE110638@google.com","threadId":"46133","inReplyTo":"3325827e66b1a9e693f81e8aa4cae44f41dfdc25.1496851544.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 5/9] t1308: relax the test verifying that empty alias values are disallowed","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-07T18:15:53Z","receivedAt":"2017-06-07T18:16:01Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/07, Johannes Schindelin wrote:\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\nNot a comment on the patch but just a genuine question: Is there any\nreason why they complain in a different way? Doesn't it make sense for\nthe errors to be reported consistently?\n\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> \n> test_expect_success 'error on modifying repo config without repo' '\n> -- \n> 2.13.0.windows.1.460.g13f583bedb5\n> \n> \n\n-- \nBrandon Williams\n"},{"id":"321700","messageId":"20170607182614.GF110638@google.com","threadId":"46133","inReplyTo":"3b916e1f5055486795c784d55704b4720598f1a9.1496851544.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 8/9] Use the early config machinery to expand aliases","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-07T18:26:14Z","receivedAt":"2017-06-07T18:26:28Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/07, Johannes Schindelin wrote:\n> We already taught alias_lookup() to use the early config if the .git/\n> directory was not yet discovered, of course, however, since we called\n> setup_git_directory_gently() before expanding the alias (if any), we\n> only used the early config code path only if outside of any Git-managed\n> directory.\n> \n> With this commit, we finally switch over to really using the early\n> config code path.\n> \n> Rather than just chdir()ing into the indicated directory in case of an\n> alias expanding to a shell command, we simply set up the .git/ directory\n> so that e.g. GIT_PREFIX is set as expected.\n> \n> This change also fixes a known issue where Git tried to read the pager\n> config from an incorrect path in a subdirectory of a Git worktree if an\n> alias expanded to a shell command.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> git.c | 59 +++++++-------------------------------------------------\n> t/t7006-pager.sh | 2 +-\n> 2 files changed, 8 insertions(+), 53 deletions(-)\n> \n> diff --git a/git.c b/git.c\n> index 4163beaead4..c82cd455948 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\nI like seeing chunks of old code being deleted :D\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 worktree_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, &worktree_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\t\tsetup_git_directory();\n\nSo if there is a worktree then we run the setup code explicitly. I\nassume that is to ensure that all envvars are setup properly before\nrunning the alias. Just interesting to note that the actual value of\nworktree_dir is never used, its just used essentially as a boolean\nindicator of there being a worktree/gitdir/repository. I'm not\nsuggesting to change from what you have here, its just food for thought.\n\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(&worktree_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\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;\n> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n> index 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> -- \n> 2.13.0.windows.1.460.g13f583bedb5\n\n-- \nBrandon Williams\n"},{"id":"321702","messageId":"20170607183008.GG110638@google.com","threadId":"46133","inReplyTo":"cover.1496851544.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/9] Avoid problem where git_dir is set after alias expansion","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-07T18:30:08Z","receivedAt":"2017-06-07T18:30:15Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/07, Johannes Schindelin wrote:\n> 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\n> working directory) so that the command specified by the alias can run\n> correctly.\n> \n> What we failed to reset was the git_dir, meaning that in the most common\n> case, it was now pointing to a .git/ directory *in the subdirectory*.\n> \n> This problem was identified in the GVFS fork, where a pre-command hook\n> was introduced to allow pre-fetching missing blobs.\n> \n> An early quick fix in the GVFS fork simply built on top of the\n> save_env_before_alias() hack, introducing another hack that saves the\n> git_dir and restores it after an alias is expanded:\n> \n> \thttps://github.com/Microsoft/git/commit/2d859ba3b\n> \n> That is very hacky, though, and it is much better (although much more\n> involved, too) to fix this \"properly\", i.e. by replacing the ugly\n> save/restore logic by simply using the early config code path.\n> \n> However, aliases are strange beasts.\n> \n> When an alias refers to a single Git command (originally the sole\n> intention of aliases), the current working directory is restored to what\n> it had been before expanding the alias.\n> \n> But when an alias starts with an exclamation point, i.e. referring to a\n> command-line to be interpreted by the shell, the current working\n> directory is no longer in the subdirectory but instead in the worktree's\n> top-level directory.\n> \n> This is even true for worktrees added by `git worktree add`.\n> \n> But when we are inside the .git/ directory, the current working\n> directory is *restored* to the subdirectory inside the .git/ directory.\n> \n> In short, the logic is a bit complicated what is the expected current\n> working directory after expanding an alias and before actually running\n> it.\n> \n> That is why this patch series had to expand the signature of the early\n> config machinery to return the additional information for aliases'\n> benefit.\n> \n\nLooks good, I don't have any major issues with the series, just some\ncomments for clarity mostly. And relevant to this series, you may be\ninterested in looking at patch 03/31 in my repository object series as\nthat may have an impact on the early config stuff.\n\n> \n> Johannes Schindelin (9):\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> TODO:\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> \n> base-commit: 8d1b10321b20bd2a73a5b561cfc3cf2e8051b70b\n> Published-As: https://github.com/dscho/git/releases/tag/alias-early-config-v1\n> Fetch-It-Via: git fetch https://github.com/dscho/git alias-early-config-v1\n> -- \n> 2.13.0.windows.1.460.g13f583bedb5\n> \n\n-- \nBrandon Williams\n"},{"id":"321742","messageId":"alpine.DEB.2.21.1.1706081143010.171564@virtualbox","threadId":"46133","inReplyTo":"20170607174529.GB110638@google.com","subject":"Re: [PATCH 1/9] discover_git_directory(): avoid setting invalid git_dir","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T10:00:56Z","receivedAt":"2017-06-08T10:01:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Wed, 7 Jun 2017, Brandon Williams wrote:\n\n> On 06/07, Johannes Schindelin wrote:\n>\n> > diff --git a/setup.c b/setup.c\n> > index 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> \n> There is another part of this function that returns NULL (which isn't\n> shown by this diff) after performing 'setup_git_dir_gently_1', do we\n> need to worry about anything that 'setup_git_dir_gently_1' has\n> potentially appended to 'gitdir' upon 'setup_git_dir_gently_1' failing?\n\nThe setup_git_dir_gently_1() function used to set the git_dir variable\ndirectly in case of success, and leave it unchanged in case of error. My\nwork to allow an early config did not change that, with the exception that\nit is the gitdir parameter that is changed instead of git_dir.\n\nIn other words: that `return NULL` is safe, as it returns due to an error\nreported by the setup_git_directory_gently_1() function (meaning that the\ngitdir strbuf has not been touched).\n\nI amended the commit message accordingly, and force-pushed the result.\nMeaning: v2 will have that change.\n\nCiao,\nDscho\n"},{"id":"321743","messageId":"alpine.DEB.2.21.1.1706081202200.171564@virtualbox","threadId":"46133","inReplyTo":"20170607175122.GC110638@google.com","subject":"Re: [PATCH 3/9] help: use early config when autocorrecting aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T10:02:37Z","receivedAt":"2017-06-08T10:03:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Wed, 7 Jun 2017, Brandon Williams wrote:\n\n> On 06/07, Johannes Schindelin wrote:\n> > Git has this feature where suggests similar commands (including aliases)\n> \n> nit: s/where/which/\n> \n> > in case that the user specified an unknown command.\n> \n> s/that//\n\nOh my. Thank you so much!\nDscho\n"},{"id":"321744","messageId":"alpine.DEB.2.21.1.1706081204330.171564@virtualbox","threadId":"46133","inReplyTo":"20170607181333.GD110638@google.com","subject":"Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T10:20:52Z","receivedAt":"2017-06-08T10:21:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Wed, 7 Jun 2017, Brandon Williams wrote:\n\n> On 06/07, Johannes Schindelin wrote:\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, worktree_dir))\n> > \t\topts.git_dir = buf.buf;\n> \n> It feels kind of weird to get back worktree info after calling\n> read_early_config but I understand why you need to get it.\n\nYeah. It is awkward. Required for backwards-compatibility, though (and it\nis hard to explain *when* it is needed, and even harder under what\ncircumstances it is *not* needed even if there is a worktre).\n\n> One thing to consider is the 'if' statement not shown here since you\n> aren't adding any worktree information if that branch is taken.\n\nRight. For lurkers, that `if` statement reads thusly:\n\n\tif (have_git_dir())\n\t\topts.git_dir = get_git_dir();\n\n> Maybe we can drop the first if statement all together as\n> 'read_early_config' is used before setup is run and it should really\n> only be triggered when setup has been run.\n\nThe `read_early_config()` function is *sometimes* used *after* setup has\nrun. Look at `run_builtin()` in git.c:\n\n\tif (p->option & RUN_SETUP)\n\t\tprefix = setup_git_directory();\n\telse if (p->option & RUN_SETUP_GENTLY) {\n\t\tint nongit_ok;\n\t\tprefix = setup_git_directory_gently(&nongit_ok);\n\t}\n\n\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))\n\t\tuse_pager = check_pager_config(p->cmd);\n\nFor builtins having either the RUN_SETUP or the RUN_SETUP_GENTLY flag, we\ndo not need to re-discover the .git/ directory at all when checking the\npager config.\n\nBack to the worktree_dir variable.\n\nI think part of the confusion here is that it may be left alone even when\nthere is a worktree. For example, if we are already in the top-level\ndirectory. Or if the worktree somehow points to a different directory than\nthe one containing the .git/ directory.\n\nTherefore, I renamed this variable to `cdup_dir` to reflect the fact that\nit is only touched if Git determines that it is in a subdirectory of the\ndirectory containing the .git/ directory.\n\nCiao,\nDscho\n"},{"id":"321745","messageId":"alpine.DEB.2.21.1.1706081221090.171564@virtualbox","threadId":"46133","inReplyTo":"20170607181553.GE110638@google.com","subject":"Re: [PATCH 5/9] t1308: relax the test verifying that empty alias values are disallowed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T10:22:35Z","receivedAt":"2017-06-08T10:22:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Wed, 7 Jun 2017, Brandon Williams wrote:\n\n> On 06/07, Johannes Schindelin wrote:\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> Not a comment on the patch but just a genuine question: Is there any\n> reason why they complain in a different way? Doesn't it make sense for\n> the errors to be reported consistently?\n\nYes, I agree that they should not complain in different ways.\n\nI had a brief look to see whether I could quickly fix that \"while at it\".\nBut it seems to be quite a bit more involved than I am comfortable\nslipping into this patch series (whose purpose is not to fix the dichotomy\nbetween direct and cached config parsing, after all).\n\nCiao,\nDscho\n"},{"id":"321746","messageId":"alpine.DEB.2.21.1.1706081223130.171564@virtualbox","threadId":"46133","inReplyTo":"20170607182614.GF110638@google.com","subject":"Re: [PATCH 8/9] Use the early config machinery to expand aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T10:25:16Z","receivedAt":"2017-06-08T10:25:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Wed, 7 Jun 2017, Brandon Williams wrote:\n\n> I like seeing chunks of old code being deleted :D\n\nMe, too. In particular as hacky code as this one. It caused quite a couple\nof gray hairs here.\n\n> On 06/07, 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 worktree_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, &worktree_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\t\tsetup_git_directory();\n> \n> So if there is a worktree then we run the setup code explicitly. I\n> assume that is to ensure that all envvars are setup properly before\n> running the alias. Just interesting to note that the actual value of\n> worktree_dir is never used, its just used essentially as a boolean\n> indicator of there being a worktree/gitdir/repository. I'm not\n> suggesting to change from what you have here, its just food for thought.\n\nIndeed.\n\nThat is what I tried to indicate by this paragraph in the commit message:\n\n\tRather than just chdir()ing into the indicated directory in case of an\n\talias expanding to a shell command, we simply set up the .git/ directory\n\tso that e.g. GIT_PREFIX is set as expected.\n\n"},{"id":"321747","messageId":"alpine.DEB.2.21.1.1706081225450.171564@virtualbox","threadId":"46133","inReplyTo":"20170607183008.GG110638@google.com","subject":"Re: [PATCH 0/9] Avoid problem where git_dir is set after alias expansion","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T10:27:10Z","receivedAt":"2017-06-08T10:27:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Wed, 7 Jun 2017, Brandon Williams wrote:\n\n> Looks good, I don't have any major issues with the series, just some\n> comments for clarity mostly.\n\nThank you for the review!\n\nI will wait a couple of hours to see whether anybody else sees anything\nthat needs to be changed, before sending out v2.\n\n> And relevant to this series, you may be interested in looking at patch\n> 03/31 in my repository object series as that may have an impact on the\n> early config stuff.\n\nThanks for the prod. I will have a specific look at this patch right after\nsending this mail.\n\nCiao,\nDscho\n"},{"id":"321753","messageId":"20170608144628.GH110638@google.com","threadId":"46133","inReplyTo":"alpine.DEB.2.21.1.1706081204330.171564@virtualbox","subject":"Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-08T14:46:28Z","receivedAt":"2017-06-08T14:46:35Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/08, Johannes Schindelin wrote:\n> Hi Brandon,\n> \n> On Wed, 7 Jun 2017, Brandon Williams wrote:\n> \n> > On 06/07, Johannes Schindelin wrote:\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, worktree_dir))\n> > > \t\topts.git_dir = buf.buf;\n> > \n> > It feels kind of weird to get back worktree info after calling\n> > read_early_config but I understand why you need to get it.\n> \n> Yeah. It is awkward. Required for backwards-compatibility, though (and it\n> is hard to explain *when* it is needed, and even harder under what\n> circumstances it is *not* needed even if there is a worktre).\n> \n> > One thing to consider is the 'if' statement not shown here since you\n> > aren't adding any worktree information if that branch is taken.\n> \n> Right. For lurkers, that `if` statement reads thusly:\n> \n> \tif (have_git_dir())\n> \t\topts.git_dir = get_git_dir();\n> \n> > Maybe we can drop the first if statement all together as\n> > 'read_early_config' is used before setup is run and it should really\n> > only be triggered when setup has been run.\n> \n> The `read_early_config()` function is *sometimes* used *after* setup has\n> run. Look at `run_builtin()` in git.c:\n> \n> \tif (p->option & RUN_SETUP)\n> \t\tprefix = setup_git_directory();\n> \telse if (p->option & RUN_SETUP_GENTLY) {\n> \t\tint nongit_ok;\n> \t\tprefix = setup_git_directory_gently(&nongit_ok);\n> \t}\n> \n> \tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))\n> \t\tuse_pager = check_pager_config(p->cmd);\n> \n\nAh, ok so my comment was incorrect, thanks for pointing this out.\n\n> For builtins having either the RUN_SETUP or the RUN_SETUP_GENTLY flag, we\n> do not need to re-discover the .git/ directory at all when checking the\n> pager config.\n> \n> Back to the worktree_dir variable.\n> \n> I think part of the confusion here is that it may be left alone even when\n> there is a worktree. For example, if we are already in the top-level\n> directory. Or if the worktree somehow points to a different directory than\n> the one containing the .git/ directory.\n> \n> Therefore, I renamed this variable to `cdup_dir` to reflect the fact that\n> it is only touched if Git determines that it is in a subdirectory of the\n> directory containing the .git/ directory.\n\nOk, maybe I'm just not following but just from reading the variable name I can't\nreally understand what 'cdup_dir' means.\n\n> \n> Ciao,\n> Dscho\n\n-- \nBrandon Williams\n"},{"id":"321754","messageId":"20170608144734.GI110638@google.com","threadId":"46133","inReplyTo":"alpine.DEB.2.21.1.1706081221090.171564@virtualbox","subject":"Re: [PATCH 5/9] t1308: relax the test verifying that empty alias values are disallowed","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-08T14:47:34Z","receivedAt":"2017-06-08T14:47:41Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/08, Johannes Schindelin wrote:\n> Hi Brandon,\n> \n> On Wed, 7 Jun 2017, Brandon Williams wrote:\n> \n> > On 06/07, Johannes Schindelin wrote:\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> > Not a comment on the patch but just a genuine question: Is there any\n> > reason why they complain in a different way? Doesn't it make sense for\n> > the errors to be reported consistently?\n> \n> Yes, I agree that they should not complain in different ways.\n> \n> I had a brief look to see whether I could quickly fix that \"while at it\".\n> But it seems to be quite a bit more involved than I am comfortable\n> slipping into this patch series (whose purpose is not to fix the dichotomy\n> between direct and cached config parsing, after all).\n\nFair enough, thanks!\n\n> \n> Ciao,\n> Dscho\n\n-- \nBrandon Williams\n"},{"id":"321755","messageId":"20170608145100.GJ110638@google.com","threadId":"46133","inReplyTo":"alpine.DEB.2.21.1.1706081223130.171564@virtualbox","subject":"Re: [PATCH 8/9] Use the early config machinery to expand aliases","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-08T14:51:00Z","receivedAt":"2017-06-08T14:51:12Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/08, Johannes Schindelin wrote:\n> Hi Brandon,\n> \n> On Wed, 7 Jun 2017, Brandon Williams wrote:\n> \n> > I like seeing chunks of old code being deleted :D\n> \n> Me, too. In particular as hacky code as this one. It caused quite a couple\n> of gray hairs here.\n> \n> > On 06/07, 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 worktree_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, &worktree_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\t\tsetup_git_directory();\n> > \n> > So if there is a worktree then we run the setup code explicitly. I\n> > assume that is to ensure that all envvars are setup properly before\n> > running the alias. Just interesting to note that the actual value of\n> > worktree_dir is never used, its just used essentially as a boolean\n> > indicator of there being a worktree/gitdir/repository. I'm not\n> > suggesting to change from what you have here, its just food for thought.\n> \n> Indeed.\n> \n> That is what I tried to indicate by this paragraph in the commit message:\n> \n> \tRather than just chdir()ing into the indicated directory in case of an\n> \talias expanding to a shell command, we simply set up the .git/ directory\n> \tso that e.g. GIT_PREFIX is set as expected.\n\nI was more commenting on \"oh that's what he meant by that\" for my own\nunderstanding. But thanks again for pointing out that you did include a\ncomment in the commit message reflecting that.\n\n-- \nBrandon Williams\n"},{"id":"321756","messageId":"alpine.DEB.2.21.1.1706081729450.171564@virtualbox","threadId":"46133","inReplyTo":"20170608144628.GH110638@google.com","subject":"Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T15:30:49Z","receivedAt":"2017-06-08T15:31:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Thu, 8 Jun 2017, Brandon Williams wrote:\n\n> On 06/08, Johannes Schindelin wrote:\n> \n> > Back to the worktree_dir variable.\n> > \n> > I think part of the confusion here is that it may be left alone even\n> > when there is a worktree. For example, if we are already in the\n> > top-level directory. Or if the worktree somehow points to a different\n> > directory than the one containing the .git/ directory.\n> > \n> > Therefore, I renamed this variable to `cdup_dir` to reflect the fact\n> > that it is only touched if Git determines that it is in a subdirectory\n> > of the directory containing the .git/ directory.\n> \n> Ok, maybe I'm just not following but just from reading the variable name\n> I can't really understand what 'cdup_dir' means.\n\nMy idea would be that this is in line with the\n\n\tgit rev-parse --show-cdup\n\nincantation.\n\nCiao,\nDscho\n"},{"id":"321757","messageId":"20170608163230.GK110638@google.com","threadId":"46133","inReplyTo":"alpine.DEB.2.21.1.1706081729450.171564@virtualbox","subject":"Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-08T16:32:30Z","receivedAt":"2017-06-08T16:32:48Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/08, Johannes Schindelin wrote:\n> Hi Brandon,\n> \n> On Thu, 8 Jun 2017, Brandon Williams wrote:\n> \n> > On 06/08, Johannes Schindelin wrote:\n> > \n> > > Back to the worktree_dir variable.\n> > > \n> > > I think part of the confusion here is that it may be left alone even\n> > > when there is a worktree. For example, if we are already in the\n> > > top-level directory. Or if the worktree somehow points to a different\n> > > directory than the one containing the .git/ directory.\n> > > \n> > > Therefore, I renamed this variable to `cdup_dir` to reflect the fact\n> > > that it is only touched if Git determines that it is in a subdirectory\n> > > of the directory containing the .git/ directory.\n> > \n> > Ok, maybe I'm just not following but just from reading the variable name\n> > I can't really understand what 'cdup_dir' means.\n> \n> My idea would be that this is in line with the\n> \n> \tgit rev-parse --show-cdup\n> \n> incantation.\n\nAh ok, 'cdup' just doesn't really mean much to me. Is it supposed to\nstand for something?\n\n-- \nBrandon Williams\n"},{"id":"321758","messageId":"20170608163334.GL110638@google.com","threadId":"46133","inReplyTo":"alpine.DEB.2.21.1.1706081225450.171564@virtualbox","subject":"Re: [PATCH 0/9] Avoid problem where git_dir is set after alias expansion","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-08T16:33:34Z","receivedAt":"2017-06-08T16:33:59Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/08, Johannes Schindelin wrote:\n> Hi Brandon,\n> \n> On Wed, 7 Jun 2017, Brandon Williams wrote:\n> \n> > Looks good, I don't have any major issues with the series, just some\n> > comments for clarity mostly.\n> \n> Thank you for the review!\n\nOf course! I've been poking around the setup logic so this was\ninteresting to me :)\n\n> \n> I will wait a couple of hours to see whether anybody else sees anything\n> that needs to be changed, before sending out v2.\n> \n> > And relevant to this series, you may be interested in looking at patch\n> > 03/31 in my repository object series as that may have an impact on the\n> > early config stuff.\n> \n> Thanks for the prod. I will have a specific look at this patch right after\n> sending this mail.\n> \n> Ciao,\n> Dscho\n\n-- \nBrandon Williams\n"},{"id":"321761","messageId":"alpine.DEB.2.21.1.1706082052040.171564@virtualbox","threadId":"46133","inReplyTo":"20170608163230.GK110638@google.com","subject":"Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-08T18:52:30Z","receivedAt":"2017-06-08T18:53:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Thu, 8 Jun 2017, Brandon Williams wrote:\n\n> On 06/08, Johannes Schindelin wrote:\n> > \n> > On Thu, 8 Jun 2017, Brandon Williams wrote:\n> > \n> > > On 06/08, Johannes Schindelin wrote:\n> > > \n> > > > Back to the worktree_dir variable.\n> > > > \n> > > > I think part of the confusion here is that it may be left alone even\n> > > > when there is a worktree. For example, if we are already in the\n> > > > top-level directory. Or if the worktree somehow points to a different\n> > > > directory than the one containing the .git/ directory.\n> > > > \n> > > > Therefore, I renamed this variable to `cdup_dir` to reflect the fact\n> > > > that it is only touched if Git determines that it is in a subdirectory\n> > > > of the directory containing the .git/ directory.\n> > > \n> > > Ok, maybe I'm just not following but just from reading the variable name\n> > > I can't really understand what 'cdup_dir' means.\n> > \n> > My idea would be that this is in line with the\n> > \n> > \tgit rev-parse --show-cdup\n> > \n> > incantation.\n> \n> Ah ok, 'cdup' just doesn't really mean much to me. Is it supposed to\n> stand for something?\n\nYes, it stands for \"change directory up\", as in `cd ../../`. ;-)\n\nCiao,\nDscho\n"},{"id":"321762","messageId":"20170608185412.GA41637@google.com","threadId":"46133","inReplyTo":"alpine.DEB.2.21.1.1706082052040.171564@virtualbox","subject":"Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-08T18:54:12Z","receivedAt":"2017-06-08T18:54:21Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/08, Johannes Schindelin wrote:\n> Hi Brandon,\n> \n> On Thu, 8 Jun 2017, Brandon Williams wrote:\n> \n> > On 06/08, Johannes Schindelin wrote:\n> > > \n> > > On Thu, 8 Jun 2017, Brandon Williams wrote:\n> > > \n> > > > On 06/08, Johannes Schindelin wrote:\n> > > > \n> > > > > Back to the worktree_dir variable.\n> > > > > \n> > > > > I think part of the confusion here is that it may be left alone even\n> > > > > when there is a worktree. For example, if we are already in the\n> > > > > top-level directory. Or if the worktree somehow points to a different\n> > > > > directory than the one containing the .git/ directory.\n> > > > > \n> > > > > Therefore, I renamed this variable to `cdup_dir` to reflect the fact\n> > > > > that it is only touched if Git determines that it is in a subdirectory\n> > > > > of the directory containing the .git/ directory.\n> > > > \n> > > > Ok, maybe I'm just not following but just from reading the variable name\n> > > > I can't really understand what 'cdup_dir' means.\n> > > \n> > > My idea would be that this is in line with the\n> > > \n> > > \tgit rev-parse --show-cdup\n> > > \n> > > incantation.\n> > \n> > Ah ok, 'cdup' just doesn't really mean much to me. Is it supposed to\n> > stand for something?\n> \n> Yes, it stands for \"change directory up\", as in `cd ../../`. ;-)\n\nThanks for clearing that up! I was reading it as 'c-dup' and not 'cd-up' XD\n\n-- \nBrandon Williams\n"},{"id":"321842","messageId":"xmqqmv9h5c4r.fsf@gitster.mtv.corp.google.com","threadId":"46133","inReplyTo":"078fd144ddefa22892861488a7d3f48eb8d5b775.1496851544.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/9] config: report correct line number upon error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-09T17:01:08Z","receivedAt":"2017-06-09T17:01:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\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\nSounds sane.\n\nIs this something we can protect easily with a new test or two?\n\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> config.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\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"},{"id":"321844","messageId":"xmqqink55bxs.fsf@gitster.mtv.corp.google.com","threadId":"46133","inReplyTo":"d7b77cbd688b7536aebee49df652af594d2a1293.1496851544.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 3/9] help: use early config when autocorrecting aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-09T17:05:19Z","receivedAt":"2017-06-09T17:05:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\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\nYeah, the list of unknown but similar-sounding commands is produced\nfor interactive human consumption, and the above reasoning is\nperfectly sound.\n\nLooks good.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> help.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/help.c b/help.c\n> index 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"},{"id":"321863","messageId":"xmqqefus636w.fsf@gitster.mtv.corp.google.com","threadId":"46133","inReplyTo":"3325827e66b1a9e693f81e8aa4cae44f41dfdc25.1496851544.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 5/9] t1308: relax the test verifying that empty alias values are disallowed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-10T01:28:55Z","receivedAt":"2017-06-10T01:29:04Z","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\ns/lin/&e/;\n\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> \n> test_expect_success 'error on modifying repo config without repo' '\n"},{"id":"321864","messageId":"xmqqa85g62yx.fsf@gitster.mtv.corp.google.com","threadId":"46133","inReplyTo":"3b916e1f5055486795c784d55704b4720598f1a9.1496851544.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 8/9] Use the early config machinery to expand aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-10T01:33:42Z","receivedAt":"2017-06-10T01:33:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\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> -...\n> -}\n> -\n> -static void restore_env(int external_alias)\n> -{\n> -...\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 worktree_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> @@ -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\nThe save/restore sequence was an unnecessary hassle, and being able\nto do this without chdir'ing around is really nice.\n\nThanks for working on this.\n"}]}