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

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: portability fix for string printing

[Danny Lin]

Replace echo using printf in debug() and say() for
better portability.

Also re-wrap previous 'say -n "$str<CR>"' using a new
function progress() 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 | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index fa1a583..6f6ddbe 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -51,14 +51,21 @@ 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
+}
+
+progress()
+{
+	if [ -z "$quiet" ]; then
+		printf "%s\r" "$*" >&2
 	fi
 }
 
@@ -247,7 +254,7 @@ find_latest_squash()
 						sq="$sub"
 					fi
 					debug "Squash found: $sq $sub"
-					echo "$sq" "$sub"
+					log "$sq" "$sub"
 					break
 				fi
 				sq=
@@ -599,7 +606,7 @@ cmd_split()
 	eval "$grl" |
 	while read rev parents; do
 		revcount=$(($revcount + 1))
-		say -n "$revcount/$revmax ($createcount)
"
+		progress "$revcount/$revmax ($createcount)"
 		debug "Processing commit: $rev"
 		exists=$(cache_get $rev)
 		if [ -n "$exists" ]; then
-- 
2.3.7.windows.1

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

[Danny Lin]

Replace echo using printf in debug() and say() for
better portability.

Also re-wrap previous 'say -n "$str<CR>"' using a new
function progress() 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 | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index fa1a583..d4dae7a 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -51,14 +51,21 @@ 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
+}
+
+progress()
+{
+	if [ -z "$quiet" ]; then
+		printf "%s\r" "$*" >&2
 	fi
 }
 
@@ -599,7 +606,7 @@ cmd_split()
 	eval "$grl" |
 	while read rev parents; do
 		revcount=$(($revcount + 1))
-		say -n "$revcount/$revmax ($createcount)
"
+		progress "$revcount/$revmax ($createcount)"
 		debug "Processing commit: $rev"
 		exists=$(cache_get $rev)
 		if [ -n "$exists" ]; then
-- 
2.3.7.windows.1

Previous patch had a flaw, revised.

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

[Junio C Hamano]

Danny Lin <danny0838@gmail.com> writes:

> Replace echo using printf in debug() and say() for
> better portability.
>
> Also re-wrap previous 'say -n "$str<CR>"' using a new
> function progress() 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>
> ---

Thanks, this looks good.  Will apply with a little bit of tweak in
the log message.

Just for future reference, when shooting many iterations of the same
patch in a short timeframe, please be aware that the recipient may
not get the messages in the order you sent, and that it may not be
apparent to the recipients what changed between the iterations.
What we commonly do around here to address these issues is to
mention what changed from the previous one below the "---" line
before the diffstat.  I would have done something like this if I
were doing this patch, for example:

        ...
        even if it uses CRLF as linefeeds.

        Signed-off-by: Danny Lin <danny0838@gmail.com>
        ---

        * The previous one still used "log" helper by mistake even
          though I removed the implementation of it and decided to
          use "echo" for non-tricky cases.  This fixes it.

          contrib/subtree/git-subtree.sh | 13 ++++++++++---
        ...

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

[Eric Sunshine]

On Fri, May 8, 2015 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Danny Lin <danny0838@gmail.com> writes:
>
>> Replace echo using printf in debug() and say() for
>> better portability.
>>
>> Also re-wrap previous 'say -n "$str<CR>"' using a new
>> function progress() 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>
>> ---
>
> Thanks, this looks good.  Will apply with a little bit of tweak in
> the log message.

Hmm, I would say that the changes to debug() and say() should either
be dropped or moved to a separate patch (along with the first
paragraph of the commit message). With the introduction of the
progress() abstraction, there is no longer any need for changes to
say(), and the "better portability" rationale for changing say() and
debug() is never properly explained, and is thus nebulous at best.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, May 8, 2015 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Danny Lin <danny0838@gmail.com> writes:
>>
>>> Replace echo using printf in debug() and say() for
>>> better portability.
>>>
>>> Also re-wrap previous 'say -n "$str<CR>"' using a new
>>> function progress() 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>
>>> ---
>>
>> Thanks, this looks good.  Will apply with a little bit of tweak in
>> the log message.
>
> Hmm, I would say that the changes to debug() and say() should either
> be dropped or moved to a separate patch (along with the first
> paragraph of the commit message). With the introduction of the
> progress() abstraction, there is no longer any need for changes to
> say(), and the "better portability" rationale for changing say() and
> debug() is never properly explained, and is thus nebulous at best.

I justified them in this way.

    contrib/subtree: portability fix for string printing
    
    'echo -n' is not portable, but this script used it as a way to give
    a string followed by a carriage return for progress messages.
    Introduce a new helper shell function "progress" and use printf as a
    more portable way to do this.  As a side effect, this makes it
    unnecessary to have a raw CR in our source, which can be munged 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.
    
    While at it, replace "echo" using printf in debug() and say() to
    avoid tempting people introducing the same bug.
    
    Signed-off-by: Danny Lin <danny0838@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

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

[Eric Sunshine]

On Fri, May 8, 2015 at 2:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> On Fri, May 8, 2015 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Thanks, this looks good.  Will apply with a little bit of tweak in
>>> the log message.
>>
>> Hmm, I would say that the changes to debug() and say() should either
>> be dropped or moved to a separate patch (along with the first
>> paragraph of the commit message). With the introduction of the
>> progress() abstraction, there is no longer any need for changes to
>> say(), and the "better portability" rationale for changing say() and
>> debug() is never properly explained, and is thus nebulous at best.
>
> I justified them in this way.
>
>     contrib/subtree: portability fix for string printing
>
>     'echo -n' is not portable, but this script used it as a way to give
>     a string followed by a carriage return for progress messages.
>     Introduce a new helper shell function "progress" and use printf as a
>     more portable way to do this.  As a side effect, this makes it
>     unnecessary to have a raw CR in our source, which can be munged 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.

Very nicely explained.

>     While at it, replace "echo" using printf in debug() and say() to
>     avoid tempting people introducing the same bug.

Okay, this works as reasonable justification for including those
changes in the same patch.

It might read a bit more fluidly if rephrased something like this:

    While at it, replace 'echo' with 'printf' in debug() and say() to
    eliminate the temptation of reintroducing the same bug.