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

[Jeff King <peff@peff.net>]

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