t7005-editor.sh failure

t7005-editor.sh failure

[Alexander Pyhalov]

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

On updating git to 2.19 we've suddenly got t7005-editor.sh test failures.
The issue seems to be that generated "e space.sh" file can't handle
files with spaces.
Instead of 'echo space >$1' it should be 'echo space > "$1"' or git
editor fails when gets file with spaces in name.

The patch is simple:



-- 
Best regards,
Alexander Pyhalov,
system administrator of Southern Federal University IT department

[-- Attachment #2: 0005-t7005-editor.sh.patch --]
[-- Type: text/x-patch, Size: 413 bytes --]

--- git-2.19.0/t/t7005-editor.sh.~1~	Mon Sep 10 21:39:13 2018
+++ git-2.19.0/t/t7005-editor.sh	Wed Sep 26 09:41:30 2018
@@ -112,7 +112,7 @@
 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)"

Re: t7005-editor.sh failure

[Martin Ågren]

Hi Alexander,

Welcome to the list!

On Wed, 26 Sep 2018 at 08:54, Alexander Pyhalov <apyhalov@gmail.com> wrote:
> On updating git to 2.19 we've suddenly got t7005-editor.sh test failures.
> The issue seems to be that generated "e space.sh" file can't handle
> files with spaces.
> Instead of 'echo space >$1' it should be 'echo space > "$1"' or git
> editor fails when gets file with spaces in name.

Thanks for finding, analysing and reporting. I haven't bisected, but I'm
guessing this comes from 4362da078e (t7005-editor: get rid of the
SPACES_IN_FILENAMES prereq, 2018-05-14), which only happens to have to
do with spaces in filenames. But in rewriting the test, it introduced
/another/ instance of spaces-matter-here and didn't quote $1 properly.
Cute. :-)

We try to snuggle the filename to the >redirector, so it would be 'echo
space >"$1"' and similar.

Could we have your sign-off for this? Please see [1] for what that
means. If you want to re-submit as a "proper" patch with commit message
and all, great. If not, I could do it for you, with you as "Author:", if
you just let me know.

By the way, could you say something about which shell or which
environment this bug triggered in? Just so we can better understand how
this snuck past us.

[1] https://github.com/git/git/blob/master/Documentation/SubmittingPatches

Thanks
Martin

Re: t7005-editor.sh failure

[Alexander Pyhalov]

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

Hi.
As for sign-off, do I understand correctly that you just want to know
that I'm the original author of the code? Yes, it's so.

I see this on OpenIndiana in
https://github.com/OpenIndiana/oi-userland/pull/4456 , when running
test suite.
Not sure why it wasn't noticed earlier, as 'trash directory' is used in path.

When I use truss to look at failure, I see

2653:   execve("/export/home/alp/srcs/oi-userland/components/developer/git/build/amd64/git",
0x00564EC8, 0x00566008)  argc = 3
2653:    argv:
2653:     /export/home/alp/srcs/oi-userland/components/developer/git/build/amd64/git
2653:     commit --amend
hint: Waiting for your editor to close the file... 2655:
execve("/bin/bash", 0x007EA898, 0x007EA960)  argc = 5
2655:    argv: /bin/bash -c ./e\ space.sh "$@" ./e\ space.sh
2655:     /export/home/alp/srcs/oi-userland/components/developer/git/build/amd64/t/trash
directory.t7005-editor/.git/COMMIT_EDITMSG
2655:   execve("./e space.sh", 0x005655C8, 0x00564008)  Err#8 ENOEXEC
./e space.sh: line 1: $1: ambiguous redirect
2653:       Received signal #18, SIGCLD, in waitid() [default]
2653:         siginfo: SIGCLD CLD_EXITED pid=2655 status=0x0001
error: There was a problem with the editor './e\ space.sh'.
Please supply the message using either -m or -F option.

Shell is bash, as you can see (GNU bash, version 4.4.23(1)-release
(i386-pc-solaris2.11))


2018-09-26 10:59 GMT+03:00 Martin Ågren <martin.agren@gmail.com>:
> Hi Alexander,
>
> Welcome to the list!
>
> On Wed, 26 Sep 2018 at 08:54, Alexander Pyhalov <apyhalov@gmail.com> wrote:
>> On updating git to 2.19 we've suddenly got t7005-editor.sh test failures.
>> The issue seems to be that generated "e space.sh" file can't handle
>> files with spaces.
>> Instead of 'echo space >$1' it should be 'echo space > "$1"' or git
>> editor fails when gets file with spaces in name.
>
> Thanks for finding, analysing and reporting. I haven't bisected, but I'm
> guessing this comes from 4362da078e (t7005-editor: get rid of the
> SPACES_IN_FILENAMES prereq, 2018-05-14), which only happens to have to
> do with spaces in filenames. But in rewriting the test, it introduced
> /another/ instance of spaces-matter-here and didn't quote $1 properly.
> Cute. :-)
>
> We try to snuggle the filename to the >redirector, so it would be 'echo
> space >"$1"' and similar.
>
> Could we have your sign-off for this? Please see [1] for what that
> means. If you want to re-submit as a "proper" patch with commit message
> and all, great. If not, I could do it for you, with you as "Author:", if
> you just let me know.
>
> By the way, could you say something about which shell or which
> environment this bug triggered in? Just so we can better understand how
> this snuck past us.
>
> [1] https://github.com/git/git/blob/master/Documentation/SubmittingPatches
>
> Thanks
> Martin



-- 
С уважением,
Александр Пыхалов

[-- Attachment #2: 0001-t7005-editor.sh-e-space.sh-should-properly-escape-fi.patch --]
[-- Type: text/x-patch, Size: 809 bytes --]

From 171e2404a5e19b1b9b5112ad408a5cc1cf963289 Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <apyhalov@gmail.com>
Date: Wed, 26 Sep 2018 11:54:58 +0300
Subject: [PATCH] t7005-editor.sh: "e space.sh" should properly escape file
 path

Signed-off-by: Alexander Pyhalov <apyhalov@gmail.com>
---
 t/t7005-editor.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index b2ca77b33..66243c43c 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)"
-- 
2.19.0

Re: t7005-editor.sh failure

[Martin Ågren]

On Wed, 26 Sep 2018 at 11:00, Alexander Pyhalov <apyhalov@gmail.com> wrote:
> As for sign-off, do I understand correctly that you just want to know
> that I'm the original author of the code? Yes, it's so.

Right. Plus that you agree that the code (the commit) may be
redistributed basically forever.

> I see this on OpenIndiana in
> https://github.com/OpenIndiana/oi-userland/pull/4456 , when running
> test suite.
> Not sure why it wasn't noticed earlier, as 'trash directory' is used in path.

My first theory was that my shell and that of other developers was
"modern" or "clever" enough to realize that the space belongs to the
filename, so it just takes everything to the end of line. Whereas your
shell would be "dumber". I see now that you have a newer bash than I
do... Maybe this cleverness can be configured (at compile-time?), or
maybe something else is happening.

> execve("/bin/bash", 0x007EA898, 0x007EA960)  argc = 5
> 2655:    argv: /bin/bash -c ./e\ space.sh "$@" ./e\ space.sh
> 2655:     /export/home/alp/srcs/oi-userland/components/developer/git/build/amd64/t/trash
> directory.t7005-editor/.git/COMMIT_EDITMSG
> 2655:   execve("./e space.sh", 0x005655C8, 0x00564008)  Err#8 ENOEXEC
> ./e space.sh: line 1: $1: ambiguous redirect

> Shell is bash, as you can see (GNU bash, version 4.4.23(1)-release
> (i386-pc-solaris2.11))

I came up with the following commit message. What do you think about it?

    t7005-editor: quote filename to fix whitespace-issue

    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, the commit introduced a regression around
    how spaces are handled, not in the /name/ of the script, but /in/ the
    script itself. The editor-script created looks like this:

      echo space >$1

    We will try to execute something like

      echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG

    Most shells seem to be able to figure out that the filename doesn't end
    with "trash" but continues all the way to "COMMIT_EDITMSG", but at least
    one shell chokes on this.

    Make sure that the editor-script quotes "$1".

Martin

Re: t7005-editor.sh failure

[Alexander Pyhalov]

2018-09-26 12:52 GMT+03:00 Martin Ågren <martin.agren@gmail.com>:
> On Wed, 26 Sep 2018 at 11:00, Alexander Pyhalov <apyhalov@gmail.com> wrote:
>> As for sign-off, do I understand correctly that you just want to know
>> that I'm the original author of the code? Yes, it's so.
>
> Right. Plus that you agree that the code (the commit) may be
> redistributed basically forever.

I'm fine with it.


> I came up with the following commit message. What do you think about it?
>
>     t7005-editor: quote filename to fix whitespace-issue
>
>     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, the commit introduced a regression around
>     how spaces are handled, not in the /name/ of the script, but /in/ the
>     script itself. The editor-script created looks like this:
>
>       echo space >$1
>
>     We will try to execute something like
>
>       echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG
>
>     Most shells seem to be able to figure out that the filename doesn't end
>     with "trash" but continues all the way to "COMMIT_EDITMSG", but at least
>     one shell chokes on this.
>
>     Make sure that the editor-script quotes "$1".
>


Looks fine. Thanks.


-- 
С уважением,
Александр Пыхалов

Re: t7005-editor.sh failure

[Eric Sunshine]

On Wed, Sep 26, 2018 at 5:53 AM Martin Ågren <martin.agren@gmail.com> wrote:
> I came up with the following commit message. What do you think about it?
>
>     t7005-editor: quote filename to fix whitespace-issue
>
>     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, the commit introduced a regression around
>     how spaces are handled, not in the /name/ of the script, but /in/ the
>     script itself. The editor-script created looks like this:
>
>       echo space >$1
>
>     We will try to execute something like
>
>       echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG
>
>     Most shells seem to be able to figure out that the filename doesn't end
>     with "trash" but continues all the way to "COMMIT_EDITMSG", but at least
>     one shell chokes on this.
>
>     Make sure that the editor-script quotes "$1".

This description of the behavior is misleading (actually, actively
wrong). Shells are not somehow inferring that the space is part of the
redirect filename. The missing piece is that the following all behave
the same:

    echo foo bar >cow
    echo >cow foo bar
    echo foo >cow bar

That is, they all create a file named "cow" with content "foo bar".
So, in your example:

    echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG

what is actually happening is that it is creating a file named
"/foo/t/trash" with content "space
directory.t7005-editor/.git/COMMIT_EDITMSG".

As for the "ambiguous redirect" diagnostic, that seems to be Bash
trying to be helpful in reporting what is likely a programming error
(that is, forgetting to double-quote the expansion).

Re: t7005-editor.sh failure

[Martin Ågren]

On Wed, 26 Sep 2018 at 13:59, Eric Sunshine <sunshine@sunshineco.com> wrote:
> This description of the behavior is misleading (actually, actively
> wrong).

Hmm, that's bad, my apologies.

>     echo foo bar >cow
>     echo >cow foo bar
>     echo foo >cow bar
>
> That is, they all create a file named "cow" with content "foo bar".

Somehow I knew that, as in "I've seen that before", but I guess I've
never thought about it long enough to really incorporate it.

> So, in your example:
>
>     echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG
>
> what is actually happening is that it is creating a file named
> "/foo/t/trash" with content "space
> directory.t7005-editor/.git/COMMIT_EDITMSG".

Thanks for clarifying.

> As for the "ambiguous redirect" diagnostic, that seems to be Bash
> trying to be helpful in reporting what is likely a programming error
> (that is, forgetting to double-quote the expansion).

I see that SZEDER has posted some interesting reading. I'll make sure I
understand this better before coming back to this later today.

Thanks
Martin

Re: t7005-editor.sh failure

[SZEDER Gábor]

On Wed, Sep 26, 2018 at 11:52:42AM +0200, Martin Ågren wrote:
> On Wed, 26 Sep 2018 at 11:00, Alexander Pyhalov <apyhalov@gmail.com> wrote:
> > As for sign-off, do I understand correctly that you just want to know
> > that I'm the original author of the code? Yes, it's so.
> 
> Right. Plus that you agree that the code (the commit) may be
> redistributed basically forever.
> 
> > I see this on OpenIndiana in
> > https://github.com/OpenIndiana/oi-userland/pull/4456 , when running
> > test suite.
> > Not sure why it wasn't noticed earlier, as 'trash directory' is used in path.
> 
> My first theory was that my shell and that of other developers was
> "modern" or "clever" enough to realize that the space belongs to the
> filename, so it just takes everything to the end of line.

(Note that redirections can occur anywhere in the command, i.e. these
are all equivalent: 'echo foo >out', 'echo >out foo' '>out echo foo')

> Whereas your
> shell would be "dumber". I see now that you have a newer bash than I
> do... Maybe this cleverness can be configured (at compile-time?), or
> maybe something else is happening.

Let me put on my POSIX-lawyer hat for a moment to explain this :)

Long story short: Bash doesn't conform to POSIX in this respect.

So, the Shell Command Language specification section 2.6 Word
Expansions [1] says among many other things the following:

  Tilde expansions, parameter expansions, command substitutions,
  arithmetic expansions, and quote removals that occur within a single
  word expand to a single field. It is only field splitting or
  pathname expansion that can create multiple fields from a single
  word.

Later, in section 2.7 Redirection [2]:

  [...] the word that follows the redirection operator shall be
  subjected to tilde expansion, parameter expansion, command
  substitution, arithmetic expansion, and quote removal. Pathname
  expansion shall not be performed on the word by a non-interactive
  shell; an interactive shell may perform it, but shall do so only
  when the expansion would result in one word.

Note that this "word" is _not_ subject of field splitting, i.e. in a
redirection like

  echo foo >$file

it's not necessary to quote $file, because it will remain a single
field even if it contains spaces.

Most shells I have at hand follow the specs:

  $ for shell in dash mksh ksh ksh93 zsh ; do $shell ./e\ space.sh "output from $shell" ; done
  $ ls -1 output\ from*
  output from dash
  output from ksh
  output from ksh93
  output from mksh
  output from zsh

Bash doesn't:

  $ bash ./e\ space.sh "output from bash"
  ./e space.sh: line 1: $1: ambiguous redirect

And this behaviour is documented in its man page (though the text
calls "word splitting" what the specs call "field splitting"):

  The word following the redirection operator in the following descrip
  tions, unless otherwise noted, is subjected to brace expansion,
  tilde expansion, parameter and variable expansion, command
  substitution, arithmetic expansion, quote removal, pathname
  expansion, and word splitting.

When run in posix mode, however, even Bash follows the specs:

  $ ls -l /bin/sh
  lrwxrwxrwx 1 root root 4 Sep 26 12:04 /bin/sh -> bash
  $ sh ./e\ space.sh "output from bash as sh"
  $ bash --posix ./e\ space.sh "output from bash --posix"
  $ ls -1 output\ from\ bash*
  output from bash as sh
  output from bash --posix

That's why we didn't noticed it yet, not even on macOS, which uses
Bash as /bin/sh.  You have to build Git with 'SHELL_PATH=/bin/bash' to
make t7005 fail because of this issue, and based on the trace that
Alexander showed us it seems that OpenIndiana folks do build Git that
way.

Good.  *throws the POSIX-lawyer hat into the farthest corner*

Having said all that, I didn't omit the quotes in 4362da078e with the
above in mind; in fact I tend to use quotes even when they are
unnecessary (e.g. in variable assignments: var="$1"), because unquoted
variables and command substitutions freak me out before I can think
through whether its safe to omit the quotes or not :)


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



[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06
[2] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07


> > execve("/bin/bash", 0x007EA898, 0x007EA960)  argc = 5
> > 2655:    argv: /bin/bash -c ./e\ space.sh "$@" ./e\ space.sh
> > 2655:     /export/home/alp/srcs/oi-userland/components/developer/git/build/amd64/t/trash
> > directory.t7005-editor/.git/COMMIT_EDITMSG
> > 2655:   execve("./e space.sh", 0x005655C8, 0x00564008)  Err#8 ENOEXEC
> > ./e space.sh: line 1: $1: ambiguous redirect
> 
> > Shell is bash, as you can see (GNU bash, version 4.4.23(1)-release
> > (i386-pc-solaris2.11))
> 
> I came up with the following commit message. What do you think about it?
> 
>     t7005-editor: quote filename to fix whitespace-issue
> 
>     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, the commit introduced a regression around
>     how spaces are handled, not in the /name/ of the script, but /in/ the
>     script itself. The editor-script created looks like this:
> 
>       echo space >$1
> 
>     We will try to execute something like
> 
>       echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG
> 
>     Most shells seem to be able to figure out that the filename doesn't end
>     with "trash" but continues all the way to "COMMIT_EDITMSG", but at least
>     one shell chokes on this.
> 
>     Make sure that the editor-script quotes "$1".
> 
> Martin

[PATCH] t7005-editor: quote filename to fix whitespace-issue

[Martin Ågren]

From: Alexander Pyhalov <apyhalov@gmail.com>

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 <apyhalov@gmail.com>
Commit-message-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 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(-)

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)"
-- 
2.19.0.216.g2d3b1c576c

Re: [PATCH] t7005-editor: quote filename to fix whitespace-issue

[Taylor Blau]

On Wed, Sep 26, 2018 at 06:14:11PM +0200, Martin Ågren wrote:
> From: Alexander Pyhalov <apyhalov@gmail.com>
>
> 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 <apyhalov@gmail.com>
> Commit-message-by: Martin Ågren <martin.agren@gmail.com>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>

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 <me@ttaylorr.com>

Thanks,
Taylor

Re: [PATCH] t7005-editor: quote filename to fix whitespace-issue

[Jeff King]

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

Re: t7005-editor.sh failure

[Junio C Hamano]

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

> Having said all that, I didn't omit the quotes in 4362da078e with the
> above in mind; in fact I tend to use quotes even when they are
> unnecessary (e.g. in variable assignments: var="$1"), because unquoted
> variables and command substitutions freak me out before I can think
> through whether its safe to omit the quotes or not :)

I quote >"$file" (but not var=$var) because the CodingGuidelines
tells me to:

 - Redirection operators should be written with space before, but no
   space after them.  In other words, write 'echo test >"$file"'
   instead of 'echo test> $file' or 'echo test > $file'.  Note that
   even though it is not required by POSIX to double-quote the
   redirection target in a variable (as shown above), our code does so
   because some versions of bash issue a warning without the quotes.

;-)

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

Good suggestion.

Re: t7005-editor.sh failure

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> Having said all that, I didn't omit the quotes in 4362da078e with the
>> above in mind; in fact I tend to use quotes even when they are
>> unnecessary (e.g. in variable assignments: var="$1"), because unquoted
>> variables and command substitutions freak me out before I can think
>> through whether its safe to omit the quotes or not :)
>
> I quote >"$file" (but not var=$var) because the CodingGuidelines
> tells me to:
>
>  - Redirection operators should be written with space before, but no
>    space after them.  In other words, write 'echo test >"$file"'
>    instead of 'echo test> $file' or 'echo test > $file'.  Note that
>    even though it is not required by POSIX to double-quote the
>    redirection target in a variable (as shown above), our code does so
>    because some versions of bash issue a warning without the quotes.
>
> ;-)
>
>> Sidenote: this test should use the write_script helper to create this
>> editor script.
>
> Good suggestion.

Perhaps like this.

-- >8 --
Subject: t7005: make sure it passes under /bin/bash

In POSIX.1 compliant shells, you should be able to use a variable
reference without quoting for the target of the redirection, e.g.

	echo foo >$file
	echo bar >$1

without fear of substitution of $file getting split at $IFS.
However, some versions of bash throws a warning, especially when
they are invoked as /bin/bash (not as /bin/sh).  Those who build
with SHELL_PATH=/bin/bash and run t/t7005-editor.sh triggers an
unnecessary failure due to this issue.

Fix it by making sure that the generated "editor" script quotes the
target of redirection.  

While at it, update the way to creatd these scripts to use the
write_script wrapper, so that we do not have to worry about writing
the she-bang line and making the result executable.

Reported-by: Alexander Pyhalov <apyhalov@gmail.com>
Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7005-editor.sh | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index b2ca77b338..b0c4cc4ca0 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -20,11 +20,9 @@ fi
 
 for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
 do
-	cat >e-$i.sh <<-EOF
-	#!$SHELL_PATH
+	write_script "e-$i.sh" <<-EOF
 	echo "Edited by $i" >"\$1"
 	EOF
-	chmod +x e-$i.sh
 done
 
 if ! test -z "$vi"
@@ -112,8 +110,9 @@ do
 done
 
 test_expect_success 'editor with a space' '
-	echo "echo space >\$1" >"e space.sh" &&
-	chmod a+x "e space.sh" &&
+	write_script "e space.sh" <<-\EOF &&
+	echo space >"$1"
+	EOF
 	GIT_EDITOR="./e\ space.sh" git commit --amend &&
 	test space = "$(git show -s --pretty=format:%s)"
 

Re: t7005-editor.sh failure

[Andrei Rybak]

On 2018-09-26 21:16, Junio C Hamano wrote:
> While at it, update the way to creatd these scripts to use the

s/creatd/create/

Or rewording as "... update the way these scripts are created ..."

> write_script wrapper, so that we do not have to worry about writing
> the she-bang line and making the result executable.

Re: t7005-editor.sh failure

[SZEDER Gábor]

On Wed, Sep 26, 2018 at 12:16:16PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:

> > I quote >"$file" (but not var=$var) because the CodingGuidelines
> > tells me to:
> >
> >  - Redirection operators should be written with space before, but no
> >    space after them.  In other words, write 'echo test >"$file"'
> >    instead of 'echo test> $file' or 'echo test > $file'.  Note that
> >    even though it is not required by POSIX to double-quote the
> >    redirection target in a variable (as shown above), our code does so
> >    because some versions of bash issue a warning without the quotes.
> >
> > ;-)

Oh, indeed, I didn't notice that.

> Subject: t7005: make sure it passes under /bin/bash
> 
> In POSIX.1 compliant shells, you should be able to use a variable
> reference without quoting for the target of the redirection, e.g.
> 
> 	echo foo >$file
> 	echo bar >$1
> 
> without fear of substitution of $file getting split at $IFS.
> However, some versions of bash throws a warning, especially when

I would say it's an error, not a warning.

Regarding the "some versions", it's unclear when Bash started to apply
word splitting to the filename in redirection.  The changelog of
version 2.04-beta2 contains the following entry:

  When running in POSIX.2 mode, bash no longer performs word
  splitting on the expanded value of the word supplied as the filename
  argument to redirection operators.

so it must have started earlier, but I found further no sign of it in
the changelog.  In its man page the first ever mention of word
splitting affecting redirections came only later, in version 2.04.

v2.04 was release in 2000, so it's bordering on "since forever".


> they are invoked as /bin/bash (not as /bin/sh).  Those who build
> with SHELL_PATH=/bin/bash and run t/t7005-editor.sh triggers an

The grammar here confused me a bit...  maybe s/triggers/trigger/ ?

> unnecessary failure due to this issue.
> 
> Fix it by making sure that the generated "editor" script quotes the
> target of redirection.  
> 
> While at it, update the way to creatd these scripts to use the
> write_script wrapper, so that we do not have to worry about writing
> the she-bang line and making the result executable.
> 
> Reported-by: Alexander Pyhalov <apyhalov@gmail.com>
> Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t7005-editor.sh | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> index b2ca77b338..b0c4cc4ca0 100755
> --- a/t/t7005-editor.sh
> +++ b/t/t7005-editor.sh
> @@ -20,11 +20,9 @@ fi
>  
>  for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
>  do
> -	cat >e-$i.sh <<-EOF
> -	#!$SHELL_PATH
> +	write_script "e-$i.sh" <<-EOF

This can't be <<-\EOF ...

>  	echo "Edited by $i" >"\$1"

... because here we have to expand $i, and, therefore, we need both
double-quotes and \-escaping for $1.  Ok.

>  	EOF
> -	chmod +x e-$i.sh
>  done
>  
>  if ! test -z "$vi"
> @@ -112,8 +110,9 @@ do
>  done
>  
>  test_expect_success 'editor with a space' '
> -	echo "echo space >\$1" >"e space.sh" &&
> -	chmod a+x "e space.sh" &&
> +	write_script "e space.sh" <<-\EOF &&

But here it can be <<-\EOF ...

> +	echo space >"$1"

... so here we don't need the \-escaping.  Good.

> +	EOF
>  	GIT_EDITOR="./e\ space.sh" git commit --amend &&
>  	test space = "$(git show -s --pretty=format:%s)"
>