t7005-editor: quote filename to fix whitespace-issue

Commit Message

Martin Ågren Sept. 26, 2018, 4:14 p.m. UTC

From: Alexander Pyhalov <[e-mail]>

Commit 4362da078e (t7005-editor: get rid of the SPACES_IN_FILENAMES
prereq, 2018-05-14) removed code for detecting whether spaces in
filenames work. Since we rely on spaces throughout the test suite
("trash directory.t1234-foo"), testing whether we can use the filename
"e space.sh" was redundant and unnecessary.

In simplifying the code, though, this introduced a regression around how
spaces are handled, not in the /name/ of the editor script, but /in/ the
script itself. The script just does `echo space >$1`, where $1 is for
example "/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG".

With most shells, or with Bash in posix mode, $1 will not be subjected
to field splitting. But if we invoke Bash directly, which will happen if
we build Git with SHELL_PATH=/bin/bash, it will detect and complain
about an "ambiguous redirect". More details can be found in [1], thanks
to SZEDER Gábor.

Make sure that the editor script quotes "$1" to remove the ambiguity.

[1] https://public-inbox.org/git/20180926121107.GH27036@localhost/

Signed-off-by: Alexander Pyhalov <[e-mail]>
Commit-message-by: Martin Ågren <[e-mail]>
Signed-off-by: Martin Ågren <[e-mail]>
---
 SZEDER wrote:
 > Let me put on my POSIX-lawyer hat for a moment to explain this :)

 Thanks for an excellent explanation.

 > Sidenote: this test should use the write_script helper to create this
 > editor script.

 Yes. I've punted on that for now.

 Here's my updated commit message as part of a proper patch. Thanks
 Alexander for the analysis and the diff, and thanks Eric and SZEDER for
 getting me on the right track with the commit message.

 t/t7005-editor.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Taylor Blau Sept. 26, 2018, 6:14 p.m. UTC | #1

On Wed, Sep 26, 2018 at 06:14:11PM +0200, Martin Ågren wrote:
> From: Alexander Pyhalov <[e-mail]>
>
> Commit 4362da078e (t7005-editor: get rid of the SPACES_IN_FILENAMES
> prereq, 2018-05-14) removed code for detecting whether spaces in
> filenames work. Since we rely on spaces throughout the test suite
> ("trash directory.t1234-foo"), testing whether we can use the filename
> "e space.sh" was redundant and unnecessary.
>
> In simplifying the code, though, this introduced a regression around how
> spaces are handled, not in the /name/ of the editor script, but /in/ the
> script itself. The script just does `echo space >$1`, where $1 is for
> example "/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG".
>
> With most shells, or with Bash in posix mode, $1 will not be subjected
> to field splitting. But if we invoke Bash directly, which will happen if
> we build Git with SHELL_PATH=/bin/bash, it will detect and complain
> about an "ambiguous redirect". More details can be found in [1], thanks
> to SZEDER Gábor.
>
> Make sure that the editor script quotes "$1" to remove the ambiguity.
>
> [1] https://public-inbox.org/git/20180926121107.GH27036@localhost/
>
> Signed-off-by: Alexander Pyhalov <[e-mail]>
> Commit-message-by: Martin Ågren <[e-mail]>
> Signed-off-by: Martin Ågren <[e-mail]>

Thanks. I find that the explanation of the regression is a helpful one,
and the changes below look sane to me.

Since I couldn't find any other style in the surrounding script that
needed matching against, this has my:

  Reviewed-by: Taylor Blau <[e-mail]>

Thanks,
Taylor

Jeff King Sept. 26, 2018, 7:21 p.m. UTC | #2

On Wed, Sep 26, 2018 at 06:14:11PM +0200, Martin Ågren wrote:

> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> index b2ca77b338..5fcf281dfb 100755
> --- a/t/t7005-editor.sh
> +++ b/t/t7005-editor.sh
> @@ -112,7 +112,7 @@ do
>  done
>  
>  test_expect_success 'editor with a space' '
> -	echo "echo space >\$1" >"e space.sh" &&
> +	echo "echo space >\"\$1\"" >"e space.sh" &&
>  	chmod a+x "e space.sh" &&
>  	GIT_EDITOR="./e\ space.sh" git commit --amend &&

I was actually puzzled how SHELL_PATH matters here at all, since the
resulting script does not mention it.

What happens is that we first try to execve("./e space.sh"), can get
ENOEXEC. And then we resort to passing it to the shell, which then uses
historical shell magic (which apparently predates the invention of #!
entirely!) to decide to run it as a script using the current shell. And
that shell is selected via the SHELL_PATH at build time (and not the
$SHELL_PATH we have in our environment here).

So I think this fix and the explanation are correct. I do think it would
be a lot less subtle (and a lot more readable) as:

  write_script "e space.sh" <<-\EOF &&
  echo space >"$1"
  EOF

but that is orthogonal to what you're fixing.

-Peff

Patch

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index b2ca77b338..5fcf281dfb 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -112,7 +112,7 @@  do
 done
 
 test_expect_success 'editor with a space' '
-	echo "echo space >\$1" >"e space.sh" &&
+	echo "echo space >\"\$1\"" >"e space.sh" &&
 	chmod a+x "e space.sh" &&
 	GIT_EDITOR="./e\ space.sh" git commit --amend &&
 	test space = "$(git show -s --pretty=format:%s)"