| Message ID | CALOYryFX4PPR+=1L+sjEqPsdmNh_+vNxGf0wwuAgoYzau=GShw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t7005-editor.sh failure | expand |
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
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
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
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.
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).
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
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
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.
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)"
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.
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)" >
--- 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)"