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

2013-07-05 Thread Junio C Hamano
Alexey Shumkin alex.crez...@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.

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

 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' '
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-07-05 Thread Alexey Shumkin
On Thu, Jul 04, 2013 at 11:45:57PM -0700, Junio C Hamano wrote:
 Alexey Shumkin alex.crez...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-07-05 Thread Junio C Hamano
Alexey Shumkin alex.crez...@gmail.com writes:

 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

Whatever.

The above is an example of justifying a more vague simple (is
better is implied) with a concrete point (i.e. By moving that to
the end, you removed the need to conditionally shift $@ in the
function to simplify the codepath), based on my _guess_ of what you
possibly meant to say, from reading your description that did not
give much clue for me to guess why you thought the result was more
elegant.  If my guess missed what your true justification was,
please replace it with the more correct one ;-)

 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?

Documentation/CodingGuidelines::

 - We prefer a space between the function name and the parentheses. The
   opening { should also be on the same line.
   E.g.: my_function () {
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-07-05 Thread Alexey Shumkin
On Fri, Jul 05, 2013 at 01:44:07AM -0700, Junio C Hamano wrote:
 Alexey Shumkin alex.crez...@gmail.com writes:
 
  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
 
 Whatever.
 
 The above is an example of justifying a more vague simple (is
 better is implied) with a concrete point (i.e. By moving that to
 the end, you removed the need to conditionally shift $@ in the
 function to simplify the codepath), based on my _guess_ of what you
 possibly meant to say, from reading your description that did not
 give much clue for me to guess why you thought the result was more
 elegant.  If my guess missed what your true justification was,
 please replace it with the more correct one ;-)
Ok
 
  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?
 
 Documentation/CodingGuidelines::
Oh! :)
thank you
  
  - We prefer a space between the function name and the parentheses. The
opening { should also be on the same line.
E.g.: my_function () {
Aha

-- 
Alexey Shumkin
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-07-05 Thread Junio C Hamano
Alexey Shumkin alex.crez...@gmail.com writes:

  Could you point me to the coding style guide, please?
 
 Documentation/CodingGuidelines::
 Oh! :)
 thank you

The most important part of the coding guidelines is to match the
style to existing code when writing a new one:

$ git grep '^[a-z_]* ()' -- *.sh | wc -l
132
$ git grep '^[a-z_]*()' -- *.sh | wc -l
55

We have acquired quite a few violators, but that is no reason to add
more.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html