{"thread":{"id":"46168","subject":"[PATCH v3 0/6] Avoid problem where git_dir is set after alias expansion","startedAt":"2017-06-13T12:04:34Z","lastAt":"2017-06-14T17:21:04Z","messageCount":15,"participants":["Johannes Schindelin","Junio C Hamano","Brandon Williams","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":6},"messages":[{"id":"322077","messageId":"cover.1497355444.git.johannes.schindelin@gmx.de","threadId":"46168","inReplyTo":null,"subject":"[PATCH v3 0/6] Avoid problem where git_dir is set after alias expansion","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-13T12:04:18Z","receivedAt":"2017-06-13T12:04:34Z","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 v2:\n\n- fixed tyop in the commit message where \"line number\" was lacking the\n  first \"e\"\n\n- added a test for the \"report line number\" fix, and test the `ret` variable\n  to be non-negative (instead of zero)\n\n- dropped 'read_early_config(): optionally return the worktree's\n  top-level directory' as well as most parts of 'alias_lookup():\n  optionally return top-level directory', as we have to run\n  setup_git_directory() in the shell alias code path anyway\n\n\nJohannes Schindelin (6):\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  t1308: relax the test verifying that empty alias values are disallowed\n  t7006: demonstrate a problem with aliases in subdirectories\n  Use the early config machinery to expand aliases\n\n alias.c                | 31 +++++++++++++++++++++-------\n config.c               |  3 ++-\n git.c                  | 55 ++++----------------------------------------------\n help.c                 |  2 +-\n setup.c                |  1 +\n t/t1300-repo-config.sh |  6 ++++++\n t/t1308-config-set.sh  |  4 +++-\n t/t7006-pager.sh       | 11 ++++++++++\n 8 files changed, 52 insertions(+), 61 deletions(-)\n\n\nbase-commit: 41dd4330a1210003bd702ec4a9301ed68e60864d\nPublished-As: https://github.com/dscho/git/releases/tag/alias-early-config-v3\nFetch-It-Via: git fetch https://github.com/dscho/git alias-early-config-v3\n\nInterdiff vs v2:\n $(git diff $rebasedtag..$branchname)\n-- \n2.13.0.windows.1.460.g13f583bedb5\n\n"},{"id":"322078","messageId":"7a43a10ffb2e579a809e3b9cace0be76c8c18ee4.1497355444.git.johannes.schindelin@gmx.de","threadId":"46168","inReplyTo":"cover.1497355444.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 1/6] discover_git_directory(): avoid setting invalid git_dir","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-13T12:04:22Z","receivedAt":"2017-06-13T12:04:35Z","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 <johannes.schindelin@gmx.de>\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":"322079","messageId":"2ecd0c526be76e78a2b7589f465401c3a13d13ca.1497355444.git.johannes.schindelin@gmx.de","threadId":"46168","inReplyTo":"cover.1497355444.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 3/6] help: use early config when autocorrecting aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-13T12:04:28Z","receivedAt":"2017-06-13T12:04:41Z","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 <johannes.schindelin@gmx.de>\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":"322080","messageId":"e9698a39789f51a4e3cdff41bfeeba9fe46be605.1497355444.git.johannes.schindelin@gmx.de","threadId":"46168","inReplyTo":"cover.1497355444.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 2/6] config: report correct line number upon error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-13T12:04:25Z","receivedAt":"2017-06-13T12:04:43Z","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 <johannes.schindelin@gmx.de>\n---\n config.c               | 3 ++-\n t/t1300-repo-config.sh | 6 ++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 146cb3452ad..12c172e298a 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 >= 0)\n+\t\tcf->linenr++;\n \treturn ret;\n }\n \ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 13b7851f7c2..a37ef042221 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 'line number is reported correctly' '\n+\tprintf \"[bool]\\n\\tvar\\n\" >invalid &&\n+\ttest_must_fail git config -f invalid --path bool.var 2>actual &&\n+\ttest_i18ngrep \"line 2\" 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-- \n2.13.0.windows.1.460.g13f583bedb5\n\n\n"},{"id":"322081","messageId":"b1db4c96807da38cfe50148af36faf7ed28d6f4c.1497355444.git.johannes.schindelin@gmx.de","threadId":"46168","inReplyTo":"cover.1497355444.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 4/6] t1308: relax the test verifying that empty alias values are disallowed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-13T12:04:31Z","receivedAt":"2017-06-13T12:04:53Z","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 line number. It is really sufficient to verify that all the relevant\ninformation is given to the user.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\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":"322082","messageId":"b4a9d03aa663c00e3f068ab2e2be37868825ead2.1497355444.git.johannes.schindelin@gmx.de","threadId":"46168","inReplyTo":"cover.1497355444.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 5/6] t7006: demonstrate a problem with aliases in subdirectories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-13T12:04:34Z","receivedAt":"2017-06-13T12:04:55Z","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 <johannes.schindelin@gmx.de>\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":"322083","messageId":"822765b002488f03523bf440097492be3c14931a.1497355444.git.johannes.schindelin@gmx.de","threadId":"46168","inReplyTo":"cover.1497355444.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 6/6] Use the early config machinery to expand aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-13T12:04:39Z","receivedAt":"2017-06-13T12:04:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Instead of discovering the .git/ directory, read the config and then\ntrying to painstakingly reset all the global state if we did not find a\nmatching alias, let's use the early config machinery instead.\n\nIt may look like unnecessary work to discover the .git/ directory in the\nearly config machinery and then call setup_git_directory_gently() in the\ncase of a shell alias, repeating the very same discovery *again*.\nHowever, we have to do this as the early config machinery takes pains\n*not* to touch any global state, while shell aliases expect a possibly\nchanged working directory and at least the GIT_PREFIX and GIT_DIR\nvariables to be set.\n\nAlso, one might be tempted to streamline the code in alias_lookup() to\n*not* use a strbuf for the key. However, if the config reports an error,\nit is far superior to tell the user that the `alias.xyz` key had a\nproblem than to claim that it was the `xyz` key.\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 <johannes.schindelin@gmx.de>\n---\n alias.c          | 31 ++++++++++++++++++++++++-------\n git.c            | 55 ++++---------------------------------------------------\n t/t7006-pager.sh |  2 +-\n 3 files changed, 29 insertions(+), 59 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex 3b90397a99d..6bdc9363037 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 8ff44f081d4..58ef570294d 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,18 @@ 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\tstruct child_process child = CHILD_PROCESS_INIT;\n+\t\t\tint nongit_ok;\n+\n+\t\t\t/* Aliases expect GIT_PREFIX, GIT_DIR etc to be set */\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 +263,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":"322099","messageId":"xmqqk24f97ud.fsf@gitster.mtv.corp.google.com","threadId":"46168","inReplyTo":"822765b002488f03523bf440097492be3c14931a.1497355444.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 6/6] Use the early config machinery to expand aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-13T16:21:30Z","receivedAt":"2017-06-13T16:21:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> Instead of discovering the .git/ directory, read the config and then\n> trying to painstakingly reset all the global state if we did not find a\n> matching alias, let's use the early config machinery instead.\n\ns/read/&ing/, I think.  My reading hiccupped while trying to figure\nout what two alternative approaches are being compared.\n\n> It may look like unnecessary work to discover the .git/ directory in the\n> early config machinery and then call setup_git_directory_gently() in the\n> case of a shell alias, repeating the very same discovery *again*.\n> However, we have to do this as the early config machinery takes pains\n> *not* to touch any global state, while shell aliases expect a possibly\n> changed working directory and at least the GIT_PREFIX and GIT_DIR\n> variables to be set.\n\nMakes sense.  Nicely explained.\n\n> Also, one might be tempted to streamline the code in alias_lookup() to\n> *not* use a strbuf for the key. However, if the config reports an error,\n> it is far superior to tell the user that the `alias.xyz` key had a\n> problem than to claim that it was the `xyz` key.\n\nThe mention of \"streamline\" is puzzling to me.  When we are trying\n\"git xyz\", \"alias.xyz\" is the key we would look up, not \"xyz\"; it is\nnot clear to anybody who didn't read the discussion on v2 (which\nincludes the readers of \"git log\" in a few months) what kind of flawed\nstreamlining could look up \"xyz\" and result in a bad configuration\nreported on it.\n\n>  alias.c          | 31 ++++++++++++++++++++++++-------\n>  git.c            | 55 ++++---------------------------------------------------\n>  t/t7006-pager.sh |  2 +-\n>  3 files changed, 29 insertions(+), 59 deletions(-)\n\nHappy to see the deletion of all the save/restore-env stuff.\n\nExcept for the puzzlement in one paragraph in the log, looks very\ngood.  Thanks for a pleasant reading.\n\n"},{"id":"322128","messageId":"20170613182606.GO154599@google.com","threadId":"46168","inReplyTo":"822765b002488f03523bf440097492be3c14931a.1497355444.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 6/6] Use the early config machinery to expand aliases","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-13T18:26:06Z","receivedAt":"2017-06-13T18:26:15Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/13, Johannes Schindelin wrote:\n> Instead of discovering the .git/ directory, read the config and then\n> trying to painstakingly reset all the global state if we did not find a\n> matching alias, let's use the early config machinery instead.\n> \n> It may look like unnecessary work to discover the .git/ directory in the\n> early config machinery and then call setup_git_directory_gently() in the\n> case of a shell alias, repeating the very same discovery *again*.\n> However, we have to do this as the early config machinery takes pains\n> *not* to touch any global state, while shell aliases expect a possibly\n> changed working directory and at least the GIT_PREFIX and GIT_DIR\n> variables to be set.\n> \n> Also, one might be tempted to streamline the code in alias_lookup() to\n> *not* use a strbuf for the key. However, if the config reports an error,\n> it is far superior to tell the user that the `alias.xyz` key had a\n> problem than to claim that it was the `xyz` key.\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 <johannes.schindelin@gmx.de>\n\nSo because I've been looking at the config machinery lately, I've\nnoticed a lot of issues with how things are handled with respect to\ngitdir vs commondir.  Essentially the config resides at commondir/config\nalways, and only at gitdir/config when not working with a worktree.\nBecause of this, your patches point out a bug in how early config is\nhandled.  I'll illustrate this using aliases.\n\nBefore this series (because aliases are read using the standard config\nmachinery):\n\n  > git init main\n  > git -C main config alias.test '!echo hello'\n  > git -C main test\n    hello\n  > git -C main worktree add ../worktree\n  > git -C worktree test\n    hello\n\nAfter this series (using read_early_config()):\n\n  > git init main\n  > git -C main config alias.test '!echo hello'\n  > git -C main test\n    hello\n  > git -C main worktree add ../worktree\n  > git -C worktree test\n    git: 'test' is not a git command. See 'git --help'.\n\nThe issue is that read_early_config passes the gitdir and not the\ncommondir when reading the config.\n\nThe solution would be to add a 'commondir' field to the config_options\nstruct and populate that before reading the config.  I'm planning on\nfixing this in v2 of my config cleanup series which I'll hopefully have\nfinished by the end of the day.\n\n> ---\n>  alias.c          | 31 ++++++++++++++++++++++++-------\n>  git.c            | 55 ++++---------------------------------------------------\n>  t/t7006-pager.sh |  2 +-\n>  3 files changed, 29 insertions(+), 59 deletions(-)\n> \n> diff --git a/alias.c b/alias.c\n> index 3b90397a99d..6bdc9363037 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 8ff44f081d4..58ef570294d 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,18 @@ 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\tstruct child_process child = CHILD_PROCESS_INIT;\n> +\t\t\tint nongit_ok;\n> +\n> +\t\t\t/* Aliases expect GIT_PREFIX, GIT_DIR etc to be set */\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 +263,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":"322190","messageId":"20170614055824.p2ueyiyfsdf6inve@sigill.intra.peff.net","threadId":"46168","inReplyTo":"20170613182606.GO154599@google.com","subject":"Re: [PATCH v3 6/6] Use the early config machinery to expand aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T05:58:24Z","receivedAt":"2017-06-14T05:58:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 13, 2017 at 11:26:06AM -0700, Brandon Williams wrote:\n\n> So because I've been looking at the config machinery lately, I've\n> noticed a lot of issues with how things are handled with respect to\n> gitdir vs commondir.  Essentially the config resides at commondir/config\n> always, and only at gitdir/config when not working with a worktree.\n> Because of this, your patches point out a bug in how early config is\n> handled.  I'll illustrate this using aliases.\n> \n> Before this series (because aliases are read using the standard config\n> machinery):\n> \n>   > git init main\n>   > git -C main config alias.test '!echo hello'\n>   > git -C main test\n>     hello\n>   > git -C main worktree add ../worktree\n>   > git -C worktree test\n>     hello\n> \n> After this series (using read_early_config()):\n> \n>   > git init main\n>   > git -C main config alias.test '!echo hello'\n>   > git -C main test\n>     hello\n>   > git -C main worktree add ../worktree\n>   > git -C worktree test\n>     git: 'test' is not a git command. See 'git --help'.\n> \n> The issue is that read_early_config passes the gitdir and not the\n> commondir when reading the config.\n\nGood catch.\n\n> The solution would be to add a 'commondir' field to the config_options\n> struct and populate that before reading the config.  I'm planning on\n> fixing this in v2 of my config cleanup series which I'll hopefully have\n> finished by the end of the day.\n\nI think that read_early_config() really meant to set the commondir, as\nit was always about actual config-file lookup. So it was sort-of buggy\nalready, though I suspect it was pretty hard to trigger.\n\nBut I agree that since include_by_gitdir now reads the same struct\nfield, swapping it out fixes the config-reading at the cost of breaking\nthat function. And we really need to pass both in.\n\nI'm not sure whether we should care about this for Dscho's series or\nnot. I think his series _does_ make the problem easier to trigger. But\nit's a minor enough bug that I think I'd be OK with letting your\nsolution proceed independently.\n\n-Peff\n"},{"id":"322192","messageId":"20170614060502.ljuvdxcbara3zfjx@sigill.intra.peff.net","threadId":"46168","inReplyTo":"xmqqk24f97ud.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 6/6] Use the early config machinery to expand aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T06:05:02Z","receivedAt":"2017-06-14T06:05:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 13, 2017 at 09:21:30AM -0700, Junio C Hamano wrote:\n\n> >  alias.c          | 31 ++++++++++++++++++++++++-------\n> >  git.c            | 55 ++++---------------------------------------------------\n> >  t/t7006-pager.sh |  2 +-\n> >  3 files changed, 29 insertions(+), 59 deletions(-)\n> \n> Happy to see the deletion of all the save/restore-env stuff.\n> \n> Except for the puzzlement in one paragraph in the log, looks very\n> good.  Thanks for a pleasant reading.\n\nThe whole thing looks good to me, too, though we should decide what to\ndo with the point that Brandon raised.\n\nAs far as the \"streamline\" puzzlement goes, I'm OK with either of:\n\n  - taking your suggestion from the other thread and actually doing that\n    streamlining, which is one less thing to have to explain\n\n  - leaving the code as-is and tweaking the commit message (though it\n    made sense to me)\n\n-Peff\n"},{"id":"322222","messageId":"alpine.DEB.2.21.1.1706141219260.171564@virtualbox","threadId":"46168","inReplyTo":"xmqqk24f97ud.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 6/6] Use the early config machinery to expand aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T10:20:42Z","receivedAt":"2017-06-14T10:21:00Z","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> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> \n> > Instead of discovering the .git/ directory, read the config and then\n> > trying to painstakingly reset all the global state if we did not find a\n> > matching alias, let's use the early config machinery instead.\n> \n> s/read/&ing/, I think.  My reading hiccupped while trying to figure\n> out what two alternative approaches are being compared.\n\nWhoa. Brainfart on my side. Sorry.\n\n> > Also, one might be tempted to streamline the code in alias_lookup() to\n> > *not* use a strbuf for the key. However, if the config reports an error,\n> > it is far superior to tell the user that the `alias.xyz` key had a\n> > problem than to claim that it was the `xyz` key.\n> \n> The mention of \"streamline\" is puzzling to me.  When we are trying\n> \"git xyz\", \"alias.xyz\" is the key we would look up, not \"xyz\"; it is\n> not clear to anybody who didn't read the discussion on v2 (which\n> includes the readers of \"git log\" in a few months) what kind of flawed\n> streamlining could look up \"xyz\" and result in a bad configuration\n> reported on it.\n\nAs I changed it (thanks for pointing out my mistake to assume that\nskip_prefix() has to change the pointer passed as first parameter), this\nparagraph is now gone anyway.\n\n> >  alias.c          | 31 ++++++++++++++++++++++++-------\n> >  git.c            | 55 ++++---------------------------------------------------\n> >  t/t7006-pager.sh |  2 +-\n> >  3 files changed, 29 insertions(+), 59 deletions(-)\n> \n> Happy to see the deletion of all the save/restore-env stuff.\n\nYep.\n\nCiao,\nDscho\n"},{"id":"322223","messageId":"alpine.DEB.2.21.1.1706141221020.171564@virtualbox","threadId":"46168","inReplyTo":"20170614060502.ljuvdxcbara3zfjx@sigill.intra.peff.net","subject":"Re: [PATCH v3 6/6] Use the early config machinery to expand aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T10:21:42Z","receivedAt":"2017-06-14T10:22:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 14 Jun 2017, Jeff King wrote:\n\n> On Tue, Jun 13, 2017 at 09:21:30AM -0700, Junio C Hamano wrote:\n> \n> > >  alias.c          | 31 ++++++++++++++++++++++++-------\n> > >  git.c            | 55 ++++---------------------------------------------------\n> > >  t/t7006-pager.sh |  2 +-\n> > >  3 files changed, 29 insertions(+), 59 deletions(-)\n> > \n> > Happy to see the deletion of all the save/restore-env stuff.\n> > \n> > Except for the puzzlement in one paragraph in the log, looks very\n> > good.  Thanks for a pleasant reading.\n> \n> The whole thing looks good to me, too, though we should decide what to\n> do with the point that Brandon raised.\n> \n> As far as the \"streamline\" puzzlement goes, I'm OK with either of:\n> \n>   - taking your suggestion from the other thread and actually doing that\n>     streamlining, which is one less thing to have to explain\n> \n>   - leaving the code as-is and tweaking the commit message (though it\n>     made sense to me)\n\nI fixed the thing in the way you suggested (which worked after Junio hit\nme with the cluebat about skip_prefix() and git_config_string()).\n\nCiao,\nDscho\n"},{"id":"322226","messageId":"alpine.DEB.2.21.1.1706141223021.171564@virtualbox","threadId":"46168","inReplyTo":"20170614055824.p2ueyiyfsdf6inve@sigill.intra.peff.net","subject":"Re: [PATCH v3 6/6] Use the early config machinery to expand aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T10:24:36Z","receivedAt":"2017-06-14T10:25:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff & Brandon,\n\nOn Wed, 14 Jun 2017, Jeff King wrote:\n\n> On Tue, Jun 13, 2017 at 11:26:06AM -0700, Brandon Williams wrote:\n> \n> > So because I've been looking at the config machinery lately, I've\n> > noticed a lot of issues with how things are handled with respect to\n> > gitdir vs commondir.  Essentially the config resides at commondir/config\n> > always, and only at gitdir/config when not working with a worktree.\n> > Because of this, your patches point out a bug in how early config is\n> > handled.  I'll illustrate this using aliases.\n> > \n> > Before this series (because aliases are read using the standard config\n> > machinery):\n> > \n> >   > git init main\n> >   > git -C main config alias.test '!echo hello'\n> >   > git -C main test\n> >     hello\n> >   > git -C main worktree add ../worktree\n> >   > git -C worktree test\n> >     hello\n> > \n> > After this series (using read_early_config()):\n> > \n> >   > git init main\n> >   > git -C main config alias.test '!echo hello'\n> >   > git -C main test\n> >     hello\n> >   > git -C main worktree add ../worktree\n> >   > git -C worktree test\n> >     git: 'test' is not a git command. See 'git --help'.\n> > \n> > The issue is that read_early_config passes the gitdir and not the\n> > commondir when reading the config.\n> \n> Good catch.\n\nOh wow.\n\n> > The solution would be to add a 'commondir' field to the config_options\n> > struct and populate that before reading the config.  I'm planning on\n> > fixing this in v2 of my config cleanup series which I'll hopefully have\n> > finished by the end of the day.\n> \n> I think that read_early_config() really meant to set the commondir, as\n> it was always about actual config-file lookup. So it was sort-of buggy\n> already, though I suspect it was pretty hard to trigger.\n> \n> But I agree that since include_by_gitdir now reads the same struct\n> field, swapping it out fixes the config-reading at the cost of breaking\n> that function. And we really need to pass both in.\n> \n> I'm not sure whether we should care about this for Dscho's series or\n> not. I think his series _does_ make the problem easier to trigger. But\n> it's a minor enough bug that I think I'd be OK with letting your\n> solution proceed independently.\n\nBrandon, how hard would it be to build on top of my series? I ask because\nI have to take care of some other things and would not have the time to\nadjust my patch series on top of yours anytime soon.\n\nCiao,\nDscho\n"},{"id":"322267","messageId":"20170614172057.GC55677@google.com","threadId":"46168","inReplyTo":"alpine.DEB.2.21.1.1706141223021.171564@virtualbox","subject":"Re: [PATCH v3 6/6] Use the early config machinery to expand aliases","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-14T17:20:57Z","receivedAt":"2017-06-14T17:21:04Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/14, Johannes Schindelin wrote:\n> Hi Peff & Brandon,\n> \n> On Wed, 14 Jun 2017, Jeff King wrote:\n> \n> > On Tue, Jun 13, 2017 at 11:26:06AM -0700, Brandon Williams wrote:\n> > \n> > > So because I've been looking at the config machinery lately, I've\n> > > noticed a lot of issues with how things are handled with respect to\n> > > gitdir vs commondir.  Essentially the config resides at commondir/config\n> > > always, and only at gitdir/config when not working with a worktree.\n> > > Because of this, your patches point out a bug in how early config is\n> > > handled.  I'll illustrate this using aliases.\n> > > \n> > > Before this series (because aliases are read using the standard config\n> > > machinery):\n> > > \n> > >   > git init main\n> > >   > git -C main config alias.test '!echo hello'\n> > >   > git -C main test\n> > >     hello\n> > >   > git -C main worktree add ../worktree\n> > >   > git -C worktree test\n> > >     hello\n> > > \n> > > After this series (using read_early_config()):\n> > > \n> > >   > git init main\n> > >   > git -C main config alias.test '!echo hello'\n> > >   > git -C main test\n> > >     hello\n> > >   > git -C main worktree add ../worktree\n> > >   > git -C worktree test\n> > >     git: 'test' is not a git command. See 'git --help'.\n> > > \n> > > The issue is that read_early_config passes the gitdir and not the\n> > > commondir when reading the config.\n> > \n> > Good catch.\n> \n> Oh wow.\n> \n> > > The solution would be to add a 'commondir' field to the config_options\n> > > struct and populate that before reading the config.  I'm planning on\n> > > fixing this in v2 of my config cleanup series which I'll hopefully have\n> > > finished by the end of the day.\n> > \n> > I think that read_early_config() really meant to set the commondir, as\n> > it was always about actual config-file lookup. So it was sort-of buggy\n> > already, though I suspect it was pretty hard to trigger.\n> > \n> > But I agree that since include_by_gitdir now reads the same struct\n> > field, swapping it out fixes the config-reading at the cost of breaking\n> > that function. And we really need to pass both in.\n> > \n> > I'm not sure whether we should care about this for Dscho's series or\n> > not. I think his series _does_ make the problem easier to trigger. But\n> > it's a minor enough bug that I think I'd be OK with letting your\n> > solution proceed independently.\n> \n> Brandon, how hard would it be to build on top of my series? I ask because\n> I have to take care of some other things and would not have the time to\n> adjust my patch series on top of yours anytime soon.\n\nIt should be pretty easy to just rebase ontop of your series.  I'll make\nsure to do that before sending out the next revision of mine.\n\n-- \nBrandon Williams\n"}]}