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 <[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?
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
signature.asc
Description: OpenPGP digital signature
