From: David Aguilar <hidden> Date: 2016-06-15 22:55:55
This is round two of this series.
I think this touched on everything brought up in the code review.
4/4 could use a review as I'm not completely familiar with the
makefile dependencies, though it seems to work correctly.
David Aguilar (4):
mergetool--lib: Simplify command expressions
mergetool--lib: Improve the help text in guess_merge_tool()
mergetool--lib: Add functions for finding available tools
doc: Generate a list of valid merge tools
Documentation/.gitignore | 1 +
Documentation/Makefile | 22 +++++++-
Documentation/diff-config.txt | 13 ++---
Documentation/merge-config.txt | 12 ++---
git-mergetool--lib.sh | 116 ++++++++++++++++++++++-------------------
5 files changed, 96 insertions(+), 68 deletions(-)
--
1.8.0.13.g3ff16bb
From: David Aguilar <hidden> Date: 2016-06-15 22:55:55
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 <redacted>
---
I reworded the commit message to be more clear.
git-mergetool--lib.sh | 40 ++++++++++++++++------------------------
1 file changed, 16 insertions(+), 24 deletions(-)
@@ -96,14 +89,13 @@ setup_tool () {} get_merge_tool_cmd(){-# Prints the custom command for a merge toolmerge_tool="$1"ifdiff_modethen-echo"$(gitconfigdifftool.$merge_tool.cmd||-gitconfigmergetool.$merge_tool.cmd)"+gitconfig"difftool.$merge_tool.cmd"||+gitconfig"mergetool.$merge_tool.cmd"else-echo"$(gitconfigmergetool.$merge_tool.cmd)"+gitconfig"mergetool.$merge_tool.cmd"fi}
@@ -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")iftest-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")iftest-n"$merge_tool_cmd"then-trust_exit_code="$(gitconfig--bool\-mergetool."$1".trustExitCode||echofalse)"+trust_exit_code=$(gitconfig--bool\+"mergetool.$1.trustExitCode"||echofalse)iftest"$trust_exit_code"="false"thentouch"$BACKUP"
@@ -253,7 +245,7 @@ guess_merge_tool () {# Loop over each candidate and stop when a valid merge tool is found.foriin$toolsdo-merge_tool_path="$(translate_merge_tool_path"$i")"+merge_tool_path=$(translate_merge_tool_path"$i")iftype"$merge_tool_path">/dev/null2>&1thenecho"$i"
@@ -300,9 +292,9 @@ get_merge_tool_path () {fiiftest-z"$merge_tool_path"then-merge_tool_path="$(translate_merge_tool_path"$merge_tool")"+merge_tool_path=$(translate_merge_tool_path"$merge_tool")fi-iftest-z"$(get_merge_tool_cmd"$merge_tool")"&&+iftest-z$(get_merge_tool_cmd"$merge_tool")&&!type"$merge_tool_path">/dev/null2>&1thenecho>&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.iftest-z"$merge_tool"then-merge_tool="$(guess_merge_tool)"||exit+merge_tool=$(guess_merge_tool)||exitfiecho"$merge_tool"}
From: David Aguilar <hidden> Date: 2016-06-15 22:55:55
Refactor show_tool_help() so that the tool-finding logic is broken out
into a separate show_tool_names() function.
Signed-off-by: David Aguilar <redacted>
---
filter_tools renamed to show_tool_names() and simplfied
to use ls -1. show_tool_names() now has a preamble as discussed.
git-mergetool--lib.sh | 68 +++++++++++++++++++++++++++++----------------------
1 file changed, 39 insertions(+), 29 deletions(-)
@@ -2,6 +2,35 @@# git-mergetool--lib is a library for common merge tool functionsMERGE_TOOLS_DIR=$(git--exec-path)/mergetools+mode_ok(){+diff_mode&&can_diff||+merge_mode&&can_merge+}++is_available(){+merge_tool_path=$(translate_merge_tool_path"$1")&&+type"$merge_tool_path">/dev/null2>&1+}++show_tool_names(){+condition=${1:-true}per_line_prefix=${2:-}preamble=${3:-}++(cd"$MERGE_TOOLS_DIR"&&ls-1*)|+whilereadtoolname+do+ifsetup_tool"$toolname"2>/dev/null&&+(eval"$condition""$toolname")+then+iftest-n"$preamble"+then+echo"$preamble"+preamble=+fi+printf"%s%s\n""$per_line_prefix""$tool"+fi+done+}+ diff_mode(){test"$TOOL_MODE"=diff}
@@ -199,35 +228,21 @@ list_merge_tool_candidates () {} show_tool_help(){-unavailable=available=LF='-'-foriin"$MERGE_TOOLS_DIR"/*-do-tool=$(basename"$i")-setup_tool"$tool"2>/dev/null||continue--merge_tool_path=$(translate_merge_tool_path"$tool")-iftype"$merge_tool_path">/dev/null2>&1-then-available="$available$tool$LF"-else-unavailable="$unavailable$tool$LF"-fi-done--cmd_name=${TOOL_MODE}tool+tool_opt="'git ${TOOL_MODE}tool --tool-<tool>'"+available=$(show_tool_names'mode_ok && is_available''\t\t'\+"$tool_opt may be set to one of the following:")+unavailable=$(show_tool_names'mode_ok && ! is_available''\t\t'\+"The following tools are valid, but not currently available:")iftest-n"$available"then-echo"'git $cmd_name --tool=<tool>' may be set to one of the following:"-echo"$available"|sort|sed-e's/^/ /'+echo"$available"elseecho"No suitable tool for 'git $cmd_name --tool=<tool>' found."fiiftest-n"$unavailable"thenecho-echo'The following tools are valid, but not currently available:'-echo"$unavailable"|sort|sed-e's/^/ /'+echo"$unavailable"fiiftest-n"$unavailable$available"then
@@ -248,17 +263,12 @@ See 'git ${TOOL_MODE}tool --tool-help' or 'git help config' for more details.$tools EOF# Loop over each candidate and stop when a valid merge tool is found.-foriin$tools+fortoolin$toolsdo-merge_tool_path=$(translate_merge_tool_path"$i")-iftype"$merge_tool_path">/dev/null2>&1-then-echo"$i"-return0-fi+is_available"$tool"&&echo"$tool"&&return0done-echo>&2"No known merge resolution program available."+echo>&2"No known ${TOOL_MODE} tool is available."return1}
From: David Aguilar <hidden> Date: 2016-06-15 22:55:55
This code path is only activated when the user does not have a valid
configured tool. Add a message to guide new users towards configuring a
default tool.
Signed-off-by: David Aguilar <redacted>
---
This now uses a cat << here-doc.
git-mergetool--lib.sh | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
@@ -240,8 +240,13 @@ show_tool_help () { guess_merge_tool(){list_merge_tool_candidates-echo>&2"merge tool candidates: $tools"+cat>&2<<-EOF+Thismessageisdisplayedbecause'$TOOL_MODE.tool'isnotconfigured.+See'git ${TOOL_MODE}tool --tool-help'or'git help config'formoredetails.+'git ${TOOL_MODE}tool'willnowattempttouseoneofthefollowingtools:+$tools+EOF# Loop over each candidate and stop when a valid merge tool is found.foriin$toolsdo
From: David Aguilar <hidden> Date: 2016-06-15 22:55:55
Use the show_tool_names() function to build lists of all
the built-in tools supported by difftool and mergetool.
This frees us from needing to update the documentation
whenever a new tool is added.
Signed-off-by: David Aguilar <redacted>
---
Adjusted to use show_tool_names() and reworked the makefile dependencies.
I could use another set of eyes on the Makefile..
Documentation/.gitignore | 1 +
Documentation/Makefile | 22 ++++++++++++++++++++--
Documentation/diff-config.txt | 13 +++++++------
Documentation/merge-config.txt | 12 ++++++------
git-mergetool--lib.sh | 3 ++-
5 files changed, 36 insertions(+), 15 deletions(-)
@@ -132,9 +132,10 @@ diff.<driver>.cachetextconv:: conversion outputs. See linkgit:gitattributes[5] for details. diff.tool::- The diff tool to be used by linkgit:git-difftool[1]. This- option overrides `merge.tool`, and has the same valid built-in- values as `merge.tool` minus "tortoisemerge" and plus- "kompare". Any other value is treated as a custom diff tool,- and there must be a corresponding `difftool.<tool>.cmd`- option.+ Controls which diff tool is used by linkgit:git-difftool[1].+ This variable overrides the value configured in `merge.tool`.+ The list below shows the valid built-in values.+ Any other value is treated as a custom diff tool and requires+ that a corresponding difftool.<tool>.cmd variable is defined.++include::mergetools-diff.txt[]
@@ -52,12 +52,12 @@ merge.stat:: at the end of the merge. True by default. merge.tool::- Controls which merge resolution program is used by- linkgit:git-mergetool[1]. Valid built-in values are: "araxis",- "bc3", "diffuse", "ecmerge", "emerge", "gvimdiff", "kdiff3", "meld",- "opendiff", "p4merge", "tkdiff", "tortoisemerge", "vimdiff"- and "xxdiff". Any other value is treated is custom merge tool- and there must be a corresponding mergetool.<tool>.cmd option.+ Controls which merge tool is used by linkgit:git-mergetool[1].+ The list below shows the valid built-in values.+ Any other value is treated as a custom merge tool and requires+ that a corresponding mergetool.<tool>.cmd variable is defined.++include::mergetools-merge.txt[] merge.verbosity:: Controls the amount of output shown by the recursive merge
@@ -1,6 +1,7 @@#!/bin/sh# git-mergetool--lib is a library for common merge tool functions-MERGE_TOOLS_DIR=$(git--exec-path)/mergetools++:${MERGE_TOOLS_DIR=$(git--exec-path)/mergetools} mode_ok(){diff_mode&&can_diff||
From: Philip Oakley <hidden> Date: 2016-06-15 22:55:56
From: "David Aguilar" <redacted>
Sent: Monday, January 28, 2013 12:52 AM
This is round two of this series.
I think this touched on everything brought up in the code review.
4/4 could use a review as I'm not completely familiar with the
makefile dependencies, though it seems to work correctly.
Does this 4/4 have any effect on the Msysgit / Git for Windows
documentation which simply refers [IIRC] to HTML documenation made by
Junio?
That is, how easy is it to create a 'default' set of docs, rather than
personalised documenation. Or have I misunderstood how it is working?
David Aguilar (4):
mergetool--lib: Simplify command expressions
mergetool--lib: Improve the help text in guess_merge_tool()
mergetool--lib: Add functions for finding available tools
doc: Generate a list of valid merge tools
Documentation/.gitignore | 1 +
Documentation/Makefile | 22 +++++++-
Documentation/diff-config.txt | 13 ++---
Documentation/merge-config.txt | 12 ++---
git-mergetool--lib.sh | 116
++++++++++++++++++++++-------------------
5 files changed, 96 insertions(+), 68 deletions(-)
--
1.8.0.13.g3ff16bb
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
-----
No virus found in this message.
Checked by AVG - www.avg.com
Version: 2013.0.2890 / Virus Database: 2639/6061 - Release Date:
01/27/13
From: David Aguilar <hidden> Date: 2016-06-15 22:55:56
On Mon, Jan 28, 2013 at 12:20 AM, Philip Oakley [off-list ref] wrote:
From: "David Aguilar" <redacted>
Sent: Monday, January 28, 2013 12:52 AM
quoted
This is round two of this series.
I think this touched on everything brought up in the code review.
4/4 could use a review as I'm not completely familiar with the
makefile dependencies, though it seems to work correctly.
Does this 4/4 have any effect on the Msysgit / Git for Windows documentation
which simply refers [IIRC] to HTML documenation made by Junio?
That is, how easy is it to create a 'default' set of docs, rather than
personalised documenation. Or have I misunderstood how it is working?
It doesn't have any effect on Msysgit. The resulting documentation
lists all available tools, on all platforms.
--
David
From: John Keeping <hidden> Date: 2016-06-15 22:55:56
On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:
quoted hunk
Refactor show_tool_help() so that the tool-finding logic is broken out
into a separate show_tool_names() function.
Signed-off-by: David Aguilar <redacted>
---
filter_tools renamed to show_tool_names() and simplfied
to use ls -1. show_tool_names() now has a preamble as discussed.
git-mergetool--lib.sh | 68 +++++++++++++++++++++++++++++----------------------
1 file changed, 39 insertions(+), 29 deletions(-)
@@ -2,6 +2,35 @@# git-mergetool--lib is a library for common merge tool functionsMERGE_TOOLS_DIR=$(git--exec-path)/mergetools+mode_ok(){+diff_mode&&can_diff||+merge_mode&&can_merge+}++is_available(){+merge_tool_path=$(translate_merge_tool_path"$1")&&+type"$merge_tool_path">/dev/null2>&1+}+
Can we move show_tool_names() to be above show_tool_help()? It's a
very minor nit but I prefer having related functionality grouped
together.
Would this be better with one value on each line? Also perhaps
per_line_prefix -> line_prefix.
+
+ ( cd "$MERGE_TOOLS_DIR" && ls -1 * ) |
+ while read toolname
+ do
+ if setup_tool "$toolname" 2>/dev/null &&
+ (eval "$condition" "$toolname")
+ then
+ if test -n "$preamble"
+ then
+ echo "$preamble"
+ preamble=
+ fi
+ printf "%s%s\n" "$per_line_prefix" "$tool"
This needs to be:
printf "$per_line_prefix%s\n" "$tool"
since $per_line_prefix is usually '\t\t' which isn't expanded if we
format it with %s - an alternative would be to change the value passed
in to '$TAB$TAB' with literal tabs.
+ fi
+ done
+}
+
diff_mode() {
test "$TOOL_MODE" = diff
}
From: Philip Oakley <hidden> Date: 2016-06-15 22:55:56
From: "David Aguilar" <redacted>
Sent: Monday, January 28, 2013 9:16 AM
On Mon, Jan 28, 2013 at 12:20 AM, Philip Oakley [off-list ref]
wrote:
quoted
From: "David Aguilar" <redacted>
Sent: Monday, January 28, 2013 12:52 AM
quoted
This is round two of this series.
I think this touched on everything brought up in the code review.
4/4 could use a review as I'm not completely familiar with the
makefile dependencies, though it seems to work correctly.
Does this 4/4 have any effect on the Msysgit / Git for Windows
documentation
which simply refers [IIRC] to HTML documenation made by Junio?
That is, how easy is it to create a 'default' set of docs, rather
than
personalised documenation. Or have I misunderstood how it is working?
It doesn't have any effect on Msysgit. The resulting documentation
lists all available tools, on all platforms.
That's useful to know. I must have misunderstood one of the earlier
messages suggesting it would also list all the users other (non typical)
installed mergetools and hence add them into the documentation.
Philip
From: John Keeping <hidden> Date: 2016-06-15 22:55:57
On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote:
quoted hunk
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 <redacted>
---
@@ -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"\
@@ -2,6 +2,35 @@# git-mergetool--lib is a library for common merge tool functionsMERGE_TOOLS_DIR=$(git--exec-path)/mergetools+mode_ok(){+diff_mode&&can_diff||+merge_mode&&can_merge+}++is_available(){+merge_tool_path=$(translate_merge_tool_path"$1")&&+type"$merge_tool_path">/dev/null2>&1+}++show_tool_names(){+condition=${1:-true}per_line_prefix=${2:-}preamble=${3:-}++(cd"$MERGE_TOOLS_DIR"&&ls-1*)|
Is the '*' necessary here? I would expect ls to list the current
directory if given no arguments, but perhaps some platforms behave
differently?
quoted hunk
+ while read toolname
+ do
+ if setup_tool "$toolname" 2>/dev/null &&
+ (eval "$condition" "$toolname")
+ then
+ if test -n "$preamble"
+ then
+ echo "$preamble"
+ preamble=
+ fi
+ printf "%s%s\n" "$per_line_prefix" "$tool"
+ fi
+ done
+}
+
diff_mode() {
test "$TOOL_MODE" = diff
}
@@ -199,35 +228,21 @@ list_merge_tool_candidates () { } show_tool_help () {- unavailable= available= LF='-'- for i in "$MERGE_TOOLS_DIR"/*- do- tool=$(basename "$i")- setup_tool "$tool" 2>/dev/null || continue-- merge_tool_path=$(translate_merge_tool_path "$tool")- if type "$merge_tool_path" >/dev/null 2>&1- then- available="$available$tool$LF"- else- unavailable="$unavailable$tool$LF"- fi- done-- cmd_name=${TOOL_MODE}tool+ tool_opt="'git ${TOOL_MODE}tool --tool-<tool>'"+ available=$(show_tool_names 'mode_ok && is_available' '\t\t' \+ "$tool_opt may be set to one of the following:")+ unavailable=$(show_tool_names 'mode_ok && ! is_available' '\t\t' \+ "The following tools are valid, but not currently available:") if test -n "$available" then- echo "'git $cmd_name --tool=<tool>' may be set to one of the following:"- echo "$available" | sort | sed -e 's/^/ /'+ echo "$available" else echo "No suitable tool for 'git $cmd_name --tool=<tool>' found." fi if test -n "$unavailable" then echo- echo 'The following tools are valid, but not currently available:'- echo "$unavailable" | sort | sed -e 's/^/ /'+ echo "$unavailable" fi if test -n "$unavailable$available" then
You haven't taken full advantage of the simplification Junio suggested
in response to v1 here. We can change the "unavailable" block to be:
show_tool_names 'mode_ok && ! is_available' "$TAB$TAB" \
"${LF}The following tools are valid, but not currently available:"
If you also add a "not_found_msg" parameter to show_tool_names then the
"available" case is also simplified:
show_tool_names 'mode_ok && is_available' "$TAB$TAB" \
"$tool_opt may be set to one of the following:" \
"No suitable tool for 'git $cmd_name --tool=<tool>' found."
with this at the end of show_tool_names:
test -n "$preamble" && test -n "$not_found_msg" && \
echo "$not_found_msg"
John
From: David Aguilar <hidden> Date: 2016-06-15 22:55:57
On Tue, Jan 29, 2013 at 11:22 AM, John Keeping [off-list ref] wrote:
On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote:
quoted
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 <redacted>
---
@@ -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