{"thread":{"id":"39253","subject":"Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split()","startedAt":"2015-05-05T17:20:08Z","lastAt":"2015-05-06T19:58:33Z","messageCount":7,"participants":["Danny Lin","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"260595","messageId":"CAMbsUu6xZrMu_jrV=jR4XNLf1UXLApBiAWJiWJuKRb4xN90QJQ@mail.gmail.com","threadId":"39253","inReplyTo":null,"subject":"Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split()","fromName":"Danny Lin","fromEmail":"danny0838@gmail.com","sentAt":"2015-05-05T17:20:08Z","receivedAt":"2015-05-05T17:20:08Z","isPatch":true,"sender":{"key":"danny0838@gmail.com","avatar":"https://avatars.githubusercontent.com/u/531417?v=4"},"body":"2015-05-05 5:14 GMT+08:00 Junio C Hamano :\n> Danny Lin writes:\n>\n>> From dc549b6b4ec36f8faf9c6f7bb1e343ef7babd14f Mon Sep 17 00:00:00 2001\n>> From: Danny Lin \n>> Date: Mon, 4 May 2015 14:09:38 +0800\n>> Subject: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split()\n>\n> Please do not use multipart/mixed attachments, but instead inline\n> your patch. When doing so, please drop all these four lines above.\n>\nOops. How to drop the lines? I'm using Gmail and don't know the way to go.\n\n>>\n>> cmd_split() prints the info message using \"say -n\", which\n>> makes no sense and could cause the linefeed be trimmed in\n>> some cases. This patch fixes the issue.\n>\n> I think this was written knowing that \"say\" is merely a thin wrapper\n> of \"echo\" (which is a bad manner but happens to be correct) and\n> assuming that everybody's \"echo\" understands \"-n\" (which is not a\n> good assumption) to implement \"progress display\" that shows the \"N\n> out of M done\" output over and over on the same physical line.\n>\n> So,... contrary to your \"makes no sense\" claim, what it tries to do\n> makes perfect sense to me, even though its execution seems somewhat\n> poor.\n>\nThe original version has a CR (yes, it's CR, not LF) at the end of the\n\"say -n\" string, which is weird. If it's meant to print a linefeed, we should\nremove the CR and use \"say\". If it's meant not to print a linefeed, we still\nshould remove the CR.\n\nCR makes the shell behave weird, sometimes a linefeed is shown and\nsometimes not.\n\nFor example, in my shell (git version 2.3.7.windows.1), I frequently get\na crowded message like this:\n\n$ git subtree split -P subdir/\n1/3 (0)2/3 (1)3/3 (2)c9ad5da42e2bc00c76616207fe73978887656235\n\nWhile sometimes like this:\n$ git subtree split -P subdir/\n1/3 (0)2/3 (1)\n3/3 (2)\nc9ad5da42e2bc00c76616207fe73978887656235\n\nThe two behaviors happen almost randomly, at least I cannot predict.\n\n\n>> ---\n>> contrib/subtree/git-subtree.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n>> index fa1a583..28a1377 100755\n>> --- a/contrib/subtree/git-subtree.sh\n>> +++ b/contrib/subtree/git-subtree.sh\n>> @@ -599,7 +599,7 @@ cmd_split()\n>> eval \"$grl\" |\n>> while read rev parents; do\n>> revcount=$(($revcount + 1))\n>> - say -n \"$revcount/$revmax ($createcount)\"\n>> + say \"$revcount/$revmax ($createcount)\"\n>> debug \"Processing commit: $rev\"\n>> exists=$(cache_get $rev)\n>> if [ -n \"$exists\" ]; then\n"},{"id":"260611","messageId":"xmqq4mnqet5d.fsf@gitster.dls.corp.google.com","threadId":"39253","inReplyTo":"CAMbsUu6xZrMu_jrV=jR4XNLf1UXLApBiAWJiWJuKRb4xN90QJQ@mail.gmail.com","subject":"Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-05T19:11:42Z","receivedAt":"2015-05-05T19:11:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Danny Lin writes:\n\n>> I think this was written knowing that \"say\" is merely a thin wrapper\n>> of \"echo\" (which is a bad manner but happens to be correct) and\n>> assuming that everybody's \"echo\" understands \"-n\" (which is not a\n>> good assumption) to implement \"progress display\" that shows the \"N\n>> out of M done\" output over and over on the same physical line.\n>>\n>> So,... contrary to your \"makes no sense\" claim, what it tries to do\n>> makes perfect sense to me, even though its execution seems somewhat\n>> poor.\n>>\n> The original version has a CR (yes, it's CR, not LF) at the end of the\n> \"say -n\" string, which is weird. If it's meant to print a linefeed, we should\n> remove the CR and use \"say\". If it's meant not to print a linefeed, we still\n> should remove the CR.\n\nNeither. It is meant to print a carriage-return, i.e. \"go back to\nthe left-most column on the same line, without feeding a new line to\nthe terminal (causing the output to scroll-up by one line)\".\n\nIt sounds to me that your terminal is not supporting carriage-return\nin a way everybody else expects it to? It is not just this script,\nbut all the progress output we generate use CR for that purpose.\n\nDo you see a similar \"garbled\" output from say \"git fetch\" or \"git\ncheckout\" that takes more than a few hundred milliseconds?\n"},{"id":"260661","messageId":"CAMbsUu6=U92TRo-UeOL1qtaTipMQFzD+m+wM7sn1o-AjD6LJBw@mail.gmail.com","threadId":"39253","inReplyTo":"xmqq4mnqet5d.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split()","fromName":"Danny Lin","fromEmail":"danny0838@gmail.com","sentAt":"2015-05-06T09:57:53Z","receivedAt":"2015-05-06T09:57:53Z","isPatch":true,"sender":{"key":"danny0838@gmail.com","avatar":"https://avatars.githubusercontent.com/u/531417?v=4"},"body":"Thank you for clearifying this. It seems that it's my terminal\ntrimming the from the source code.\n\nIf I run a script file with:\necho -n \"Hello, world1\"\necho -n \"Hello, world2\"\necho -n \"Hello, world3\"\necho -n \"Hello, world4\"\n\nI get this on the screen:\nHello, world1Hello, world2Hello, world3Hello, world4\n\nIf I run with:\nprintf \"Hello, world1\\r\"\nprintf \"Hello, world2\\r\"\nprintf \"Hello, world3\\r\"\nprintf \"Hello, world4\\r\"\n\nI get this on the screen:\nHello, world4\n\nI don't see a problem in 'git fetch' or 'git checkout'\n\nMaybe using printf is the way to go?\n\n2015-05-06 3:11 GMT+08:00 Junio C Hamano :\n> Danny Lin writes:\n>\n>>> I think this was written knowing that \"say\" is merely a thin wrapper\n>>> of \"echo\" (which is a bad manner but happens to be correct) and\n>>> assuming that everybody's \"echo\" understands \"-n\" (which is not a\n>>> good assumption) to implement \"progress display\" that shows the \"N\n>>> out of M done\" output over and over on the same physical line.\n>>>\n>>> So,... contrary to your \"makes no sense\" claim, what it tries to do\n>>> makes perfect sense to me, even though its execution seems somewhat\n>>> poor.\n>>>\n>> The original version has a CR (yes, it's CR, not LF) at the end of the\n>> \"say -n\" string, which is weird. If it's meant to print a linefeed, we should\n>> remove the CR and use \"say\". If it's meant not to print a linefeed, we still\n>> should remove the CR.\n>\n> Neither. It is meant to print a carriage-return, i.e. \"go back to\n> the left-most column on the same line, without feeding a new line to\n> the terminal (causing the output to scroll-up by one line)\".\n>\n> It sounds to me that your terminal is not supporting carriage-return\n> in a way everybody else expects it to? It is not just this script,\n> but all the progress output we generate use CR for that purpose.\n>\n> Do you see a similar \"garbled\" output from say \"git fetch\" or \"git\n> checkout\" that takes more than a few hundred milliseconds?\n>\n>\n"},{"id":"260666","messageId":"xmqqwq0lbp87.fsf@gitster.dls.corp.google.com","threadId":"39253","inReplyTo":"CAMbsUu6=U92TRo-UeOL1qtaTipMQFzD+m+wM7sn1o-AjD6LJBw@mail.gmail.com","subject":"Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-06T17:16:56Z","receivedAt":"2015-05-06T17:16:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Danny Lin writes:\n\n> If I run with:\n> printf \"Hello, world1\\r\"\n> printf \"Hello, world2\\r\"\n> printf \"Hello, world3\\r\"\n> printf \"Hello, world4\\r\"\n>\n> I get this on the screen:\n> Hello, world4\n>\n> I don't see a problem in 'git fetch' or 'git checkout'\n>\n> Maybe using printf is the way to go?\n\nYes. Thanks.\n"},{"id":"260677","messageId":"CAMbsUu4bix6pJA4OOoMSwYu0M6nO1+aZ7RLXU5sSOdOevN_Wzw@mail.gmail.com","threadId":"39253","inReplyTo":"xmqqwq0lbp87.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split()","fromName":"Danny Lin","fromEmail":"danny0838@gmail.com","sentAt":"2015-05-06T18:58:21Z","receivedAt":"2015-05-06T18:58:21Z","isPatch":true,"sender":{"key":"danny0838@gmail.com","avatar":"https://avatars.githubusercontent.com/u/531417?v=4"},"body":"cmd_split() prints a CR char by assigning a variable\nwith a literal CR in the source code, which could be\ntrimmed or mis-processed in some terminals. Replace\nwith $(printf '\\r') to fix it.\n\nSigned-off-by: Danny Lin \n---\n contrib/subtree/git-subtree.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex fa1a583..3a581fc 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -596,10 +596,11 @@ cmd_split()\n revmax=$(eval \"$grl\" | wc -l)\n revcount=0\n createcount=0\n+ CR=$(printf '\\r')\n eval \"$grl\" |\n while read rev parents; do\n revcount=$(($revcount + 1))\n- say -n \"$revcount/$revmax ($createcount)\n\"\n+ say -n \"$revcount/$revmax ($createcount)$CR\"\n debug \"Processing commit: $rev\"\n exists=$(cache_get $rev)\n if [ -n \"$exists\" ]; then\n-- \n2.3.7.windows.1\n\n\n\n2015-05-07 1:16 GMT+08:00 Junio C Hamano :\n> Danny Lin writes:\n>\n>> If I run with:\n>> printf \"Hello, world1\\r\"\n>> printf \"Hello, world2\\r\"\n>> printf \"Hello, world3\\r\"\n>> printf \"Hello, world4\\r\"\n>>\n>> I get this on the screen:\n>> Hello, world4\n>>\n>> I don't see a problem in 'git fetch' or 'git checkout'\n>>\n>> Maybe using printf is the way to go?\n>\n> Yes. Thanks.\n"},{"id":"260684","messageId":"xmqqfv79trk8.fsf@gitster.dls.corp.google.com","threadId":"39253","inReplyTo":"CAMbsUu4bix6pJA4OOoMSwYu0M6nO1+aZ7RLXU5sSOdOevN_Wzw@mail.gmail.com","subject":"Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-06T19:49:11Z","receivedAt":"2015-05-06T19:49:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Danny Lin writes:\n\n> cmd_split() prints a CR char by assigning a variable\n> with a literal CR in the source code, which could be\n> trimmed or mis-processed in some terminals. Replace\n> with $(printf '\\r') to fix it.\n>\n> Signed-off-by: Danny Lin \n> ---\n> contrib/subtree/git-subtree.sh | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index fa1a583..3a581fc 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -596,10 +596,11 @@ cmd_split()\n> revmax=$(eval \"$grl\" | wc -l)\n> revcount=0\n> createcount=0\n> + CR=$(printf '\\r')\n> eval \"$grl\" |\n> while read rev parents; do\n> revcount=$(($revcount + 1))\n> - say -n \"$revcount/$revmax ($createcount)\n> \"\n> + say -n \"$revcount/$revmax ($createcount)$CR\"\n\nInteresting. I would have expected, especially this is a portability-fix\nchange, that the change would be a single liner\n\n-\tsay -n ...\n+\tprintf \"%s\\r\" \"$revcount/$revmax ($createcount)\"\n\nthat does not touch any other line.\n\n> debug \"Processing commit: $rev\"\n> exists=$(cache_get $rev)\n> if [ -n \"$exists\" ]; then\n"},{"id":"260685","messageId":"CAPig+cT1JY2N6gkzj1kbQKR+nXBMu19-Mkw7V7BNewsOj4mm0Q@mail.gmail.com","threadId":"39253","inReplyTo":"xmqqfv79trk8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-06T19:58:33Z","receivedAt":"2015-05-06T19:58:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 6, 2015 at 3:49 PM, Junio C Hamano wrote:\n> Danny Lin writes:\n>\n>> cmd_split() prints a CR char by assigning a variable\n>> with a literal CR in the source code, which could be\n>> trimmed or mis-processed in some terminals. Replace\n>> with $(printf '\\r') to fix it.\n\nFor future readers of the patch who haven't followed the email\ndiscussion, it might be a good idea to explain the problem in more\ndetail. Saying merely \"could be trimmed or mis-processed in some\nterminals\" doesn't give much for people to latch onto if they want to\nunderstand the specific problem. Concrete information would help.\n\n>> Signed-off-by: Danny Lin \n>> ---\n>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n>> index fa1a583..3a581fc 100755\n>> --- a/contrib/subtree/git-subtree.sh\n>> +++ b/contrib/subtree/git-subtree.sh\n>> @@ -596,10 +596,11 @@ cmd_split()\n>> revmax=$(eval \"$grl\" | wc -l)\n>> revcount=0\n>> createcount=0\n>> + CR=$(printf '\\r')\n>> eval \"$grl\" |\n>> while read rev parents; do\n>> revcount=$(($revcount + 1))\n>> - say -n \"$revcount/$revmax ($createcount)\n>> \"\n>> + say -n \"$revcount/$revmax ($createcount)$CR\"\n>\n> Interesting. I would have expected, especially this is a portability-fix\n> change, that the change would be a single liner\n>\n> - say -n ...\n> + printf \"%s\\r\" \"$revcount/$revmax ($createcount)\"\n>\n> that does not touch any other line.\n\nUnfortunately, that solution does not respect the $quiet flag like\nsay() does. I had envisioned the patch as reimplementing say() using\nprintf rather than echo, and having say() itself either recognizing\nthe -n flag or just update callers to specify \\n when they want it\n(which is probably the cleaner of the two approaches).\n\n>\n>> debug \"Processing commit: $rev\"\n>> exists=$(cache_get $rev)\n>> if [ -n \"$exists\" ]; then\n> --\n"}]}