On Wed, Oct 09, 2013 at 02:03:24PM +0200, Paolo Giarrusso wrote: > On Wed, Oct 9, 2013 at 1:26 PM, Matthieu Moy > <Matthieu.Moy@xxxxxxxxxxxxxxx> wrote: > > Paolo Giarrusso <p.giarrusso@xxxxxxxxx> writes: > > > >> Otherwise, one could > >> change say to use printf, but that's more invasive. > > > > "invasive" in the sense that it impacts indirectly more callers, but are > > there really cases where "echo" is needed when calling "say"? Aren't > > there other potential bugs when arbitrary strings are passed to "say", > > that would be fixed by using printf once and for all? > > (1) Changing the implementation of say to use printf "%s\n" would be > trivial, and I think would address your concerns. Yeah, I think we should do that. I had the same thought as Matthieu when I read your initial patch; there are real portability bugs caused by using "echo" that would be fixed. However, that is somewhat orthogonal to the bug you are fixing. For handling this one site, I think it would be fine to just convert it to use printf, as your patch did. As you noted, the alternatives: > (2) add an explicit \n to all callers (invasive & error prone), or > (3) make `say` parse the `-n` option and conditionally add "\n" to the > format string or to a final argument, if -n is not specified; this > would affect no current caller, but complicate the implementation of > say. Doing that for just one call site has too much potential for > breakage, so I'm not sure I'd do it. (I'm not even sure on what should > `say` do when `-n` is not the first argument). ...are either annoying or complicated (and in particular, parsing "-n" means that callers need to be aware that 'say "$foo"' might accidentally trigger "-n" if $foo comes from the user). So the sanest interface is probably "say_nonl" or something similar. But since there would only be one caller, I don't see much point in factoring it out. > Options (1), (2) and (3) are mutually alternative; my favorite is (1). Me too. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html