threads / discuss / 48866

rev-parse --show-toplevel broken during exec'ed rebase?

Subject: rev-parse --show-toplevel broken during exec'ed rebase?

## tl;dr

22 messages between Jul 12, 2018 and Jul 19, 2018.

replies: 21people: 6as markdown or json

Vitali Lovich· Jul 12, 2018, 02:50 UTC · lore

Typically git rev-parse --show-toplevel prints the folder containing the .git folder regardless what subdirectory one is in. One exception I have found is that if one is within the context of git rebase --exec then show-toplevel always just prints the current directory it's running from.

Repro (starting with cwd within git project):
> (cd xdiff; git rev-parse --show-toplevel)
... path to git repository
> git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd
# Stop at some commit for editing
> (cd xdiff; git rev-parse --show-toplevel)
... path to git repository
> git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git rev-parse --show-toplevel)"
... path to git repository/xdiff !!!

This seems like incorrect behaviour to me since it's a weird inconsistency (even with other rebase contexts) & the documentation says "Show the absolute path of the top-level directory." with no caveats.

Thanks, Vital

Vitali Lovich· Jul 12, 2018, 02:53 UTC · re: Vitali Lovich · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

Sorry. Forgot to include the git versions I tested with (2.13.1, 2.17.0, 2.18.0) On Wed, Jul 11, 2018 at 7:50 PM Vitali Lovich <vlovich@gmail.com> wrote:

Show 24 quoted lines
>
> Typically git rev-parse --show-toplevel prints the folder containing
> the .git folder regardless what subdirectory one is in.  One exception
> I have found is that if one is within the context of git rebase --exec
> then show-toplevel always just prints the current directory it's
> running from.
>
> Repro (starting with cwd within git project):
> > (cd xdiff; git rev-parse --show-toplevel)
> ... path to git repository
> > git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd
> # Stop at some commit for editing
> > (cd xdiff; git rev-parse --show-toplevel)
> ... path to git repository
> > git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git rev-parse --show-toplevel)"
> ... path to git repository/xdiff !!!
>
> This seems like incorrect behaviour to me since it's a weird
> inconsistency (even with other rebase contexts) & the documentation
> says "Show the absolute path of the top-level directory." with no
> caveats.
>
> Thanks,
> Vital
Johannes Schindelin· Jul 12, 2018, 14:49 UTC · re: Vitali Lovich · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

Hi Vitali,
[please avoid top-posting on this mailing list]
On Wed, 11 Jul 2018, Vitali Lovich wrote:
Show 25 quoted lines
> On Wed, Jul 11, 2018 at 7:50 PM Vitali Lovich <vlovich@gmail.com> wrote:
> >
> > Typically git rev-parse --show-toplevel prints the folder containing
> > the .git folder regardless what subdirectory one is in.  One exception
> > I have found is that if one is within the context of git rebase --exec
> > then show-toplevel always just prints the current directory it's
> > running from.
> >
> > Repro (starting with cwd within git project):
> > > (cd xdiff; git rev-parse --show-toplevel)
> > ... path to git repository
> > > git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd
> > # Stop at some commit for editing
> > > (cd xdiff; git rev-parse --show-toplevel)
> > ... path to git repository
> > > git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git rev-parse --show-toplevel)"
> > ... path to git repository/xdiff !!!
> >
> > This seems like incorrect behaviour to me since it's a weird
> > inconsistency (even with other rebase contexts) & the documentation
> > says "Show the absolute path of the top-level directory." with no
> > caveats.
>
> Sorry.  Forgot to include the git versions I tested with (2.13.1,
> 2.17.0, 2.18.0)

This is actually not so much a bug in `rebase` as in `rev-parse --show-top-level`:

	$ GIT_DIR=$PWD/.git git -C xdiff rev-parse --show-toplevel
	C:/git-sdk-64/usr/src/git/xdiff

Ciao, Johannes

Johannes Schindelin· Jul 12, 2018, 15:02 UTC · re: Johannes Schindelin · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

Hi,
On Thu, 12 Jul 2018, Johannes Schindelin wrote:
Show 33 quoted lines
> On Wed, 11 Jul 2018, Vitali Lovich wrote:
> 
> > On Wed, Jul 11, 2018 at 7:50 PM Vitali Lovich <vlovich@gmail.com> wrote:
> > >
> > > Typically git rev-parse --show-toplevel prints the folder containing
> > > the .git folder regardless what subdirectory one is in.  One exception
> > > I have found is that if one is within the context of git rebase --exec
> > > then show-toplevel always just prints the current directory it's
> > > running from.
> > >
> > > Repro (starting with cwd within git project):
> > > > (cd xdiff; git rev-parse --show-toplevel)
> > > ... path to git repository
> > > > git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd
> > > # Stop at some commit for editing
> > > > (cd xdiff; git rev-parse --show-toplevel)
> > > ... path to git repository
> > > > git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git rev-parse --show-toplevel)"
> > > ... path to git repository/xdiff !!!
> > >
> > > This seems like incorrect behaviour to me since it's a weird
> > > inconsistency (even with other rebase contexts) & the documentation
> > > says "Show the absolute path of the top-level directory." with no
> > > caveats.
> >
> > Sorry.  Forgot to include the git versions I tested with (2.13.1,
> > 2.17.0, 2.18.0)
> 
> This is actually not so much a bug in `rebase` as in `rev-parse
> --show-top-level`:
> 
> 	$ GIT_DIR=$PWD/.git git -C xdiff rev-parse --show-toplevel
> 	C:/git-sdk-64/usr/src/git/xdiff

And the reason for this behavior is that setting `GIT_DIR` explicitly makes Git *always* consider the current working directory to be the top-level directory:

	https://github.com/git/git/blob/v2.18.0/setup.c#L1061-L1063

I wonder whether changing the line (https://github.com/git/git/blob/v2.18.0/sequencer.c#L2635)

   argv_array_pushf(&child_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
to
   argv_array_push(&child_env, "GIT_DIR");

breaks anything (and whether it fixes the bug you demonstrated via `rev-parse --show-toplevel`).

Ciao, Johannes

Junio C Hamano· Jul 12, 2018, 15:23 UTC · re: Vitali Lovich · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

Vitali Lovich <vlovich@gmail.com> writes:
Show 14 quoted lines
> Repro (starting with cwd within git project):
>> (cd xdiff; git rev-parse --show-toplevel)
> ... path to git repository
>> git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd
> # Stop at some commit for editing
>> (cd xdiff; git rev-parse --show-toplevel)
> ... path to git repository
>> git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git rev-parse --show-toplevel)"
> ... path to git repository/xdiff !!!
>
> This seems like incorrect behaviour to me since it's a weird
> inconsistency (even with other rebase contexts) & the documentation
> says "Show the absolute path of the top-level directory." with no
> caveats.
Does it reproduce with older rebase (say from v2.10 days), too?

I suspect that the above is because "git rebase" is exporting GIT_DIR without exporting GIT_WORK_TREE. When the former is given without the latter, that tells Git that you are at the top-level of the working tree (if that is not the case, you also export the latter to tell Git where the top-level is).

I suspect that "git rebase" before the ongoing piecemeal rewrite to C started (or scripted Porcelain in general) refrained from exporting GIT_DIR to the environment, and if my suspicion is correct, older "git rebase" would behave differently to your test case.

Vitali Lovich· Jul 12, 2018, 17:39 UTC · re: Junio C Hamano · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

On Thu, Jul 12, 2018 at 8:23 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 30 quoted lines
>
> Vitali Lovich <vlovich@gmail.com> writes:
>
> > Repro (starting with cwd within git project):
> >> (cd xdiff; git rev-parse --show-toplevel)
> > ... path to git repository
> >> git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd
> > # Stop at some commit for editing
> >> (cd xdiff; git rev-parse --show-toplevel)
> > ... path to git repository
> >> git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git rev-parse --show-toplevel)"
> > ... path to git repository/xdiff !!!
> >
> > This seems like incorrect behaviour to me since it's a weird
> > inconsistency (even with other rebase contexts) & the documentation
> > says "Show the absolute path of the top-level directory." with no
> > caveats.
>
> Does it reproduce with older rebase (say from v2.10 days), too?
>
> I suspect that the above is because "git rebase" is exporting
> GIT_DIR without exporting GIT_WORK_TREE.  When the former is given
> without the latter, that tells Git that you are at the top-level of
> the working tree (if that is not the case, you also export the
> latter to tell Git where the top-level is).
>
> I suspect that "git rebase" before the ongoing piecemeal rewrite to
> C started (or scripted Porcelain in general) refrained from
> exporting GIT_DIR to the environment, and if my suspicion is correct,
> older "git rebase" would behave differently to your test case.
Unfortunately I don't have an older git version handy to test this out.
brian m. carlson· Jul 13, 2018, 18:47 UTC · re: Junio C Hamano · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

On Thu, Jul 12, 2018 at 08:23:02AM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> Vitali Lovich <vlovich@gmail.com> writes:
> 
> > Repro (starting with cwd within git project):
> >> (cd xdiff; git rev-parse --show-toplevel)
> > ... path to git repository
> >> git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd
> > # Stop at some commit for editing
> >> (cd xdiff; git rev-parse --show-toplevel)
> > ... path to git repository
> >> git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git rev-parse --show-toplevel)"
> > ... path to git repository/xdiff !!!
> >
> > This seems like incorrect behaviour to me since it's a weird
> > inconsistency (even with other rebase contexts) & the documentation
> > says "Show the absolute path of the top-level directory." with no
> > caveats.
> 
> Does it reproduce with older rebase (say from v2.10 days), too?

It appears to work correctly on the CentOS SCL Git 2.9.3. We print the top level of the repository there.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Jeff King· Jul 13, 2018, 20:19 UTC · re: brian m. carlson · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

On Fri, Jul 13, 2018 at 06:47:32PM +0000, brian m. carlson wrote:
Show 12 quoted lines
> > >> git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git rev-parse --show-toplevel)"
> > > ... path to git repository/xdiff !!!
> > >
> > > This seems like incorrect behaviour to me since it's a weird
> > > inconsistency (even with other rebase contexts) & the documentation
> > > says "Show the absolute path of the top-level directory." with no
> > > caveats.
> > 
> > Does it reproduce with older rebase (say from v2.10 days), too?
> 
> It appears to work correctly on the CentOS SCL Git 2.9.3.  We print the
> top level of the repository there.

Bisecting is tricky, because there are actually three distinct behaviors.

The command above prints the correct top-level until 18633e1a22 (rebase -i: use the rebase--helper builtin, 2017-02-09). After that, rev-parse prints "Not a git repository".

That goes on until 09d7b6c6fa (sequencer: pass absolute GIT_DIR to exec commands, 2017-10-31).

None of which is too surprising. The root of the bug is in the conversion to rebase--helper, I think, when presumably we started setting GIT_DIR at all (but I didn't dig further). Then 09d7b6c6fa fixed _one_ fallout of that, which was relative paths, but didn't help the subdirectory case.

Just reading over this thread, I suspect the simplest fix is to pass GIT_DIR and GIT_WORK_TREE together, which is almost always the right thing to do.

-Peff
brian m. carlson· Jul 13, 2018, 23:05 UTC · re: Jeff King · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

On Fri, Jul 13, 2018 at 04:19:49PM -0400, Jeff King wrote:
> Just reading over this thread, I suspect the simplest fix is to pass
> GIT_DIR and GIT_WORK_TREE together, which is almost always the right
> thing to do.
I agree.  I'll write up a patch.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
brian m. carlson· Jul 14, 2018, 00:35 UTC · re: brian m. carlson · lore

[PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands

The sequencer currently passes GIT_DIR, but not GIT_WORK_TREE, to exec commands. In that configuration, we assume that whatever directory we're in is the top level of the work tree, and git rev-parse --show-toplevel responds accordingly. However, when we're in a subdirectory, that isn't correct: we respond with the subdirectory as the top level, resulting in unexpected behavior.

Ensure that we pass GIT_WORK_TREE as well as GIT_DIR so that git operations within subdirectories work correctly.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
Dscho, is this test going to cause a problem on Windows with the forward
slash in the grep statement?
 sequencer.c                   | 2 ++
 t/t3404-rebase-interactive.sh | 9 +++++++++
 2 files changed, 11 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..c8e16f9168 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2636,6 +2636,8 @@ static int do_exec(const char *command_line)
 	fprintf(stderr, "Executing: %s\n", command_line);
 	child_argv[0] = command_line;
 	argv_array_pushf(&child_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+	argv_array_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.argv);
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 352a52e59d..d03055d149 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -119,6 +119,15 @@ test_expect_success 'rebase -i with exec allows git commands in subdirs' '
 	)
 '
 
+test_expect_success 'rebase -i sets work tree properly' '
+	test_when_finished "rm -rf subdir" &&
+	test_when_finished "test_might_fail git rebase --abort" &&
+	mkdir subdir &&
+	git rebase -x "(cd subdir && git rev-parse --show-toplevel)" HEAD^ \
+		>actual &&
+	! grep "/subdir$" actual
+'
+
 test_expect_success 'rebase -i with the exec command checks tree cleanness' '
 	git checkout master &&
 	set_fake_editor &&
Jeff King· Jul 14, 2018, 00:57 UTC · re: brian m. carlson · lore

Re: [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands

On Sat, Jul 14, 2018 at 12:35:05AM +0000, brian m. carlson wrote:
Show 13 quoted lines
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..c8e16f9168 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2636,6 +2636,8 @@ static int do_exec(const char *command_line)
>  	fprintf(stderr, "Executing: %s\n", command_line);
>  	child_argv[0] = command_line;
>  	argv_array_pushf(&child_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
> +	argv_array_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.argv);
>

As a general rule of "always pass GIT_WORK_TREE with GIT_DIR", you have to deal with the case where there _isn't_ a work tree. Are we OK here because this code is inside sequencer.c, and you cannot do a sequencer operation without a work tree?

This all seemed vaguely familiar, so I dug up 2cd83d10bb (setup: suppress implicit "." work-tree for bare repos, 2013-03-08), which handles a similar case in the alias code.

So I think it would also work to set:
  GIT_IMPLICIT_WORK_TREE=0

here. But if the answer to my "are we OK" above is yes, I am fine with either that solution or the one you show here (but I think the commit message should probably mention it).

Show 16 quoted lines
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 352a52e59d..d03055d149 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -119,6 +119,15 @@ test_expect_success 'rebase -i with exec allows git commands in subdirs' '
>  	)
>  '
>  
> +test_expect_success 'rebase -i sets work tree properly' '
> +	test_when_finished "rm -rf subdir" &&
> +	test_when_finished "test_might_fail git rebase --abort" &&
> +	mkdir subdir &&
> +	git rebase -x "(cd subdir && git rev-parse --show-toplevel)" HEAD^ \
> +		>actual &&
> +	! grep "/subdir$" actual
> +'
This test looks good to me.
-Peff
brian m. carlson· Jul 14, 2018, 17:55 UTC · re: Jeff King · lore

Re: [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands

On Fri, Jul 13, 2018 at 08:57:03PM -0400, Jeff King wrote:
Show 20 quoted lines
> On Sat, Jul 14, 2018 at 12:35:05AM +0000, brian m. carlson wrote:
> 
> > diff --git a/sequencer.c b/sequencer.c
> > index 5354d4d51e..c8e16f9168 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2636,6 +2636,8 @@ static int do_exec(const char *command_line)
> >  	fprintf(stderr, "Executing: %s\n", command_line);
> >  	child_argv[0] = command_line;
> >  	argv_array_pushf(&child_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
> > +	argv_array_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.argv);
> >  
> 
> As a general rule of "always pass GIT_WORK_TREE with GIT_DIR", you have
> to deal with the case where there _isn't_ a work tree. Are we OK here
> because this code is inside sequencer.c, and you cannot do a sequencer
> operation without a work tree?

I believe that's correct. We only call this code path from sequencer_pick_commits or sequencer_continue, and those code paths are only called from revert, commit, and rebase--helper; all of those require a working tree.

Show 7 quoted lines
> So I think it would also work to set:
> 
>   GIT_IMPLICIT_WORK_TREE=0
> 
> here. But if the answer to my "are we OK" above is yes, I am fine with
> either that solution or the one you show here (but I think the commit
> message should probably mention it).

I'll reroll with a mention of that in the commit message. Thanks for a careful review.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
brian m. carlson· Jul 14, 2018, 18:38 UTC · re: brian m. carlson · lore

[PATCH v2] sequencer: pass absolute GIT_WORK_TREE to exec commands

The sequencer currently passes GIT_DIR, but not GIT_WORK_TREE, to exec commands. In that configuration, we assume that whatever directory we're in is the top level of the work tree, and git rev-parse --show-toplevel responds accordingly. However, when we're in a subdirectory, that isn't correct: we respond with the subdirectory as the top level, resulting in unexpected behavior.

Ensure that we pass GIT_WORK_TREE as well as GIT_DIR so that git operations within subdirectories work correctly.

Note that we are guaranteed to have a work tree in this case: the relevant sequencer functions are called only from revert, cherry-pick, and rebase--helper; all of these commands require a working tree.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 sequencer.c                   | 2 ++
 t/t3404-rebase-interactive.sh | 9 +++++++++
 2 files changed, 11 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..c8e16f9168 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2636,6 +2636,8 @@ static int do_exec(const char *command_line)
 	fprintf(stderr, "Executing: %s\n", command_line);
 	child_argv[0] = command_line;
 	argv_array_pushf(&child_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+	argv_array_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.argv);
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 352a52e59d..d03055d149 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -119,6 +119,15 @@ test_expect_success 'rebase -i with exec allows git commands in subdirs' '
 	)
 '
 
+test_expect_success 'rebase -i sets work tree properly' '
+	test_when_finished "rm -rf subdir" &&
+	test_when_finished "test_might_fail git rebase --abort" &&
+	mkdir subdir &&
+	git rebase -x "(cd subdir && git rev-parse --show-toplevel)" HEAD^ \
+		>actual &&
+	! grep "/subdir$" actual
+'
+
 test_expect_success 'rebase -i with the exec command checks tree cleanness' '
 	git checkout master &&
 	set_fake_editor &&
Jeff King· Jul 14, 2018, 21:19 UTC · re: brian m. carlson · lore

Re: [PATCH v2] sequencer: pass absolute GIT_WORK_TREE to exec commands

On Sat, Jul 14, 2018 at 06:38:59PM +0000, brian m. carlson wrote:
Show 13 quoted lines
> The sequencer currently passes GIT_DIR, but not GIT_WORK_TREE, to exec
> commands.  In that configuration, we assume that whatever directory
> we're in is the top level of the work tree, and git rev-parse
> --show-toplevel responds accordingly.  However, when we're in a
> subdirectory, that isn't correct: we respond with the subdirectory as
> the top level, resulting in unexpected behavior.
> 
> Ensure that we pass GIT_WORK_TREE as well as GIT_DIR so that git
> operations within subdirectories work correctly.
> 
> Note that we are guaranteed to have a work tree in this case: the
> relevant sequencer functions are called only from revert, cherry-pick,
> and rebase--helper; all of these commands require a working tree.
Thanks for digging into this corner case.

It's possible, of course, that some day we may have a caller without a work-tree, but it seems pretty unlikely for sequencer code. If we did, then:

Show 10 quoted lines
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..c8e16f9168 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2636,6 +2636,8 @@ static int do_exec(const char *command_line)
>  	fprintf(stderr, "Executing: %s\n", command_line);
>  	child_argv[0] = command_line;
>  	argv_array_pushf(&child_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
> +	argv_array_pushf(&child_env, "GIT_WORK_TREE=%s",
> +			 absolute_path(get_git_work_tree()));

I think this would just segfault (feeding NULL to absolute_path). That's probably OK, as it would be a big red flag to whomever is trying to teach the sequencer to work in a bare repo. :)

So this version looks good to me.
-Peff
Johannes Schindelin· Jul 14, 2018, 21:05 UTC · re: brian m. carlson · lore

Re: [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands

Hi Brian,
On Sat, 14 Jul 2018, brian m. carlson wrote:
Show 14 quoted lines
> The sequencer currently passes GIT_DIR, but not GIT_WORK_TREE, to exec
> commands.  In that configuration, we assume that whatever directory
> we're in is the top level of the work tree, and git rev-parse
> --show-toplevel responds accordingly.  However, when we're in a
> subdirectory, that isn't correct: we respond with the subdirectory as
> the top level, resulting in unexpected behavior.
> 
> Ensure that we pass GIT_WORK_TREE as well as GIT_DIR so that git
> operations within subdirectories work correctly.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> Dscho, is this test going to cause a problem on Windows with the forward
> slash in the grep statement?

It passes on Windows. The reason is that you are asking *Git* for a path, and Git will always try to use forward slashes (which work on Windows under most circumstances).

Ciao, Dscho

Show 38 quoted lines
>  sequencer.c                   | 2 ++
>  t/t3404-rebase-interactive.sh | 9 +++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..c8e16f9168 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2636,6 +2636,8 @@ static int do_exec(const char *command_line)
>  	fprintf(stderr, "Executing: %s\n", command_line);
>  	child_argv[0] = command_line;
>  	argv_array_pushf(&child_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
> +	argv_array_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.argv);
>  
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 352a52e59d..d03055d149 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -119,6 +119,15 @@ test_expect_success 'rebase -i with exec allows git commands in subdirs' '
>  	)
>  '
>  
> +test_expect_success 'rebase -i sets work tree properly' '
> +	test_when_finished "rm -rf subdir" &&
> +	test_when_finished "test_might_fail git rebase --abort" &&
> +	mkdir subdir &&
> +	git rebase -x "(cd subdir && git rev-parse --show-toplevel)" HEAD^ \
> +		>actual &&
> +	! grep "/subdir$" actual
> +'
> +
>  test_expect_success 'rebase -i with the exec command checks tree cleanness' '
>  	git checkout master &&
>  	set_fake_editor &&
>
brian m. carlson· Jul 14, 2018, 21:09 UTC · re: Johannes Schindelin · lore

Re: [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands

On Sat, Jul 14, 2018 at 11:05:34PM +0200, Johannes Schindelin wrote:
Show 9 quoted lines
> Hi Brian,
> 
> On Sat, 14 Jul 2018, brian m. carlson wrote:
> > Dscho, is this test going to cause a problem on Windows with the forward
> > slash in the grep statement?
> 
> It passes on Windows. The reason is that you are asking *Git* for a path,
> and Git will always try to use forward slashes (which work on Windows
> under most circumstances).
Great, thanks.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Junio C Hamano· Jul 16, 2018, 18:14 UTC · re: Jeff King · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> None of which is too surprising. The root of the bug is in the
> conversion to rebase--helper, I think, when presumably we started
> setting GIT_DIR at all (but I didn't dig further). Then 09d7b6c6fa fixed
> _one_ fallout of that, which was relative paths, but didn't help the
> subdirectory case.
>
> Just reading over this thread, I suspect the simplest fix is to pass
> GIT_DIR and GIT_WORK_TREE together, which is almost always the right
> thing to do.

Perhaps. Not exporting GIT_DIR (unless the end-user already did to the environment before starting "git rebase"---it would be a bad change to unexport it unconditionally) may probably be a way to make rebase--helper conversion more faithful to the original scripted Porcelain, but I suspect in practice always giving GIT_DIR and GIT_WORK_TREE would work well for many existing hooks.

Jeff King· Jul 16, 2018, 18:39 UTC · re: Junio C Hamano · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

On Mon, Jul 16, 2018 at 11:14:51AM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > None of which is too surprising. The root of the bug is in the
> > conversion to rebase--helper, I think, when presumably we started
> > setting GIT_DIR at all (but I didn't dig further). Then 09d7b6c6fa fixed
> > _one_ fallout of that, which was relative paths, but didn't help the
> > subdirectory case.
> >
> > Just reading over this thread, I suspect the simplest fix is to pass
> > GIT_DIR and GIT_WORK_TREE together, which is almost always the right
> > thing to do.
> 
> Perhaps.  Not exporting GIT_DIR (unless the end-user already did to
> the environment before starting "git rebase"---it would be a bad
> change to unexport it unconditionally) may probably be a way to make
> rebase--helper conversion more faithful to the original scripted
> Porcelain, but I suspect in practice always giving GIT_DIR and
> GIT_WORK_TREE would work well for many existing hooks.

Yeah, that may be an option. I don't remember if this was discussed in this thread or elsewhere, but setting GIT_DIR is a regression for hooks, etc, which do:

  git -C /some/other/repo log

or similar. I'm not sure if that falls under "actual regression" or "just how it happened to work in the past". I'm not even sure it worked that way _consistently_ in the past.

The best practice if you're switching directories is to do:
  unset $(git rev-parse --local-env-vars)
though I highly doubt that most people bother.
-Peff
Junio C Hamano· Jul 16, 2018, 21:10 UTC · re: Jeff King · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> On Mon, Jul 16, 2018 at 11:14:51AM -0700, Junio C Hamano wrote:
>
>> Porcelain, but I suspect in practice always giving GIT_DIR and
>> GIT_WORK_TREE would work well for many existing hooks.
>
> Yeah, that may be an option. I don't remember if this was discussed in
> this thread or elsewhere, but setting GIT_DIR is a regression for hooks,
> etc, which do:
>
>   git -C /some/other/repo log
>
> or similar. I'm not sure if that falls under "actual regression" or
> "just how it happened to work in the past". I'm not even sure it worked
> that way _consistently_ in the past.
>
> The best practice if you're switching directories is to do:
>
>   unset $(git rev-parse --local-env-vars)
>
> though I highly doubt that most people bother.

Yeah, that is exactly where my "in practice" comes from. I think I caught and killed another potential regression last week while it is still in the draft status during my review ;-)

Johannes Schindelin· Jul 19, 2018, 20:28 UTC · re: Junio C Hamano · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

Hi Junio,
On Mon, 16 Jul 2018, Junio C Hamano wrote:
Show 18 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > None of which is too surprising. The root of the bug is in the
> > conversion to rebase--helper, I think, when presumably we started
> > setting GIT_DIR at all (but I didn't dig further). Then 09d7b6c6fa fixed
> > _one_ fallout of that, which was relative paths, but didn't help the
> > subdirectory case.
> >
> > Just reading over this thread, I suspect the simplest fix is to pass
> > GIT_DIR and GIT_WORK_TREE together, which is almost always the right
> > thing to do.
> 
> Perhaps.  Not exporting GIT_DIR (unless the end-user already did to
> the environment before starting "git rebase"---it would be a bad
> change to unexport it unconditionally) may probably be a way to make
> rebase--helper conversion more faithful to the original scripted
> Porcelain, but I suspect in practice always giving GIT_DIR and
> GIT_WORK_TREE would work well for many existing hooks.
Forgetting the code in git-sh-setup, are we?

git_dir_init() rather specifically set GIT_DIR to the absolute path, and since that variable is already exported, the `exec` commands launched via `git-rebase--interactive` all saw it.

That is the reason why the sequencer.c was taught to set GIT_DIR to an absolute path rathern than not setting it: for backwards compatibility.

Ciao, Dscho

SZEDER Gábor· Jul 19, 2018, 22:09 UTC · re: Johannes Schindelin · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

Show 29 quoted lines
> On Mon, 16 Jul 2018, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > None of which is too surprising. The root of the bug is in the
> > > conversion to rebase--helper, I think, when presumably we started
> > > setting GIT_DIR at all (but I didn't dig further). Then 09d7b6c6fa fixed
> > > _one_ fallout of that, which was relative paths, but didn't help the
> > > subdirectory case.
> > >
> > > Just reading over this thread, I suspect the simplest fix is to pass
> > > GIT_DIR and GIT_WORK_TREE together, which is almost always the right
> > > thing to do.
> > 
> > Perhaps.  Not exporting GIT_DIR (unless the end-user already did to
> > the environment before starting "git rebase"---it would be a bad
> > change to unexport it unconditionally) may probably be a way to make
> > rebase--helper conversion more faithful to the original scripted
> > Porcelain, but I suspect in practice always giving GIT_DIR and
> > GIT_WORK_TREE would work well for many existing hooks.
> 
> Forgetting the code in git-sh-setup, are we?
> 
> git_dir_init() rather specifically set GIT_DIR to the absolute path, and
> since that variable is already exported, the `exec` commands launched via
> `git-rebase--interactive` all saw it.
> 
> That is the reason why the sequencer.c was taught to set GIT_DIR to an
> absolute path rathern than not setting it: for backwards compatibility.

GIT_DIR was not exported to 'exec' commands during an interactive rebase prior to 18633e1a22 (rebase -i: use the rebase--helper builtin, 2017-02-09) (nor was GIT_PREFIX):

  $ git log -Sgit_dir_init master git-rebase*.sh
  # Nothing.
  $ git checkout 18633e1a22a6^ && make -j4 prefix=/tmp/BEFORE install
  <....>
  $ git checkout 18633e1a22a6 && make -j4 prefix=/tmp/AFTER install
  <....>
  $ GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"' /tmp/BEFORE/bin/git rebase -i HEAD^
  Executing: set |grep ^GIT
  GIT_CHERRY_PICK_HELP=$'\nWhen you have resolved this problem, run "git rebase --continue".\nIf you prefer to skip this patch, run "git rebase --skip" instead.\nTo check out the original branch and stop rebasing, run "git rebase --abort".\n'
  GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"'
  GIT_INTERNAL_GETTEXT_SH_SCHEME=gnu
  GIT_REFLOG_ACTION='rebase -i (start): checkout HEAD^'
  warning: notes ref refs/notes/commits is invalid
  Successfully rebased and updated refs/heads/master.
  $ GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"' /tmp/AFTER/bin/git rebase -i HEAD^
  Executing: set |grep ^GIT
  GIT_CHERRY_PICK_HELP='
  GIT_DIR='.git'
  GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"'
  GIT_INTERNAL_GETTEXT_SH_SCHEME='gnu'
  GIT_PREFIX=''
  GIT_REFLOG_ACTION='rebase -i (start): checkout HEAD^'
  warning: notes ref refs/notes/commits is invalid
  Successfully rebased and updated refs/heads/master.

And then recently came 226c0ddd0d (exec_cmd: RUNTIME_PREFIX on some POSIX systems, 2018-04-10), which then started to export GIT_EXEC_PATH to 'exec' commands as well...

Junio C Hamano· Jul 19, 2018, 22:34 UTC · re: SZEDER Gábor · lore

Re: rev-parse --show-toplevel broken during exec'ed rebase?

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 41 quoted lines
>> Forgetting the code in git-sh-setup, are we?
>> 
>> git_dir_init() rather specifically set GIT_DIR to the absolute path, and
>> since that variable is already exported, the `exec` commands launched via
>> `git-rebase--interactive` all saw it.
>> 
>> That is the reason why the sequencer.c was taught to set GIT_DIR to an
>> absolute path rathern than not setting it: for backwards compatibility.
>
> GIT_DIR was not exported to 'exec' commands during an interactive
> rebase prior to 18633e1a22 (rebase -i: use the rebase--helper builtin,
> 2017-02-09) (nor was GIT_PREFIX):
>
>   $ git log -Sgit_dir_init master git-rebase*.sh
>   # Nothing.
>   $ git checkout 18633e1a22a6^ && make -j4 prefix=/tmp/BEFORE install
>   <....>
>   $ git checkout 18633e1a22a6 && make -j4 prefix=/tmp/AFTER install
>   <....>
>   $ GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"' /tmp/BEFORE/bin/git rebase -i HEAD^
>   Executing: set |grep ^GIT
>   GIT_CHERRY_PICK_HELP=$'\nWhen you have resolved this problem, run "git rebase --continue".\nIf you prefer to skip this patch, run "git rebase --skip" instead.\nTo check out the original branch and stop rebasing, run "git rebase --abort".\n'
>   GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"'
>   GIT_INTERNAL_GETTEXT_SH_SCHEME=gnu
>   GIT_REFLOG_ACTION='rebase -i (start): checkout HEAD^'
>   warning: notes ref refs/notes/commits is invalid
>   Successfully rebased and updated refs/heads/master.
>   $ GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"' /tmp/AFTER/bin/git rebase -i HEAD^
>   Executing: set |grep ^GIT
>   GIT_CHERRY_PICK_HELP='
>   GIT_DIR='.git'
>   GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"'
>   GIT_INTERNAL_GETTEXT_SH_SCHEME='gnu'
>   GIT_PREFIX=''
>   GIT_REFLOG_ACTION='rebase -i (start): checkout HEAD^'
>   warning: notes ref refs/notes/commits is invalid
>   Successfully rebased and updated refs/heads/master.
>
> And then recently came 226c0ddd0d (exec_cmd: RUNTIME_PREFIX on some
> POSIX systems, 2018-04-10), which then started to export GIT_EXEC_PATH
> to 'exec' commands as well...

Correct. git-sh-setup does assign to GIT_DIR, but unless the end-user has it exported, it does not export, which has long been very deliberate. But it does not really matter, as exporting BOTH GIT_DIR and GIT_WORK_TREE is a lot easier to understand and probably safe (even though technically a regressing) solution, and fewer and fewer things will be relying on git-sh-setup in the future anyway.

← back to recent threads