Re: [PATCH v9 3/5] t4205, t6006, t7102: make functions more readable
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:58:00
Alexey Shumkin [off-list ref] 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.
+ # 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".
quoted hunk
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' '