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

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

[Danny Lin]

2015-05-05 5:14 GMT+08:00 Junio C Hamano <gitster@pobox.com>:
> 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.

>>
>> 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

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

[Junio C Hamano]

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?

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

[Danny Lin]

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>:
> 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?
>
>

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

[Junio C Hamano]

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.

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

[Danny Lin]

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"
         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.

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

[Junio C Hamano]

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.
>
> 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

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

[Eric Sunshine]

On Wed, May 6, 2015 at 3:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>> 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).

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

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

[Danny Lin]

Replace all echo using printf for better portability.

Also re-wrap previous 'say -n "$str<CR>"' using a new
function state() so to prevent CR chars included in
the source code, which could be mal-processed on some
shells (e.g. 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>
---
 contrib/subtree/git-subtree.sh | 56 +++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index fa1a583..2da1433 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -29,7 +29,7 @@ rejoin        merge the new branch back into HEAD
  options for 'add', 'merge', 'pull' and 'push'
 squash        merge subtree changes as a single commit
 "
-eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
+eval "$(printf %s "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" ||
printf %s "exit $?")"

 PATH=$PATH:$(git --exec-path)
 . git-sh-setup
@@ -51,17 +51,29 @@ prefix=
 debug()
 {
     if [ -n "$debug" ]; then
-        echo "$@" >&2
+        printf "%s\n" "$*" >&2
     fi
 }

 say()
 {
     if [ -z "$quiet" ]; then
-        echo "$@" >&2
+        printf "%s\n" "$*" >&2
     fi
 }

+state()
+{
+    if [ -z "$quiet" ]; then
+        printf "%s\r" "$*" >&2
+    fi
+}
+
+log()
+{
+    printf "%s\n" "$*"
+}
+
 assert()
 {
     if "$@"; then
@@ -72,7 +84,7 @@ assert()
 }


-#echo "Options: $*"
+#log "Options: $*"

 while [ $# -gt 0 ]; do
     opt="$1"
@@ -149,7 +161,7 @@ cache_get()
     for oldrev in $*; do
         if [ -r "$cachedir/$oldrev" ]; then
             read newrev <"$cachedir/$oldrev"
-            echo $newrev
+            log $newrev
         fi
     done
 }
@@ -158,7 +170,7 @@ cache_miss()
 {
     for oldrev in $*; do
         if [ ! -r "$cachedir/$oldrev" ]; then
-            echo $oldrev
+            log $oldrev
         fi
     done
 }
@@ -175,7 +187,7 @@ check_parents()

 set_notree()
 {
-    echo "1" > "$cachedir/notree/$1"
+    log "1" > "$cachedir/notree/$1"
 }

 cache_set()
@@ -187,7 +199,7 @@ cache_set()
          -a -e "$cachedir/$oldrev" ]; then
         die "cache for $oldrev already exists!"
     fi
-    echo "$newrev" >"$cachedir/$oldrev"
+    log "$newrev" >"$cachedir/$oldrev"
 }

 rev_exists()
@@ -219,7 +231,7 @@ rev_is_descendant_of_branch()
 try_remove_previous()
 {
     if rev_exists "$1^"; then
-        echo "^$1^"
+        log "^$1^"
     fi
 }

@@ -247,7 +259,7 @@ find_latest_squash()
                         sq="$sub"
                     fi
                     debug "Squash found: $sq $sub"
-                    echo "$sq" "$sub"
+                    log "$sq" "$sub"
                     break
                 fi
                 sq=
@@ -339,9 +351,9 @@ add_msg()
 add_squashed_msg()
 {
     if [ -n "$message" ]; then
-        echo "$message"
+        log "$message"
     else
-        echo "Merge commit '$1' as '$2'"
+        log "Merge commit '$1' as '$2'"
     fi
 }

@@ -373,17 +385,17 @@ squash_msg()

     if [ -n "$oldsub" ]; then
         oldsub_short=$(git rev-parse --short "$oldsub")
-        echo "Squashed '$dir/' changes from $oldsub_short..$newsub_short"
+        log "Squashed '$dir/' changes from $oldsub_short..$newsub_short"
         echo
         git log --pretty=tformat:'%h %s' "$oldsub..$newsub"
         git log --pretty=tformat:'REVERT: %h %s' "$newsub..$oldsub"
     else
-        echo "Squashed '$dir/' content from commit $newsub_short"
+        log "Squashed '$dir/' content from commit $newsub_short"
     fi

     echo
-    echo "git-subtree-dir: $dir"
-    echo "git-subtree-split: $newsub"
+    log "git-subtree-dir: $dir"
+    log "git-subtree-split: $newsub"
 }

 toptree_for_commit()
@@ -401,7 +413,7 @@ subtree_for_commit()
         assert [ "$name" = "$dir" ]
         assert [ "$type" = "tree" -o "$type" = "commit" ]
         [ "$type" = "commit" ] && continue  # ignore submodules
-        echo $tree
+        log $tree
         break
     done
 }
@@ -474,7 +486,7 @@ copy_or_skip()
     done

     if [ -n "$identical" ]; then
-        echo $identical
+        log $identical
     else
         copy_commit $rev $tree "$p" || exit $?
     fi
@@ -526,7 +538,7 @@ cmd_add()

 cmd_add_repository()
 {
-    echo "git fetch" "$@"
+    log "git fetch" "$@"
     repository=$1
     refspec=$2
     git fetch "$@" || exit $?
@@ -599,7 +611,7 @@ cmd_split()
     eval "$grl" |
     while read rev parents; do
         revcount=$(($revcount + 1))
-        say -n "$revcount/$revmax ($createcount)
"
+        state "$revcount/$revmax ($createcount)"
         debug "Processing commit: $rev"
         exists=$(cache_get $rev)
         if [ -n "$exists" ]; then
@@ -656,7 +668,7 @@ cmd_split()
         git update-ref -m 'subtree split' "refs/heads/$branch"
$latest_new || exit $?
         say "$action branch '$branch'"
     fi
-    echo $latest_new
+    log $latest_new
     exit 0
 }

@@ -726,7 +738,7 @@ cmd_push()
     if [ -e "$dir" ]; then
         repository=$1
         refspec=$2
-        echo "git push using: " $repository $refspec
+        log "git push using: " $repository $refspec
         localrev=$(git subtree split --prefix="$prefix") || die
         git push $repository $localrev:refs/heads/$refspec
     else
-- 
2.3.7.windows.1



2015-05-07 3:58 GMT+08:00 Eric Sunshine <sunshine@sunshineco.com>:
> On Wed, May 6, 2015 at 3:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.
>
Added related information.

>>> 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).
>
If a more thorough portability fix is desired, I'd prefer a work like this
(see the patch above).

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

[Danny Lin]

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

Replace all echo using printf for better portability.

Also re-wrap previous 'say -n "$str<CR>"' using a new
function state() to prevent CR chars included in the
source code, which could be mal-processed on 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>
---
 contrib/subtree/git-subtree.sh | 56 +++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index fa1a583..2da1433 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -29,7 +29,7 @@ rejoin        merge the new branch back into HEAD
  options for 'add', 'merge', 'pull' and 'push'
 squash        merge subtree changes as a single commit
 "
-eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
+eval "$(printf %s "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" ||
printf %s "exit $?")"

 PATH=$PATH:$(git --exec-path)
 . git-sh-setup
@@ -51,17 +51,29 @@ prefix=
 debug()
 {
     if [ -n "$debug" ]; then
-        echo "$@" >&2
+        printf "%s\n" "$*" >&2
     fi
 }

 say()
 {
     if [ -z "$quiet" ]; then
-        echo "$@" >&2
+        printf "%s\n" "$*" >&2
     fi
 }

+state()
+{
+    if [ -z "$quiet" ]; then
+        printf "%s\r" "$*" >&2
+    fi
+}
+
+log()
+{
+    printf "%s\n" "$*"
+}
+
 assert()
 {
     if "$@"; then
@@ -72,7 +84,7 @@ assert()
 }


-#echo "Options: $*"
+#log "Options: $*"

 while [ $# -gt 0 ]; do
     opt="$1"
@@ -149,7 +161,7 @@ cache_get()
     for oldrev in $*; do
         if [ -r "$cachedir/$oldrev" ]; then
             read newrev <"$cachedir/$oldrev"
-            echo $newrev
+            log $newrev
         fi
     done
 }
@@ -158,7 +170,7 @@ cache_miss()
 {
     for oldrev in $*; do
         if [ ! -r "$cachedir/$oldrev" ]; then
-            echo $oldrev
+            log $oldrev
         fi
     done
 }
@@ -175,7 +187,7 @@ check_parents()

 set_notree()
 {
-    echo "1" > "$cachedir/notree/$1"
+    log "1" > "$cachedir/notree/$1"
 }

 cache_set()
@@ -187,7 +199,7 @@ cache_set()
          -a -e "$cachedir/$oldrev" ]; then
         die "cache for $oldrev already exists!"
     fi
-    echo "$newrev" >"$cachedir/$oldrev"
+    log "$newrev" >"$cachedir/$oldrev"
 }

 rev_exists()
@@ -219,7 +231,7 @@ rev_is_descendant_of_branch()
 try_remove_previous()
 {
     if rev_exists "$1^"; then
-        echo "^$1^"
+        log "^$1^"
     fi
 }

@@ -247,7 +259,7 @@ find_latest_squash()
                         sq="$sub"
                     fi
                     debug "Squash found: $sq $sub"
-                    echo "$sq" "$sub"
+                    log "$sq" "$sub"
                     break
                 fi
                 sq=
@@ -339,9 +351,9 @@ add_msg()
 add_squashed_msg()
 {
     if [ -n "$message" ]; then
-        echo "$message"
+        log "$message"
     else
-        echo "Merge commit '$1' as '$2'"
+        log "Merge commit '$1' as '$2'"
     fi
 }

@@ -373,17 +385,17 @@ squash_msg()

     if [ -n "$oldsub" ]; then
         oldsub_short=$(git rev-parse --short "$oldsub")
-        echo "Squashed '$dir/' changes from $oldsub_short..$newsub_short"
+        log "Squashed '$dir/' changes from $oldsub_short..$newsub_short"
         echo
         git log --pretty=tformat:'%h %s' "$oldsub..$newsub"
         git log --pretty=tformat:'REVERT: %h %s' "$newsub..$oldsub"
     else
-        echo "Squashed '$dir/' content from commit $newsub_short"
+        log "Squashed '$dir/' content from commit $newsub_short"
     fi

     echo
-    echo "git-subtree-dir: $dir"
-    echo "git-subtree-split: $newsub"
+    log "git-subtree-dir: $dir"
+    log "git-subtree-split: $newsub"
 }

 toptree_for_commit()
@@ -401,7 +413,7 @@ subtree_for_commit()
         assert [ "$name" = "$dir" ]
         assert [ "$type" = "tree" -o "$type" = "commit" ]
         [ "$type" = "commit" ] && continue  # ignore submodules
-        echo $tree
+        log $tree
         break
     done
 }
@@ -474,7 +486,7 @@ copy_or_skip()
     done

     if [ -n "$identical" ]; then
-        echo $identical
+        log $identical
     else
         copy_commit $rev $tree "$p" || exit $?
     fi
@@ -526,7 +538,7 @@ cmd_add()

 cmd_add_repository()
 {
-    echo "git fetch" "$@"
+    log "git fetch" "$@"
     repository=$1
     refspec=$2
     git fetch "$@" || exit $?
@@ -599,7 +611,7 @@ cmd_split()
     eval "$grl" |
     while read rev parents; do
         revcount=$(($revcount + 1))
-        say -n "$revcount/$revmax ($createcount)
"
+        state "$revcount/$revmax ($createcount)"
         debug "Processing commit: $rev"
         exists=$(cache_get $rev)
         if [ -n "$exists" ]; then
@@ -656,7 +668,7 @@ cmd_split()
         git update-ref -m 'subtree split' "refs/heads/$branch"
$latest_new || exit $?
         say "$action branch '$branch'"
     fi
-    echo $latest_new
+    log $latest_new
     exit 0
 }

@@ -726,7 +738,7 @@ cmd_push()
     if [ -e "$dir" ]; then
         repository=$1
         refspec=$2
-        echo "git push using: " $repository $refspec
+        log "git push using: " $repository $refspec
         localrev=$(git subtree split --prefix="$prefix") || die
         git push $repository $localrev:refs/heads/$refspec
     else
-- 
2.3.7.windows.1


Typo fix for previous patch.

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

[Danny Lin]

[-- Attachment #1: Type: text/plain, Size: 136 bytes --]

I'm sorry that I cannot get git send-email work currently.
I'd submit the updated patch with an attachment until it's
working right. ><

[-- Attachment #2: 0001-contrib-subtree-portability-fix-for-string-printing.patch --]
[-- Type: application/octet-stream, Size: 5094 bytes --]

From f36d4eab24565894cfe3abba0131f01668a5934b Mon Sep 17 00:00:00 2001
From: Danny Lin <danny0838@gmail.com>
Date: Thu, 7 May 2015 10:51:31 +0800
Subject: [PATCH] contrib/subtree: portability fix for string printing

Replace all echo using printf for better portability.

Also re-wrap previous 'say -n "$str<CR>"' using a new
function state() 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>
---
 contrib/subtree/git-subtree.sh | 56 +++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index fa1a583..2da1433 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -29,7 +29,7 @@ rejoin        merge the new branch back into HEAD
  options for 'add', 'merge', 'pull' and 'push'
 squash        merge subtree changes as a single commit
 "
-eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
+eval "$(printf %s "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || printf %s "exit $?")"
 
 PATH=$PATH:$(git --exec-path)
 . git-sh-setup
@@ -51,17 +51,29 @@ prefix=
 debug()
 {
 	if [ -n "$debug" ]; then
-		echo "$@" >&2
+		printf "%s\n" "$*" >&2
 	fi
 }
 
 say()
 {
 	if [ -z "$quiet" ]; then
-		echo "$@" >&2
+		printf "%s\n" "$*" >&2
 	fi
 }
 
+state()
+{
+	if [ -z "$quiet" ]; then
+		printf "%s\r" "$*" >&2
+	fi
+}
+
+log()
+{
+	printf "%s\n" "$*"
+}
+
 assert()
 {
 	if "$@"; then
@@ -72,7 +84,7 @@ assert()
 }
 
 
-#echo "Options: $*"
+#log "Options: $*"
 
 while [ $# -gt 0 ]; do
 	opt="$1"
@@ -149,7 +161,7 @@ cache_get()
 	for oldrev in $*; do
 		if [ -r "$cachedir/$oldrev" ]; then
 			read newrev <"$cachedir/$oldrev"
-			echo $newrev
+			log $newrev
 		fi
 	done
 }
@@ -158,7 +170,7 @@ cache_miss()
 {
 	for oldrev in $*; do
 		if [ ! -r "$cachedir/$oldrev" ]; then
-			echo $oldrev
+			log $oldrev
 		fi
 	done
 }
@@ -175,7 +187,7 @@ check_parents()
 
 set_notree()
 {
-	echo "1" > "$cachedir/notree/$1"
+	log "1" > "$cachedir/notree/$1"
 }
 
 cache_set()
@@ -187,7 +199,7 @@ cache_set()
 	     -a -e "$cachedir/$oldrev" ]; then
 		die "cache for $oldrev already exists!"
 	fi
-	echo "$newrev" >"$cachedir/$oldrev"
+	log "$newrev" >"$cachedir/$oldrev"
 }
 
 rev_exists()
@@ -219,7 +231,7 @@ rev_is_descendant_of_branch()
 try_remove_previous()
 {
 	if rev_exists "$1^"; then
-		echo "^$1^"
+		log "^$1^"
 	fi
 }
 
@@ -247,7 +259,7 @@ find_latest_squash()
 						sq="$sub"
 					fi
 					debug "Squash found: $sq $sub"
-					echo "$sq" "$sub"
+					log "$sq" "$sub"
 					break
 				fi
 				sq=
@@ -339,9 +351,9 @@ add_msg()
 add_squashed_msg()
 {
 	if [ -n "$message" ]; then
-		echo "$message"
+		log "$message"
 	else
-		echo "Merge commit '$1' as '$2'"
+		log "Merge commit '$1' as '$2'"
 	fi
 }
 
@@ -373,17 +385,17 @@ squash_msg()
 	
 	if [ -n "$oldsub" ]; then
 		oldsub_short=$(git rev-parse --short "$oldsub")
-		echo "Squashed '$dir/' changes from $oldsub_short..$newsub_short"
+		log "Squashed '$dir/' changes from $oldsub_short..$newsub_short"
 		echo
 		git log --pretty=tformat:'%h %s' "$oldsub..$newsub"
 		git log --pretty=tformat:'REVERT: %h %s' "$newsub..$oldsub"
 	else
-		echo "Squashed '$dir/' content from commit $newsub_short"
+		log "Squashed '$dir/' content from commit $newsub_short"
 	fi
 	
 	echo
-	echo "git-subtree-dir: $dir"
-	echo "git-subtree-split: $newsub"
+	log "git-subtree-dir: $dir"
+	log "git-subtree-split: $newsub"
 }
 
 toptree_for_commit()
@@ -401,7 +413,7 @@ subtree_for_commit()
 		assert [ "$name" = "$dir" ]
 		assert [ "$type" = "tree" -o "$type" = "commit" ]
 		[ "$type" = "commit" ] && continue  # ignore submodules
-		echo $tree
+		log $tree
 		break
 	done
 }
@@ -474,7 +486,7 @@ copy_or_skip()
 	done
 	
 	if [ -n "$identical" ]; then
-		echo $identical
+		log $identical
 	else
 		copy_commit $rev $tree "$p" || exit $?
 	fi
@@ -526,7 +538,7 @@ cmd_add()
 
 cmd_add_repository()
 {
-	echo "git fetch" "$@"
+	log "git fetch" "$@"
 	repository=$1
 	refspec=$2
 	git fetch "$@" || exit $?
@@ -599,7 +611,7 @@ cmd_split()
 	eval "$grl" |
 	while read rev parents; do
 		revcount=$(($revcount + 1))
-		say -n "$revcount/$revmax ($createcount)\r"
+		state "$revcount/$revmax ($createcount)"
 		debug "Processing commit: $rev"
 		exists=$(cache_get $rev)
 		if [ -n "$exists" ]; then
@@ -656,7 +668,7 @@ cmd_split()
 		git update-ref -m 'subtree split' "refs/heads/$branch" $latest_new || exit $?
 		say "$action branch '$branch'"
 	fi
-	echo $latest_new
+	log $latest_new
 	exit 0
 }
 
@@ -726,7 +738,7 @@ cmd_push()
 	if [ -e "$dir" ]; then
 	    repository=$1
 	    refspec=$2
-	    echo "git push using: " $repository $refspec
+	    log "git push using: " $repository $refspec
 	    localrev=$(git subtree split --prefix="$prefix") || die
 	    git push $repository $localrev:refs/heads/$refspec
 	else
-- 
2.3.7.windows.1

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

[Junio C Hamano]

Danny Lin <danny0838@gmail.com> writes:

> Replace all echo using printf for better portability.

I doubt this change is sensible.

It is not like "echo is bad, don't use it".  It is more about "some
features of 'echo', like 'echo -n $msg' vs 'echo $msg\c' are not
portable".

>  "
> -eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
> +eval "$(printf %s "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || printf %s "exit $?")"

I do not think we want this.

>  PATH=$PATH:$(git --exec-path)
>  . git-sh-setup
> @@ -51,17 +51,29 @@ prefix=
>  debug()
>  {
>  	if [ -n "$debug" ]; then
> -		echo "$@" >&2
> +		printf "%s\n" "$*" >&2
>  	fi
>  }
>  
>  say()
>  {
>  	if [ -z "$quiet" ]; then
> -		echo "$@" >&2
> +		printf "%s\n" "$*" >&2
>  	fi
>  }

These are OK.

> +state()
> +{
> +	if [ -z "$quiet" ]; then
> +		printf "%s\r" "$*" >&2
> +	fi
> +}

This is good, but I think it is misnamed.  "progress" might be more
appropriate.

> +
> +log()
> +{
> +	printf "%s\n" "$*"
> +}

I do not think we need this.

> @@ -72,7 +84,7 @@ assert()
>  }
>  
>  
> -#echo "Options: $*"
> +#log "Options: $*"

Definitely not.

>  while [ $# -gt 0 ]; do
>  	opt="$1"
> @@ -149,7 +161,7 @@ cache_get()
>  	for oldrev in $*; do
>  		if [ -r "$cachedir/$oldrev" ]; then
>  			read newrev <"$cachedir/$oldrev"
> -			echo $newrev
> +			log $newrev

We know this is 40-hex, and there is no magic, don't we?

> @@ -158,7 +170,7 @@ cache_miss()
>  {
>  	for oldrev in $*; do
>  		if [ ! -r "$cachedir/$oldrev" ]; then
> -			echo $oldrev
> +			log $oldrev

Likewise.

And I'll stop saying "Likewise" at this point.

> @@ -599,7 +611,7 @@ cmd_split()
>  	eval "$grl" |
>  	while read rev parents; do
>  		revcount=$(($revcount + 1))
> -		say -n "$revcount/$revmax ($createcount)"
> +		state "$revcount/$revmax ($createcount)"
>  		debug "Processing commit: $rev"
>  		exists=$(cache_get $rev)
>  		if [ -n "$exists" ]; then

Good.

If we wanted to make "state" (or "progress") to be usable in a wider
context, we may want to change its implementation a little bit, but
that is a separate topic.  It only has a single caller, and it only
feeds ever growing string, so the "print and then carriage-return"
is sufficient for now.

Thanks.

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

[Danny Lin]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 0001-contrib-subtree-fix-linefeeds-trimming-for-cmd_split.patch --]
[-- Type: application/octet-stream, Size: 957 bytes --]

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()

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.
---
 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)\r"
+		say "$revcount/$revmax ($createcount)"
 		debug "Processing commit: $rev"
 		exists=$(cache_get $rev)
 		if [ -n "$exists" ]; then
-- 
2.3.7.windows.1

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

[Junio C Hamano]

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.

>
> 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.

> ---
>  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