{"thread":{"id":"37732","subject":"[PATCH] mergetools/meld: do not rely on the output of `meld --help`","startedAt":"2014-10-15T08:30:48Z","lastAt":"2014-10-16T04:45:14Z","messageCount":4,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"250667","messageId":"1413361848-16923-1-git-send-email-davvid@gmail.com","threadId":"37732","inReplyTo":null,"subject":"[PATCH] mergetools/meld: do not rely on the output of `meld --help`","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-10-15T08:30:48Z","receivedAt":"2014-10-15T08:30:48Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"We cannot rely on the output of `meld --help` when determining\nwhether or not meld understands the --output option.\n\nNewer versions of meld print a generic help message that does not\nmention --output even though it is supported.\n\nAdd a mergetool.meld.compat variable to enable the historical\nbehavior and make the --output mode the default.\n\nReported-by: Andrey Novoseltsev \nSigned-off-by: David Aguilar \n---\n Documentation/config.txt | 7 +++++++\n mergetools/meld | 4 ++--\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 04a1e2f..a942bfe 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1755,6 +1755,13 @@ mergetool..trustExitCode::\n \tif the file has been updated, otherwise the user is prompted to\n \tindicate the success of the merge.\n \n+mergetool.meld.compat::\n+\tGit passes the `--output` option to `meld` by default when\n+\tusing the `meld` merge tool. Older versions of `meld` do not\n+\tunderstand the `--output` option. Set `mergetool.meld.compat`\n+\tto `true` if your version of `meld` is older and does not\n+\tunderstand the `--output` option. Defaults to false.\n+\n mergetool.keepBackup::\n \tAfter performing a merge, the original file with conflict markers\n \tcan be saved as a file with a `.orig` extension. If this variable\ndiff --git a/mergetools/meld b/mergetools/meld\nindex cb672a5..9e2b8d2 100644\n--- a/mergetools/meld\n+++ b/mergetools/meld\n@@ -18,12 +18,12 @@ merge_cmd () {\n \tcheck_unchanged\n }\n \n-# Check whether 'meld --output ' is supported\n+# Use 'meld --output ' unless mergetool.meld.compat is true.\n check_meld_for_output_version () {\n \tmeld_path=\"$(git config mergetool.meld.path)\"\n \tmeld_path=\"${meld_path:-meld}\"\n \n-\tif \"$meld_path\" --help 2>&1 | grep -e --output >/dev/null\n+\tif test \"$(git config --bool mergetool.meld.compat)\" != true\n \tthen\n \t\tmeld_has_output_option=true\n \telse\n-- \n2.1.2.453.g1b015e3\n"},{"id":"250697","messageId":"xmqqoatdw3wk.fsf@gitster.dls.corp.google.com","threadId":"37732","inReplyTo":"1413361848-16923-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] mergetools/meld: do not rely on the output of `meld --help`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-15T19:04:27Z","receivedAt":"2014-10-15T19:04:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> We cannot rely on the output of `meld --help` when determining\n> whether or not meld understands the --output option.\n>\n> Newer versions of meld print a generic help message that does not\n> mention --output even though it is supported.\n\nThis obviously breaks those who have happily been using their\ninstalled version of meld that understands and shows --output in the\nhelp text. Is that a minority that is rapidly diminishing?\n\nI would understand it if the change were\n\n - a configuration tells us to use or not use --output; when it is\n set, then we do not try auto-detect by reading --help output\n\n - when that new configuration is not set, we keep the current code\n to read --help output, which may fail for recent meld but that is\n not a regression.\n\nWhen versions of meld that support --output but do not mention it in\ntheir --help text are overwhelming majority, we would want to flip\nthe fallback codepath from \"read --help and decide\" to \"assume that\n--output can be used\", but I do not know if now is the time to do\nso.\n"},{"id":"250698","messageId":"xmqqh9z5w38h.fsf@gitster.dls.corp.google.com","threadId":"37732","inReplyTo":"xmqqoatdw3wk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] mergetools/meld: do not rely on the output of `meld --help`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-15T19:18:54Z","receivedAt":"2014-10-15T19:18:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> This obviously breaks those who have happily been using their\n> installed version of meld that understands and shows --output in the\n> help text. Is that a minority that is rapidly diminishing?\n>\n> I would understand it if the change were\n>\n> - a configuration tells us to use or not use --output; when it is\n> set, then we do not try auto-detect by reading --help output\n>\n> - when that new configuration is not set, we keep the current code\n> to read --help output, which may fail for recent meld but that is\n> not a regression.\n>\n> When versions of meld that support --output but do not mention it in\n> their --help text are overwhelming majority, we would want to flip\n> the fallback codepath from \"read --help and decide\" to \"assume that\n> --output can be used\", but I do not know if now is the time to do\n> so.\n\nIn other words, I am wondering if a milder fix would be along this\nline of change instead. Older versions seem to list --output\nexplicitly, and we assume newer ones including the one reported by\nAndrey begin their output like so:\n\n $ meld --help\n Usage:\n meld [OPTION...]\n\nhence we catch either of these patterns.\n\n mergetools/meld | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/mergetools/meld b/mergetools/meld\nindex cb672a5..b6169c9 100644\n--- a/mergetools/meld\n+++ b/mergetools/meld\n@@ -23,8 +23,12 @@ check_meld_for_output_version () {\n \tmeld_path=\"$(git config mergetool.meld.path)\"\n \tmeld_path=\"${meld_path:-meld}\"\n \n-\tif \"$meld_path\" --help 2>&1 | grep -e --output >/dev/null\n+\tif meld_has_output_option=\"$(git config --bool mergetool.meld.hasOutput)\"\n \tthen\n+\t\t: use whatever is configured\n+\telif \"$meld_path\" --help 2>&1 | grep -e '--output=' -e '\\[OPTION\\.\\.\\.\\]' /dev/null\n+\tthen\n+\t\t: old ones explicitly listed --output and new ones just say OPTION...\n \t\tmeld_has_output_option=true\n \telse\n \t\tmeld_has_output_option=false\n"},{"id":"250743","messageId":"1413434714-20424-1-git-send-email-davvid@gmail.com","threadId":"37732","inReplyTo":"xmqqh9z5w38h.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] mergetools/meld: make usage of `--output` configurable and more robust","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-10-16T04:45:14Z","receivedAt":"2014-10-16T04:45:14Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Older versions of meld listed --output in `meld --help`.\nNewer versions only mention `meld [OPTIONS...]`.\nImprove the checks to catch these newer versions.\n\nAdd a `mergetool.meld.hasOutput` configuration to allow\noverriding the heuristic.\n\nReported-by: Andrey Novoseltsev \nHelped-by: Junio C Hamano \nSigned-off-by: David Aguilar \n---\nChanges since v1:\n\nThis uses Junio's improved approach of checking for both --help\nstyles and uses more focused name for the configuration variable.\nThe documentation was reworded accordingly.\n\n Documentation/config.txt | 9 +++++++++\n mergetools/meld | 9 +++++++--\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 04a1e2f..0f823eb 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1755,6 +1755,15 @@ mergetool..trustExitCode::\n \tif the file has been updated, otherwise the user is prompted to\n \tindicate the success of the merge.\n \n+mergetool.meld.hasOutput::\n+\tOlder versions of `meld` do not support the `--output` option.\n+\tGit will attempt to detect whether `meld` supports `--output`\n+\tby inspecting the output of `meld --help`. Configuring\n+\t`mergetool.meld.hasOutput` will make Git skip these checks and\n+\tuse the configured value instead. Setting `mergetool.meld.hasOutput`\n+\tto `true` tells Git to unconditionally use the `--output` option,\n+\tand `false` avoids using `--output`.\n+\n mergetool.keepBackup::\n \tAfter performing a merge, the original file with conflict markers\n \tcan be saved as a file with a `.orig` extension. If this variable\ndiff --git a/mergetools/meld b/mergetools/meld\nindex cb672a5..83ebdfb 100644\n--- a/mergetools/meld\n+++ b/mergetools/meld\n@@ -18,13 +18,18 @@ merge_cmd () {\n \tcheck_unchanged\n }\n \n-# Check whether 'meld --output ' is supported\n+# Check whether we should use 'meld --output '\n check_meld_for_output_version () {\n \tmeld_path=\"$(git config mergetool.meld.path)\"\n \tmeld_path=\"${meld_path:-meld}\"\n \n-\tif \"$meld_path\" --help 2>&1 | grep -e --output >/dev/null\n+\tif meld_has_output_option=$(git config --bool mergetool.meld.hasOutput)\n \tthen\n+\t\t: use configured value\n+\telif \"$meld_path\" --help 2>&1 |\n+\t\tgrep -e '--output=' -e '\\[OPTION\\.\\.\\.\\]' >/dev/null\n+\tthen\n+\t\t: old ones mention --output and new ones just say OPTION...\n \t\tmeld_has_output_option=true\n \telse\n \t\tmeld_has_output_option=false\n-- \n2.1.2.444.g0cfad43\n"}]}