Re: [PATCH v3 2/2] mergetools: Simplify how we handle vim and defaults

2013-01-26 Thread Junio C Hamano
David Aguilar dav...@gmail.com writes:

 @@ -44,19 +46,9 @@ valid_tool () {
  }
  
  setup_tool () {
 - case $1 in
 - vim*|gvim*)
 - tool=vim
 - ;;
 - *)
 - tool=$1
 - ;;
 - esac

This part was an eyesore every time I looked at mergetools scripts.
Good riddance.

Is there still other special case like this, or was this the last
one?

Thanks, both of you, for working on this.
--
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 v3 2/2] mergetools: Simplify how we handle vim and defaults

2013-01-26 Thread David Aguilar
On Sat, Jan 26, 2013 at 7:15 PM, Junio C Hamano gits...@pobox.com wrote:
 David Aguilar dav...@gmail.com writes:

 @@ -44,19 +46,9 @@ valid_tool () {
  }

  setup_tool () {
 - case $1 in
 - vim*|gvim*)
 - tool=vim
 - ;;
 - *)
 - tool=$1
 - ;;
 - esac

 This part was an eyesore every time I looked at mergetools scripts.
 Good riddance.

 Is there still other special case like this, or was this the last
 one?

 Thanks, both of you, for working on this.

I believe that was the last special case. :-)  It should be easier
to auto-generate a list of tools for use in the documentation now.
That'll be the the next topic I look into.

I have a question. John mentioned that we can replace the use of
$(..) with $(..) in shell.

I have a trivial style patches to replace $(..) with $(..)
sitting uncommitted in my tree for mergetool--lib.

Grepping the rest of the tree shows that there are many
occurrences of the $(..) idiom in the shell code.

Is this a general rule that should be in CodingStyle,
or is it better left as-is?  Are there cases where DQ should
be used around $(..)?  My understanding is no, but I don't
know whether that is correct.

Doing the documentation stuff is more important IMO so I probably
shouldn't let myself get too distracted by it, though I am curious.
-- 
David
--
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 v3 2/2] mergetools: Simplify how we handle vim and defaults

2013-01-26 Thread Junio C Hamano
David Aguilar dav...@gmail.com writes:

 I have a question. John mentioned that we can replace the use of
 $(..) with $(..) in shell.

I think it isn't so much about $(...) but more about quoting the RHS
of assignment.

Consider these:

var=$another_var
var=$(command)
var=$one_var$two_var
var=$another_var$(command)

all of which _can_ be done without dq around the RHS, because the
result of variable substitution and command substitution will not be
subject to further word splitting.

I however find it easier to read with dq around the latter two, i.e.
substitution and then concatenation of the result of substitution.
The extra dq makes the intent of the author clearer, especially
while reviewing a script written by other people whose understanding
of the syntax I am not sure about ;-).  Between var=$another and
var=$another, the latter is slightly preferrable for the same
reason.

One questionable case is:

var=$(command with quoted parameter)

It makes it a bit harder to scan if there is an extra set of dq
around RHS, i.e.

var=$(command with quoted parameter)

That case is easier to read without dq around it, or you could cheat
by doing this:

var=$(command 'with quoted parameter')

In any case, as long as the result is correct, I do not have very
strong preference either way.
--
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