On Tue, Nov 23, 2021 at 09:48:03AM -0800, Elijah Newren wrote: > // Resending this email from 8 days ago; Dscho pointed that I accidentally > // responded to the GitHub PR email instead of the original email, and > // while Johannes already said he likes my V2, perhaps there are other > // comments here that benefit other current or future reviewers > > Hi Johannes, > > Thanks for looking over the patch and commit message closely; very cool. > > On Sun, Nov 14, 2021 at 12:21 PM Johannes Altmanninger > <aclopte@xxxxxxxxx> wrote: > > > > On Fri, Nov 12, 2021 at 05:42:52PM +0000, Elijah Newren via GitGitGadget wrote: > > > From: Elijah Newren <newren@xxxxxxxxx> > > > > > > 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. > > Well, rebase does actually delegate to other git processes -- git > commit (in some cases), git stash (if --autostash is specified), git > merge (when a non-default, non-built-in strategy is selected), running > the post-rewrite hook (if it exists), and indirectly through git > commit all the various hooks it might call, and when calling git-stash > the huge pile of subcommands it invokes. > > Some of those should definitely be replaced with library calls instead > of forking a subprocess. And I'm sure the PATH/EXEC_PATH could be > made specific to the places that really need it, but it's so much > easier to just make one global replacement and it avoids people > forgetting to do so before calling subcommands that then might run the > wrong git version in a different setup. So, I'm a bit conflicted > about fixing PATH/EXEC_PATH right away. Perhaps if we just modified > those back to their original value just the for --exec'd command? Yeah that sounds like a good approach, and it seems like a reasonable cleanup. > That probably deserves a series all of its own, but might be > interesting for someone else to look at. > ... > the following passes: > > test_expect_success 'setup alternate GIT_DIR and GIT_WORK_TREE' ' > git clone . other-copy && > git worktree add other-worktree > ' > > test_expect_success 'already exported GIT_DIR is passed on to rebase > --exec commands' ' > test_when_finished "rm other-worktree/actual" && > > GIT_DIR=other-copy/.git GIT_WORK_TREE=other-worktree \ > git rebase HEAD~1 --exec \ > "printf %s\\\\n \"\$GIT_DIR\" \"\$GIT_WORK_TREE\" \"\$PWD\" >actual" && > > cat >expect <<-EOF && > $(cd other-copy && pwd -P)/.git > . > $(cd other-worktree && pwd -P) > EOF > > test_cmp expect other-worktree/actual > ' > > I don't know if that's correct or buggy Me neither. I feel like the current behavior (exporting GIT_WORK_TREE=.) is a bit better than dropping GIT_WORK_TREE from the environment of exec runs. > but it's starting to stray > from what I was intending to test so I might leave it out. > > > 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) > > It sets the variables; the following passes (which differs from above > only in using --git-dir and --work-tree rather than GIT_DIR and > GIT_WORK_TREE): Looking at this again, --git-dir is meant to be able to dominate $GIT_DIR, so the current behavior where --git-dir exports that variable seems logical.
