From: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>,
Johannes Altmanninger <aclopte@gmail.com>,
Phillip Wood <phillip.wood123@gmail.com>,
Elijah Newren <newren@gmail.com>,
Elijah Newren <newren@gmail.com>,
Elijah Newren <newren@gmail.com>
Subject: [PATCH v3] sequencer: do not export GIT_DIR and GIT_WORK_TREE for 'exec'
Date: Sat, 04 Dec 2021 05:36:59 +0000 [thread overview]
Message-ID: <pull.1134.v3.git.git.1638596219656.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1134.v2.git.git.1637041986945.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
Commands executed from `git rebase --exec` can give different behavior
from within that environment than they would outside of it, due to the
fact that sequencer.c exports both GIT_DIR and GIT_WORK_TREE. For
example, if the relevant script calls something like
git -C ../otherdir log --format=%H --no-walk
the user may be surprised to find that the command above does not show a
commit hash from ../otherdir, because $GIT_DIR prevents automatic gitdir
detection and makes the -C option useless.
This is a regression in behavior from the original legacy
implemented-in-shell rebase. It is perhaps rare for it to cause
problems in practice, especially since most small problems that were
caused by this area of bugs has been fixed-up in the past in a way that
masked the particular bug observed without fixing the real underlying
problem.
An explanation of how we arrived at the current situation is perhaps
merited. The setting of GIT_DIR and GIT_WORK_TREE done by sequencer.c
arose from a sequence of historical accidents:
* When rebase was implemented as a shell command, it would call
git-sh-setup, which among other things would set GIT_DIR -- but not
export it. This meant that when rebase --exec commands were run via
/bin/sh -c "$COMMAND"
they would not inherit the GIT_DIR setting. The fact that GIT_DIR
was not set in the run $COMMAND is the behavior we'd like to restore.
* When the rebase--helper builtin was introduced to allow incrementally
replacing shell with C code, we had an implementation that was half
shell, half C. In particular, commit 18633e1a22 ("rebase -i: use the
rebase--helper builtin", 2017-02-09) added calls to
exec git rebase--helper ...
which caused rebase--helper to inherit the GIT_DIR environment
variable from the shell. git's setup would change the environment
variable from an absolute path to a relative one (".git"), but would
leave it set. This meant that when rebase --exec commands were run
via
run_command_v_opt(...)
they would inherit the GIT_DIR setting.
* In commit 09d7b6c6fa ("sequencer: pass absolute GIT_DIR to exec
commands", 2017-10-31), it was noted that the GIT_DIR caused problems
with some commands; e.g.
git rebase --exec 'cd subdir && git describe' ...
would have GIT_DIR=.git which was invalid due to the change to the
subdirectory. Instead of questioning why GIT_DIR was set, that commit
instead made sequencer change GIT_DIR to be an absolute path and
explicitly export it via
argv_array_pushf(&child_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
run_command_v_opt_cd_env(..., child_env.argv)
* In commit ab5e67d751 ("sequencer: pass absolute GIT_WORK_TREE to exec
commands", 2018-07-14), it was noted that when GIT_DIR is set but
GIT_WORK_TREE is not, that we do not discover GIT_WORK_TREE but just
assume it is '.'. That is incorrect if trying to run commands from a
subdirectory. However, rather than question why GIT_DIR was set, that
commit instead also added GIT_WORK_TREE to the list of things to
export.
Each of the above problems would have been fixed automatically when
git-rebase became a full builtin, had it not been for the fact that
sequencer.c started exporting GIT_DIR and GIT_WORK_TREE in the interim.
Stop exporting them now.
Signed-off-by: Elijah Newren <newren@gmail.com>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Acked-by: Johannes Altmanninger <aclopte@gmail.com>
Acked-by: Phillip Wood <phillip.wood123@gmail.com>
---
sequencer: fix environment that 'exec' commands run under
Changes since v2:
* Make sure I've included all 3 Acked-by's.
* The 2.35 cycle has started and it's been weeks since I sent v2, so
it's time to resend. :-)
Changes since v1:
* Fix wording in multiple locations pointed out by Johannes
Altmanninger
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1134%2Fnewren%2Ffix-rebase-exec-environ-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1134/newren/fix-rebase-exec-environ-v3
Pull-Request: https://github.com/git/git/pull/1134
Range-diff vs v2:
1: c647c45375a ! 1: 4f5862c212f sequencer: do not export GIT_DIR and GIT_WORK_TREE for 'exec'
@@ Commit message
Signed-off-by: Elijah Newren <newren@gmail.com>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
+ Acked-by: Johannes Altmanninger <aclopte@gmail.com>
+ Acked-by: Phillip Wood <phillip.wood123@gmail.com>
## sequencer.c ##
@@ sequencer.c: static int error_failed_squash(struct repository *r,
sequencer.c | 9 +--------
t/t3409-rebase-environ.sh | 23 +++++++++++++++++++++++
2 files changed, 24 insertions(+), 8 deletions(-)
create mode 100755 t/t3409-rebase-environ.sh
diff --git a/sequencer.c b/sequencer.c
index ea96837cde3..9afdbe3e3d1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3495,17 +3495,12 @@ static int error_failed_squash(struct repository *r,
static int do_exec(struct repository *r, const char *command_line)
{
- struct strvec child_env = STRVEC_INIT;
const char *child_argv[] = { NULL, NULL };
int dirty, status;
fprintf(stderr, _("Executing: %s\n"), command_line);
child_argv[0] = command_line;
- strvec_pushf(&child_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
- strvec_pushf(&child_env, "GIT_WORK_TREE=%s",
- absolute_path(get_git_work_tree()));
- status = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,
- child_env.v);
+ status = run_command_v_opt(child_argv, RUN_USING_SHELL);
/* force re-reading of the cache */
if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
@@ -3535,8 +3530,6 @@ static int do_exec(struct repository *r, const char *command_line)
status = 1;
}
- strvec_clear(&child_env);
-
return status;
}
diff --git a/t/t3409-rebase-environ.sh b/t/t3409-rebase-environ.sh
new file mode 100755
index 00000000000..83ffb39d9ff
--- /dev/null
+++ b/t/t3409-rebase-environ.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+test_description='git rebase interactive environment'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit one &&
+ test_commit two &&
+ test_commit three
+'
+
+test_expect_success 'rebase --exec does not muck with GIT_DIR' '
+ git rebase --exec "printf %s \$GIT_DIR >environ" HEAD~1 &&
+ test_must_be_empty environ
+'
+
+test_expect_success 'rebase --exec does not muck with GIT_WORK_TREE' '
+ git rebase --exec "printf %s \$GIT_WORK_TREE >environ" HEAD~1 &&
+ test_must_be_empty environ
+'
+
+test_done
base-commit: 88d915a634b449147855041d44875322de2b286d
--
gitgitgadget
prev parent reply other threads:[~2021-12-04 5:37 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-12 17:42 [PATCH] sequencer: fix environment that 'exec' commands run under Elijah Newren via GitGitGadget
2021-11-14 20:21 ` Johannes Altmanninger
2021-11-23 17:48 ` Elijah Newren
2021-12-05 8:45 ` Johannes Altmanninger
2021-11-16 5:53 ` [PATCH v2] sequencer: do not export GIT_DIR and GIT_WORK_TREE for 'exec' Elijah Newren via GitGitGadget
2021-11-16 6:06 ` Johannes Altmanninger
2021-11-16 9:59 ` Phillip Wood
2021-12-04 5:36 ` Elijah Newren via GitGitGadget [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.1134.v3.git.git.1638596219656.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=aclopte@gmail.com \
--cc=git@vger.kernel.org \
--cc=johannes.schindelin@gmx.de \
--cc=newren@gmail.com \
--cc=phillip.wood123@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.