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

On Thu, Jul 04, 2013 at 11:45:57PM -0700, Junio C Hamano wrote:

Alexey Shumkin [off-list ref] writes:

quoted

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'm not sure whether this "last parameter" is needed in that code as far as we
already removed expected to fail tests

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

quoted

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

Could you point me to the coding style guide, please?

quoted

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

Well, this is "better formatted comment", I guess :)

quoted

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.

That was a proposition of J6t :-D
(see http://article.gmane.org/gmane.comp.version-control.git/229291):
    >If you wanted to, you could write this as
    >
    >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.
    >    printf "modify 2nd file (ge\303\244ndert)" |
    >    if test -n "$1"
    >    then
    >        iconv -f utf-8 -t $1
    >    else
    >        cat
    >    fi
    >}
    >
    >but I'm not sure whether it's a lot better.

Last sentence has apperared to be a key

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

quoted

 }
 
 test_expect_success 'creating initial files and commits' '

-- 
Alexey Shumkin