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. But I was concerned about code duplication; one could additionally make say reusable in this single call site, instead of inlining and customizing it by replacing the "\n" with "\r". But for that, you need to either (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). Options (1), (2) and (3) are mutually alternative; my favorite is (1). I can see your points about opportunity, especially after looking at the commit message of the patch of yours you linked. > The patch would look like the one I did in 89b0230a20 (Wed Aug 7 2013, > die_with_status: use "printf '%s\n'", not "echo"). I see your point. But note that using printf like in die_with_status after that commit wouldn't be reusable here in all call sites, because it always prints a newline. Cheers, -- Paolo G. Giarrusso - Ph.D. Student, Philipps-University Marburg http://www.informatik.uni-marburg.de/~pgiarrusso/ -- 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