[v7,4/4] vimdiff: add description to already existing diff/merge tools

Commit Message

Fernando Ramos March 28, 2022, 10:30 p.m. UTC

---
 mergetools/araxis        | 8 ++++++++
 mergetools/bc            | 8 ++++++++
 mergetools/codecompare   | 8 ++++++++
 mergetools/deltawalker   | 8 ++++++++
 mergetools/diffmerge     | 8 ++++++++
 mergetools/diffuse       | 8 ++++++++
 mergetools/ecmerge       | 8 ++++++++
 mergetools/emerge        | 8 ++++++++
 mergetools/examdiff      | 8 ++++++++
 mergetools/guiffy        | 8 ++++++++
 mergetools/kdiff3        | 8 ++++++++
 mergetools/kompare       | 8 ++++++++
 mergetools/meld          | 8 ++++++++
 mergetools/opendiff      | 8 ++++++++
 mergetools/p4merge       | 8 ++++++++
 mergetools/smerge        | 8 ++++++++
 mergetools/tkdiff        | 8 ++++++++
 mergetools/tortoisemerge | 8 ++++++++
 mergetools/winmerge      | 8 ++++++++
 mergetools/xxdiff        | 8 ++++++++
 20 files changed, 160 insertions(+)

Comments

Junio C Hamano March 29, 2022, 4:38 p.m. UTC | #1

Fernando Ramos <[e-mail]> writes:

> ---

Missing log message and sign off.  "add description" tells us what
it did, which is easily visible in the patch text already.  The log
message should say why we are adding them, and the rationale has to
be better than "adding is better than not adding".  E.g. "in output
of X and Y, we only show the names without explanation on what they
are, which is not helpful enough" would be a helpful log message.

Doesn't the change in [3/4] to include these strings in generated
mergetools-*.txt file depend on this in place?

Thanks.

Philippe Blain March 29, 2022, 5:24 p.m. UTC | #2

Hi Junio, Fernando,

Le 2022-03-29 à 12:38, Junio C Hamano a écrit :
> Fernando Ramos <[e-mail]> writes:
> 
>> ---
> 
> Missing log message and sign off.  "add description" tells us what
> it did, which is easily visible in the patch text already.  The log
> message should say why we are adding them, and the rationale has to
> be better than "adding is better than not adding".  E.g. "in output
> of X and Y, we only show the names without explanation on what they
> are, which is not helpful enough" would be a helpful log message.

I might add that the prefix of the commit message title should be changed
to 'mergetools: ' 

> 
> Doesn't the change in [3/4] to include these strings in generated
> mergetools-*.txt file depend on this in place?
> 
> Thanks.
> 

The list of available values are already generated in the  mergetools-diff.txt and
mergetools-merge.txt files before this series. After 3/4 we also
include the description of the values. In 3/4 descriptions are only added for
vimdiff and friends, so the rest of the tools would simply be listed without 
descriptions. After 4/4 all tools have a description.

Junio C Hamano March 29, 2022, 6:50 p.m. UTC | #3

Philippe Blain <[e-mail]> writes:

> Hi Junio, Fernando,
>
> Le 2022-03-29 à 12:38, Junio C Hamano a écrit :
>> Fernando Ramos <[e-mail]> writes:
>> 
>>> ---
>> 
>> Missing log message and sign off.  "add description" tells us what
>> it did, which is easily visible in the patch text already.  The log
>> message should say why we are adding them, and the rationale has to
>> be better than "adding is better than not adding".  E.g. "in output
>> of X and Y, we only show the names without explanation on what they
>> are, which is not helpful enough" would be a helpful log message.
>
> I might add that the prefix of the commit message title should be changed
> to 'mergetools: ' 
>
>> 
>> Doesn't the change in [3/4] to include these strings in generated
>> mergetools-*.txt file depend on this in place?
>> 
>> Thanks.
>> 
>
> The list of available values are already generated in the  mergetools-diff.txt and
> mergetools-merge.txt files before this series. After 3/4 we also
> include the description of the values. In 3/4 descriptions are only added for
> vimdiff and friends, so the rest of the tools would simply be listed without 
> descriptions. After 4/4 all tools have a description.

OK.  I wonder how that affects our discussion to switch to the
description list?  This adds what appears as the description.
Before this step, it would be <dl>...</dl> inside which <dt> appears
but not <dd>.

Fernando Ramos March 29, 2022, 10:39 p.m. UTC | #4

I'll prepare v8 with all the previously discussed changes so that we can
continue the discussion there :)

Patch

diff --git a/mergetools/araxis b/mergetools/araxis
index e2407b65b7..eb32a7da95 100644
--- a/mergetools/araxis
+++ b/mergetools/araxis
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" -wait -2 "$LOCAL" "$REMOTE" >/dev/null 2>&1
 }
 
+diff_cmd_help () {
+	echo "Use Araxis Merge (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -13,6 +17,10 @@  merge_cmd () {
 	fi
 }
 
+merge_cmd_help () {
+	echo "Use Araxis Merge (requires a graphical session)"
+}
+
 translate_merge_tool_path() {
 	echo compare
 }
diff --git a/mergetools/bc b/mergetools/bc
index 26c19d46a5..2922667ddd 100644
--- a/mergetools/bc
+++ b/mergetools/bc
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE"
 }
 
+diff_cmd_help () {
+	echo "Use Beyond Compare (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -13,6 +17,10 @@  merge_cmd () {
 	fi
 }
 
+merge_cmd_help () {
+	echo "Use Beyond Compare (requires a graphical session)"
+}
+
 translate_merge_tool_path() {
 	if type bcomp >/dev/null 2>/dev/null
 	then
diff --git a/mergetools/codecompare b/mergetools/codecompare
index 9f60e8da65..610963d377 100644
--- a/mergetools/codecompare
+++ b/mergetools/codecompare
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE"
 }
 
+diff_cmd_help () {
+	echo "Use Code Compare (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -13,6 +17,10 @@  merge_cmd () {
 	fi
 }
 
+merge_cmd_help () {
+	echo "Use Code Compare (requires a graphical session)"
+}
+
 translate_merge_tool_path() {
 	if merge_mode
 	then
diff --git a/mergetools/deltawalker b/mergetools/deltawalker
index ee6f374bce..efae4c285c 100644
--- a/mergetools/deltawalker
+++ b/mergetools/deltawalker
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE" >/dev/null 2>&1
 }
 
+diff_cmd_help () {
+	echo "Use DeltaWalker (requires a graphical session)"
+}
+
 merge_cmd () {
 	# Adding $(pwd)/ in front of $MERGED should not be necessary.
 	# However without it, DeltaWalker (at least v1.9.8 on Windows)
@@ -16,6 +20,10 @@  merge_cmd () {
 	fi >/dev/null 2>&1
 }
 
+merge_cmd_help () {
+	echo "Use DeltaWalker (requires a graphical session)"
+}
+
 translate_merge_tool_path () {
 	echo DeltaWalker
 }
diff --git a/mergetools/diffmerge b/mergetools/diffmerge
index 9b6355b98a..9b5b62d1ca 100644
--- a/mergetools/diffmerge
+++ b/mergetools/diffmerge
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE" >/dev/null 2>&1
 }
 
+diff_cmd_help () {
+	echo "Use DiffMerge (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -13,6 +17,10 @@  merge_cmd () {
 	fi
 }
 
+merge_cmd_help () {
+	echo "Use DiffMerge (requires a graphical session)"
+}
+
 exit_code_trustable () {
 	true
 }
diff --git a/mergetools/diffuse b/mergetools/diffuse
index 5a3ae8b569..ebfaba5172 100644
--- a/mergetools/diffuse
+++ b/mergetools/diffuse
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
 }
 
+diff_cmd_help () {
+	echo "Use Diffuse (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -13,3 +17,7 @@  merge_cmd () {
 			"$LOCAL" "$MERGED" "$REMOTE" | cat
 	fi
 }
+
+merge_cmd_help () {
+	echo "Use Diffuse (requires a graphical session)"
+}
diff --git a/mergetools/ecmerge b/mergetools/ecmerge
index 6c5101c4f7..0d4d609874 100644
--- a/mergetools/ecmerge
+++ b/mergetools/ecmerge
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" --default --mode=diff2 "$LOCAL" "$REMOTE"
 }
 
+diff_cmd_help () {
+	echo "Use ECMerge (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -12,3 +16,7 @@  merge_cmd () {
 			--default --mode=merge2 --to="$MERGED"
 	fi
 }
+
+merge_cmd_help () {
+	echo "Use ECMerge (requires a graphical session)"
+}
diff --git a/mergetools/emerge b/mergetools/emerge
index d1ce513ff5..fc6892cc95 100644
--- a/mergetools/emerge
+++ b/mergetools/emerge
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" -f emerge-files-command "$LOCAL" "$REMOTE"
 }
 
+diff_cmd_help () {
+	echo "Use Emacs' Emerge"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -17,6 +21,10 @@  merge_cmd () {
 	fi
 }
 
+merge_cmd_help () {
+	echo "Use Emacs' Emerge"
+}
+
 translate_merge_tool_path() {
 	echo emacs
 }
diff --git a/mergetools/examdiff b/mergetools/examdiff
index e72b06fc4d..6f53ca9161 100644
--- a/mergetools/examdiff
+++ b/mergetools/examdiff
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE" -nh
 }
 
+diff_cmd_help () {
+	echo "Use ExamDiff Pro (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -11,6 +15,10 @@  merge_cmd () {
 	fi
 }
 
+merge_cmd_help () {
+	echo "Use ExamDiff Pro (requires a graphical session)"
+}
+
 translate_merge_tool_path() {
 	mergetool_find_win32_cmd "ExamDiff.com" "ExamDiff Pro"
 }
diff --git a/mergetools/guiffy b/mergetools/guiffy
index 8b23a13c41..3ed07efd16 100644
--- a/mergetools/guiffy
+++ b/mergetools/guiffy
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE"
 }
 
+diff_cmd_help () {
+	echo "Use Guiffy's Diff Tool (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -13,6 +17,10 @@  merge_cmd () {
 	fi
 }
 
+merge_cmd_help () {
+	echo "Use Guiffy's Diff Tool (requires a graphical session)"
+}
+
 exit_code_trustable () {
 	true
 }
diff --git a/mergetools/kdiff3 b/mergetools/kdiff3
index 520cb914a1..ee8b3a0570 100644
--- a/mergetools/kdiff3
+++ b/mergetools/kdiff3
@@ -4,6 +4,10 @@  diff_cmd () {
 		"$LOCAL" "$REMOTE" >/dev/null 2>&1
 }
 
+diff_cmd_help () {
+	echo "Use KDiff3 (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -22,6 +26,10 @@  merge_cmd () {
 	fi
 }
 
+merge_cmd_help () {
+	echo "Use KDiff3 (requires a graphical session)"
+}
+
 exit_code_trustable () {
 	true
 }
diff --git a/mergetools/kompare b/mergetools/kompare
index e8c0bfa678..4ce23dbe8b 100644
--- a/mergetools/kompare
+++ b/mergetools/kompare
@@ -2,10 +2,18 @@  can_merge () {
 	return 1
 }
 
+diff_cmd_help () {
+	echo "Use Kompare (requires a graphical session)"
+}
+
 diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE"
 }
 
+merge_cmd_help () {
+	echo "Use Kompare (requires a graphical session)"
+}
+
 exit_code_trustable () {
 	true
 }
diff --git a/mergetools/meld b/mergetools/meld
index aab4ebb935..8ec0867e03 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE"
 }
 
+diff_cmd_help () {
+	echo "Use Meld (requires a graphical session)"
+}
+
 merge_cmd () {
 	check_meld_for_features
 
@@ -20,6 +24,10 @@  merge_cmd () {
 	fi
 }
 
+merge_cmd_help () {
+	echo "Use Meld (requires a graphical session) with optional \`auto merge\` (see \`git help mergetool\`'s \`CONFIGURATION\` section)"
+}
+
 # Get meld help message
 init_meld_help_msg () {
 	if test -z "$meld_help_msg"
diff --git a/mergetools/opendiff b/mergetools/opendiff
index b608dd6de3..44adf8f951 100644
--- a/mergetools/opendiff
+++ b/mergetools/opendiff
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
 }
 
+diff_cmd_help () {
+	echo "Use FileMerge (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -12,3 +16,7 @@  merge_cmd () {
 			-merge "$MERGED" | cat
 	fi
 }
+
+merge_cmd_help () {
+	echo "Use FileMerge (requires a graphical session)"
+}
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 7a5b291dd2..f3cb197e58 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -19,6 +19,10 @@  diff_cmd () {
 	fi
 }
 
+diff_cmd_help () {
+	echo "Use HelixCore P4Merge (requires a graphical session)"
+}
+
 merge_cmd () {
 	if ! $base_present
 	then
@@ -34,3 +38,7 @@  create_empty_file () {
 
 	printf "%s" "$empty_file"
 }
+
+merge_cmd_help () {
+	echo "Use HelixCore P4Merge (requires a graphical session)"
+}
diff --git a/mergetools/smerge b/mergetools/smerge
index 9c2e6f6fd7..5410835a6b 100644
--- a/mergetools/smerge
+++ b/mergetools/smerge
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" mergetool "$LOCAL" "$REMOTE" -o "$MERGED"
 }
 
+diff_cmd_help () {
+	echo "Use Sublime Merge (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -10,3 +14,7 @@  merge_cmd () {
 		"$merge_tool_path" mergetool "$LOCAL" "$REMOTE" -o "$MERGED"
 	fi
 }
+
+merge_cmd_help () {
+	echo "Use Sublime Merge (requires a graphical session)"
+}
diff --git a/mergetools/tkdiff b/mergetools/tkdiff
index eee5cb57e3..66906a720d 100644
--- a/mergetools/tkdiff
+++ b/mergetools/tkdiff
@@ -2,6 +2,10 @@  diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE"
 }
 
+diff_cmd_help () {
+	echo "Use TkDiff (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -14,3 +18,7 @@  merge_cmd () {
 exit_code_trustable () {
 	true
 }
+
+merge_cmd_help () {
+	echo "Use TkDiff (requires a graphical session)"
+}
diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index d7ab666a59..507edcd444 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -2,6 +2,10 @@  can_diff () {
 	return 1
 }
 
+diff_cmd_help () {
+	echo "Use TortoiseMerge (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -30,3 +34,7 @@  translate_merge_tool_path() {
 		echo tortoisemerge
 	fi
 }
+
+merge_cmd_help () {
+	echo "Use TortoiseMerge (requires a graphical session)"
+}
diff --git a/mergetools/winmerge b/mergetools/winmerge
index 74d03259fd..36c72dde6e 100644
--- a/mergetools/winmerge
+++ b/mergetools/winmerge
@@ -3,6 +3,10 @@  diff_cmd () {
 	return 0
 }
 
+diff_cmd_help () {
+	echo "Use WinMerge (requires a graphical session)"
+}
+
 merge_cmd () {
 	# mergetool.winmerge.trustExitCode is implicitly false.
 	# touch $BACKUP so that we can check_unchanged.
@@ -13,3 +17,7 @@  merge_cmd () {
 translate_merge_tool_path() {
 	mergetool_find_win32_cmd "WinMergeU.exe" "WinMerge"
 }
+
+merge_cmd_help () {
+	echo "Use WinMerge (requires a graphical session)"
+}
diff --git a/mergetools/xxdiff b/mergetools/xxdiff
index d5ce467995..cd205f9842 100644
--- a/mergetools/xxdiff
+++ b/mergetools/xxdiff
@@ -12,6 +12,10 @@  diff_cmd () {
 	fi
 }
 
+diff_cmd_help () {
+	echo "Use xxdiff (requires a graphical session)"
+}
+
 merge_cmd () {
 	if $base_present
 	then
@@ -28,3 +32,7 @@  merge_cmd () {
 			--merged-file "$MERGED" "$LOCAL" "$REMOTE"
 	fi
 }
+
+merge_cmd_help () {
+	echo "Use xxdiff (requires a graphical session)"
+}