* t7005-editor.sh failure @ 2018-09-26 6:52 Alexander Pyhalov 2018-09-26 7:59 ` Martin Ågren 0 siblings, 1 reply; 15+ messages in thread From: Alexander Pyhalov @ 2018-09-26 6:52 UTC (permalink / raw) To: git [-- 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)" ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: t7005-editor.sh failure 2018-09-26 6:52 t7005-editor.sh failure Alexander Pyhalov @ 2018-09-26 7:59 ` Martin Ågren 2018-09-26 9:00 ` Alexander Pyhalov 0 siblings, 1 reply; 15+ messages in thread From: Martin Ågren @ 2018-09-26 7:59 UTC (permalink / raw) To: apyhalov; +Cc: Git Mailing List, SZEDER Gábor 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: t7005-editor.sh failure 2018-09-26 7:59 ` Martin Ågren @ 2018-09-26 9:00 ` Alexander Pyhalov 2018-09-26 9:52 ` Martin Ågren 0 siblings, 1 reply; 15+ messages in thread From: Alexander Pyhalov @ 2018-09-26 9:00 UTC (permalink / raw) To: Martin Ågren; +Cc: Git Mailing List, SZEDER Gábor [-- 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: t7005-editor.sh failure 2018-09-26 9:00 ` Alexander Pyhalov @ 2018-09-26 9:52 ` Martin Ågren 2018-09-26 10:02 ` Alexander Pyhalov ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Martin Ågren @ 2018-09-26 9:52 UTC (permalink / raw) To: Alexander Pyhalov; +Cc: Git Mailing List, SZEDER Gábor 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: t7005-editor.sh failure 2018-09-26 9:52 ` Martin Ågren @ 2018-09-26 10:02 ` Alexander Pyhalov 2018-09-26 11:59 ` Eric Sunshine 2018-09-26 12:11 ` SZEDER Gábor 2 siblings, 0 replies; 15+ messages in thread From: Alexander Pyhalov @ 2018-09-26 10:02 UTC (permalink / raw) To: Martin Ågren; +Cc: Git Mailing List, SZEDER Gábor 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. -- С уважением, Александр Пыхалов ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: t7005-editor.sh failure 2018-09-26 9:52 ` Martin Ågren 2018-09-26 10:02 ` Alexander Pyhalov @ 2018-09-26 11:59 ` Eric Sunshine 2018-09-26 13:23 ` Martin Ågren 2018-09-26 12:11 ` SZEDER Gábor 2 siblings, 1 reply; 15+ messages in thread From: Eric Sunshine @ 2018-09-26 11:59 UTC (permalink / raw) To: Martin Ågren; +Cc: apyhalov, Git List, SZEDER Gábor 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). ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: t7005-editor.sh failure 2018-09-26 11:59 ` Eric Sunshine @ 2018-09-26 13:23 ` Martin Ågren 0 siblings, 0 replies; 15+ messages in thread From: Martin Ågren @ 2018-09-26 13:23 UTC (permalink / raw) To: Eric Sunshine; +Cc: Alexander Pyhalov, Git Mailing List, SZEDER Gábor 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: t7005-editor.sh failure 2018-09-26 9:52 ` Martin Ågren 2018-09-26 10:02 ` Alexander Pyhalov 2018-09-26 11:59 ` Eric Sunshine @ 2018-09-26 12:11 ` SZEDER Gábor 2018-09-26 16:14 ` [PATCH] t7005-editor: quote filename to fix whitespace-issue Martin Ågren 2018-09-26 18:16 ` t7005-editor.sh failure Junio C Hamano 2 siblings, 2 replies; 15+ messages in thread From: SZEDER Gábor @ 2018-09-26 12:11 UTC (permalink / raw) To: Martin Ågren; +Cc: Alexander Pyhalov, Git Mailing List 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] t7005-editor: quote filename to fix whitespace-issue 2018-09-26 12:11 ` SZEDER Gábor @ 2018-09-26 16:14 ` Martin Ågren 2018-09-26 18:14 ` Taylor Blau 2018-09-26 19:21 ` Jeff King 2018-09-26 18:16 ` t7005-editor.sh failure Junio C Hamano 1 sibling, 2 replies; 15+ messages in thread From: Martin Ågren @ 2018-09-26 16:14 UTC (permalink / raw) To: git; +Cc: Alexander Pyhalov, SZEDER Gábor, Eric Sunshine 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] t7005-editor: quote filename to fix whitespace-issue 2018-09-26 16:14 ` [PATCH] t7005-editor: quote filename to fix whitespace-issue Martin Ågren @ 2018-09-26 18:14 ` Taylor Blau 2018-09-26 19:21 ` Jeff King 1 sibling, 0 replies; 15+ messages in thread From: Taylor Blau @ 2018-09-26 18:14 UTC (permalink / raw) To: Martin Ågren Cc: git, Alexander Pyhalov, SZEDER Gábor, Eric Sunshine 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] t7005-editor: quote filename to fix whitespace-issue 2018-09-26 16:14 ` [PATCH] t7005-editor: quote filename to fix whitespace-issue Martin Ågren 2018-09-26 18:14 ` Taylor Blau @ 2018-09-26 19:21 ` Jeff King 1 sibling, 0 replies; 15+ messages in thread From: Jeff King @ 2018-09-26 19:21 UTC (permalink / raw) To: Martin Ågren Cc: git, Alexander Pyhalov, SZEDER Gábor, Eric Sunshine 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: t7005-editor.sh failure 2018-09-26 12:11 ` SZEDER Gábor 2018-09-26 16:14 ` [PATCH] t7005-editor: quote filename to fix whitespace-issue Martin Ågren @ 2018-09-26 18:16 ` Junio C Hamano 2018-09-26 19:16 ` Junio C Hamano 1 sibling, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2018-09-26 18:16 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Martin Ågren, Alexander Pyhalov, Git Mailing List 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: t7005-editor.sh failure 2018-09-26 18:16 ` t7005-editor.sh failure Junio C Hamano @ 2018-09-26 19:16 ` Junio C Hamano 2018-09-26 19:29 ` Andrei Rybak 2018-09-27 20:53 ` SZEDER Gábor 0 siblings, 2 replies; 15+ messages in thread From: Junio C Hamano @ 2018-09-26 19:16 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Martin Ågren, Alexander Pyhalov, Git Mailing List 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)" ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: t7005-editor.sh failure 2018-09-26 19:16 ` Junio C Hamano @ 2018-09-26 19:29 ` Andrei Rybak 2018-09-27 20:53 ` SZEDER Gábor 1 sibling, 0 replies; 15+ messages in thread From: Andrei Rybak @ 2018-09-26 19:29 UTC (permalink / raw) To: Junio C Hamano, SZEDER Gábor Cc: Martin Ågren, Alexander Pyhalov, Git Mailing List 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: t7005-editor.sh failure 2018-09-26 19:16 ` Junio C Hamano 2018-09-26 19:29 ` Andrei Rybak @ 2018-09-27 20:53 ` SZEDER Gábor 1 sibling, 0 replies; 15+ messages in thread From: SZEDER Gábor @ 2018-09-27 20:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Martin Ågren, Alexander Pyhalov, Git Mailing List 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)" > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-09-27 20:53 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-26 6:52 t7005-editor.sh failure Alexander Pyhalov 2018-09-26 7:59 ` Martin Ågren 2018-09-26 9:00 ` Alexander Pyhalov 2018-09-26 9:52 ` Martin Ågren 2018-09-26 10:02 ` Alexander Pyhalov 2018-09-26 11:59 ` Eric Sunshine 2018-09-26 13:23 ` Martin Ågren 2018-09-26 12:11 ` SZEDER Gábor 2018-09-26 16:14 ` [PATCH] t7005-editor: quote filename to fix whitespace-issue Martin Ågren 2018-09-26 18:14 ` Taylor Blau 2018-09-26 19:21 ` Jeff King 2018-09-26 18:16 ` t7005-editor.sh failure Junio C Hamano 2018-09-26 19:16 ` Junio C Hamano 2018-09-26 19:29 ` Andrei Rybak 2018-09-27 20:53 ` SZEDER Gábor
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).