Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 6, 2015, 19:58 UTC
Message-ID: <CAPig+cT1JY2N6gkzj1kbQKR+nXBMu19-Mkw7V7BNewsOj4mm0Q@mail.gmail.com>
In-Reply-To: <xmqqfv79trk8.fsf@gitster.dls.corp.google.com>

On Wed, May 6, 2015 at 3:49 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

> Danny Lin <danny0838@gmail.com> writes:
>
>> cmd_split() prints a CR char by assigning a variable
>> with a literal CR in the source code, which could be
>> trimmed or mis-processed in some terminals. Replace
>> with $(printf '\r') to fix it.

For future readers of the patch who haven't followed the email discussion, it might be a good idea to explain the problem in more detail. Saying merely "could be trimmed or mis-processed in some terminals" doesn't give much for people to latch onto if they want to understand the specific problem. Concrete information would help.

Show 25 quoted lines

>> Signed-off-by: Danny Lin <danny0838@gmail.com>
>> ---
>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
>> index fa1a583..3a581fc 100755
>> --- a/contrib/subtree/git-subtree.sh
>> +++ b/contrib/subtree/git-subtree.sh
>> @@ -596,10 +596,11 @@ cmd_split()
>>      revmax=$(eval "$grl" | wc -l)
>>      revcount=0
>>      createcount=0
>> +    CR=$(printf '\r')
>>      eval "$grl" |
>>      while read rev parents; do
>>          revcount=$(($revcount + 1))
>> -        say -n "$revcount/$revmax ($createcount)
>> "
>> +        say -n "$revcount/$revmax ($createcount)$CR"
>
> Interesting.  I would have expected, especially this is a portability-fix
> change, that the change would be a single liner
>
> -       say -n ...
> +       printf "%s\r" "$revcount/$revmax ($createcount)"
>
> that does not touch any other line.

Unfortunately, that solution does not respect the $quiet flag like say() does. I had envisioned the patch as reimplementing say() using printf rather than echo, and having say() itself either recognizing the -n flag or just update callers to specify \n when they want it (which is probably the cleaner of the two approaches).

Show 5 quoted lines

>
>>          debug "Processing commit: $rev"
>>          exists=$(cache_get $rev)
>>          if [ -n "$exists" ]; then
> --