Re: [PATCH] contrib/subtree: portability fix for string printing

Eric Sunshine [off-list ref] writes:

On Fri, May 8, 2015 at 1:49 PM, Junio C Hamano [off-list ref] wrote:

quoted

Danny Lin [off-list ref] writes:

quoted

Replace echo using printf in debug() and say() for
better portability.

Also re-wrap previous 'say -n "$str<CR>"' using a new
function progress() to prevent CR chars included in the
source code, which could be mal-processed in some shells.
For example, MsysGit trims CR before executing a shell
script file in order to make it work right on Windows
even if it uses CRLF as linefeeds.

Signed-off-by: Danny Lin <redacted>
---

Thanks, this looks good.  Will apply with a little bit of tweak in
the log message.

Hmm, I would say that the changes to debug() and say() should either
be dropped or moved to a separate patch (along with the first
paragraph of the commit message). With the introduction of the
progress() abstraction, there is no longer any need for changes to
say(), and the "better portability" rationale for changing say() and
debug() is never properly explained, and is thus nebulous at best.

I justified them in this way.

    contrib/subtree: portability fix for string printing
    
    'echo -n' is not portable, but this script used it as a way to give
    a string followed by a carriage return for progress messages.
    Introduce a new helper shell function "progress" and use printf as a
    more portable way to do this.  As a side effect, this makes it
    unnecessary to have a raw CR in our source, which can be munged in
    some shells.  For example, MsysGit trims CR before executing a shell
    script file in order to make it work right on Windows even if it
    uses CRLF as linefeeds.
    
    While at it, replace "echo" using printf in debug() and say() to
    avoid tempting people introducing the same bug.
    
    Signed-off-by: Danny Lin [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]

On Fri, May 8, 2015 at 2:44 PM, Junio C Hamano [off-list ref] wrote:

Eric Sunshine [off-list ref] writes:

quoted

On Fri, May 8, 2015 at 1:49 PM, Junio C Hamano [off-list ref] wrote:

quoted

Thanks, this looks good.  Will apply with a little bit of tweak in
the log message.

Hmm, I would say that the changes to debug() and say() should either
be dropped or moved to a separate patch (along with the first
paragraph of the commit message). With the introduction of the
progress() abstraction, there is no longer any need for changes to
say(), and the "better portability" rationale for changing say() and
debug() is never properly explained, and is thus nebulous at best.

I justified them in this way.

    contrib/subtree: portability fix for string printing

    'echo -n' is not portable, but this script used it as a way to give
    a string followed by a carriage return for progress messages.
    Introduce a new helper shell function "progress" and use printf as a
    more portable way to do this.  As a side effect, this makes it
    unnecessary to have a raw CR in our source, which can be munged in
    some shells.  For example, MsysGit trims CR before executing a shell
    script file in order to make it work right on Windows even if it
    uses CRLF as linefeeds.

Very nicely explained.

    While at it, replace "echo" using printf in debug() and say() to
    avoid tempting people introducing the same bug.

Okay, this works as reasonable justification for including those
changes in the same patch.

It might read a bit more fluidly if rephrased something like this:

    While at it, replace 'echo' with 'printf' in debug() and say() to
    eliminate the temptation of reintroducing the same bug.