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

From: Junio C Hamano <gitster@pobox.com>
Date: May 8, 2015, 18:44 UTC
Message-ID: <xmqqpp6alxiw.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAPig+cQQSrQiSzp7Jat8LYH+RqYdpJ2XCXweAtrYE_QoLzSznQ@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 25 quoted lines

> On Fri, May 8, 2015 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Danny Lin <danny0838@gmail.com> writes:
>>
>>> 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 <danny0838@gmail.com>
>>> ---
>>
>> 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 <danny0838@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>