Re: [PATCH 1/4] mergetool--lib: Simplify command expressions

2013-01-27 Thread Johannes Sixt
Am 27.01.2013 22:24, schrieb David Aguilar:
 Use $(command $arg) instead of $(command $arg) as the latter is
 harder to read.

If at all, you should restrict yourself to simplify only variable
assignments. Because this case:

 - if test -z $(get_merge_tool_cmd $merge_tool) 
 + if test -z $(get_merge_tool_cmd $merge_tool) 

cannot work as intended: If the output of $() is empty, then without the
outer quotes this becomes

  test -z

without an operand for -z, which is a syntax error (of the test command).

-- Hannes

--
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 1/4] mergetool--lib: Simplify command expressions

2013-01-27 Thread David Aguilar
On Sun, Jan 27, 2013 at 2:08 PM, Johannes Sixt j...@kdbg.org wrote:
 Am 27.01.2013 22:24, schrieb David Aguilar:
 Use $(command $arg) instead of $(command $arg) as the latter is
 harder to read.

 If at all, you should restrict yourself to simplify only variable
 assignments. Because this case:

 - if test -z $(get_merge_tool_cmd $merge_tool) 
 + if test -z $(get_merge_tool_cmd $merge_tool) 

 cannot work as intended: If the output of $() is empty, then without the
 outer quotes this becomes

   test -z

 without an operand for -z, which is a syntax error (of the test command).

Definitely.  I learned this the hard way when the tests broke on me while
working it ;-)  My patch rewrites things to always use var=$(command)
expressions with separate test $var evaluating them.
Thanks for the tip,
-- 
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 1/4] mergetool--lib: Simplify command expressions

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

 Use $(command $arg) instead of $(command $arg) as the latter is
 harder to read.

Did you miss my comment that this is about RHS of an assignment?
--
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 1/4] mergetool--lib: Simplify command expressions

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

 Definitely.  I learned this the hard way when the tests broke on me while
 working it ;-)  My patch rewrites things to always use var=$(command)
 expressions with separate test $var evaluating them.

OK; that wasn't clear from the log message.

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