git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* rev-parse --show-toplevel broken during exec'ed rebase?
@ 2018-07-12  2:50 Vitali Lovich
  2018-07-12  2:53 ` Vitali Lovich
  2018-07-12 15:23 ` Junio C Hamano
  0 siblings, 2 replies; 19+ messages in thread
From: Vitali Lovich @ 2018-07-12  2:50 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: rev-parse --show-toplevel broken during exec'ed rebase?
  2018-07-12  2:50 rev-parse --show-toplevel broken during exec'ed rebase? Vitali Lovich
@ 2018-07-12  2:53 ` Vitali Lovich
  2018-07-12 14:49   ` Johannes Schindelin
  2018-07-12 15:23 ` Junio C Hamano
  1 sibling, 1 reply; 19+ messages in thread
From: Vitali Lovich @ 2018-07-12  2:53 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: rev-parse --show-toplevel broken during exec'ed rebase?
  2018-07-12  2:53 ` Vitali Lovich
@ 2018-07-12 14:49   ` Johannes Schindelin
  2018-07-12 15:02     ` Johannes Schindelin
  0 siblings, 1 reply; 19+ messages in thread
From: Johannes Schindelin @ 2018-07-12 14:49 UTC (permalink / raw)
  To: Vitali Lovich; +Cc: git

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: rev-parse --show-toplevel broken during exec'ed rebase?
  2018-07-12 14:49   ` Johannes Schindelin
@ 2018-07-12 15:02     ` Johannes Schindelin
  0 siblings, 0 replies; 19+ messages in thread
From: Johannes Schindelin @ 2018-07-12 15:02 UTC (permalink / raw)
  To: Vitali Lovich; +Cc: git

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: rev-parse --show-toplevel broken during exec'ed rebase?
  2018-07-12  2:50 rev-parse --show-toplevel broken during exec'ed rebase? Vitali Lovich
  2018-07-12  2:53 ` Vitali Lovich
@ 2018-07-12 15:23 ` Junio C Hamano
  2018-07-12 17:39   ` Vitali Lovich
  2018-07-13 18:47   ` brian m. carlson
  1 sibling, 2 replies; 19+ messages in thread
From: Junio C Hamano @ 2018-07-12 15:23 UTC (permalink / raw)
  To: Vitali Lovich; +Cc: git

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: rev-parse --show-toplevel broken during exec'ed rebase?
  2018-07-12 15:23 ` Junio C Hamano
@ 2018-07-12 17:39   ` Vitali Lovich
  2018-07-13 18:47   ` brian m. carlson
  1 sibling, 0 replies; 19+ messages in thread
From: Vitali Lovich @ 2018-07-12 17:39 UTC (permalink / raw)
  To: gitster; +Cc: git

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: rev-parse --show-toplevel broken during exec'ed rebase?
  2018-07-12 15:23 ` Junio C Hamano
  2018-07-12 17:39   ` Vitali Lovich
@ 2018-07-13 18:47   ` brian m. carlson
  2018-07-13 20:19     ` Jeff King
  1 sibling, 1 reply; 19+ messages in thread
From: brian m. carlson @ 2018-07-13 18:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Vitali Lovich, git

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: rev-parse --show-toplevel broken during exec'ed rebase?
  2018-07-13 18:47   ` brian m. carlson
@ 2018-07-13 20:19     ` Jeff King
  2018-07-13 23:05       ` brian m. carlson
  2018-07-16 18:14       ` rev-parse --show-toplevel broken during exec'ed rebase? Junio C Hamano
  0 siblings, 2 replies; 19+ messages in thread
From: Jeff King @ 2018-07-13 20:19 UTC (permalink / raw)
  To: brian m. carlson, Junio C Hamano, Vitali Lovich, git
  Cc: Johannes Schindelin, Jacob Keller

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: rev-parse --show-toplevel broken during exec'ed rebase?
  2018-07-13 20:19     ` Jeff King
@ 2018-07-13 23:05       ` brian m. carlson
  2018-07-14  0:35         ` [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands brian m. carlson
  2018-07-16 18:14       ` rev-parse --show-toplevel broken during exec'ed rebase? Junio C Hamano
  1 sibling, 1 reply; 19+ messages in thread
From: brian m. carlson @ 2018-07-13 23:05 UTC (permalink / raw)
  To: Jeff King
  Cc: Junio C Hamano, Vitali Lovich, git, Johannes Schindelin, Jacob Keller

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands
  2018-07-13 23:05       ` brian m. carlson
@ 2018-07-14  0:35         ` brian m. carlson
  2018-07-14  0:57           ` Jeff King
  2018-07-14 21:05           ` [PATCH] " Johannes Schindelin
  0 siblings, 2 replies; 19+ messages in thread
From: brian m. carlson @ 2018-07-14  0:35 UTC (permalink / raw)
  To: git
  Cc: Jacob Keller, Jeff King, Junio C Hamano, Johannes Schindelin,
	Vitali Lovich

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands
  2018-07-14  0:35         ` [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands brian m. carlson
@ 2018-07-14  0:57           ` Jeff King
  2018-07-14 17:55             ` brian m. carlson
  2018-07-14 21:05           ` [PATCH] " Johannes Schindelin
  1 sibling, 1 reply; 19+ messages in thread
From: Jeff King @ 2018-07-14  0:57 UTC (permalink / raw)
  To: brian m. carlson
  Cc: git, Jacob Keller, Junio C Hamano, Johannes Schindelin, Vitali Lovich

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands
  2018-07-14  0:57           ` Jeff King
@ 2018-07-14 17:55             ` brian m. carlson
  2018-07-14 18:38               ` [PATCH v2] " brian m. carlson
  0 siblings, 1 reply; 19+ messages in thread
From: brian m. carlson @ 2018-07-14 17:55 UTC (permalink / raw)
  To: Jeff King
  Cc: git, Jacob Keller, Junio C Hamano, Johannes Schindelin, Vitali Lovich

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2] sequencer: pass absolute GIT_WORK_TREE to exec commands
  2018-07-14 17:55             ` brian m. carlson
@ 2018-07-14 18:38               ` " brian m. carlson
  2018-07-14 21:19                 ` Jeff King
  0 siblings, 1 reply; 19+ messages in thread
From: brian m. carlson @ 2018-07-14 18:38 UTC (permalink / raw)
  To: git
  Cc: Jacob Keller, Jeff King, Junio C Hamano, Johannes Schindelin,
	Vitali Lovich

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands
  2018-07-14  0:35         ` [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands brian m. carlson
  2018-07-14  0:57           ` Jeff King
@ 2018-07-14 21:05           ` " Johannes Schindelin
  2018-07-14 21:09             ` brian m. carlson
  1 sibling, 1 reply; 19+ messages in thread
From: Johannes Schindelin @ 2018-07-14 21:05 UTC (permalink / raw)
  To: brian m. carlson
  Cc: git, Jacob Keller, Jeff King, Junio C Hamano, Vitali Lovich

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands
  2018-07-14 21:05           ` [PATCH] " Johannes Schindelin
@ 2018-07-14 21:09             ` brian m. carlson
  0 siblings, 0 replies; 19+ messages in thread
From: brian m. carlson @ 2018-07-14 21:09 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: git, Jacob Keller, Jeff King, Junio C Hamano, Vitali Lovich

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] sequencer: pass absolute GIT_WORK_TREE to exec commands
  2018-07-14 18:38               ` [PATCH v2] " brian m. carlson
@ 2018-07-14 21:19                 ` Jeff King
  0 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2018-07-14 21:19 UTC (permalink / raw)
  To: brian m. carlson
  Cc: git, Jacob Keller, Junio C Hamano, Johannes Schindelin, Vitali Lovich

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: rev-parse --show-toplevel broken during exec'ed rebase?
  2018-07-13 20:19     ` Jeff King
  2018-07-13 23:05       ` brian m. carlson
@ 2018-07-16 18:14       ` Junio C Hamano
  2018-07-16 18:39         ` Jeff King
  1 sibling, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2018-07-16 18:14 UTC (permalink / raw)
  To: Jeff King
  Cc: brian m. carlson, Vitali Lovich, git, Johannes Schindelin, Jacob Keller

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: rev-parse --show-toplevel broken during exec'ed rebase?
  2018-07-16 18:14       ` rev-parse --show-toplevel broken during exec'ed rebase? Junio C Hamano
@ 2018-07-16 18:39         ` Jeff King
  2018-07-16 21:10           ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Jeff King @ 2018-07-16 18:39 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: brian m. carlson, Vitali Lovich, git, Johannes Schindelin, Jacob Keller

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: rev-parse --show-toplevel broken during exec'ed rebase?
  2018-07-16 18:39         ` Jeff King
@ 2018-07-16 21:10           ` Junio C Hamano
  0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2018-07-16 21:10 UTC (permalink / raw)
  To: Jeff King
  Cc: brian m. carlson, Vitali Lovich, git, Johannes Schindelin, Jacob Keller

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, back to index

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-12  2:50 rev-parse --show-toplevel broken during exec'ed rebase? Vitali Lovich
2018-07-12  2:53 ` Vitali Lovich
2018-07-12 14:49   ` Johannes Schindelin
2018-07-12 15:02     ` Johannes Schindelin
2018-07-12 15:23 ` Junio C Hamano
2018-07-12 17:39   ` Vitali Lovich
2018-07-13 18:47   ` brian m. carlson
2018-07-13 20:19     ` Jeff King
2018-07-13 23:05       ` brian m. carlson
2018-07-14  0:35         ` [PATCH] sequencer: pass absolute GIT_WORK_TREE to exec commands brian m. carlson
2018-07-14  0:57           ` Jeff King
2018-07-14 17:55             ` brian m. carlson
2018-07-14 18:38               ` [PATCH v2] " brian m. carlson
2018-07-14 21:19                 ` Jeff King
2018-07-14 21:05           ` [PATCH] " Johannes Schindelin
2018-07-14 21:09             ` brian m. carlson
2018-07-16 18:14       ` rev-parse --show-toplevel broken during exec'ed rebase? Junio C Hamano
2018-07-16 18:39         ` Jeff King
2018-07-16 21:10           ` Junio C Hamano

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox