Re: [PATCH v2 1/4] mergetool--lib: Simplify command expressions
David Aguilar writes: > On Tue, Jan 29, 2013 at 11:22 AM, John Keeping wrote: >> On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote: >>> Update variable assignments to always use $(command "$arg") >>> in their RHS instead of "$(command "$arg")" as the latter >>> is harder to read. Make get_merge_tool_cmd() simpler by >>> avoiding "echo" and $(command) substitutions completely. >>> >>> Signed-off-by: David Aguilar >>> --- >>> @@ -300,9 +292,9 @@ get_merge_tool_path () { >>> fi >>> if test -z "$merge_tool_path" >>> then >>> - merge_tool_path="$(translate_merge_tool_path "$merge_tool")" >>> + merge_tool_path=$(translate_merge_tool_path "$merge_tool") >>> fi >>> - if test -z "$(get_merge_tool_cmd "$merge_tool")" && >>> + if test -z $(get_merge_tool_cmd "$merge_tool") && >> >> This change should be reverted to avoid calling "test -z" without any >> other arguments, as Johannes pointed out in v1. >> >> The rest of this patch looks good to me. > > You're right. My eyes have probably been staring at it too long and I > missed this (even though I thought I had checked). By now you (and people who were following this thread) are beginning to see why I said "I'd feel safer with extra dq" ;-) I'll amend locally and push the result out. -- 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 v2 1/4] mergetool--lib: Simplify command expressions
On Tue, Jan 29, 2013 at 11:22 AM, John Keeping wrote: > On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote: >> Update variable assignments to always use $(command "$arg") >> in their RHS instead of "$(command "$arg")" as the latter >> is harder to read. Make get_merge_tool_cmd() simpler by >> avoiding "echo" and $(command) substitutions completely. >> >> Signed-off-by: David Aguilar >> --- >> @@ -300,9 +292,9 @@ get_merge_tool_path () { >> fi >> if test -z "$merge_tool_path" >> then >> - merge_tool_path="$(translate_merge_tool_path "$merge_tool")" >> + merge_tool_path=$(translate_merge_tool_path "$merge_tool") >> fi >> - if test -z "$(get_merge_tool_cmd "$merge_tool")" && >> + if test -z $(get_merge_tool_cmd "$merge_tool") && > > This change should be reverted to avoid calling "test -z" without any > other arguments, as Johannes pointed out in v1. > > The rest of this patch looks good to me. You're right. My eyes have probably been staring at it too long and I missed this (even though I thought I had checked). Junio, how would you like these patches? Incrementals on top of da/mergetool-docs? I won't be able to get to them until later tonight (PST) at the earliest, though. -- 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 v2 1/4] mergetool--lib: Simplify command expressions
On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote: > Update variable assignments to always use $(command "$arg") > in their RHS instead of "$(command "$arg")" as the latter > is harder to read. Make get_merge_tool_cmd() simpler by > avoiding "echo" and $(command) substitutions completely. > > Signed-off-by: David Aguilar > --- > @@ -300,9 +292,9 @@ get_merge_tool_path () { > fi > if test -z "$merge_tool_path" > then > - merge_tool_path="$(translate_merge_tool_path "$merge_tool")" > + merge_tool_path=$(translate_merge_tool_path "$merge_tool") > fi > - if test -z "$(get_merge_tool_cmd "$merge_tool")" && > + if test -z $(get_merge_tool_cmd "$merge_tool") && This change should be reverted to avoid calling "test -z" without any other arguments, as Johannes pointed out in v1. The rest of this patch looks good to me. > ! type "$merge_tool_path" >/dev/null 2>&1 > then > echo >&2 "The $TOOL_MODE tool $merge_tool is not available as"\ -- 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
[PATCH v2 1/4] mergetool--lib: Simplify command expressions
Update variable assignments to always use $(command "$arg") in their RHS instead of "$(command "$arg")" as the latter is harder to read. Make get_merge_tool_cmd() simpler by avoiding "echo" and $(command) substitutions completely. Signed-off-by: David Aguilar --- I reworded the commit message to be more clear. git-mergetool--lib.sh | 40 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 1d0fb12..9a5aae9 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -32,17 +32,10 @@ check_unchanged () { fi } -valid_tool_config () { - if test -n "$(get_merge_tool_cmd "$1")" - then - return 0 - else - return 1 - fi -} - valid_tool () { - setup_tool "$1" || valid_tool_config "$1" + setup_tool "$1" && return 0 + cmd=$(get_merge_tool_cmd "$1") + test -n "$cmd" } setup_tool () { @@ -96,14 +89,13 @@ setup_tool () { } get_merge_tool_cmd () { - # Prints the custom command for a merge tool merge_tool="$1" if diff_mode then - echo "$(git config difftool.$merge_tool.cmd || - git config mergetool.$merge_tool.cmd)" + git config "difftool.$merge_tool.cmd" || + git config "mergetool.$merge_tool.cmd" else - echo "$(git config mergetool.$merge_tool.cmd)" + git config "mergetool.$merge_tool.cmd" fi } @@ -114,7 +106,7 @@ run_merge_tool () { GIT_PREFIX=${GIT_PREFIX:-.} export GIT_PREFIX - merge_tool_path="$(get_merge_tool_path "$1")" || exit + merge_tool_path=$(get_merge_tool_path "$1") || exit base_present="$2" status=0 @@ -145,7 +137,7 @@ run_merge_tool () { # Run a either a configured or built-in diff tool run_diff_cmd () { - merge_tool_cmd="$(get_merge_tool_cmd "$1")" + merge_tool_cmd=$(get_merge_tool_cmd "$1") if test -n "$merge_tool_cmd" then ( eval $merge_tool_cmd ) @@ -158,11 +150,11 @@ run_diff_cmd () { # Run a either a configured or built-in merge tool run_merge_cmd () { - merge_tool_cmd="$(get_merge_tool_cmd "$1")" + merge_tool_cmd=$(get_merge_tool_cmd "$1") if test -n "$merge_tool_cmd" then - trust_exit_code="$(git config --bool \ - mergetool."$1".trustExitCode || echo false)" + trust_exit_code=$(git config --bool \ + "mergetool.$1.trustExitCode" || echo false) if test "$trust_exit_code" = "false" then touch "$BACKUP" @@ -253,7 +245,7 @@ guess_merge_tool () { # Loop over each candidate and stop when a valid merge tool is found. for i in $tools do - merge_tool_path="$(translate_merge_tool_path "$i")" + merge_tool_path=$(translate_merge_tool_path "$i") if type "$merge_tool_path" >/dev/null 2>&1 then echo "$i" @@ -300,9 +292,9 @@ get_merge_tool_path () { fi if test -z "$merge_tool_path" then - merge_tool_path="$(translate_merge_tool_path "$merge_tool")" + merge_tool_path=$(translate_merge_tool_path "$merge_tool") fi - if test -z "$(get_merge_tool_cmd "$merge_tool")" && + if test -z $(get_merge_tool_cmd "$merge_tool") && ! type "$merge_tool_path" >/dev/null 2>&1 then echo >&2 "The $TOOL_MODE tool $merge_tool is not available as"\ @@ -314,11 +306,11 @@ get_merge_tool_path () { get_merge_tool () { # Check if a merge tool has been configured - merge_tool="$(get_configured_merge_tool)" + merge_tool=$(get_configured_merge_tool) # Try to guess an appropriate merge tool if no tool has been set. if test -z "$merge_tool" then - merge_tool="$(guess_merge_tool)" || exit + merge_tool=$(guess_merge_tool) || exit fi echo "$merge_tool" } -- 1.8.0.13.g3ff16bb -- 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