threads / patch / 13895

patchImprove sed portability

Subject: [PATCH] Improve sed portability

## tl;dr

9 messages between Jun 11, 2008 and Jul 13, 2008. Diffs are folded; open one to read it.

replies: 8people: 5as markdown or json

Chris Ridd· Jun 11, 2008, 13:09 UTC · lore

On Solaris /usr/bin/sed apparently fails to process input that doesn't end in a \n. Consequently constructs like

  re=$(printf '%s' foo | sed -e 's/bar/BAR/g' $)

cause re to be set to the empty string. Such a construct is used in git-submodule.sh.

Changing the printf to add a \n seems the safest change. The POSIX-compliant seds shipped with Solaris do not have this problem.

Signed-off-by: Chris Ridd <chris.ridd@isode.com>
---
 git-submodule.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to git-submodule.sh +1 −1
diff --git a/git-submodule.sh b/git-submodule.sh
index 1007372..e515bcc 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -73,7 +73,7 @@ resolve_relative_url ()
 module_name()
 {
 	# Do we have "submodule.<something>.path = $1" defined in .gitmodules file?
-	re=$(printf '%s' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
+	re=$(printf "%s\n" "$1" | sed -e 's/[].[^$\\*]/\\&/g')
 	name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
 		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
        test -z "$name" &&

-- 
1.5.3.6
Johannes Sixt· Jun 11, 2008, 14:04 UTC · re: Chris Ridd · lore

Re: [PATCH] Improve sed portability

Chris Ridd schrieb:
Show 6 quoted lines
> On Solaris /usr/bin/sed apparently fails to process input that doesn't
> end in a \n. Consequently constructs like
> 
>   re=$(printf '%s' foo | sed -e 's/bar/BAR/g' $)
> 
> cause re to be set to the empty string.
So does /usr/bin/sed of AIX 4.3!
Show 6 quoted lines
> @@ -73,7 +73,7 @@ resolve_relative_url ()
>  module_name()
>  {
>  	# Do we have "submodule.<something>.path = $1" defined in .gitmodules file?
> -	re=$(printf '%s' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
> +	re=$(printf "%s\n" "$1" | sed -e 's/[].[^$\\*]/\\&/g')

You change sq into dq. Is this not dangerous? Shouldn't backslash-en be hidden from the shell so that printf can interpret it?

>  	name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
>  		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )

I trust you have tested this. But I wonder whether this leaves a stray newline in $re that gets in the way inside the sed expression...

>         test -z "$name" &&
-- Hannes
Chris Ridd· Jun 11, 2008, 15:29 UTC · re: Johannes Sixt · lore

Re: [PATCH] Improve sed portability

Johannes Sixt wrote:
Show 9 quoted lines
> Chris Ridd schrieb:
>> On Solaris /usr/bin/sed apparently fails to process input that doesn't
>> end in a \n. Consequently constructs like
>>
>>   re=$(printf '%s' foo | sed -e 's/bar/BAR/g' $)
>>
>> cause re to be set to the empty string.
> 
> So does /usr/bin/sed of AIX 4.3!

I ought to have mentioned this occurs on Solaris 8, 10, build 90 of OpenSolaris, and on HP-UX 11iv1. I stared at that regex for quite a while before realising the problem was with the input :-)

Show 9 quoted lines
>> @@ -73,7 +73,7 @@ resolve_relative_url ()
>>  module_name()
>>  {
>>  	# Do we have "submodule.<something>.path = $1" defined in .gitmodules file?
>> -	re=$(printf '%s' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
>> +	re=$(printf "%s\n" "$1" | sed -e 's/[].[^$\\*]/\\&/g')
> 
> You change sq into dq. Is this not dangerous? Shouldn't backslash-en be
> hidden from the shell so that printf can interpret it?
It is necessary to use double quotes. This:
     printf '%s\n' foobar
prints a literal \, a literal n, and no newline:
     foobar\n
Not desirable :-(
Of course, using a plain old:
     echo "$1"
should work well too. Why is printf being used here and not echo, anyway?
Show 5 quoted lines
>>  	name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
>>  		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
> 
> I trust you have tested this. But I wonder whether this leaves a stray
> newline in $re that gets in the way inside the sed expression...

Yes, I've tested this as we use submodules heavily. I think the $( .. ) notation will remove the trailing \n printed by sed, but to be sure I inserted a 'set -x' at the top of the module_name() function and double-checked that the re variable didn't get any stray \n character(s). Bash versions 2 and 3 were used.

So without the change, on Solaris I get:
     No submodule mapping found in .gitmodules for path 'foobar'
for the first submodule that we use, and the repository clone fails.
With the change, all our repositories clone OK.
Cheers,
Chris
Jeff King· Jun 11, 2008, 16:39 UTC · re: Chris Ridd · lore

Re: [PATCH] Improve sed portability

On Wed, Jun 11, 2008 at 04:29:53PM +0100, Chris Ridd wrote:
Show 9 quoted lines
> It is necessary to use double quotes. This:
>
>     printf '%s\n' foobar
>
> prints a literal \, a literal n, and no newline:
>
>     foobar\n
>
> Not desirable :-(
On what platform?
Show 5 quoted lines
> Of course, using a plain old:
>
>     echo "$1"
>
> should work well too. Why is printf being used here and not echo, anyway?

Because the original didn't have a newline, and "echo -n" isn't portable?

-Peff
Johannes Sixt· Jun 12, 2008, 07:46 UTC · re: Chris Ridd · lore

Re: [PATCH] Improve sed portability

Chris Ridd schrieb:
Show 22 quoted lines
> Johannes Sixt wrote:
>> Chris Ridd schrieb:
>>> @@ -73,7 +73,7 @@ resolve_relative_url ()
>>>  module_name()
>>>  {
>>>      # Do we have "submodule.<something>.path = $1" defined in
>>> .gitmodules file?
>>> -    re=$(printf '%s' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
>>> +    re=$(printf "%s\n" "$1" | sed -e 's/[].[^$\\*]/\\&/g')
>>
>> You change sq into dq. Is this not dangerous? Shouldn't backslash-en be
>> hidden from the shell so that printf can interpret it?
> 
> It is necessary to use double quotes. This:
> 
>     printf '%s\n' foobar
> 
> prints a literal \, a literal n, and no newline:
> 
>     foobar\n
> 
> Not desirable :-(
On both Linux and AIX 4.3 I see:

$ printf 'x\ny'; echo z x yz

The printf turns the \n into LF.

I mentioned this in the first place because I don't know what various shells do with \n when they see "%s\n". But one way or the other, the \n will be turned into LF, either by the shell or by printf. So it's not a big deal.

Show 5 quoted lines
> Of course, using a plain old:
> 
>     echo "$1"
> 
> should work well too. Why is printf being used here and not echo, anyway?

Because the "$1" could contain character sequences that some 'echo' implementations mangle.

-- Hannes
Chris Ridd· Jun 12, 2008, 08:29 UTC · re: Johannes Sixt · lore

Re: [PATCH] Improve sed portability

Johannes Sixt wrote:
Show 29 quoted lines
> Chris Ridd schrieb:
>> Johannes Sixt wrote:
>>> Chris Ridd schrieb:
>>>> @@ -73,7 +73,7 @@ resolve_relative_url ()
>>>>  module_name()
>>>>  {
>>>>      # Do we have "submodule.<something>.path = $1" defined in
>>>> .gitmodules file?
>>>> -    re=$(printf '%s' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
>>>> +    re=$(printf "%s\n" "$1" | sed -e 's/[].[^$\\*]/\\&/g')
>>> You change sq into dq. Is this not dangerous? Shouldn't backslash-en be
>>> hidden from the shell so that printf can interpret it?
>> It is necessary to use double quotes. This:
>>
>>     printf '%s\n' foobar
>>
>> prints a literal \, a literal n, and no newline:
>>
>>     foobar\n
>>
>> Not desirable :-(
> 
> On both Linux and AIX 4.3 I see:
> 
> $  printf 'x\ny'; echo z
> x
> yz
> 
> The printf turns the \n into LF.

Yes, and I don't know *what* I did yesterday, but Solaris 8, 10, (every OS I mentioned before) behave the same as your test.

I did actually have my eyes tested later on yesterday :-)
> I mentioned this in the first place because I don't know what various
> shells do with \n when they see "%s\n". But one way or the other, the \n
> will be turned into LF, either by the shell or by printf. So it's not a
> big deal.
I agree.
Show 8 quoted lines
>> Of course, using a plain old:
>>
>>     echo "$1"
>>
>> should work well too. Why is printf being used here and not echo, anyway?
> 
> Because the "$1" could contain character sequences that some 'echo'
> implementations mangle.
Indeed. If $1 started with -n that might cause problems on some platforms.
Should I revise my commit to use single quotes again?
Cheers,
Chris
Jeff King· Jun 12, 2008, 09:07 UTC · re: Chris Ridd · lore

Re: [PATCH] Improve sed portability

On Thu, Jun 12, 2008 at 09:29:27AM +0100, Chris Ridd wrote:
>> Because the "$1" could contain character sequences that some 'echo'
>> implementations mangle.
>
> Indeed. If $1 started with -n that might cause problems on some platforms.

It's much worse than that. Any backslash sequence can be interpolated. 4b7cc26 (git-am: use printf instead of echo on user-supplied strings).

-Peff
Jakub Narebski· Jul 13, 2008, 20:00 UTC · re: Chris Ridd · lore

Re: [PATCH] Improve sed portability

Chris Ridd <chris.ridd@isode.com> writes:
Show 6 quoted lines
> Of course, using a plain old:
> 
>      echo "$1"
> 
> should work well too. Why is printf being used here and not echo,
> anyway?

Uh, because 'echo -n' is not portable enough, and for some reason it was though that there shouldn't be final newline?

-- 
Jakub Narebski
Poland
ShadeHawk on #git
Junio C Hamano· Jun 12, 2008, 08:33 UTC · re: Johannes Sixt · lore

Re: [PATCH] Improve sed portability

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 19 quoted lines
> Chris Ridd schrieb:
>> On Solaris /usr/bin/sed apparently fails to process input that doesn't
>> end in a \n. Consequently constructs like
>> 
>>   re=$(printf '%s' foo | sed -e 's/bar/BAR/g' $)
>> 
>> cause re to be set to the empty string.
>
> So does /usr/bin/sed of AIX 4.3!
>
>> @@ -73,7 +73,7 @@ resolve_relative_url ()
>>  module_name()
>>  {
>>  	# Do we have "submodule.<something>.path = $1" defined in .gitmodules file?
>> -	re=$(printf '%s' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
>> +	re=$(printf "%s\n" "$1" | sed -e 's/[].[^$\\*]/\\&/g')
>
> You change sq into dq. Is this not dangerous? Shouldn't backslash-en be
> hidden from the shell so that printf can interpret it?

"\n" inside dq is _not_ interpreted by the shell (printf interprets it), but I tend to agree that using sq is worry-free and better.

Show 5 quoted lines
>>  	name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
>>  		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
>
> I trust you have tested this. But I wonder whether this leaves a stray
> newline in $re that gets in the way inside the sed expression...

I suspect the very original was written (or copied from something that wrote) like this:

	re=$(echo -n "$1" | sed -e '...')
and mechanically replaced to
	re=$(printf '%s' "$1" | sed -e '...')
because "echo" is not quite portable.

But the original misunderstands the command substitution. The trailing LF is removed by it, so as long as "$1" is a single line, $re will get a line without the trailing LF _anyway_.

← back to recent threads