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

From: Junio C Hamano <gitster@pobox.com>
Date: May 7, 2015, 18:33 UTC
Message-ID: <xmqqmw1gp7aa.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAMbsUu6XT4hB0L0PjgJKniyBJ9svkQoJqnxiYaRWo9EZUXnNhg@mail.gmail.com>

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.

Show 18 quoted lines

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

Show 6 quoted lines

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

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

Show 5 quoted lines

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

I do not think we need this.

Show 6 quoted lines

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

Definitely not.

Show 8 quoted lines

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

Show 6 quoted lines

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

Show 9 quoted lines

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