# Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split() 7 messages from 2015-05-05 to 2015-05-06. Participants: Danny Lin, Junio C Hamano, Eric Sunshine. Thread: https://gitlist.dev/t/39253 ## Danny Lin, 2015-05-05 17:20 Subject: Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split() Message-ID: URL: https://gitlist.dev/e/CAMbsUu6xZrMu_jrV%3DjR4XNLf1UXLApBiAWJiWJuKRb4xN90QJQ%40mail.gmail.com ``` 2015-05-05 5:14 GMT+08:00 Junio C Hamano : > Danny Lin writes: > >> From dc549b6b4ec36f8faf9c6f7bb1e343ef7babd14f Mon Sep 17 00:00:00 2001 >> From: Danny Lin >> 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. >> >> 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. >> --- >> 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, 2015-05-05 19:11 Subject: Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split() Message-ID: URL: https://gitlist.dev/e/xmqq4mnqet5d.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Danny Lin 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? ``` ## Danny Lin, 2015-05-06 09:57 Subject: Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split() Message-ID: URL: https://gitlist.dev/e/CAMbsUu6%3DU92TRo-UeOL1qtaTipMQFzD%2Bm%2BwM7sn1o-AjD6LJBw%40mail.gmail.com In-Reply-To: ``` Thank you for clearifying this. It seems that it's my terminal trimming the from the source code. If I run a script file with: echo -n "Hello, world1" echo -n "Hello, world2" echo -n "Hello, world3" echo -n "Hello, world4" 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 : > Danny Lin 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, 2015-05-06 17:16 Subject: Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split() Message-ID: URL: https://gitlist.dev/e/xmqqwq0lbp87.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Danny Lin 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. ``` ## Danny Lin, 2015-05-06 18:58 Subject: Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split() Message-ID: URL: https://gitlist.dev/e/CAMbsUu4bix6pJA4OOoMSwYu0M6nO1%2BaZ7RLXU5sSOdOevN_Wzw%40mail.gmail.com In-Reply-To: ``` 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 --- 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" 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 : > Danny Lin 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, 2015-05-06 19:49 Subject: Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split() Message-ID: URL: https://gitlist.dev/e/xmqqfv79trk8.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Danny Lin 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. > > Signed-off-by: Danny Lin > --- > 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, 2015-05-06 19:58 Subject: Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcT1JY2N6gkzj1kbQKR%2BnXBMu19-Mkw7V7BNewsOj4mm0Q%40mail.gmail.com In-Reply-To: ``` On Wed, May 6, 2015 at 3:49 PM, Junio C Hamano wrote: > Danny Lin 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. >> Signed-off-by: Danny Lin >> --- >> 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). > >> debug "Processing commit: $rev" >> exists=$(cache_get $rev) >> if [ -n "$exists" ]; then > -- ```