threads / patch / 39253

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

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

## tl;dr

7 messages between May 5, 2015 and May 6, 2015. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Danny Lin· May 5, 2015, 17:20 UTC · lore
2015-05-05 5:14 GMT+08:00 Junio C Hamano <gitster@pobox.com>:
Show 10 quoted lines
> Danny Lin <danny0838@gmail.com> writes:
>
>> From dc549b6b4ec36f8faf9c6f7bb1e343ef7babd14f Mon Sep 17 00:00:00 2001
>> From: Danny Lin <danny0838@gmail.com>
>> Date: Mon, 4 May 2015 14:09:38 +0800
>> Subject: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split()
>
> Please do not use multipart/mixed attachments, but instead inline
> your patch.  When doing so, please drop all these four lines above.
>
Oops. How to drop the lines? I'm using Gmail and don't know the way to go.
Show 15 quoted lines
>>
>> cmd_split() prints the info message using "say -n", which
>> makes no sense and could cause the linefeed be trimmed in
>> some cases. This patch fixes the issue.
>
> I think this was written knowing that "say" is merely a thin wrapper
> of "echo" (which is a bad manner but happens to be correct) and
> assuming that everybody's "echo" understands "-n" (which is not a
> good assumption) to implement "progress display" that shows the "N
> out of M done" output over and over on the same physical line.
>
> So,... contrary to your "makes no sense" claim, what it tries to do
> makes perfect sense to me, even though its execution seems somewhat
> poor.
>

The original version has a CR (yes, it's CR, not LF) at the end of the "say -n" string, which is weird. If it's meant to print a linefeed, we should remove the CR and use "say". If it's meant not to print a linefeed, we still should remove the CR.

CR makes the shell behave weird, sometimes a linefeed is shown and sometimes not.

For example, in my shell (git version 2.3.7.windows.1), I frequently get a crowded message like this:

$ git subtree split -P subdir/ 1/3 (0)2/3 (1)3/3 (2)c9ad5da42e2bc00c76616207fe73978887656235

While sometimes like this: $ git subtree split -P subdir/ 1/3 (0)2/3 (1) 3/3 (2) c9ad5da42e2bc00c76616207fe73978887656235

The two behaviors happen almost randomly, at least I cannot predict.
Show 17 quoted lines
>> ---
>>  contrib/subtree/git-subtree.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
>> index fa1a583..28a1377 100755
>> --- a/contrib/subtree/git-subtree.sh
>> +++ b/contrib/subtree/git-subtree.sh
>> @@ -599,7 +599,7 @@ cmd_split()
>>       eval "$grl" |
>>       while read rev parents; do
>>               revcount=$(($revcount + 1))
>> -             say -n "$revcount/$revmax ($createcount)"
>> +             say "$revcount/$revmax ($createcount)"
>>               debug "Processing commit: $rev"
>>               exists=$(cache_get $rev)
>>               if [ -n "$exists" ]; then
Junio C Hamano· May 5, 2015, 19:11 UTC · re: Danny Lin · lore
Danny Lin <danny0838@gmail.com> writes:
Show 14 quoted lines
>> I think this was written knowing that "say" is merely a thin wrapper
>> of "echo" (which is a bad manner but happens to be correct) and
>> assuming that everybody's "echo" understands "-n" (which is not a
>> good assumption) to implement "progress display" that shows the "N
>> out of M done" output over and over on the same physical line.
>>
>> So,... contrary to your "makes no sense" claim, what it tries to do
>> makes perfect sense to me, even though its execution seems somewhat
>> poor.
>>
> The original version has a CR (yes, it's CR, not LF) at the end of the
> "say -n" string, which is weird. If it's meant to print a linefeed, we should
> remove the CR and use "say". If it's meant not to print a linefeed, we still
> should remove the CR.

Neither. It is meant to print a carriage-return, i.e. "go back to the left-most column on the same line, without feeding a new line to the terminal (causing the output to scroll-up by one line)".

It sounds to me that your terminal is not supporting carriage-return in a way everybody else expects it to? It is not just this script, but all the progress output we generate use CR for that purpose.

Do you see a similar "garbled" output from say "git fetch" or "git checkout" that takes more than a few hundred milliseconds?

Danny Lin· May 6, 2015, 09:57 UTC · re: Junio C Hamano · lore

Thank you for clearifying this. It seems that it's my terminal trimming the <CR> from the source code.

If I run a script file with: echo -n "Hello, world1<CR>" echo -n "Hello, world2<CR>" echo -n "Hello, world3<CR>" echo -n "Hello, world4<CR>"

I get this on the screen: Hello, world1Hello, world2Hello, world3Hello, world4

If I run with: printf "Hello, world1\r" printf "Hello, world2\r" printf "Hello, world3\r" printf "Hello, world4\r"

I get this on the screen: Hello, world4

I don't see a problem in 'git fetch' or 'git checkout'
Maybe using printf is the way to go?
2015-05-06 3:11 GMT+08:00 Junio C Hamano <gitster@pobox.com>:
Show 29 quoted lines
> Danny Lin <danny0838@gmail.com> writes:
>
>>> I think this was written knowing that "say" is merely a thin wrapper
>>> of "echo" (which is a bad manner but happens to be correct) and
>>> assuming that everybody's "echo" understands "-n" (which is not a
>>> good assumption) to implement "progress display" that shows the "N
>>> out of M done" output over and over on the same physical line.
>>>
>>> So,... contrary to your "makes no sense" claim, what it tries to do
>>> makes perfect sense to me, even though its execution seems somewhat
>>> poor.
>>>
>> The original version has a CR (yes, it's CR, not LF) at the end of the
>> "say -n" string, which is weird. If it's meant to print a linefeed, we should
>> remove the CR and use "say". If it's meant not to print a linefeed, we still
>> should remove the CR.
>
> Neither.  It is meant to print a carriage-return, i.e. "go back to
> the left-most column on the same line, without feeding a new line to
> the terminal (causing the output to scroll-up by one line)".
>
> It sounds to me that your terminal is not supporting carriage-return
> in a way everybody else expects it to?  It is not just this script,
> but all the progress output we generate use CR for that purpose.
>
> Do you see a similar "garbled" output from say "git fetch" or "git
> checkout" that takes more than a few hundred milliseconds?
>
>
Junio C Hamano· May 6, 2015, 17:16 UTC · re: Danny Lin · lore
Danny Lin <danny0838@gmail.com> writes:
Show 12 quoted lines
> If I run with:
> printf "Hello, world1\r"
> printf "Hello, world2\r"
> printf "Hello, world3\r"
> printf "Hello, world4\r"
>
> I get this on the screen:
> Hello, world4
>
> I don't see a problem in 'git fetch' or 'git checkout'
>
> Maybe using printf is the way to go?
Yes.  Thanks.
Danny Lin· May 6, 2015, 18:58 UTC · re: Junio C Hamano · lore

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.

Signed-off-by: Danny Lin <danny0838@gmail.com>
---
 contrib/subtree/git-subtree.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to contrib/subtree/git-subtree.sh +2 −1
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"
         debug "Processing commit: $rev"
         exists=$(cache_get $rev)
         if [ -n "$exists" ]; then

-- 
2.3.7.windows.1



2015-05-07 1:16 GMT+08:00 Junio C Hamano <gitster@pobox.com>:
> Danny Lin <danny0838@gmail.com> writes:
>
>> If I run with:
>> printf "Hello, world1\r"
>> printf "Hello, world2\r"
>> printf "Hello, world3\r"
>> printf "Hello, world4\r"
>>
>> I get this on the screen:
>> Hello, world4
>>
>> I don't see a problem in 'git fetch' or 'git checkout'
>>
>> Maybe using printf is the way to go?
>
> Yes.  Thanks.
Junio C Hamano· May 6, 2015, 19:49 UTC · re: Danny Lin · lore
Danny Lin <danny0838@gmail.com> writes:
Show 25 quoted lines
> 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.
>
> Signed-off-by: Danny Lin <danny0838@gmail.com>
> ---
>  contrib/subtree/git-subtree.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> 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.
>          debug "Processing commit: $rev"
>          exists=$(cache_get $rev)
>          if [ -n "$exists" ]; then
Eric Sunshine· May 6, 2015, 19:58 UTC · re: Junio C Hamano · lore
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
> --

← back to recent threads