qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [PATCH v3] tests: Avoid non-portable 'echo -ARG'


From: Max Reitz
Subject: Re: [Qemu-trivial] [PATCH v3] tests: Avoid non-portable 'echo -ARG'
Date: Mon, 3 Jul 2017 18:16:34 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0

On 2017-07-03 16:32, Eric Blake wrote:
> On 07/03/2017 07:57 AM, Max Reitz wrote:
>> On 2017-07-03 14:31, Eric Blake wrote:
>>> POSIX says that backslashes in the arguments to 'echo', as well as
>>> any use of 'echo -n' and 'echo -e', are non-portable; it recommends
>>> people should favor 'printf' instead.  This is definitely true where
>>> we do not control which shell is running (such as in makefile snippets
>>> or in documentation examples).  But even for scripts where we
>>> require bash (and therefore, where echo does what we want by default),
>>> it is still possible to use 'shopt -s xpg_echo' to change bash's
>>> behavior of echo.  And setting a good example never hurts when we are
>>> not sure if a snippet will be copied from a bash-only script to a
>>> general shell script (although I don't change the use of non-portable
>>> \e for ESC when we know the running shell is bash).
>>>
> 
>>>
>>>          if [ -f $seq.notrun ]
>>>          then
>>> -            $timestamp || echo -n " [not run] "
>>> -            $timestamp && echo " [not run]" && echo -n "        $seq -- "
>>> +            $timestamp || printf " [not run] "
>>> +            $timestamp && echo " [not run]" && printf "        $seq -- "
>>
>> Everywhere else, $seq got its own %s, but not here. That's at least
>> inconsistent.
> 
> True, and it's because $ is a bit easy to miss when I'm trying to
> special case which $ are important.
> 
>>
>> I don't quite know why you're not simply using %s and %b everywhere
>> there is a non-constant format string. Yes, if it is an exit code, it is
>> pretty clear that it won't contain a %. If it's a return value from
>> "date +%T", it is kind of, too (but then you have to know what that does
>> in order to review the change -- and I myself had to consult the man
>> page, to be honest). If they are variables named "img_offset" and
>> "img_size", then you'd hope they are numbers, but in order to check you
>> still have to look at the rest of the code (especially since they are
>> embedded as strings into the JSON object).
>> If you simply used %s and %b everywhere there is a $, reviewing would be
>> absolutely trivial. Yes, it seems a bit unnecessary, but it does make
>> reviewing much easier and I think since this is also about setting a
>> good example, that would be a good example, IMHO.
>> (Sure, reviewing this already is easy enough, but there still is a
>> difference between "very easy" and "trivial".)
> 
> It's not every day a "trivial" patch gets to v4, but your arguments are
> persuasive enough that I'll respin.
> 
>>
>> Although at this point I have to admit that reviewing a really trivial
>> v4 would take me more time than to just write the following:
>>
>> With a %s added to the printf above:
>>
>> Reviewed-by: Max Reitz <address@hidden>
>>
>>>              cat $seq.notrun
>>>              notrun="$notrun $seq"
>>>          else
>>>              if [ $sts -ne 0 ]
>>>              then
>>> -                echo -n " [failed, exit status $sts]"
>>> +                printf " [failed, exit status $sts]"
> 
> So, before I respin, let me double-check:
> 
> Here's a case where the context makes it obvious that $sts is numeric
> (if not, the '[ $sts -ne 0 ]' would have failed); do you want the %s
> here or not?

Well, I'd put a %s there anyway, just because that would mean exactly
zero dependency on any context (even if the context is very small here).

Max

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]