{"thread":{"id":"13895","subject":"[PATCH] Improve sed portability","startedAt":"2008-06-11T13:09:19Z","lastAt":"2008-07-13T20:00:34Z","messageCount":9,"participants":["Chris Ridd","Johannes Sixt","Jeff King","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"79449","messageId":"1213189759-11565-1-git-send-email-chris.ridd@isode.com","threadId":"13895","inReplyTo":null,"subject":"[PATCH] Improve sed portability","fromName":"Chris Ridd","fromEmail":"chris.ridd@isode.com","sentAt":"2008-06-11T13:09:19Z","receivedAt":"2008-06-11T13:09:19Z","isPatch":true,"sender":{"key":"chris.ridd@isode.com","avatar":null},"body":"On Solaris /usr/bin/sed apparently fails to process input that doesn't\nend in a \\n. Consequently constructs like\n\n re=$(printf '%s' foo | sed -e 's/bar/BAR/g' $)\n\ncause re to be set to the empty string. Such a construct is used in\ngit-submodule.sh.\n\nChanging the printf to add a \\n seems the safest change. The\nPOSIX-compliant seds shipped with Solaris do not have this problem.\n\nSigned-off-by: Chris Ridd \n---\n git-submodule.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 1007372..e515bcc 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -73,7 +73,7 @@ resolve_relative_url ()\n module_name()\n {\n \t# Do we have \"submodule..path = $1\" defined in .gitmodules file?\n-\tre=$(printf '%s' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n+\tre=$(printf \"%s\\n\" \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n \tname=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n \t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n test -z \"$name\" &&\n-- \n1.5.3.6\n"},{"id":"79452","messageId":"484FDB5D.7060606@viscovery.net","threadId":"13895","inReplyTo":"1213189759-11565-1-git-send-email-chris.ridd@isode.com","subject":"Re: [PATCH] Improve sed portability","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-06-11T14:04:13Z","receivedAt":"2008-06-11T14:04:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Chris Ridd schrieb:\n> On Solaris /usr/bin/sed apparently fails to process input that doesn't\n> end in a \\n. Consequently constructs like\n> \n> re=$(printf '%s' foo | sed -e 's/bar/BAR/g' $)\n> \n> cause re to be set to the empty string.\n\nSo does /usr/bin/sed of AIX 4.3!\n\n> @@ -73,7 +73,7 @@ resolve_relative_url ()\n> module_name()\n> {\n> \t# Do we have \"submodule..path = $1\" defined in .gitmodules file?\n> -\tre=$(printf '%s' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n> +\tre=$(printf \"%s\\n\" \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n\nYou change sq into dq. Is this not dangerous? Shouldn't backslash-en be\nhidden from the shell so that printf can interpret it?\n\n> \tname=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n> \t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n\nI trust you have tested this. But I wonder whether this leaves a stray\nnewline in $re that gets in the way inside the sed expression...\n\n> test -z \"$name\" &&\n\n\n-- Hannes\n"},{"id":"79460","messageId":"484FEF71.2030909@isode.com","threadId":"13895","inReplyTo":"484FDB5D.7060606@viscovery.net","subject":"Re: [PATCH] Improve sed portability","fromName":"Chris Ridd","fromEmail":"chris.ridd@isode.com","sentAt":"2008-06-11T15:29:53Z","receivedAt":"2008-06-11T15:29:53Z","isPatch":true,"sender":{"key":"chris.ridd@isode.com","avatar":null},"body":"Johannes Sixt wrote:\n> Chris Ridd schrieb:\n>> On Solaris /usr/bin/sed apparently fails to process input that doesn't\n>> end in a \\n. Consequently constructs like\n>>\n>> re=$(printf '%s' foo | sed -e 's/bar/BAR/g' $)\n>>\n>> cause re to be set to the empty string.\n> \n> So does /usr/bin/sed of AIX 4.3!\n\nI ought to have mentioned this occurs on Solaris 8, 10, build 90 of \nOpenSolaris, and on HP-UX 11iv1. I stared at that regex for quite a \nwhile before realising the problem was with the input :-)\n\n>> @@ -73,7 +73,7 @@ resolve_relative_url ()\n>> module_name()\n>> {\n>> \t# Do we have \"submodule..path = $1\" defined in .gitmodules file?\n>> -\tre=$(printf '%s' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n>> +\tre=$(printf \"%s\\n\" \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n> \n> You change sq into dq. Is this not dangerous? Shouldn't backslash-en be\n> hidden from the shell so that printf can interpret it?\n\nIt is necessary to use double quotes. This:\n\n printf '%s\\n' foobar\n\nprints a literal \\, a literal n, and no newline:\n\n foobar\\n\n\nNot desirable :-(\n\nOf course, using a plain old:\n\n echo \"$1\"\n\nshould work well too. Why is printf being used here and not echo, anyway?\n\n>> \tname=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n>> \t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n> \n> I trust you have tested this. But I wonder whether this leaves a stray\n> newline in $re that gets in the way inside the sed expression...\n\nYes, I've tested this as we use submodules heavily. I think the $( .. ) \nnotation will remove the trailing \\n printed by sed, but to be sure I \ninserted a 'set -x' at the top of the module_name() function and \ndouble-checked that the re variable didn't get any stray \\n \ncharacter(s). Bash versions 2 and 3 were used.\n\nSo without the change, on Solaris I get:\n\n No submodule mapping found in .gitmodules for path 'foobar'\n\nfor the first submodule that we use, and the repository clone fails.\n\nWith the change, all our repositories clone OK.\n\nCheers,\n\nChris\n"},{"id":"79462","messageId":"20080611163904.GB19172@sigill.intra.peff.net","threadId":"13895","inReplyTo":"484FEF71.2030909@isode.com","subject":"Re: [PATCH] Improve sed portability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-11T16:39:04Z","receivedAt":"2008-06-11T16:39:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 11, 2008 at 04:29:53PM +0100, Chris Ridd wrote:\n\n> It is necessary to use double quotes. This:\n>\n> printf '%s\\n' foobar\n>\n> prints a literal \\, a literal n, and no newline:\n>\n> foobar\\n\n>\n> Not desirable :-(\n\nOn what platform?\n\n> Of course, using a plain old:\n>\n> echo \"$1\"\n>\n> should work well too. Why is printf being used here and not echo, anyway?\n\nBecause the original didn't have a newline, and \"echo -n\" isn't\nportable?\n\n-Peff\n"},{"id":"79573","messageId":"4850D45E.8000802@viscovery.net","threadId":"13895","inReplyTo":"484FEF71.2030909@isode.com","subject":"Re: [PATCH] Improve sed portability","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-06-12T07:46:38Z","receivedAt":"2008-06-12T07:46:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Chris Ridd schrieb:\n> Johannes Sixt wrote:\n>> Chris Ridd schrieb:\n>>> @@ -73,7 +73,7 @@ resolve_relative_url ()\n>>> module_name()\n>>> {\n>>> # Do we have \"submodule..path = $1\" defined in\n>>> .gitmodules file?\n>>> - re=$(printf '%s' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n>>> + re=$(printf \"%s\\n\" \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n>>\n>> You change sq into dq. Is this not dangerous? Shouldn't backslash-en be\n>> hidden from the shell so that printf can interpret it?\n> \n> It is necessary to use double quotes. This:\n> \n> printf '%s\\n' foobar\n> \n> prints a literal \\, a literal n, and no newline:\n> \n> foobar\\n\n> \n> Not desirable :-(\n\nOn both Linux and AIX 4.3 I see:\n\n$ printf 'x\\ny'; echo z\nx\nyz\n\nThe printf turns the \\n into LF.\n\nI mentioned this in the first place because I don't know what various\nshells do with \\n when they see \"%s\\n\". But one way or the other, the \\n\nwill be turned into LF, either by the shell or by printf. So it's not a\nbig deal.\n\n> Of course, using a plain old:\n> \n> echo \"$1\"\n> \n> should work well too. Why is printf being used here and not echo, anyway?\n\nBecause the \"$1\" could contain character sequences that some 'echo'\nimplementations mangle.\n\n-- Hannes\n"},{"id":"79576","messageId":"4850DE67.703@isode.com","threadId":"13895","inReplyTo":"4850D45E.8000802@viscovery.net","subject":"Re: [PATCH] Improve sed portability","fromName":"Chris Ridd","fromEmail":"chris.ridd@isode.com","sentAt":"2008-06-12T08:29:27Z","receivedAt":"2008-06-12T08:29:27Z","isPatch":true,"sender":{"key":"chris.ridd@isode.com","avatar":null},"body":"Johannes Sixt wrote:\n> Chris Ridd schrieb:\n>> Johannes Sixt wrote:\n>>> Chris Ridd schrieb:\n>>>> @@ -73,7 +73,7 @@ resolve_relative_url ()\n>>>> module_name()\n>>>> {\n>>>> # Do we have \"submodule..path = $1\" defined in\n>>>> .gitmodules file?\n>>>> - re=$(printf '%s' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n>>>> + re=$(printf \"%s\\n\" \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n>>> You change sq into dq. Is this not dangerous? Shouldn't backslash-en be\n>>> hidden from the shell so that printf can interpret it?\n>> It is necessary to use double quotes. This:\n>>\n>> printf '%s\\n' foobar\n>>\n>> prints a literal \\, a literal n, and no newline:\n>>\n>> foobar\\n\n>>\n>> Not desirable :-(\n> \n> On both Linux and AIX 4.3 I see:\n> \n> $ printf 'x\\ny'; echo z\n> x\n> yz\n> \n> The printf turns the \\n into LF.\n\nYes, and I don't know *what* I did yesterday, but Solaris 8, 10, (every \nOS I mentioned before) behave the same as your test.\n\nI did actually have my eyes tested later on yesterday :-)\n\n> I mentioned this in the first place because I don't know what various\n> shells do with \\n when they see \"%s\\n\". But one way or the other, the \\n\n> will be turned into LF, either by the shell or by printf. So it's not a\n> big deal.\n\nI agree.\n\n>> Of course, using a plain old:\n>>\n>> echo \"$1\"\n>>\n>> should work well too. Why is printf being used here and not echo, anyway?\n> \n> Because the \"$1\" could contain character sequences that some 'echo'\n> implementations mangle.\n\nIndeed. If $1 started with -n that might cause problems on some platforms.\n\nShould I revise my commit to use single quotes again?\n\nCheers,\n\nChris\n"},{"id":"79577","messageId":"7vy75b833p.fsf@gitster.siamese.dyndns.org","threadId":"13895","inReplyTo":"484FDB5D.7060606@viscovery.net","subject":"Re: [PATCH] Improve sed portability","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-12T08:33:14Z","receivedAt":"2008-06-12T08:33:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Chris Ridd schrieb:\n>> On Solaris /usr/bin/sed apparently fails to process input that doesn't\n>> end in a \\n. Consequently constructs like\n>> \n>> re=$(printf '%s' foo | sed -e 's/bar/BAR/g' $)\n>> \n>> cause re to be set to the empty string.\n>\n> So does /usr/bin/sed of AIX 4.3!\n>\n>> @@ -73,7 +73,7 @@ resolve_relative_url ()\n>> module_name()\n>> {\n>> \t# Do we have \"submodule..path = $1\" defined in .gitmodules file?\n>> -\tre=$(printf '%s' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n>> +\tre=$(printf \"%s\\n\" \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n>\n> You change sq into dq. Is this not dangerous? Shouldn't backslash-en be\n> hidden from the shell so that printf can interpret it?\n\n\"\\n\" inside dq is _not_ interpreted by the shell (printf interprets it),\nbut I tend to agree that using sq is worry-free and better.\n\n>> \tname=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n>> \t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n>\n> I trust you have tested this. But I wonder whether this leaves a stray\n> newline in $re that gets in the way inside the sed expression...\n\nI suspect the very original was written (or copied from something that\nwrote) like this:\n\n\tre=$(echo -n \"$1\" | sed -e '...')\n\nand mechanically replaced to\n\n\tre=$(printf '%s' \"$1\" | sed -e '...')\n\nbecause \"echo\" is not quite portable.\n\nBut the original misunderstands the command substitution. The trailing LF\nis removed by it, so as long as \"$1\" is a single line, $re will get a line\nwithout the trailing LF _anyway_.\n"},{"id":"79579","messageId":"20080612090705.GA1055@sigill.intra.peff.net","threadId":"13895","inReplyTo":"4850DE67.703@isode.com","subject":"Re: [PATCH] Improve sed portability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-12T09:07:06Z","receivedAt":"2008-06-12T09:07:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 12, 2008 at 09:29:27AM +0100, Chris Ridd wrote:\n\n>> Because the \"$1\" could contain character sequences that some 'echo'\n>> implementations mangle.\n>\n> Indeed. If $1 started with -n that might cause problems on some platforms.\n\nIt's much worse than that. Any backslash sequence can be interpolated.\n4b7cc26 (git-am: use printf instead of echo on user-supplied strings).\n\n-Peff\n"},{"id":"83164","messageId":"m34p80rm5w.fsf@localhost.localdomain","threadId":"13895","inReplyTo":"484FEF71.2030909@isode.com","subject":"Re: [PATCH] Improve sed portability","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-07-13T20:00:34Z","receivedAt":"2008-07-13T20:00:34Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Chris Ridd writes:\n\n> Of course, using a plain old:\n> \n> echo \"$1\"\n> \n> should work well too. Why is printf being used here and not echo,\n> anyway?\n\nUh, because 'echo -n' is not portable enough, and for some reason it\nwas though that there shouldn't be final newline?\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"}]}