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 <[email protected]> > >> 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? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
