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

[Alexey Shumkin <alex.crezoff@gmail.com>]

On Thu, Jul 04, 2013 at 11:45:57PM -0700, Junio C Hamano wrote:
> 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'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.
> 
> > -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?
> 
> > +	# 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 :)
> 
> > 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
> 
> >  }
> >  
> >  test_expect_success 'creating initial files and commits' '

-- 
Alexey Shumkin