Re: [PATCH v9 3/5] t4205, t6006, t7102: make functions more readable

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 5, 2013, 06:45 UTC
Message-ID: <7v61wpbj2y.fsf@alter.siamese.dyndns.org>
In-Reply-To: <aeb5182a691105600edc181c08020cfaa350c7df.1372939482.git.Alex.Crezoff@gmail.com>

Alexey Shumkin <alex.crezoff@gmail.com> writes:

> Function 'test_format' is become hard to read after its change in
> de6029a2d7734a93a9e27b9c4471862a47dd8123. So, make it more elegant.
> Also, change 'commit_msg' function to make it more pretty.

I do not know where you pick up these "more elegant" and "more pretty" from, but please refrain from using _only_ such vague and subjective phrases to describe the change in the log message. Saying "make it <<better>> by doing X" (with various subjective adjectives to say "better") is fine, but make sure you have "doing X" part in the explanation.

Perhaps like this.

    Function 'test_format' has become harder to read after its
    change in de6029a2 (pretty: Add failing tests: --format output
    should honor logOutputEncoding, 2013-06-26).  Simplify it by
    moving its "should we expect it to fail?" parameter to the end.

I cannot read why you think the updated commit_msg is "more pretty" in the message or in the patch.

> -commit_msg () {
> -	# String "initial. initial" partly in German (translated with Google Translate),
> +commit_msg() {

Style.  Have SP on both sides of () in a shell function definition.

Show 5 quoted lines

> +	# String "initial. initial" partly in German
> +	# (translated with Google Translate),
>  	# encoded in UTF-8, used as a commit log message below.
>  	msg=$(printf "initial. anf\303\244nglich")
>  	if test -n "$1"

This is not "more pretty" but "better commented".

Show 24 quoted lines

> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 2ef96e9..73a1bdb 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -9,15 +9,17 @@ Documented tests for git reset'
>  
>  . ./test-lib.sh
>  
> -commit_msg () {
> -	# String "modify 2nd file (changed)" partly in German(translated with Google Translate),
> +commit_msg() {
> +	# String "modify 2nd file (changed)" partly in German
> +	# (translated with Google Translate),
>  	# encoded in UTF-8, used as a commit log message below.
> -	msg=$(printf "modify 2nd file (ge\303\244ndert)")
> +	printf "modify 2nd file (ge\303\244ndert)" |
>  	if test -n "$1"
>  	then
> -		msg=$(echo $msg | iconv -f utf-8 -t $1)
> +		iconv -f utf-8 -t $1
> +	else
> +		cat
>  	fi
> -	echo $msg

Is it "more pretty"? The "we have to have cat only because we want to pipe into a conditional" look somewhat ugly.

	msg="modify 2nd file (ge\303\244ndert)"
        if test -n "$1"
	then
		printf "$msg" | iconv -f utf-8 -t "$1"
	else
		printf "$msg"
	fi

>  }
>  
>  test_expect_success 'creating initial files and commits' '