{"thread":{"id":"24015","subject":"[PATCH] git-mergetool--lib.sh: fix mergetool..* configurations ignored for known tools","startedAt":"2010-06-05T02:31:51Z","lastAt":"2010-06-11T10:06:57Z","messageCount":9,"participants":["Sylvain Rabot","Andreas Schwab","David Aguilar","Junio C Hamano","Charles Bailey"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"143014","messageId":"1275705112-8088-1-git-send-email-sylvain@abstraction.fr","threadId":"24015","inReplyTo":null,"subject":"[PATCH] git-mergetool--lib.sh: fix mergetool..* configurations ignored for known tools","fromName":"Sylvain Rabot","fromEmail":"sylvain@abstraction.fr","sentAt":"2010-06-05T02:31:51Z","receivedAt":"2010-06-05T02:31:51Z","isPatch":true,"sender":{"key":"sylvain@abstraction.fr","avatar":"https://avatars.githubusercontent.com/u/153052?v=4"},"body":"Hi,\n\nHere a patch I made after Junio's remarks in this thread :\nhttp://thread.gmane.org/gmane.comp.version-control.git/148267\n\nI think git-mergetool--lib.sh needs some refactoring. I'm no sh expert,\nfar from it, but I know a bit about scripting (I have been developing in PHP\nfor more than 6 years now :P) and I had quite some difficulties to understand\nthe behavior of some functions. For example, get_merge_tool_path returns the\nname of the tool if no mergetool..path have been set, which, from my\npoint of view, makes no sense.\n\nI know there is maybe no time or no need to rewrite something which works\nrather well but the +5 hours I spent to write this poor patch make me \nwonder if I am overrating myself. So if someone could just agree with me \nthat would be a huge step to help me to regain some self respect :)\n\nCheers.\n"},{"id":"143013","messageId":"1275705112-8088-2-git-send-email-sylvain@abstraction.fr","threadId":"24015","inReplyTo":"1275705112-8088-1-git-send-email-sylvain@abstraction.fr","subject":"[PATCH] git-mergetool--lib.sh: fix mergetool..* configurations ignored for known tools","fromName":"Sylvain Rabot","fromEmail":"sylvain@abstraction.fr","sentAt":"2010-06-05T02:31:52Z","receivedAt":"2010-06-05T02:31:52Z","isPatch":true,"sender":{"key":"sylvain@abstraction.fr","avatar":"https://avatars.githubusercontent.com/u/153052?v=4"},"body":"At this time when you define merge.tool with a known tool,\nsuch as meld, p4merge, diffuse ... etc, mergetool..*\nconfigurations are ignored and git mergetool will use its\nown templates.\n\nThis patch adds a detection for mergetool..cmd configuration\nin the run_merge_tool function. If the configuration is set, it will\ntry to run the tool with mergetool..path if its set. It also\nconsider the mergetool..trustExitCode configuration.\n\nSigned-off-by: Sylvain Rabot \n---\n git-mergetool--lib.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 60 insertions(+), 0 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 51dd0d6..2a58d88 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -84,9 +84,69 @@ get_merge_tool_cmd () {\n \n run_merge_tool () {\n \tmerge_tool_path=\"$(get_merge_tool_path \"$1\")\" || exit\n+\tmerge_tool_cmd=\"$(get_merge_tool_cmd \"$1\")\"\n+\tmerge_tool_cmd_base=\"$(echo $merge_tool_cmd | cut -f1 -d \" \")\"\n \tbase_present=\"$2\"\n \tstatus=0\n \n+\t# if mergetool..cmd is set we execute it, not a template\n+\tif test -n \"$merge_tool_cmd\"; then\n+\t\t# mergetool..path is empty\n+\t\tif test -z \"$merge_tool_path\"; then\n+\t\t\t# mergetool..cmd not found\n+\t\t\tif ! $(which \"$merge_tool_cmd_base\" > /dev/null 2>&1); then\n+\t\t\t\techo >&2 \"Configuration mergetool.$1.cmd \\\"$merge_tool_cmd_base\\\" not found\"\n+\t\t\t\texit 1\n+\t\t\telse\n+\t\t\t\tmerge_cmd=\"$merge_tool_path/$merge_tool_cmd\"\n+\t\t\tfi\n+\t\t# mergetool..path is a path\n+\t\telif test -d \"$merge_tool_path\"; then\n+\t\t\t# mergetool..cmd not found\n+\t\t\tif !test -f \"$merge_tool_path/$merge_tool_cmd_base\"; then\n+\t\t\t\techo >&2 \"Configuration mergetool.$1.cmd \\\"$(echo \"$merge_tool_path/$merge_tool_cmd_base\" | sed 's#//\\+#/#')\\\" not found\"\n+\t\t\t\texit 1\n+\t\t\t# mergetool..cmd not executable\n+\t\t\telif !test -x \"$merge_tool_path/$merge_tool_cmd_base\"; then\n+\t\t\t\techo >&2 \"Configuration mergetool.$1.cmd \\\"$(echo \"$merge_tool_path/$merge_tool_cmd_base\" | sed 's#//\\+#/#')\\\" is not executable\"\n+\t\t\t\texit 1\n+\t\t\t# tool ok\n+\t\t\telse\n+\t\t\t\tmerge_cmd=\"$merge_tool_path/$merge_tool_cmd\"\n+\t\t\tfi\n+\t\t# mergetool..path is the same as mergetool..cmd\n+\t\telif test \"$merge_tool_path\" = \"$merge_tool_cmd_base\"; then\n+\t\t\t# mergetool..cmd not found\n+\t\t\tif ! $(which \"$merge_tool_cmd_base\" > /dev/null 2>&1); then\n+\t\t\t\techo >&2 \"Configuration mergetool.$1.cmd \\\"$merge_tool_cmd_base\\\" not found\"\n+\t\t\t\texit 1\n+\t\t\telse\n+\t\t\t\tmerge_cmd=\"$merge_tool_cmd\"\n+\t\t\tfi\n+\t\t# mergetool..path is the tool itself\n+\t\telif $(which \"$merge_tool_path\" > /dev/null 2>&1); then\n+\t\t\tmerge_cmd=\"$merge_tool_path $merge_tool_cmd\"\n+\t\t# mergetool..path invalid\n+\t\telse\n+\t\t\techo >&2 \"Configuration mergetool.$1.path \\\"$merge_tool_path\\\" is not valid path\"\n+\t\t\texit 1\n+\t\tfi\n+\n+\t\t# trust exit code\n+\t\ttrust_exit_code=\"$(git config --bool mergetool.\"$1\".trustExitCode || echo false)\"\n+\n+\t\tif test \"$trust_exit_code\" = \"false\"; then\n+\t\t\ttouch \"$BACKUP\"\n+\t\t\t(eval \"$merge_cmd\")\n+\t\t\tcheck_unchanged\n+\t\t\treturn $status\n+\t\telse\n+\t\t\t(eval \"$merge_cmd\")\n+\t\t\tstatus=$?\n+\t\t\treturn $status\n+\t\tfi\n+\tfi\n+\n \tcase \"$1\" in\n \tkdiff3)\n \t\tif merge_mode; then\n-- \n1.7.1\n"},{"id":"143026","messageId":"m2fx11n8rp.fsf@igel.home","threadId":"24015","inReplyTo":"1275705112-8088-2-git-send-email-sylvain@abstraction.fr","subject":"Re: [PATCH] git-mergetool--lib.sh: fix mergetool..* configurations ignored for known tools","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2010-06-05T09:11:54Z","receivedAt":"2010-06-05T09:11:54Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Sylvain Rabot writes:\n\n> +\t\t\tif !test -f \"$merge_tool_path/$merge_tool_cmd_base\"; then\n\nMissing space after '!'.\n\n> +\t\t\telif !test -x \"$merge_tool_path/$merge_tool_cmd_base\"; then\n\nLikewise.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"143067","messageId":"1275777749.18270.23.camel@kheops","threadId":"24015","inReplyTo":"m2fx11n8rp.fsf@igel.home","subject":"Re: [PATCH] git-mergetool--lib.sh: fix mergetool..* configurations ignored for known tools","fromName":"Sylvain Rabot","fromEmail":"sylvain@abstraction.fr","sentAt":"2010-06-05T22:42:29Z","receivedAt":"2010-06-05T22:42:29Z","isPatch":true,"sender":{"key":"sylvain@abstraction.fr","avatar":"https://avatars.githubusercontent.com/u/153052?v=4"},"body":"On Sat, 2010-06-05 at 11:11 +0200, Andreas Schwab wrote:\n> Sylvain Rabot writes:\n> \n> > +\t\t\tif !test -f \"$merge_tool_path/$merge_tool_cmd_base\"; then\n> \n> Missing space after '!'.\n> \n> > +\t\t\telif !test -x \"$merge_tool_path/$merge_tool_cmd_base\"; then\n> \n> Likewise.\n> \n> Andreas.\n> \n\nThanks, \n\nI have updated the patch, you can find it here\ngit://git.abstraction.fr/~sylvain/git.git in the mergetool-lib branch.\n\nhttp://git.abstraction.fr/~sylvain/?p=git.git;a=commitdiff;h=905bfb5cea0750a67bf9bcc2baf22079054742fa\n\n-- \nSylvain Rabot \n"},{"id":"143217","messageId":"20100608083445.GC14366@gmail.com","threadId":"24015","inReplyTo":"1275705112-8088-2-git-send-email-sylvain@abstraction.fr","subject":"Re: [PATCH] git-mergetool--lib.sh: fix mergetool..* configurations ignored for known tools","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2010-06-08T08:34:46Z","receivedAt":"2010-06-08T08:34:46Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"\nHi, sorry for the delay in responding to this email.\n\nOn Sat, Jun 05, 2010 at 04:31:52AM +0200, Sylvain Rabot wrote:\n> At this time when you define merge.tool with a known tool,\n> such as meld, p4merge, diffuse ... etc, mergetool..*\n> configurations are ignored and git mergetool will use its\n> own templates.\n> \n> This patch adds a detection for mergetool..cmd configuration\n> in the run_merge_tool function. If the configuration is set, it will\n> try to run the tool with mergetool..path if its set. It also\n> consider the mergetool..trustExitCode configuration.\n> \n> Signed-off-by: Sylvain Rabot \n> ---\n> git-mergetool--lib.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 files changed, 60 insertions(+), 0 deletions(-)\n> \n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 51dd0d6..2a58d88 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -84,9 +84,69 @@ get_merge_tool_cmd () {\n> \n> run_merge_tool () {\n> \tmerge_tool_path=\"$(get_merge_tool_path \"$1\")\" || exit\n> +\tmerge_tool_cmd=\"$(get_merge_tool_cmd \"$1\")\"\n> +\tmerge_tool_cmd_base=\"$(echo $merge_tool_cmd | cut -f1 -d \" \")\"\n> \tbase_present=\"$2\"\n> \tstatus=0\n> \n> +\t# if mergetool..cmd is set we execute it, not a template\n> +\tif test -n \"$merge_tool_cmd\"; then\n> +\t\t# mergetool..path is empty\n> +\t\tif test -z \"$merge_tool_path\"; then\n> +\t\t\t# mergetool..cmd not found\n> +\t\t\tif ! $(which \"$merge_tool_cmd_base\" > /dev/null 2>&1); then\n> +\t\t\t\techo >&2 \"Configuration mergetool.$1.cmd \\\"$merge_tool_cmd_base\\\" not found\"\n> +\t\t\t\texit 1\n> +\t\t\telse\n> +\t\t\t\tmerge_cmd=\"$merge_tool_path/$merge_tool_cmd\"\n> +\t\t\tfi\n> +\t\t# mergetool..path is a path\n\nFiles and Directories are both paths...\n\n> +\t\telif test -d \"$merge_tool_path\"; then\n\nBut...\n\n> +\t\t\t# mergetool..cmd not found\n> +\t\t\tif !test -f \"$merge_tool_path/$merge_tool_cmd_base\"; then\n> +\t\t\t\techo >&2 \"Configuration mergetool.$1.cmd \\\"$(echo \"$merge_tool_path/$merge_tool_cmd_base\" | sed 's#//\\+#/#')\\\" not found\"\n> +\t\t\t\texit 1\n> +\t\t\t# mergetool..cmd not executable\n> +\t\t\telif !test -x \"$merge_tool_path/$merge_tool_cmd_base\"; then\n> +\t\t\t\techo >&2 \"Configuration mergetool.$1.cmd \\\"$(echo \"$merge_tool_path/$merge_tool_cmd_base\" | sed 's#//\\+#/#')\\\" is not executable\"\n> +\t\t\t\texit 1\n> +\t\t\t# tool ok\n> +\t\t\telse\n> +\t\t\t\tmerge_cmd=\"$merge_tool_path/$merge_tool_cmd\"\n> +\t\t\tfi\n\nI don't think we ever signed up to support this configuration.\nmergetool..path has always (from my naive reading of the\ndocumentation) been the absolute path to .\n\nI don't think it should have a dual-role where it can be either\nthe tool's parent directory or the path to the tool itself.\nI would prefer to keep it as simple as possible, if we can.\n\n\n> +\t\t# mergetool..path is the same as mergetool..cmd\n> +\t\telif test \"$merge_tool_path\" = \"$merge_tool_cmd_base\"; then\n> +\t\t\t# mergetool..cmd not found\n> +\t\t\tif ! $(which \"$merge_tool_cmd_base\" > /dev/null 2>&1); then\n> +\t\t\t\techo >&2 \"Configuration mergetool.$1.cmd \\\"$merge_tool_cmd_base\\\" not found\"\n> +\t\t\t\texit 1\n> +\t\t\telse\n> +\t\t\t\tmerge_cmd=\"$merge_tool_cmd\"\n> +\t\t\tfi\n> +\t\t# mergetool..path is the tool itself\n> +\t\telif $(which \"$merge_tool_path\" > /dev/null 2>&1); then\n> +\t\t\tmerge_cmd=\"$merge_tool_path $merge_tool_cmd\"\n> +\t\t# mergetool..path invalid\n> +\t\telse\n> +\t\t\techo >&2 \"Configuration mergetool.$1.path \\\"$merge_tool_path\\\" is not valid path\"\n> +\t\t\texit 1\n> +\t\tfi\n> +\n> +\t\t# trust exit code\n> +\t\ttrust_exit_code=\"$(git config --bool mergetool.\"$1\".trustExitCode || echo false)\"\n> +\n> +\t\tif test \"$trust_exit_code\" = \"false\"; then\n> +\t\t\ttouch \"$BACKUP\"\n> +\t\t\t(eval \"$merge_cmd\")\n> +\t\t\tcheck_unchanged\n> +\t\t\treturn $status\n> +\t\telse\n> +\t\t\t(eval \"$merge_cmd\")\n> +\t\t\tstatus=$?\n> +\t\t\treturn $status\n> +\t\tfi\n> +\tfi\n\nThis section is getting pretty nested.\nShould we break the handling for configs-that-override-builtins\ninto a separate function?\n\n> +\n> \tcase \"$1\" in\n> \tkdiff3)\n> \t\tif merge_mode; then\n> -- \n> 1.7.1\n\nOne last thing -- I tried to fetch from the repo you\nmentioned elsewhere in this thread but it was offline.\n\nCheers,\n\n-- \n\t\tDavid\n"},{"id":"143280","messageId":"7vzkz5s0a3.fsf@alter.siamese.dyndns.org","threadId":"24015","inReplyTo":"20100608083445.GC14366@gmail.com","subject":"Re: [PATCH] git-mergetool--lib.sh: fix mergetool..* configurations ignored for known tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-08T21:05:24Z","receivedAt":"2010-06-08T21:05:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Hi, sorry for the delay in responding to this email.\n\nThanks for a review.\n\n> I don't think we ever signed up to support this configuration.\n> mergetool..path has always (from my naive reading of the\n> documentation) been the absolute path to .\n>\n> I don't think it should have a dual-role where it can be either\n> the tool's parent directory or the path to the tool itself.\n> I would prefer to keep it as simple as possible, if we can.\n\nI concur; it is not just about simplicity, but setting the value to the\nparent directory of the tool feels downright confusing.\n\n>> +\t\t# mergetool..path is the same as mergetool..cmd\n>> ...\n>> +\tfi\n>\n> This section is getting pretty nested.\n> Should we break the handling for configs-that-override-builtins\n> into a separate function?\n\nSounds like a sane thing to do.\n"},{"id":"143369","messageId":"4C0FEA7B.9030409@hashpling.org","threadId":"24015","inReplyTo":"1275705112-8088-2-git-send-email-sylvain@abstraction.fr","subject":"Re: [PATCH] git-mergetool--lib.sh: fix mergetool..* configurations ignored for known tools","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2010-06-09T19:24:43Z","receivedAt":"2010-06-09T19:24:43Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On 05/06/2010 03:31, Sylvain Rabot wrote:\n> At this time when you define merge.tool with a known tool,\n> such as meld, p4merge, diffuse ... etc, mergetool..*\n> configurations are ignored and git mergetool will use its\n> own templates.\n>\n> This patch adds a detection for mergetool..cmd configuration\n> in the run_merge_tool function. If the configuration is set, it will\n> try to run the tool with mergetool..path if its set. It also\n> consider the mergetool..trustExitCode configuration.\n>\n> Signed-off-by: Sylvain Rabot\n> ---\n> git-mergetool--lib.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 files changed, 60 insertions(+), 0 deletions(-)\n>\n\nFirst, my apologies for only having had the time to skim this so far.\n\nCan I just ask some basic questions about the purpose of this patch. Is \nit the intention that if mergetool..cmd is set then you want to \nmergetool to behave 'as if' the merge tool wasn't a 'known' tool and \njust performed the \"*)\" case ?\n\nIf so, it seems like a lot of extra boiler-plate and error handling that \ndoesn't exist in the normal \"*)\" case. Should we have have this in the \n\"*)\" case as well? If so, we should look to rework it so that we can \nre-use the code rather than duplicating it.\n\n From a user perspective, if they want to run a \"known\" tool but in a \nway that is different from the default behaviour can't they just give it \na different name, e.g. merge.tool=my_kdiff3 , mergetool.my_kdiff3.cmd=... ?\n\nThanks,\n\nCharles.\n\n-- \nAlmost dormant mergetool maintainer.\n"},{"id":"143495","messageId":"AANLkTikK0t_-H5zgzUToiTlyWCFGCJ63wok-t7wX09OW@mail.gmail.com","threadId":"24015","inReplyTo":"20100608083445.GC14366@gmail.com","subject":"Re: [PATCH] git-mergetool--lib.sh: fix mergetool..* configurations ignored for known tools","fromName":"Sylvain Rabot","fromEmail":"sylvain@abstraction.fr","sentAt":"2010-06-11T09:54:27Z","receivedAt":"2010-06-11T09:54:27Z","isPatch":true,"sender":{"key":"sylvain@abstraction.fr","avatar":"https://avatars.githubusercontent.com/u/153052?v=4"},"body":"On Tue, Jun 8, 2010 at 10:34, David Aguilar wrote:\n>\n> Hi, sorry for the delay in responding to this email.\n>\n> On Sat, Jun 05, 2010 at 04:31:52AM +0200, Sylvain Rabot wrote:\n>> At this time when you define merge.tool with a known tool,\n>> such as meld, p4merge, diffuse ... etc, mergetool..*\n>> configurations are ignored and git mergetool will use its\n>> own templates.\n>>\n>> This patch adds a detection for mergetool..cmd configuration\n>> in the run_merge_tool function. If the configuration is set, it will\n>> try to run the tool with mergetool..path if its set. It also\n>> consider the mergetool..trustExitCode configuration.\n>>\n>> Signed-off-by: Sylvain Rabot \n>> ---\n>>  git-mergetool--lib.sh |   60 +++++++++++++++++++++++++++++++++++++++++++++++++\n>>  1 files changed, 60 insertions(+), 0 deletions(-)\n>>\n>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n>> index 51dd0d6..2a58d88 100644\n>> --- a/git-mergetool--lib.sh\n>> +++ b/git-mergetool--lib.sh\n>> @@ -84,9 +84,69 @@ get_merge_tool_cmd () {\n>>\n>>  run_merge_tool () {\n>>       merge_tool_path=\"$(get_merge_tool_path \"$1\")\" || exit\n>> +     merge_tool_cmd=\"$(get_merge_tool_cmd \"$1\")\"\n>> +     merge_tool_cmd_base=\"$(echo $merge_tool_cmd | cut -f1 -d \" \")\"\n>>       base_present=\"$2\"\n>>       status=0\n>>\n>> +     # if mergetool..cmd is set we execute it, not a template\n>> +     if test -n \"$merge_tool_cmd\"; then\n>> +             # mergetool..path is empty\n>> +             if test -z \"$merge_tool_path\"; then\n>> +                     # mergetool..cmd not found\n>> +                     if ! $(which \"$merge_tool_cmd_base\" > /dev/null 2>&1); then\n>> +                             echo >&2 \"Configuration mergetool.$1.cmd \\\"$merge_tool_cmd_base\\\" not found\"\n>> +                             exit 1\n>> +                     else\n>> +                             merge_cmd=\"$merge_tool_path/$merge_tool_cmd\"\n>> +                     fi\n>> +             # mergetool..path is a path\n>\n> Files and Directories are both paths...\n>\n>> +             elif test -d \"$merge_tool_path\"; then\n>\n> But...\n>\n>> +                     # mergetool..cmd not found\n>> +                     if !test -f \"$merge_tool_path/$merge_tool_cmd_base\"; then\n>> +                             echo >&2 \"Configuration mergetool.$1.cmd \\\"$(echo \"$merge_tool_path/$merge_tool_cmd_base\" | sed 's#//\\+#/#')\\\" not found\"\n>> +                             exit 1\n>> +                     # mergetool..cmd not executable\n>> +                     elif !test -x \"$merge_tool_path/$merge_tool_cmd_base\"; then\n>> +                             echo >&2 \"Configuration mergetool.$1.cmd \\\"$(echo \"$merge_tool_path/$merge_tool_cmd_base\" | sed 's#//\\+#/#')\\\" is not executable\"\n>> +                             exit 1\n>> +                     # tool ok\n>> +                     else\n>> +                             merge_cmd=\"$merge_tool_path/$merge_tool_cmd\"\n>> +                     fi\n>\n> I don't think we ever signed up to support this configuration.\n> mergetool..path has always (from my naive reading of the\n> documentation) been the absolute path to .\n\nI did not see it that way but it seems cleaner.\nSo mergetool..path would be the absolute path to the tool and\nmergetool..cmd would be only the args to call the tool with.\n\n>\n> I don't think it should have a dual-role where it can be either\n> the tool's parent directory or the path to the tool itself.\n> I would prefer to keep it as simple as possible, if we can.\n>\n>\n>> +             # mergetool..path is the same as mergetool..cmd\n>> +             elif test \"$merge_tool_path\" = \"$merge_tool_cmd_base\"; then\n>> +                     # mergetool..cmd not found\n>> +                     if ! $(which \"$merge_tool_cmd_base\" > /dev/null 2>&1); then\n>> +                             echo >&2 \"Configuration mergetool.$1.cmd \\\"$merge_tool_cmd_base\\\" not found\"\n>> +                             exit 1\n>> +                     else\n>> +                             merge_cmd=\"$merge_tool_cmd\"\n>> +                     fi\n>> +             # mergetool..path is the tool itself\n>> +             elif $(which \"$merge_tool_path\" > /dev/null 2>&1); then\n>> +                     merge_cmd=\"$merge_tool_path $merge_tool_cmd\"\n>> +             # mergetool..path invalid\n>> +             else\n>> +                     echo >&2 \"Configuration mergetool.$1.path \\\"$merge_tool_path\\\" is not valid path\"\n>> +                     exit 1\n>> +             fi\n>> +\n>> +             # trust exit code\n>> +             trust_exit_code=\"$(git config --bool mergetool.\"$1\".trustExitCode || echo false)\"\n>> +\n>> +             if test \"$trust_exit_code\" = \"false\"; then\n>> +                     touch \"$BACKUP\"\n>> +                     (eval \"$merge_cmd\")\n>> +                     check_unchanged\n>> +                     return $status\n>> +             else\n>> +                     (eval \"$merge_cmd\")\n>> +                     status=$?\n>> +                     return $status\n>> +             fi\n>> +     fi\n>\n> This section is getting pretty nested.\n> Should we break the handling for configs-that-override-builtins\n> into a separate function?\n\nI think the whole patch can be simplified if we assume path can only\nbe the absolute path to the tool.\n\n>\n>> +\n>>       case \"$1\" in\n>>       kdiff3)\n>>               if merge_mode; then\n>> --\n>> 1.7.1\n>\n> One last thing -- I tried to fetch from the repo you\n> mentioned elsewhere in this thread but it was offline.\n\nMy bad, new box, new setup, forgot to authorize git daemon port to iptables.\n\n>\n> Cheers,\n\nThanks for your time.\n\n>\n> --\n>                David\n>\n\n\n\n-- \nSylvain\n"},{"id":"143496","messageId":"AANLkTim3UWlP7N2ucI3ncN2jzc4lSEyiJcQhYNElQVXl@mail.gmail.com","threadId":"24015","inReplyTo":"4C0FEA7B.9030409@hashpling.org","subject":"Re: [PATCH] git-mergetool--lib.sh: fix mergetool..* configurations ignored for known tools","fromName":"Sylvain Rabot","fromEmail":"sylvain@abstraction.fr","sentAt":"2010-06-11T10:06:57Z","receivedAt":"2010-06-11T10:06:57Z","isPatch":true,"sender":{"key":"sylvain@abstraction.fr","avatar":"https://avatars.githubusercontent.com/u/153052?v=4"},"body":"On Wed, Jun 9, 2010 at 21:24, Charles Bailey wrote:\n> On 05/06/2010 03:31, Sylvain Rabot wrote:\n>>\n>> At this time when you define merge.tool with a known tool,\n>> such as meld, p4merge, diffuse ... etc, mergetool..*\n>> configurations are ignored and git mergetool will use its\n>> own templates.\n>>\n>> This patch adds a detection for mergetool..cmd configuration\n>> in the run_merge_tool function. If the configuration is set, it will\n>> try to run the tool with mergetool..path if its set. It also\n>> consider the mergetool..trustExitCode configuration.\n>>\n>> Signed-off-by: Sylvain Rabot\n>> ---\n>>  git-mergetool--lib.sh |   60\n>> +++++++++++++++++++++++++++++++++++++++++++++++++\n>>  1 files changed, 60 insertions(+), 0 deletions(-)\n>>\n>\n> First, my apologies for only having had the time to skim this so far.\n\nNo worries ;)\n\n>\n> Can I just ask some basic questions about the purpose of this patch. Is it\n> the intention that if mergetool..cmd is set then you want to mergetool\n> to behave 'as if' the merge tool wasn't a 'known' tool and just performed\n> the \"*)\" case ?\n\nyes\n\n>\n> If so, it seems like a lot of extra boiler-plate and error handling that\n> doesn't exist in the normal \"*)\" case. Should we have have this in the \"*)\"\n> case as well? If so, we should look to rework it so that we can re-use the\n> code rather than duplicating it.\n\nI did not modify the \"*)\" being afraid to break it all, but that would\nbe the right thing to do.\n\n>\n> From a user perspective, if they want to run a \"known\" tool but in a way\n> that is different from the default behaviour can't they just give it a\n> different name, e.g. merge.tool=my_kdiff3 , mergetool.my_kdiff3.cmd=... ?\n\nThat's the workaround, yes, but, if you are the user and you are not\naware of this bahavior, you will do exactly like a did, i.e., lose\nyour time to torture your git configuration because it is not working\nthe way you was expecting it would.\n\n>\n> Thanks,\n\nThanks for your time.\n\n>\n> Charles.\n>\n> --\n> Almost dormant mergetool maintainer.\n>\n\n\n\n-- \nSylvain\n"}]}