On Fri, Nov 12, 2021 at 05:42:52PM +0000, Elijah Newren via GitGitGadget wrote:

From: Elijah Newren <redacted>

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.

Yep. I've had a case where "git rebase -x 'make test'" would fail because
"make test" tries to run some Git commands in a temporary repo.  The workaround
was to unset all GIT_* environment variables, just like t/test-lib.sh does.

I had the same problem when testing shell completion because git prepends
$(git --exec-path) to $PATH.  I don't see a good reason why "git rebase -x
cmd" passes a modified $PATH (and $GIT_EXEC_PATH) to cmd. The user is back in
control, so I'd expect the same environment as for the parent rebase process.

AFAICT, the main purpose of changing $PATH is to ease (cross-language)
implementation, I don't think the user is meant to notice.
Of course, exporting GIT_EXEC_PATH is desirable for some commands like gc
that delegate to a bunch of git processes without user interaction, to make
sure all children are from the same build. c90d565a46 (Propagate --exec-path
setting to external commands via GIT_EXEC_PATH, 2009-03-21). But
I don't think the same applies for rebase -x.

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.

Really interesting that we added multiple workarounds, when we just needed
to stop polluting the environment.  The fact that our test suite
unsets most GIT_* variables also masks potential problems.

How we arrived at the current situation is perhaps merited.  The setting

This sounds weirdly penitent if read the wrong way. I guess
"An explanation how we arrived ... "?

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 were in an implementation that was

More correct to say "we were in an (implementation) state" or better:
"we had an implementation"?

  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 ...

Ok I guess this was a good reason for adding the exec-path to the environment
of exec steps.

  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 become a full builtin, had it not been for the fact that

s/become/became/

sequencer.c started exporting GIT_DIR and GIT_WORK_TREE in the interim.
Stop exporting them now.

Signed-off-by: Elijah Newren <redacted>
Acked-by: Johannes Schindelin <redacted>
---
    sequencer: fix environment that 'exec' commands run under

Maybe: sequencer: do not export GIT_DIR and GIT_WORK_TREE to exec
(assuming that $PATH also needs fixing)

    
    I'm not sure if the added regression tests make sense

They are simple and correct.
We still pass on values from --git-dir= and GIT_DIR=. git.
Those are /probably/ right.

    or if t3409 is the best place to put them (taking over a recently
    removed t3409 that was used for the deprecated preserve merges option).

Looks like there is no t/t34*-rebase-exec.sh yet. Most tests of --exec are
in t/t3404-rebase-interactive.sh, but we don't need the interactive bit here.

maybe test_description='git rebase --exec 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
+'

I guess we could add a test for existing GIT_DIR in the environment, even
if it's not affected

	test_expect_success 'already exported GIT_DIR is passed on to rebase --exec commands' '
		GIT_DIR=.git GIT_WORK_TREE=. git rebase HEAD~1 --exec \
			"printf %s\\\\n \"\$GIT_DIR\" \"\$GIT_WORK_TREE\" >actual" &&
		cat >expect <<-EOF &&
		.git
		.
		EOF
		test_cmp expect actual &&
		cat actual
	'

(I tried to use non-default values lik ./.git and ./. but the weird thing
is that git canonicalizes the worktree but not the git dir, so meh)

I also wasn't sure about the behavior of --git-dir= Should it be the same as GIT_DIR=?
I think it's also conceivable that --git-dir= does *not* cause GIT_DIR to
be exported to exec commands, though that might break existing
scripts. Something like

	git --work-tree=../other-worktree --git-dir=../other-worktree/.git \
		rebase --exec "make generate-documentation && git commit -a --amend --no-edit"

(needless to say that in this case "git -C ../other-worktree" is probably
what the user wants)