public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Danny Lin <danny0838@gmail•com>
Cc: Eric Sunshine <sunshine@sunshineco•com>,
	git develop <git@vger•kernel.org>
Subject: Re: [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split()
Date: Thu, 07 May 2015 11:33:01 -0700	[thread overview]
Message-ID: <xmqqmw1gp7aa.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <CAMbsUu6XT4hB0L0PjgJKniyBJ9svkQoJqnxiYaRWo9EZUXnNhg@mail.gmail.com> (Danny Lin's message of "Thu, 7 May 2015 13:10:09 +0800")

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.

next prev parent reply	other threads:[~2015-05-07 18:33 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-05-07  3:39 [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split() Danny Lin
2015-05-07  3:43 ` Danny Lin
2015-05-07  5:10   ` Danny Lin
2015-05-07 18:33     ` Junio C Hamano [this message]
2015-05-08  0:51       ` [PATCH] contrib/subtree: portability fix for string printing Danny Lin
2015-05-08  0:56       ` Danny Lin
2015-05-08 17:49         ` Junio C Hamano
2015-05-08 17:56           ` Eric Sunshine
2015-05-08 18:44             ` Junio C Hamano
2015-05-08 18:55               ` Eric Sunshine
  -- strict thread matches above, loose matches on Subject: below --
2015-05-05 17:20 [PATCH] contrib/subtree: fix linefeeds trimming for cmd_split() Danny Lin
2015-05-05 19:11 ` Junio C Hamano
2015-05-06  9:57   ` Danny Lin
2015-05-06 17:16     ` Junio C Hamano
2015-05-06 18:58       ` Danny Lin
2015-05-06 19:49         ` Junio C Hamano
2015-05-06 19:58           ` Eric Sunshine
2015-05-04  6:13 Danny Lin
2015-05-04 21:14 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqmw1gp7aa.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=danny0838@gmail$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=sunshine@sunshineco$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox