{"thread":{"id":"56881","subject":"[PATCH] sequencer: fix environment that 'exec' commands run under","startedAt":"2021-11-12T17:42:57Z","lastAt":"2021-12-05T08:45:10Z","messageCount":8,"participants":["Elijah Newren via GitGitGadget","Johannes Altmanninger","Phillip Wood","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"440997","messageId":"pull.1134.git.git.1636738973089.gitgitgadget@gmail.com","threadId":"56881","inReplyTo":null,"subject":"[PATCH] sequencer: fix environment that 'exec' commands run under","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-12T17:42:52Z","receivedAt":"2021-11-12T17:42:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nCommands executed from `git rebase --exec` can give different behavior\nfrom within that environment than they would outside of it, due to the\nfact that sequencer.c exports both GIT_DIR and GIT_WORK_TREE.  For\nexample, if the relevant script calls something like\n\n  git -C ../otherdir log --format=%H --no-walk\n\nthe user may be surprised to find that the command above does not show a\ncommit hash from ../otherdir, because $GIT_DIR prevents automatic gitdir\ndetection and makes the -C option useless.\n\nThis is a regression in behavior from the original legacy\nimplemented-in-shell rebase.  It is perhaps rare for it to cause\nproblems in practice, especially since most small problems that were\ncaused by this area of bugs has been fixed-up in the past in a way that\nmasked the particular bug observed without fixing the real underlying\nproblem.\n\nHow we arrived at the current situation is perhaps merited.  The setting\nof GIT_DIR and GIT_WORK_TREE done by sequencer.c arose from a sequence\nof historical accidents:\n\n* When rebase was implemented as a shell command, it would call\n  git-sh-setup, which among other things would set GIT_DIR -- but not\n  export it.  This meant that when rebase --exec commands were run via\n      /bin/sh -c \"$COMMAND\"\n  they would not inherit the GIT_DIR setting.  The fact that GIT_DIR\n  was not set in the run $COMMAND is the behavior we'd like to restore.\n\n* When the rebase--helper builtin was introduced to allow incrementally\n  replacing shell with C code, we were in an implementation that was\n  half shell, half C.  In particular, commit 18633e1a22 (\"rebase -i: use\n  the rebase--helper builtin\", 2017-02-09) added calls to\n      exec git rebase--helper ...\n  which caused rebase--helper to inherit the GIT_DIR environment\n  variable from the shell.  git's setup would change the environment\n  variable from an absolute path to a relative one (\".git\"), but would\n  leave it set.  This meant that when rebase --exec commands were run\n  via\n      run_command_v_opt(...)\n  they would inherit the GIT_DIR setting.\n\n* In commit 09d7b6c6fa (\"sequencer: pass absolute GIT_DIR to exec\n  commands\", 2017-10-31), it was noted that the GIT_DIR caused problems\n  with some commands; e.g.\n      git rebase --exec 'cd subdir && git describe' ...\n  would have GIT_DIR=.git which was invalid due to the change to the\n  subdirectory.  Instead of questioning why GIT_DIR was set, that commit\n  instead made sequencer change GIT_DIR to be an absolute path and\n  explicitly export it via\n      argv_array_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n      run_command_v_opt_cd_env(..., child_env.argv)\n\n* In commit ab5e67d751 (\"sequencer: pass absolute GIT_WORK_TREE to exec\n  commands\", 2018-07-14), it was noted that when GIT_DIR is set but\n  GIT_WORK_TREE is not, that we do not discover GIT_WORK_TREE but just\n  assume it is '.'.  That is incorrect if trying to run commands from a\n  subdirectory.  However, rather than question why GIT_DIR was set, that\n  commit instead also added GIT_WORK_TREE to the list of things to\n  export.\n\nEach of the above problems would have been fixed automatically when\ngit-rebase become a full builtin, had it not been for the fact that\nsequencer.c started exporting GIT_DIR and GIT_WORK_TREE in the interim.\nStop exporting them now.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\nAcked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    sequencer: fix environment that 'exec' commands run under\n    \n    I'm not sure if the added regression tests make sense, or if t3409 is\n    the best place to put them (taking over a recently removed t3409 that\n    was used for the deprecated preserve merges option).\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1134%2Fnewren%2Ffix-rebase-exec-environ-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1134/newren/fix-rebase-exec-environ-v1\nPull-Request: https://github.com/git/git/pull/1134\n\n sequencer.c               |  9 +--------\n t/t3409-rebase-environ.sh | 23 +++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 8 deletions(-)\n create mode 100755 t/t3409-rebase-environ.sh\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ea96837cde3..9afdbe3e3d1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3495,17 +3495,12 @@ static int error_failed_squash(struct repository *r,\n \n static int do_exec(struct repository *r, const char *command_line)\n {\n-\tstruct strvec child_env = STRVEC_INIT;\n \tconst char *child_argv[] = { NULL, NULL };\n \tint dirty, status;\n \n \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n \tchild_argv[0] = command_line;\n-\tstrvec_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n-\tstrvec_pushf(&child_env, \"GIT_WORK_TREE=%s\",\n-\t\t     absolute_path(get_git_work_tree()));\n-\tstatus = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,\n-\t\t\t\t\t  child_env.v);\n+\tstatus = run_command_v_opt(child_argv, RUN_USING_SHELL);\n \n \t/* force re-reading of the cache */\n \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0)\n@@ -3535,8 +3530,6 @@ static int do_exec(struct repository *r, const char *command_line)\n \t\tstatus = 1;\n \t}\n \n-\tstrvec_clear(&child_env);\n-\n \treturn status;\n }\n \ndiff --git a/t/t3409-rebase-environ.sh b/t/t3409-rebase-environ.sh\nnew file mode 100755\nindex 00000000000..83ffb39d9ff\n--- /dev/null\n+++ b/t/t3409-rebase-environ.sh\n@@ -0,0 +1,23 @@\n+#!/bin/sh\n+\n+test_description='git rebase interactive environment'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\ttest_commit three\n+'\n+\n+test_expect_success 'rebase --exec does not muck with GIT_DIR' '\n+\tgit rebase --exec \"printf %s \\$GIT_DIR >environ\" HEAD~1 &&\n+\ttest_must_be_empty environ\n+'\n+\n+test_expect_success 'rebase --exec does not muck with GIT_WORK_TREE' '\n+\tgit rebase --exec \"printf %s \\$GIT_WORK_TREE >environ\" HEAD~1 &&\n+\ttest_must_be_empty environ\n+'\n+\n+test_done\n\nbase-commit: 88d915a634b449147855041d44875322de2b286d\n-- \ngitgitgadget\n"},{"id":"441098","messageId":"20211114202123.b2zzcso4ne7hwjvi@gmail.com","threadId":"56881","inReplyTo":"pull.1134.git.git.1636738973089.gitgitgadget@gmail.com","subject":"Re: [PATCH] sequencer: fix environment that 'exec' commands run under","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-11-14T20:21:23Z","receivedAt":"2021-11-14T20:21:32Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Fri, Nov 12, 2021 at 05:42:52PM +0000, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren <newren@gmail.com>\n> \n> Commands executed from `git rebase --exec` can give different behavior\n> from within that environment than they would outside of it, due to the\n> fact that sequencer.c exports both GIT_DIR and GIT_WORK_TREE.  For\n> example, if the relevant script calls something like\n> \n>   git -C ../otherdir log --format=%H --no-walk\n> \n> the user may be surprised to find that the command above does not show a\n> commit hash from ../otherdir, because $GIT_DIR prevents automatic gitdir\n> detection and makes the -C option useless.\n\nYep. I've had a case where \"git rebase -x 'make test'\" would fail because\n\"make test\" tries to run some Git commands in a temporary repo.  The workaround\nwas to unset all GIT_* environment variables, just like t/test-lib.sh does.\n\nI had the same problem when testing shell completion because git prepends\n$(git --exec-path) to $PATH.  I don't see a good reason why \"git rebase -x\ncmd\" passes a modified $PATH (and $GIT_EXEC_PATH) to cmd. The user is back in\ncontrol, so I'd expect the same environment as for the parent rebase process.\n\nAFAICT, the main purpose of changing $PATH is to ease (cross-language)\nimplementation, I don't think the user is meant to notice.\nOf course, exporting GIT_EXEC_PATH is desirable for some commands like gc\nthat delegate to a bunch of git processes without user interaction, to make\nsure all children are from the same build. c90d565a46 (Propagate --exec-path\nsetting to external commands via GIT_EXEC_PATH, 2009-03-21). But\nI don't think the same applies for rebase -x.\n\n> \n> This is a regression in behavior from the original legacy\n> implemented-in-shell rebase.  It is perhaps rare for it to cause\n> problems in practice, especially since most small problems that were\n> caused by this area of bugs has been fixed-up in the past in a way that\n> masked the particular bug observed without fixing the real underlying\n> problem.\n\nReally interesting that we added multiple workarounds, when we just needed\nto stop polluting the environment.  The fact that our test suite\nunsets most GIT_* variables also masks potential problems.\n\n> \n> How we arrived at the current situation is perhaps merited.  The setting\n\nThis sounds weirdly penitent if read the wrong way. I guess\n\"An explanation how we arrived ... \"?\n\n> of GIT_DIR and GIT_WORK_TREE done by sequencer.c arose from a sequence\n> of historical accidents:\n> \n> * When rebase was implemented as a shell command, it would call\n>   git-sh-setup, which among other things would set GIT_DIR -- but not\n>   export it.  This meant that when rebase --exec commands were run via\n>       /bin/sh -c \"$COMMAND\"\n>   they would not inherit the GIT_DIR setting.  The fact that GIT_DIR\n>   was not set in the run $COMMAND is the behavior we'd like to restore.\n> \n> * When the rebase--helper builtin was introduced to allow incrementally\n>   replacing shell with C code, we were in an implementation that was\n\nMore correct to say \"we were in an (implementation) state\" or better:\n\"we had an implementation\"?\n\n>   half shell, half C.  In particular, commit 18633e1a22 (\"rebase -i: use\n>   the rebase--helper builtin\", 2017-02-09) added calls to\n>       exec git rebase--helper ...\n\nOk I guess this was a good reason for adding the exec-path to the environment\nof exec steps.\n\n>   which caused rebase--helper to inherit the GIT_DIR environment\n>   variable from the shell.  git's setup would change the environment\n>   variable from an absolute path to a relative one (\".git\"), but would\n>   leave it set.  This meant that when rebase --exec commands were run\n>   via\n>       run_command_v_opt(...)\n>   they would inherit the GIT_DIR setting.\n> \n> * In commit 09d7b6c6fa (\"sequencer: pass absolute GIT_DIR to exec\n>   commands\", 2017-10-31), it was noted that the GIT_DIR caused problems\n>   with some commands; e.g.\n>       git rebase --exec 'cd subdir && git describe' ...\n>   would have GIT_DIR=.git which was invalid due to the change to the\n>   subdirectory.  Instead of questioning why GIT_DIR was set, that commit\n>   instead made sequencer change GIT_DIR to be an absolute path and\n>   explicitly export it via\n>       argv_array_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n>       run_command_v_opt_cd_env(..., child_env.argv)\n> \n> * In commit ab5e67d751 (\"sequencer: pass absolute GIT_WORK_TREE to exec\n>   commands\", 2018-07-14), it was noted that when GIT_DIR is set but\n>   GIT_WORK_TREE is not, that we do not discover GIT_WORK_TREE but just\n>   assume it is '.'.  That is incorrect if trying to run commands from a\n>   subdirectory.  However, rather than question why GIT_DIR was set, that\n>   commit instead also added GIT_WORK_TREE to the list of things to\n>   export.\n> \n> Each of the above problems would have been fixed automatically when\n> git-rebase become a full builtin, had it not been for the fact that\n\ns/become/became/\n\n> sequencer.c started exporting GIT_DIR and GIT_WORK_TREE in the interim.\n> Stop exporting them now.\n> \n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>     sequencer: fix environment that 'exec' commands run under\n\nMaybe: sequencer: do not export GIT_DIR and GIT_WORK_TREE to exec\n(assuming that $PATH also needs fixing)\n\n>     \n>     I'm not sure if the added regression tests make sense\n\nThey are simple and correct.\nWe still pass on values from --git-dir= and GIT_DIR=. git.\nThose are /probably/ right.\n\n>     or if t3409 is the best place to put them (taking over a recently\n>     removed t3409 that was used for the deprecated preserve merges option).\n\nLooks like there is no t/t34*-rebase-exec.sh yet. Most tests of --exec are\nin t/t3404-rebase-interactive.sh, but we don't need the interactive bit here.\n\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1134%2Fnewren%2Ffix-rebase-exec-environ-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1134/newren/fix-rebase-exec-environ-v1\n> Pull-Request: https://github.com/git/git/pull/1134\n> \n>  sequencer.c               |  9 +--------\n>  t/t3409-rebase-environ.sh | 23 +++++++++++++++++++++++\n>  2 files changed, 24 insertions(+), 8 deletions(-)\n>  create mode 100755 t/t3409-rebase-environ.sh\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index ea96837cde3..9afdbe3e3d1 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3495,17 +3495,12 @@ static int error_failed_squash(struct repository *r,\n>  \n>  static int do_exec(struct repository *r, const char *command_line)\n>  {\n> -\tstruct strvec child_env = STRVEC_INIT;\n>  \tconst char *child_argv[] = { NULL, NULL };\n>  \tint dirty, status;\n>  \n>  \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n>  \tchild_argv[0] = command_line;\n> -\tstrvec_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n> -\tstrvec_pushf(&child_env, \"GIT_WORK_TREE=%s\",\n> -\t\t     absolute_path(get_git_work_tree()));\n> -\tstatus = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,\n> -\t\t\t\t\t  child_env.v);\n> +\tstatus = run_command_v_opt(child_argv, RUN_USING_SHELL);\n>  \n>  \t/* force re-reading of the cache */\n>  \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0)\n> @@ -3535,8 +3530,6 @@ static int do_exec(struct repository *r, const char *command_line)\n>  \t\tstatus = 1;\n>  \t}\n>  \n> -\tstrvec_clear(&child_env);\n> -\n>  \treturn status;\n>  }\n>  \n> diff --git a/t/t3409-rebase-environ.sh b/t/t3409-rebase-environ.sh\n> new file mode 100755\n> index 00000000000..83ffb39d9ff\n> --- /dev/null\n> +++ b/t/t3409-rebase-environ.sh\n> @@ -0,0 +1,23 @@\n> +#!/bin/sh\n> +\n> +test_description='git rebase interactive environment'\n\nmaybe test_description='git rebase --exec environment'\n\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\ttest_commit one &&\n> +\ttest_commit two &&\n> +\ttest_commit three\n> +'\n> +\n> +test_expect_success 'rebase --exec does not muck with GIT_DIR' '\n> +\tgit rebase --exec \"printf %s \\$GIT_DIR >environ\" HEAD~1 &&\n> +\ttest_must_be_empty environ\n> +'\n> +\n> +test_expect_success 'rebase --exec does not muck with GIT_WORK_TREE' '\n> +\tgit rebase --exec \"printf %s \\$GIT_WORK_TREE >environ\" HEAD~1 &&\n> +\ttest_must_be_empty environ\n> +'\n\nI guess we could add a test for existing GIT_DIR in the environment, even\nif it's not affected\n\n\ttest_expect_success 'already exported GIT_DIR is passed on to rebase --exec commands' '\n\t\tGIT_DIR=.git GIT_WORK_TREE=. git rebase HEAD~1 --exec \\\n\t\t\t\"printf %s\\\\\\\\n \\\"\\$GIT_DIR\\\" \\\"\\$GIT_WORK_TREE\\\" >actual\" &&\n\t\tcat >expect <<-EOF &&\n\t\t.git\n\t\t.\n\t\tEOF\n\t\ttest_cmp expect actual &&\n\t\tcat actual\n\t'\n\n(I tried to use non-default values lik ./.git and ./. but the weird thing\nis that git canonicalizes the worktree but not the git dir, so meh)\n\nI also wasn't sure about the behavior of --git-dir= Should it be the same as GIT_DIR=?\nI think it's also conceivable that --git-dir= does *not* cause GIT_DIR to\nbe exported to exec commands, though that might break existing\nscripts. Something like\n\n\tgit --work-tree=../other-worktree --git-dir=../other-worktree/.git \\\n\t\trebase --exec \"make generate-documentation && git commit -a --amend --no-edit\"\n\n(needless to say that in this case \"git -C ../other-worktree\" is probably\nwhat the user wants)\n"},{"id":"441254","messageId":"pull.1134.v2.git.git.1637041986945.gitgitgadget@gmail.com","threadId":"56881","inReplyTo":"pull.1134.git.git.1636738973089.gitgitgadget@gmail.com","subject":"[PATCH v2] sequencer: do not export GIT_DIR and GIT_WORK_TREE for 'exec'","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-16T05:53:06Z","receivedAt":"2021-11-16T06:01:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nCommands executed from `git rebase --exec` can give different behavior\nfrom within that environment than they would outside of it, due to the\nfact that sequencer.c exports both GIT_DIR and GIT_WORK_TREE.  For\nexample, if the relevant script calls something like\n\n  git -C ../otherdir log --format=%H --no-walk\n\nthe user may be surprised to find that the command above does not show a\ncommit hash from ../otherdir, because $GIT_DIR prevents automatic gitdir\ndetection and makes the -C option useless.\n\nThis is a regression in behavior from the original legacy\nimplemented-in-shell rebase.  It is perhaps rare for it to cause\nproblems in practice, especially since most small problems that were\ncaused by this area of bugs has been fixed-up in the past in a way that\nmasked the particular bug observed without fixing the real underlying\nproblem.\n\nAn explanation of how we arrived at the current situation is perhaps\nmerited.  The setting of GIT_DIR and GIT_WORK_TREE done by sequencer.c\narose from a sequence of historical accidents:\n\n* When rebase was implemented as a shell command, it would call\n  git-sh-setup, which among other things would set GIT_DIR -- but not\n  export it.  This meant that when rebase --exec commands were run via\n      /bin/sh -c \"$COMMAND\"\n  they would not inherit the GIT_DIR setting.  The fact that GIT_DIR\n  was not set in the run $COMMAND is the behavior we'd like to restore.\n\n* When the rebase--helper builtin was introduced to allow incrementally\n  replacing shell with C code, we had an implementation that was half\n  shell, half C.  In particular, commit 18633e1a22 (\"rebase -i: use the\n  rebase--helper builtin\", 2017-02-09) added calls to\n      exec git rebase--helper ...\n  which caused rebase--helper to inherit the GIT_DIR environment\n  variable from the shell.  git's setup would change the environment\n  variable from an absolute path to a relative one (\".git\"), but would\n  leave it set.  This meant that when rebase --exec commands were run\n  via\n      run_command_v_opt(...)\n  they would inherit the GIT_DIR setting.\n\n* In commit 09d7b6c6fa (\"sequencer: pass absolute GIT_DIR to exec\n  commands\", 2017-10-31), it was noted that the GIT_DIR caused problems\n  with some commands; e.g.\n      git rebase --exec 'cd subdir && git describe' ...\n  would have GIT_DIR=.git which was invalid due to the change to the\n  subdirectory.  Instead of questioning why GIT_DIR was set, that commit\n  instead made sequencer change GIT_DIR to be an absolute path and\n  explicitly export it via\n      argv_array_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n      run_command_v_opt_cd_env(..., child_env.argv)\n\n* In commit ab5e67d751 (\"sequencer: pass absolute GIT_WORK_TREE to exec\n  commands\", 2018-07-14), it was noted that when GIT_DIR is set but\n  GIT_WORK_TREE is not, that we do not discover GIT_WORK_TREE but just\n  assume it is '.'.  That is incorrect if trying to run commands from a\n  subdirectory.  However, rather than question why GIT_DIR was set, that\n  commit instead also added GIT_WORK_TREE to the list of things to\n  export.\n\nEach of the above problems would have been fixed automatically when\ngit-rebase became a full builtin, had it not been for the fact that\nsequencer.c started exporting GIT_DIR and GIT_WORK_TREE in the interim.\nStop exporting them now.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\nAcked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    sequencer: fix environment that 'exec' commands run under\n    \n    Changes since v1:\n    \n     * Fix wording in multiple locations pointed out by Johannes\n       Altmanninger\n    \n    Note that Johaness Altmanninger also suggested some additional\n    GIT_DIR/GIT_WORK_TREE and --git-dir/--work-tree testcases, but since\n    those change the current working directory to the work tree so that\n    GIT_WORK_TREE ends up being '.' in every case, I think it's straying a\n    bit from the point of the test. If other feel strongly, I can add them\n    in.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1134%2Fnewren%2Ffix-rebase-exec-environ-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1134/newren/fix-rebase-exec-environ-v2\nPull-Request: https://github.com/git/git/pull/1134\n\nRange-diff vs v1:\n\n 1:  12713c83705 ! 1:  c647c45375a sequencer: fix environment that 'exec' commands run under\n     @@ Metadata\n      Author: Elijah Newren <newren@gmail.com>\n      \n       ## Commit message ##\n     -    sequencer: fix environment that 'exec' commands run under\n     +    sequencer: do not export GIT_DIR and GIT_WORK_TREE for 'exec'\n      \n          Commands executed from `git rebase --exec` can give different behavior\n          from within that environment than they would outside of it, due to the\n     @@ Commit message\n          masked the particular bug observed without fixing the real underlying\n          problem.\n      \n     -    How we arrived at the current situation is perhaps merited.  The setting\n     -    of GIT_DIR and GIT_WORK_TREE done by sequencer.c arose from a sequence\n     -    of historical accidents:\n     +    An explanation of how we arrived at the current situation is perhaps\n     +    merited.  The setting of GIT_DIR and GIT_WORK_TREE done by sequencer.c\n     +    arose from a sequence of historical accidents:\n      \n          * When rebase was implemented as a shell command, it would call\n            git-sh-setup, which among other things would set GIT_DIR -- but not\n     @@ Commit message\n            was not set in the run $COMMAND is the behavior we'd like to restore.\n      \n          * When the rebase--helper builtin was introduced to allow incrementally\n     -      replacing shell with C code, we were in an implementation that was\n     -      half shell, half C.  In particular, commit 18633e1a22 (\"rebase -i: use\n     -      the rebase--helper builtin\", 2017-02-09) added calls to\n     +      replacing shell with C code, we had an implementation that was half\n     +      shell, half C.  In particular, commit 18633e1a22 (\"rebase -i: use the\n     +      rebase--helper builtin\", 2017-02-09) added calls to\n                exec git rebase--helper ...\n            which caused rebase--helper to inherit the GIT_DIR environment\n            variable from the shell.  git's setup would change the environment\n     @@ Commit message\n            export.\n      \n          Each of the above problems would have been fixed automatically when\n     -    git-rebase become a full builtin, had it not been for the fact that\n     +    git-rebase became a full builtin, had it not been for the fact that\n          sequencer.c started exporting GIT_DIR and GIT_WORK_TREE in the interim.\n          Stop exporting them now.\n      \n\n\n sequencer.c               |  9 +--------\n t/t3409-rebase-environ.sh | 23 +++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 8 deletions(-)\n create mode 100755 t/t3409-rebase-environ.sh\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ea96837cde3..9afdbe3e3d1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3495,17 +3495,12 @@ static int error_failed_squash(struct repository *r,\n \n static int do_exec(struct repository *r, const char *command_line)\n {\n-\tstruct strvec child_env = STRVEC_INIT;\n \tconst char *child_argv[] = { NULL, NULL };\n \tint dirty, status;\n \n \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n \tchild_argv[0] = command_line;\n-\tstrvec_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n-\tstrvec_pushf(&child_env, \"GIT_WORK_TREE=%s\",\n-\t\t     absolute_path(get_git_work_tree()));\n-\tstatus = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,\n-\t\t\t\t\t  child_env.v);\n+\tstatus = run_command_v_opt(child_argv, RUN_USING_SHELL);\n \n \t/* force re-reading of the cache */\n \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0)\n@@ -3535,8 +3530,6 @@ static int do_exec(struct repository *r, const char *command_line)\n \t\tstatus = 1;\n \t}\n \n-\tstrvec_clear(&child_env);\n-\n \treturn status;\n }\n \ndiff --git a/t/t3409-rebase-environ.sh b/t/t3409-rebase-environ.sh\nnew file mode 100755\nindex 00000000000..83ffb39d9ff\n--- /dev/null\n+++ b/t/t3409-rebase-environ.sh\n@@ -0,0 +1,23 @@\n+#!/bin/sh\n+\n+test_description='git rebase interactive environment'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\ttest_commit three\n+'\n+\n+test_expect_success 'rebase --exec does not muck with GIT_DIR' '\n+\tgit rebase --exec \"printf %s \\$GIT_DIR >environ\" HEAD~1 &&\n+\ttest_must_be_empty environ\n+'\n+\n+test_expect_success 'rebase --exec does not muck with GIT_WORK_TREE' '\n+\tgit rebase --exec \"printf %s \\$GIT_WORK_TREE >environ\" HEAD~1 &&\n+\ttest_must_be_empty environ\n+'\n+\n+test_done\n\nbase-commit: 88d915a634b449147855041d44875322de2b286d\n-- \ngitgitgadget\n"},{"id":"441255","messageId":"20211116060635.uy5ry5huwxo2hsxy@gmail.com","threadId":"56881","inReplyTo":"pull.1134.v2.git.git.1637041986945.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] sequencer: do not export GIT_DIR and GIT_WORK_TREE for 'exec'","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-11-16T06:06:35Z","receivedAt":"2021-11-16T06:06:45Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Tue, Nov 16, 2021 at 05:53:06AM +0000, Elijah Newren via GitGitGadget wrote:\n>     sequencer: fix environment that 'exec' commands run under\n>     \n>     Changes since v1:\n>     \n>      * Fix wording in multiple locations pointed out by Johannes\n>        Altmanninger\n>     \n>     Note that Johaness Altmanninger also suggested some additional\n>     GIT_DIR/GIT_WORK_TREE and --git-dir/--work-tree testcases, but since\n>     those change the current working directory to the work tree so that\n>     GIT_WORK_TREE ends up being '.' in every case, I think it's straying a\n>     bit from the point of the test. If other feel strongly, I can add them\n>     in.\n\nYeah I also think it's better to leave them out for now. For one, the\nGIT_DIR/GIT_WORK_TREE test is not really interesting. Next, things are not so\nobivous: I'm not sure if --git-dir/--work-tree should export those variables\nto exec but that can be investigated later.\n\nThis patch looks great.\n"},{"id":"441288","messageId":"eec6add8-2935-3692-fa7e-c40cc765b675@gmail.com","threadId":"56881","inReplyTo":"pull.1134.v2.git.git.1637041986945.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] sequencer: do not export GIT_DIR and GIT_WORK_TREE for 'exec'","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-11-16T09:59:45Z","receivedAt":"2021-11-16T10:00:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 16/11/2021 05:53, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren <newren@gmail.com>\n> \n> Commands executed from `git rebase --exec` can give different behavior\n> from within that environment than they would outside of it, due to the\n> fact that sequencer.c exports both GIT_DIR and GIT_WORK_TREE.  For\n> example, if the relevant script calls something like\n> \n>    git -C ../otherdir log --format=%H --no-walk\n> \n> the user may be surprised to find that the command above does not show a\n> commit hash from ../otherdir, because $GIT_DIR prevents automatic gitdir\n> detection and makes the -C option useless.\n> \n> This is a regression in behavior from the original legacy\n> implemented-in-shell rebase.  It is perhaps rare for it to cause\n> problems in practice, especially since most small problems that were\n> caused by this area of bugs has been fixed-up in the past in a way that\n> masked the particular bug observed without fixing the real underlying\n> problem.\n> \n> An explanation of how we arrived at the current situation is perhaps\n> merited.  The setting of GIT_DIR and GIT_WORK_TREE done by sequencer.c\n> arose from a sequence of historical accidents:\n> \n> * When rebase was implemented as a shell command, it would call\n>    git-sh-setup, which among other things would set GIT_DIR -- but not\n>    export it.  This meant that when rebase --exec commands were run via\n>        /bin/sh -c \"$COMMAND\"\n>    they would not inherit the GIT_DIR setting.  The fact that GIT_DIR\n>    was not set in the run $COMMAND is the behavior we'd like to restore.\n> \n> * When the rebase--helper builtin was introduced to allow incrementally\n>    replacing shell with C code, we had an implementation that was half\n>    shell, half C.  In particular, commit 18633e1a22 (\"rebase -i: use the\n>    rebase--helper builtin\", 2017-02-09) added calls to\n>        exec git rebase--helper ...\n>    which caused rebase--helper to inherit the GIT_DIR environment\n>    variable from the shell.  git's setup would change the environment\n>    variable from an absolute path to a relative one (\".git\"), but would\n>    leave it set.  This meant that when rebase --exec commands were run\n>    via\n>        run_command_v_opt(...)\n>    they would inherit the GIT_DIR setting.\n> \n> * In commit 09d7b6c6fa (\"sequencer: pass absolute GIT_DIR to exec\n>    commands\", 2017-10-31), it was noted that the GIT_DIR caused problems\n>    with some commands; e.g.\n>        git rebase --exec 'cd subdir && git describe' ...\n>    would have GIT_DIR=.git which was invalid due to the change to the\n>    subdirectory.  Instead of questioning why GIT_DIR was set, that commit\n>    instead made sequencer change GIT_DIR to be an absolute path and\n>    explicitly export it via\n>        argv_array_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n>        run_command_v_opt_cd_env(..., child_env.argv)\n> \n> * In commit ab5e67d751 (\"sequencer: pass absolute GIT_WORK_TREE to exec\n>    commands\", 2018-07-14), it was noted that when GIT_DIR is set but\n>    GIT_WORK_TREE is not, that we do not discover GIT_WORK_TREE but just\n>    assume it is '.'.  That is incorrect if trying to run commands from a\n>    subdirectory.  However, rather than question why GIT_DIR was set, that\n>    commit instead also added GIT_WORK_TREE to the list of things to\n>    export.\n> \n> Each of the above problems would have been fixed automatically when\n> git-rebase became a full builtin, had it not been for the fact that\n> sequencer.c started exporting GIT_DIR and GIT_WORK_TREE in the interim.\n> Stop exporting them now.\n\nThanks for fixing this. The commit message does a great job of \nexplaining the problem and how we got there. The patch looks fine to me \nthough I think all the exec tests live in t3404 (interactive and non \ninteractive) so it feels a bit strange to add a new test file just for \nthis. There are plenty of uses of --exec in other test files but I think \nthey are just using the exec command as part of the test rather than \ntesting the --exec functionality (with the exception of t3418 which \ntests --reschedule-failed-exec)\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>      sequencer: fix environment that 'exec' commands run under\n>      \n>      Changes since v1:\n>      \n>       * Fix wording in multiple locations pointed out by Johannes\n>         Altmanninger\n>      \n>      Note that Johaness Altmanninger also suggested some additional\n>      GIT_DIR/GIT_WORK_TREE and --git-dir/--work-tree testcases, but since\n>      those change the current working directory to the work tree so that\n>      GIT_WORK_TREE ends up being '.' in every case, I think it's straying a\n>      bit from the point of the test. If other feel strongly, I can add them\n>      in.\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1134%2Fnewren%2Ffix-rebase-exec-environ-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1134/newren/fix-rebase-exec-environ-v2\n> Pull-Request: https://github.com/git/git/pull/1134\n> \n> Range-diff vs v1:\n> \n>   1:  12713c83705 ! 1:  c647c45375a sequencer: fix environment that 'exec' commands run under\n>       @@ Metadata\n>        Author: Elijah Newren <newren@gmail.com>\n>        \n>         ## Commit message ##\n>       -    sequencer: fix environment that 'exec' commands run under\n>       +    sequencer: do not export GIT_DIR and GIT_WORK_TREE for 'exec'\n>        \n>            Commands executed from `git rebase --exec` can give different behavior\n>            from within that environment than they would outside of it, due to the\n>       @@ Commit message\n>            masked the particular bug observed without fixing the real underlying\n>            problem.\n>        \n>       -    How we arrived at the current situation is perhaps merited.  The setting\n>       -    of GIT_DIR and GIT_WORK_TREE done by sequencer.c arose from a sequence\n>       -    of historical accidents:\n>       +    An explanation of how we arrived at the current situation is perhaps\n>       +    merited.  The setting of GIT_DIR and GIT_WORK_TREE done by sequencer.c\n>       +    arose from a sequence of historical accidents:\n>        \n>            * When rebase was implemented as a shell command, it would call\n>              git-sh-setup, which among other things would set GIT_DIR -- but not\n>       @@ Commit message\n>              was not set in the run $COMMAND is the behavior we'd like to restore.\n>        \n>            * When the rebase--helper builtin was introduced to allow incrementally\n>       -      replacing shell with C code, we were in an implementation that was\n>       -      half shell, half C.  In particular, commit 18633e1a22 (\"rebase -i: use\n>       -      the rebase--helper builtin\", 2017-02-09) added calls to\n>       +      replacing shell with C code, we had an implementation that was half\n>       +      shell, half C.  In particular, commit 18633e1a22 (\"rebase -i: use the\n>       +      rebase--helper builtin\", 2017-02-09) added calls to\n>                  exec git rebase--helper ...\n>              which caused rebase--helper to inherit the GIT_DIR environment\n>              variable from the shell.  git's setup would change the environment\n>       @@ Commit message\n>              export.\n>        \n>            Each of the above problems would have been fixed automatically when\n>       -    git-rebase become a full builtin, had it not been for the fact that\n>       +    git-rebase became a full builtin, had it not been for the fact that\n>            sequencer.c started exporting GIT_DIR and GIT_WORK_TREE in the interim.\n>            Stop exporting them now.\n>        \n> \n> \n>   sequencer.c               |  9 +--------\n>   t/t3409-rebase-environ.sh | 23 +++++++++++++++++++++++\n>   2 files changed, 24 insertions(+), 8 deletions(-)\n>   create mode 100755 t/t3409-rebase-environ.sh\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index ea96837cde3..9afdbe3e3d1 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3495,17 +3495,12 @@ static int error_failed_squash(struct repository *r,\n>   \n>   static int do_exec(struct repository *r, const char *command_line)\n>   {\n> -\tstruct strvec child_env = STRVEC_INIT;\n>   \tconst char *child_argv[] = { NULL, NULL };\n>   \tint dirty, status;\n>   \n>   \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n>   \tchild_argv[0] = command_line;\n> -\tstrvec_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n> -\tstrvec_pushf(&child_env, \"GIT_WORK_TREE=%s\",\n> -\t\t     absolute_path(get_git_work_tree()));\n> -\tstatus = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,\n> -\t\t\t\t\t  child_env.v);\n> +\tstatus = run_command_v_opt(child_argv, RUN_USING_SHELL);\n>   \n>   \t/* force re-reading of the cache */\n>   \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0)\n> @@ -3535,8 +3530,6 @@ static int do_exec(struct repository *r, const char *command_line)\n>   \t\tstatus = 1;\n>   \t}\n>   \n> -\tstrvec_clear(&child_env);\n> -\n>   \treturn status;\n>   }\n>   \n> diff --git a/t/t3409-rebase-environ.sh b/t/t3409-rebase-environ.sh\n> new file mode 100755\n> index 00000000000..83ffb39d9ff\n> --- /dev/null\n> +++ b/t/t3409-rebase-environ.sh\n> @@ -0,0 +1,23 @@\n> +#!/bin/sh\n> +\n> +test_description='git rebase interactive environment'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\ttest_commit one &&\n> +\ttest_commit two &&\n> +\ttest_commit three\n> +'\n> +\n> +test_expect_success 'rebase --exec does not muck with GIT_DIR' '\n> +\tgit rebase --exec \"printf %s \\$GIT_DIR >environ\" HEAD~1 &&\n> +\ttest_must_be_empty environ\n> +'\n> +\n> +test_expect_success 'rebase --exec does not muck with GIT_WORK_TREE' '\n> +\tgit rebase --exec \"printf %s \\$GIT_WORK_TREE >environ\" HEAD~1 &&\n> +\ttest_must_be_empty environ\n> +'\n> +\n> +test_done\n> \n> base-commit: 88d915a634b449147855041d44875322de2b286d\n> \n"},{"id":"442181","messageId":"CABPp-BHUQQiGxfY+spGT3CShP4aaETwYJcFgknPBQ4XhBwKQ2A@mail.gmail.com","threadId":"56881","inReplyTo":"20211114202123.b2zzcso4ne7hwjvi@gmail.com","subject":"Re: [PATCH] sequencer: fix environment that 'exec' commands run under","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-11-23T17:48:03Z","receivedAt":"2021-11-23T17:48:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"// Resending this email from 8 days ago; Dscho pointed that I accidentally\n// responded to the GitHub PR email instead of the original email, and\n// while Johannes already said he likes my V2, perhaps there are other\n// comments here that benefit other current or future reviewers\n\nHi Johannes,\n\nThanks for looking over the patch and commit message closely; very cool.\n\nOn Sun, Nov 14, 2021 at 12:21 PM Johannes Altmanninger\n<aclopte@gmail.com> wrote:\n>\n> On Fri, Nov 12, 2021 at 05:42:52PM +0000, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren <newren@gmail.com>\n> >\n> > Commands executed from `git rebase --exec` can give different behavior\n> > from within that environment than they would outside of it, due to the\n> > fact that sequencer.c exports both GIT_DIR and GIT_WORK_TREE.  For\n> > example, if the relevant script calls something like\n> >\n> >   git -C ../otherdir log --format=%H --no-walk\n> >\n> > the user may be surprised to find that the command above does not show a\n> > commit hash from ../otherdir, because $GIT_DIR prevents automatic gitdir\n> > detection and makes the -C option useless.\n>\n> Yep. I've had a case where \"git rebase -x 'make test'\" would fail because\n> \"make test\" tries to run some Git commands in a temporary repo.  The workaround\n> was to unset all GIT_* environment variables, just like t/test-lib.sh does.\n>\n> I had the same problem when testing shell completion because git prepends\n> $(git --exec-path) to $PATH.  I don't see a good reason why \"git rebase -x\n> cmd\" passes a modified $PATH (and $GIT_EXEC_PATH) to cmd. The user is back in\n> control, so I'd expect the same environment as for the parent rebase process.\n>\n> AFAICT, the main purpose of changing $PATH is to ease (cross-language)\n> implementation, I don't think the user is meant to notice.\n> Of course, exporting GIT_EXEC_PATH is desirable for some commands like gc\n> that delegate to a bunch of git processes without user interaction, to make\n> sure all children are from the same build. c90d565a46 (Propagate --exec-path\n> setting to external commands via GIT_EXEC_PATH, 2009-03-21). But\n> I don't think the same applies for rebase -x.\n\nWell, rebase does actually delegate to other git processes -- git\ncommit (in some cases), git stash (if --autostash is specified), git\nmerge (when a non-default, non-built-in strategy is selected), running\nthe post-rewrite hook (if it exists), and indirectly through git\ncommit all the various hooks it might call, and when calling git-stash\nthe huge pile of subcommands it invokes.\n\nSome of those should definitely be replaced with library calls instead\nof forking a subprocess.  And I'm sure the PATH/EXEC_PATH could be\nmade specific to the places that really need it, but it's so much\neasier to just make one global replacement and it avoids people\nforgetting to do so before calling subcommands that then might run the\nwrong git version in a different setup.  So, I'm a bit conflicted\nabout fixing PATH/EXEC_PATH right away.  Perhaps if we just modified\nthose back to their original value just the for --exec'd command?\n\nThat probably deserves a series all of its own, but might be\ninteresting for someone else to look at.\n\n> > This is a regression in behavior from the original legacy\n> > implemented-in-shell rebase.  It is perhaps rare for it to cause\n> > problems in practice, especially since most small problems that were\n> > caused by this area of bugs has been fixed-up in the past in a way that\n> > masked the particular bug observed without fixing the real underlying\n> > problem.\n>\n> Really interesting that we added multiple workarounds, when we just needed\n> to stop polluting the environment.  The fact that our test suite\n> unsets most GIT_* variables also masks potential problems.\n>\n> >\n> > How we arrived at the current situation is perhaps merited.  The setting\n>\n> This sounds weirdly penitent if read the wrong way. I guess\n> \"An explanation how we arrived ... \"?\n\nIndeed, thanks for catching.\n\n> > of GIT_DIR and GIT_WORK_TREE done by sequencer.c arose from a sequence\n> > of historical accidents:\n> >\n> > * When rebase was implemented as a shell command, it would call\n> >   git-sh-setup, which among other things would set GIT_DIR -- but not\n> >   export it.  This meant that when rebase --exec commands were run via\n> >       /bin/sh -c \"$COMMAND\"\n> >   they would not inherit the GIT_DIR setting.  The fact that GIT_DIR\n> >   was not set in the run $COMMAND is the behavior we'd like to restore.\n> >\n> > * When the rebase--helper builtin was introduced to allow incrementally\n> >   replacing shell with C code, we were in an implementation that was\n>\n> More correct to say \"we were in an (implementation) state\" or better:\n> \"we had an implementation\"?\n\nYes, that'd be better.\n\n> >   half shell, half C.  In particular, commit 18633e1a22 (\"rebase -i: use\n> >   the rebase--helper builtin\", 2017-02-09) added calls to\n> >       exec git rebase--helper ...\n>\n> Ok I guess this was a good reason for adding the exec-path to the environment\n> of exec steps.\n>\n> >   which caused rebase--helper to inherit the GIT_DIR environment\n> >   variable from the shell.  git's setup would change the environment\n> >   variable from an absolute path to a relative one (\".git\"), but would\n> >   leave it set.  This meant that when rebase --exec commands were run\n> >   via\n> >       run_command_v_opt(...)\n> >   they would inherit the GIT_DIR setting.\n> >\n> > * In commit 09d7b6c6fa (\"sequencer: pass absolute GIT_DIR to exec\n> >   commands\", 2017-10-31), it was noted that the GIT_DIR caused problems\n> >   with some commands; e.g.\n> >       git rebase --exec 'cd subdir && git describe' ...\n> >   would have GIT_DIR=.git which was invalid due to the change to the\n> >   subdirectory.  Instead of questioning why GIT_DIR was set, that commit\n> >   instead made sequencer change GIT_DIR to be an absolute path and\n> >   explicitly export it via\n> >       argv_array_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n> >       run_command_v_opt_cd_env(..., child_env.argv)\n> >\n> > * In commit ab5e67d751 (\"sequencer: pass absolute GIT_WORK_TREE to exec\n> >   commands\", 2018-07-14), it was noted that when GIT_DIR is set but\n> >   GIT_WORK_TREE is not, that we do not discover GIT_WORK_TREE but just\n> >   assume it is '.'.  That is incorrect if trying to run commands from a\n> >   subdirectory.  However, rather than question why GIT_DIR was set, that\n> >   commit instead also added GIT_WORK_TREE to the list of things to\n> >   export.\n> >\n> > Each of the above problems would have been fixed automatically when\n> > git-rebase become a full builtin, had it not been for the fact that\n>\n> s/become/became/\n\nYep; will fix.\n\n> > sequencer.c started exporting GIT_DIR and GIT_WORK_TREE in the interim.\n> > Stop exporting them now.\n> >\n> > Signed-off-by: Elijah Newren <newren@gmail.com>\n> > Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > ---\n> >     sequencer: fix environment that 'exec' commands run under\n>\n> Maybe: sequencer: do not export GIT_DIR and GIT_WORK_TREE to exec\n\nWorks for me.\n\n> (assuming that $PATH also needs fixing)\n\nI'll leave that to someone else.  :-)\n\n> >\n> >     I'm not sure if the added regression tests make sense\n>\n> They are simple and correct.\n> We still pass on values from --git-dir= and GIT_DIR=. git.\n> Those are /probably/ right.\n>\n> >     or if t3409 is the best place to put them (taking over a recently\n> >     removed t3409 that was used for the deprecated preserve merges option).\n>\n> Looks like there is no t/t34*-rebase-exec.sh yet. Most tests of --exec are\n> in t/t3404-rebase-interactive.sh, but we don't need the interactive bit here.\n\nYeah, I didn't want to add to t3404, because (a) there's a comment at\nthe top of the file saying all files are about a certain type of test\nsetup that just isn't applicable to my new test, (b) t3404 is already\none of the slowest tests.\n\nAlso, I considered naming it rebase-exec.sh, but (1) there are more\ntests of that in t3404 already and (2) I thought folks might have\nreason to add additional environment related tests and this would be a\ngood place for all those tests.  The fact that you brought up\nadditional environment variables suggests that might indeed be a good\ntrack.\n\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1134%2Fnewren%2Ffix-rebase-exec-environ-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1134/newren/fix-rebase-exec-environ-v1\n> > Pull-Request: https://github.com/git/git/pull/1134\n> >\n> >  sequencer.c               |  9 +--------\n> >  t/t3409-rebase-environ.sh | 23 +++++++++++++++++++++++\n> >  2 files changed, 24 insertions(+), 8 deletions(-)\n> >  create mode 100755 t/t3409-rebase-environ.sh\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index ea96837cde3..9afdbe3e3d1 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -3495,17 +3495,12 @@ static int error_failed_squash(struct repository *r,\n> >\n> >  static int do_exec(struct repository *r, const char *command_line)\n> >  {\n> > -     struct strvec child_env = STRVEC_INIT;\n> >       const char *child_argv[] = { NULL, NULL };\n> >       int dirty, status;\n> >\n> >       fprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n> >       child_argv[0] = command_line;\n> > -     strvec_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n> > -     strvec_pushf(&child_env, \"GIT_WORK_TREE=%s\",\n> > -                  absolute_path(get_git_work_tree()));\n> > -     status = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,\n> > -                                       child_env.v);\n> > +     status = run_command_v_opt(child_argv, RUN_USING_SHELL);\n> >\n> >       /* force re-reading of the cache */\n> >       if (discard_index(r->index) < 0 || repo_read_index(r) < 0)\n> > @@ -3535,8 +3530,6 @@ static int do_exec(struct repository *r, const char *command_line)\n> >               status = 1;\n> >       }\n> >\n> > -     strvec_clear(&child_env);\n> > -\n> >       return status;\n> >  }\n> >\n> > diff --git a/t/t3409-rebase-environ.sh b/t/t3409-rebase-environ.sh\n> > new file mode 100755\n> > index 00000000000..83ffb39d9ff\n> > --- /dev/null\n> > +++ b/t/t3409-rebase-environ.sh\n> > @@ -0,0 +1,23 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='git rebase interactive environment'\n>\n> maybe test_description='git rebase --exec environment'\n\nYep, I need to remove 'interactive' here and it probably makes sense\nto mention --exec unless someone adds a test to it for environment\nvariables with some other part of rebase.\n\n> > +\n> > +. ./test-lib.sh\n> > +\n> > +test_expect_success 'setup' '\n> > +     test_commit one &&\n> > +     test_commit two &&\n> > +     test_commit three\n> > +'\n> > +\n> > +test_expect_success 'rebase --exec does not muck with GIT_DIR' '\n> > +     git rebase --exec \"printf %s \\$GIT_DIR >environ\" HEAD~1 &&\n> > +     test_must_be_empty environ\n> > +'\n> > +\n> > +test_expect_success 'rebase --exec does not muck with GIT_WORK_TREE' '\n> > +     git rebase --exec \"printf %s \\$GIT_WORK_TREE >environ\" HEAD~1 &&\n> > +     test_must_be_empty environ\n> > +'\n>\n> I guess we could add a test for existing GIT_DIR in the environment, even\n> if it's not affected\n>\n>         test_expect_success 'already exported GIT_DIR is passed on to rebase --exec commands' '\n>                 GIT_DIR=.git GIT_WORK_TREE=. git rebase HEAD~1 --exec \\\n>                         \"printf %s\\\\\\\\n \\\"\\$GIT_DIR\\\" \\\"\\$GIT_WORK_TREE\\\" >actual\" &&\n>                 cat >expect <<-EOF &&\n>                 .git\n>                 .\n>                 EOF\n>                 test_cmp expect actual &&\n>                 cat actual\n>         '\n>\n> (I tried to use non-default values lik ./.git and ./. but the weird thing\n> is that git canonicalizes the worktree but not the git dir, so meh)\n\nIt also changes the current working directory to the specified work\ntree, which makes the test start testing stuff other than I was\nexpecting; the following passes:\n\ntest_expect_success 'setup alternate GIT_DIR and GIT_WORK_TREE' '\n    git clone . other-copy &&\n    git worktree add other-worktree\n'\n\ntest_expect_success 'already exported GIT_DIR is passed on to rebase\n--exec commands' '\n    test_when_finished \"rm other-worktree/actual\" &&\n\n    GIT_DIR=other-copy/.git GIT_WORK_TREE=other-worktree \\\n        git rebase HEAD~1 --exec \\\n        \"printf %s\\\\\\\\n \\\"\\$GIT_DIR\\\" \\\"\\$GIT_WORK_TREE\\\" \\\"\\$PWD\\\" >actual\" &&\n\n    cat >expect <<-EOF &&\n    $(cd other-copy && pwd -P)/.git\n    .\n    $(cd other-worktree && pwd -P)\n    EOF\n\n    test_cmp expect other-worktree/actual\n'\n\nI don't know if that's correct or buggy, but it's starting to stray\nfrom what I was intending to test so I might leave it out.\n\n> I also wasn't sure about the behavior of --git-dir= Should it be the same as GIT_DIR=?\n> I think it's also conceivable that --git-dir= does *not* cause GIT_DIR to\n> be exported to exec commands, though that might break existing\n> scripts. Something like\n>\n>         git --work-tree=../other-worktree --git-dir=../other-worktree/.git \\\n>                 rebase --exec \"make generate-documentation && git commit -a --amend --no-edit\"\n>\n> (needless to say that in this case \"git -C ../other-worktree\" is probably\n> what the user wants)\n\nIt sets the variables; the following passes (which differs from above\nonly in using --git-dir and --work-tree rather than GIT_DIR and\nGIT_WORK_TREE):\n\ntest_expect_success 'specialized --git-dir/--work-tree and rebase\n--exec commands' '\n    test_when_finished \"rm other-worktree/actual\" &&\n\n    git --git-dir=other-copy/.git --work-tree=other-worktree \\\n        rebase HEAD~1 --exec \\\n        \"printf %s\\\\\\n \\\"\\$GIT_DIR\\\" \\\"\\$GIT_WORK_TREE\\\" \\\"\\$PWD\\\" >actual\" &&\n\n    cat >expect <<-EOF &&\n    $(cd other-copy && pwd -P)/.git\n    .\n    $(cd other-worktree && pwd -P)\n    EOF\n\n    test_cmp expect other-worktree/actual\n'\n"},{"id":"443064","messageId":"pull.1134.v3.git.git.1638596219656.gitgitgadget@gmail.com","threadId":"56881","inReplyTo":"pull.1134.v2.git.git.1637041986945.gitgitgadget@gmail.com","subject":"[PATCH v3] sequencer: do not export GIT_DIR and GIT_WORK_TREE for 'exec'","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-04T05:36:59Z","receivedAt":"2021-12-04T05:37:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nCommands executed from `git rebase --exec` can give different behavior\nfrom within that environment than they would outside of it, due to the\nfact that sequencer.c exports both GIT_DIR and GIT_WORK_TREE.  For\nexample, if the relevant script calls something like\n\n  git -C ../otherdir log --format=%H --no-walk\n\nthe user may be surprised to find that the command above does not show a\ncommit hash from ../otherdir, because $GIT_DIR prevents automatic gitdir\ndetection and makes the -C option useless.\n\nThis is a regression in behavior from the original legacy\nimplemented-in-shell rebase.  It is perhaps rare for it to cause\nproblems in practice, especially since most small problems that were\ncaused by this area of bugs has been fixed-up in the past in a way that\nmasked the particular bug observed without fixing the real underlying\nproblem.\n\nAn explanation of how we arrived at the current situation is perhaps\nmerited.  The setting of GIT_DIR and GIT_WORK_TREE done by sequencer.c\narose from a sequence of historical accidents:\n\n* When rebase was implemented as a shell command, it would call\n  git-sh-setup, which among other things would set GIT_DIR -- but not\n  export it.  This meant that when rebase --exec commands were run via\n      /bin/sh -c \"$COMMAND\"\n  they would not inherit the GIT_DIR setting.  The fact that GIT_DIR\n  was not set in the run $COMMAND is the behavior we'd like to restore.\n\n* When the rebase--helper builtin was introduced to allow incrementally\n  replacing shell with C code, we had an implementation that was half\n  shell, half C.  In particular, commit 18633e1a22 (\"rebase -i: use the\n  rebase--helper builtin\", 2017-02-09) added calls to\n      exec git rebase--helper ...\n  which caused rebase--helper to inherit the GIT_DIR environment\n  variable from the shell.  git's setup would change the environment\n  variable from an absolute path to a relative one (\".git\"), but would\n  leave it set.  This meant that when rebase --exec commands were run\n  via\n      run_command_v_opt(...)\n  they would inherit the GIT_DIR setting.\n\n* In commit 09d7b6c6fa (\"sequencer: pass absolute GIT_DIR to exec\n  commands\", 2017-10-31), it was noted that the GIT_DIR caused problems\n  with some commands; e.g.\n      git rebase --exec 'cd subdir && git describe' ...\n  would have GIT_DIR=.git which was invalid due to the change to the\n  subdirectory.  Instead of questioning why GIT_DIR was set, that commit\n  instead made sequencer change GIT_DIR to be an absolute path and\n  explicitly export it via\n      argv_array_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n      run_command_v_opt_cd_env(..., child_env.argv)\n\n* In commit ab5e67d751 (\"sequencer: pass absolute GIT_WORK_TREE to exec\n  commands\", 2018-07-14), it was noted that when GIT_DIR is set but\n  GIT_WORK_TREE is not, that we do not discover GIT_WORK_TREE but just\n  assume it is '.'.  That is incorrect if trying to run commands from a\n  subdirectory.  However, rather than question why GIT_DIR was set, that\n  commit instead also added GIT_WORK_TREE to the list of things to\n  export.\n\nEach of the above problems would have been fixed automatically when\ngit-rebase became a full builtin, had it not been for the fact that\nsequencer.c started exporting GIT_DIR and GIT_WORK_TREE in the interim.\nStop exporting them now.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\nAcked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nAcked-by: Johannes Altmanninger <aclopte@gmail.com>\nAcked-by: Phillip Wood <phillip.wood123@gmail.com>\n---\n    sequencer: fix environment that 'exec' commands run under\n    \n    Changes since v2:\n    \n     * Make sure I've included all 3 Acked-by's.\n     * The 2.35 cycle has started and it's been weeks since I sent v2, so\n       it's time to resend. :-)\n    \n    Changes since v1:\n    \n     * Fix wording in multiple locations pointed out by Johannes\n       Altmanninger\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1134%2Fnewren%2Ffix-rebase-exec-environ-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1134/newren/fix-rebase-exec-environ-v3\nPull-Request: https://github.com/git/git/pull/1134\n\nRange-diff vs v2:\n\n 1:  c647c45375a ! 1:  4f5862c212f sequencer: do not export GIT_DIR and GIT_WORK_TREE for 'exec'\n     @@ Commit message\n      \n          Signed-off-by: Elijah Newren <newren@gmail.com>\n          Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n     +    Acked-by: Johannes Altmanninger <aclopte@gmail.com>\n     +    Acked-by: Phillip Wood <phillip.wood123@gmail.com>\n      \n       ## sequencer.c ##\n      @@ sequencer.c: static int error_failed_squash(struct repository *r,\n\n\n sequencer.c               |  9 +--------\n t/t3409-rebase-environ.sh | 23 +++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 8 deletions(-)\n create mode 100755 t/t3409-rebase-environ.sh\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ea96837cde3..9afdbe3e3d1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3495,17 +3495,12 @@ static int error_failed_squash(struct repository *r,\n \n static int do_exec(struct repository *r, const char *command_line)\n {\n-\tstruct strvec child_env = STRVEC_INIT;\n \tconst char *child_argv[] = { NULL, NULL };\n \tint dirty, status;\n \n \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n \tchild_argv[0] = command_line;\n-\tstrvec_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n-\tstrvec_pushf(&child_env, \"GIT_WORK_TREE=%s\",\n-\t\t     absolute_path(get_git_work_tree()));\n-\tstatus = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,\n-\t\t\t\t\t  child_env.v);\n+\tstatus = run_command_v_opt(child_argv, RUN_USING_SHELL);\n \n \t/* force re-reading of the cache */\n \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0)\n@@ -3535,8 +3530,6 @@ static int do_exec(struct repository *r, const char *command_line)\n \t\tstatus = 1;\n \t}\n \n-\tstrvec_clear(&child_env);\n-\n \treturn status;\n }\n \ndiff --git a/t/t3409-rebase-environ.sh b/t/t3409-rebase-environ.sh\nnew file mode 100755\nindex 00000000000..83ffb39d9ff\n--- /dev/null\n+++ b/t/t3409-rebase-environ.sh\n@@ -0,0 +1,23 @@\n+#!/bin/sh\n+\n+test_description='git rebase interactive environment'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\ttest_commit three\n+'\n+\n+test_expect_success 'rebase --exec does not muck with GIT_DIR' '\n+\tgit rebase --exec \"printf %s \\$GIT_DIR >environ\" HEAD~1 &&\n+\ttest_must_be_empty environ\n+'\n+\n+test_expect_success 'rebase --exec does not muck with GIT_WORK_TREE' '\n+\tgit rebase --exec \"printf %s \\$GIT_WORK_TREE >environ\" HEAD~1 &&\n+\ttest_must_be_empty environ\n+'\n+\n+test_done\n\nbase-commit: 88d915a634b449147855041d44875322de2b286d\n-- \ngitgitgadget\n"},{"id":"443100","messageId":"20211205084502.6sougq7zsbnhxlht@gmail.com","threadId":"56881","inReplyTo":"CABPp-BHUQQiGxfY+spGT3CShP4aaETwYJcFgknPBQ4XhBwKQ2A@mail.gmail.com","subject":"Re: [PATCH] sequencer: fix environment that 'exec' commands run under","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-12-05T08:45:02Z","receivedAt":"2021-12-05T08:45:10Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Tue, Nov 23, 2021 at 09:48:03AM -0800, Elijah Newren wrote:\n> // Resending this email from 8 days ago; Dscho pointed that I accidentally\n> // responded to the GitHub PR email instead of the original email, and\n> // while Johannes already said he likes my V2, perhaps there are other\n> // comments here that benefit other current or future reviewers\n> \n> Hi Johannes,\n> \n> Thanks for looking over the patch and commit message closely; very cool.\n> \n> On Sun, Nov 14, 2021 at 12:21 PM Johannes Altmanninger\n> <aclopte@gmail.com> wrote:\n> >\n> > On Fri, Nov 12, 2021 at 05:42:52PM +0000, Elijah Newren via GitGitGadget wrote:\n> > > From: Elijah Newren <newren@gmail.com>\n> > >\n> > > Commands executed from `git rebase --exec` can give different behavior\n> > > from within that environment than they would outside of it, due to the\n> > > fact that sequencer.c exports both GIT_DIR and GIT_WORK_TREE.  For\n> > > example, if the relevant script calls something like\n> > >\n> > >   git -C ../otherdir log --format=%H --no-walk\n> > >\n> > > the user may be surprised to find that the command above does not show a\n> > > commit hash from ../otherdir, because $GIT_DIR prevents automatic gitdir\n> > > detection and makes the -C option useless.\n> >\n> > Yep. I've had a case where \"git rebase -x 'make test'\" would fail because\n> > \"make test\" tries to run some Git commands in a temporary repo.  The workaround\n> > was to unset all GIT_* environment variables, just like t/test-lib.sh does.\n> >\n> > I had the same problem when testing shell completion because git prepends\n> > $(git --exec-path) to $PATH.  I don't see a good reason why \"git rebase -x\n> > cmd\" passes a modified $PATH (and $GIT_EXEC_PATH) to cmd. The user is back in\n> > control, so I'd expect the same environment as for the parent rebase process.\n> >\n> > AFAICT, the main purpose of changing $PATH is to ease (cross-language)\n> > implementation, I don't think the user is meant to notice.\n> > Of course, exporting GIT_EXEC_PATH is desirable for some commands like gc\n> > that delegate to a bunch of git processes without user interaction, to make\n> > sure all children are from the same build. c90d565a46 (Propagate --exec-path\n> > setting to external commands via GIT_EXEC_PATH, 2009-03-21). But\n> > I don't think the same applies for rebase -x.\n> \n> Well, rebase does actually delegate to other git processes -- git\n> commit (in some cases), git stash (if --autostash is specified), git\n> merge (when a non-default, non-built-in strategy is selected), running\n> the post-rewrite hook (if it exists), and indirectly through git\n> commit all the various hooks it might call, and when calling git-stash\n> the huge pile of subcommands it invokes.\n> \n> Some of those should definitely be replaced with library calls instead\n> of forking a subprocess.  And I'm sure the PATH/EXEC_PATH could be\n> made specific to the places that really need it, but it's so much\n> easier to just make one global replacement and it avoids people\n> forgetting to do so before calling subcommands that then might run the\n> wrong git version in a different setup.  So, I'm a bit conflicted\n> about fixing PATH/EXEC_PATH right away.  Perhaps if we just modified\n> those back to their original value just the for --exec'd command?\n\nYeah that sounds like a good approach, and it seems like a reasonable cleanup.\n\n> That probably deserves a series all of its own, but might be\n> interesting for someone else to look at.\n> ...\n> the following passes:\n> \n> test_expect_success 'setup alternate GIT_DIR and GIT_WORK_TREE' '\n>     git clone . other-copy &&\n>     git worktree add other-worktree\n> '\n> \n> test_expect_success 'already exported GIT_DIR is passed on to rebase\n> --exec commands' '\n>     test_when_finished \"rm other-worktree/actual\" &&\n> \n>     GIT_DIR=other-copy/.git GIT_WORK_TREE=other-worktree \\\n>         git rebase HEAD~1 --exec \\\n>         \"printf %s\\\\\\\\n \\\"\\$GIT_DIR\\\" \\\"\\$GIT_WORK_TREE\\\" \\\"\\$PWD\\\" >actual\" &&\n> \n>     cat >expect <<-EOF &&\n>     $(cd other-copy && pwd -P)/.git\n>     .\n>     $(cd other-worktree && pwd -P)\n>     EOF\n> \n>     test_cmp expect other-worktree/actual\n> '\n> \n> I don't know if that's correct or buggy\n\nMe neither.  I feel like the current behavior (exporting GIT_WORK_TREE=.) is\na bit better than dropping GIT_WORK_TREE from the environment of exec runs.\n\n> but it's starting to stray\n> from what I was intending to test so I might leave it out.\n> \n> > I also wasn't sure about the behavior of --git-dir= Should it be the same as GIT_DIR=?\n> > I think it's also conceivable that --git-dir= does *not* cause GIT_DIR to\n> > be exported to exec commands, though that might break existing\n> > scripts. Something like\n> >\n> >         git --work-tree=../other-worktree --git-dir=../other-worktree/.git \\\n> >                 rebase --exec \"make generate-documentation && git commit -a --amend --no-edit\"\n> >\n> > (needless to say that in this case \"git -C ../other-worktree\" is probably\n> > what the user wants)\n> \n> It sets the variables; the following passes (which differs from above\n> only in using --git-dir and --work-tree rather than GIT_DIR and\n> GIT_WORK_TREE):\n\nLooking at this again, --git-dir is meant to be able to dominate $GIT_DIR,\nso the current behavior where --git-dir exports that variable seems logical.\n"}]}