[PATCH] mergetools/meld: do not rely on the output of `meld --help`

[David Aguilar]

We cannot rely on the output of `meld --help` when determining
whether or not meld understands the --output option.

Newer versions of meld print a generic help message that does not
mention --output even though it is supported.

Add a mergetool.meld.compat variable to enable the historical
behavior and make the --output mode the default.

Reported-by: Andrey Novoseltsev <***@gmail.com>
Signed-off-by: David Aguilar <***@gmail.com>
---
Documentation/config.txt | 7 +++++++
mergetools/meld | 4 ++--
2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 04a1e2f..a942bfe 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1755,6 +1755,13 @@ mergetool.<tool>.trustExitCode::
if the file has been updated, otherwise the user is prompted to
indicate the success of the merge.

+mergetool.meld.compat::
+ Git passes the `--output` option to `meld` by default when
+ using the `meld` merge tool. Older versions of `meld` do not
+ understand the `--output` option. Set `mergetool.meld.compat`
+ to `true` if your version of `meld` is older and does not
+ understand the `--output` option. Defaults to false.
+
mergetool.keepBackup::
After performing a merge, the original file with conflict markers
can be saved as a file with a `.orig` extension. If this variable
diff --git a/mergetools/meld b/mergetools/meld
index cb672a5..9e2b8d2 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -18,12 +18,12 @@ merge_cmd () {
check_unchanged
}

-# Check whether 'meld --output <file>' is supported
+# Use 'meld --output <file>' unless mergetool.meld.compat is true.
check_meld_for_output_version () {
meld_path="$(git config mergetool.meld.path)"
meld_path="${meld_path:-meld}"

- if "$meld_path" --help 2>&1 | grep -e --output >/dev/null
+ if test "$(git config --bool mergetool.meld.compat)" != true
then
meld_has_output_option=true
else

[Junio C Hamano]

Post by David Aguilar
We cannot rely on the output of `meld --help` when determining
whether or not meld understands the --output option.
Newer versions of meld print a generic help message that does not
mention --output even though it is supported.

This obviously breaks those who have happily been using their
installed version of meld that understands and shows --output in the
help text. Is that a minority that is rapidly diminishing?

I would understand it if the change were

- a configuration tells us to use or not use --output; when it is
set, then we do not try auto-detect by reading --help output

- when that new configuration is not set, we keep the current code
to read --help output, which may fail for recent meld but that is
not a regression.

When versions of meld that support --output but do not mention it in
their --help text are overwhelming majority, we would want to flip
the fallback codepath from "read --help and decide" to "assume that
--output can be used", but I do not know if now is the time to do
so.

[Junio C Hamano]

Post by Junio C Hamano
This obviously breaks those who have happily been using their
installed version of meld that understands and shows --output in the
help text. Is that a minority that is rapidly diminishing?
I would understand it if the change were
- a configuration tells us to use or not use --output; when it is
set, then we do not try auto-detect by reading --help output
- when that new configuration is not set, we keep the current code
to read --help output, which may fail for recent meld but that is
not a regression.
When versions of meld that support --output but do not mention it in
their --help text are overwhelming majority, we would want to flip
the fallback codepath from "read --help and decide" to "assume that
--output can be used", but I do not know if now is the time to do
so.

In other words, I am wondering if a milder fix would be along this
line of change instead. Older versions seem to list --output
explicitly, and we assume newer ones including the one reported by
Andrey begin their output like so:

$ meld --help
Usage:
meld [OPTION...]

hence we catch either of these patterns.

mergetools/meld | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mergetools/meld b/mergetools/meld
index cb672a5..b6169c9 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -23,8 +23,12 @@ check_meld_for_output_version () {
meld_path="$(git config mergetool.meld.path)"
meld_path="${meld_path:-meld}"

- if "$meld_path" --help 2>&1 | grep -e --output >/dev/null
+ if meld_has_output_option="$(git config --bool mergetool.meld.hasOutput)"
then
+ : use whatever is configured
+ elif "$meld_path" --help 2>&1 | grep -e '--output=' -e '\[OPTION\.\.\.\]' /dev/null
+ then
+ : old ones explicitly listed --output and new ones just say OPTION...
meld_has_output_option=true
else
meld_has_output_option=false

[David Aguilar]

Older versions of meld listed --output in `meld --help`.
Newer versions only mention `meld [OPTIONS...]`.
Improve the checks to catch these newer versions.

Add a `mergetool.meld.hasOutput` configuration to allow
overriding the heuristic.

Reported-by: Andrey Novoseltsev <***@gmail.com>
Helped-by: Junio C Hamano <***@pobox.com>
Signed-off-by: David Aguilar <***@gmail.com>
---
Changes since v1:

This uses Junio's improved approach of checking for both --help
styles and uses more focused name for the configuration variable.
The documentation was reworded accordingly.

Documentation/config.txt | 9 +++++++++
mergetools/meld | 9 +++++++--
2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 04a1e2f..0f823eb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1755,6 +1755,15 @@ mergetool.<tool>.trustExitCode::
if the file has been updated, otherwise the user is prompted to
indicate the success of the merge.

+mergetool.meld.hasOutput::
+ Older versions of `meld` do not support the `--output` option.
+ Git will attempt to detect whether `meld` supports `--output`
+ by inspecting the output of `meld --help`. Configuring
+ `mergetool.meld.hasOutput` will make Git skip these checks and
+ use the configured value instead. Setting `mergetool.meld.hasOutput`
+ to `true` tells Git to unconditionally use the `--output` option,
+ and `false` avoids using `--output`.
+
mergetool.keepBackup::
After performing a merge, the original file with conflict markers
can be saved as a file with a `.orig` extension. If this variable
diff --git a/mergetools/meld b/mergetools/meld
index cb672a5..83ebdfb 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -18,13 +18,18 @@ merge_cmd () {
check_unchanged
}

-# Check whether 'meld --output <file>' is supported
+# Check whether we should use 'meld --output <file>'
check_meld_for_output_version () {
meld_path="$(git config mergetool.meld.path)"
meld_path="${meld_path:-meld}"

- if "$meld_path" --help 2>&1 | grep -e --output >/dev/null
+ if meld_has_output_option=$(git config --bool mergetool.meld.hasOutput)
then
+ : use configured value
+ elif "$meld_path" --help 2>&1 |
+ grep -e '--output=' -e '\[OPTION\.\.\.\]' >/dev/null
+ then
+ : old ones mention --output and new ones just say OPTION...
meld_has_output_option=true
else
meld_has_output_option=false