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

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

[Vitali Lovich]

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

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

[Vitali Lovich]

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

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

[Johannes Schindelin]

Hi Vitali,

[please avoid top-posting on this mailing list]

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

Ciao,
Johannes

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

[Johannes Schindelin]

Hi,

On Thu, 12 Jul 2018, Johannes Schindelin wrote:

> 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

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

[Junio C Hamano]

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.

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

[Vitali Lovich]

On Thu, Jul 12, 2018 at 8:23 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

On Thu, Jul 12, 2018 at 08:23:02AM -0700, Junio C Hamano wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

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

[Jeff King]

On Fri, Jul 13, 2018 at 06:47:32PM +0000, brian m. carlson wrote:

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

[PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands

[brian m. carlson]

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

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

[Jeff King]

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?

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

> 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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On Fri, Jul 13, 2018 at 08:57:03PM -0400, Jeff King wrote:
> 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.

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

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

[brian m. carlson]

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

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

[Johannes Schindelin]

Hi Brian,

On Sat, 14 Jul 2018, brian m. carlson wrote:

> 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

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On Sat, Jul 14, 2018 at 11:05:34PM +0200, Johannes Schindelin wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

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

[Jeff King]

On Sat, Jul 14, 2018 at 06:38:59PM +0000, brian m. carlson wrote:

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

> 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

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

[Junio C Hamano]

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.

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

[Jeff King]

On Mon, Jul 16, 2018 at 11:14:51AM -0700, 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.

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 ;-)

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

[Johannes Schindelin]

Hi Junio,

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.

Ciao,
Dscho

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

[SZEDER Gábor]

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

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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

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