{"thread":{"id":"12138","subject":"[RFC/PATCH] Teach git mergetool to use custom commands defined at config time","startedAt":"2008-02-16T18:53:49Z","lastAt":"2008-02-18T08:14:46Z","messageCount":16,"participants":["Charles Bailey","Junio C Hamano","Jakub Narebski","Steffen Prohaska","Johannes Schindelin","Theodore Tso"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"68916","messageId":"20080216185349.GA29177@hashpling.org","threadId":"12138","inReplyTo":null,"subject":"[RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-02-16T18:53:49Z","receivedAt":"2008-02-16T18:53:49Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"Currently git mergetool is restricted to a set of commands defined\nin the script. You can subvert the mergetool.<tool>.path to force\ngit mergetool to use a different command, but if you have a command\nwhose invocation syntax does not match one of the current tools then\nyou would have to write a wrapper script for it.\n\nThis patch adds three git config variable patterns which allow a more\nflexible choice of merge tool.\n\nIf you run git mergetool with -t/--tool or the merge.tool config\nvariable set to an unrecognized tool then git mergetool will query the\nmergetool.<tool>.cmd config variable. If this variable exists, then\ngit mergetool will treat the specified tool as a custom command and\nwill use a shell eval to run the command so that the appropriate shell\nvariables can be used to find the merge temporary files.\n\nmergetool.<tool>.trustExitCode can be used to indicate that the exit\ncode of the custom command can be used to determine the success of the\nmerge. mergetool.<tool>.keepBackup can be used to specify whether the\noriginal pre-merge file with conflict markers should be kept as a\n'.orig' file after the merge tool completes.\n---\n\nThis is a preliminary patch which aims to make it easier to use\na.n.other 3-way merge tool with git-mergetool without either hacking\nthe source or writing a wrapper script for the tool.\n\nIt follows filter-branch's 'eval a user shell snippet' philosophy to\nprovide the flexibility and here in lies an ugliness. It exposes\ngit-mergetool.sh's private variables to the user script. The variables\nare BASE, REMOTE, LOCAL and path.\n\nMy feeling is that we should give this consistent and documented\nnames, perhaps GIT_BASE, GIT_REMOTE, GIT_LOCAL, GIT_MERGED or similar.\n\nAlso, does anyone know of any reason why the temporary files should\nnot be cleaned up after an unsuccessful merge? As it is, every time\nyou abort a mergetool run you end up with another set of .$$.BASE\n.$$.REMOTE and .$$.LOCAL files.\n\nA further enhancement that I have thought about is providing a config\noption like mergetool.<tool>.cmdNoBase for the case where there is no\nbase file, as in many cases it might be easier for the user to provide\na second command than to have shell conditionals in their config.\n\nJust for reference I've tried this patch in the windows world with\nthings like:\n\ngit config --global mergetool.tortoise.cmd 'TortoiseMerge.exe \\\n/base:\"$BASE\" /theirs:\"$REMOTE\" /mine:\"$LOCAL\" /merged:\"$path\"'\n\ngit config --global mergetool.p4.cmd 'p4merge.exe \"$BASE\" \"$REMOTE\" \\\n\"$LOCAL\" \"$path\"'\n\ngit config --global mergetool.p4win.cmd 'P4WinMrg \"$BASE\" \"$REMOTE\" \\\n\"$LOCAL\" \"$path\"'\n\n Documentation/config.txt |   30 ++++++++++++++++++++++++++++--\n git-mergetool.sh         |   32 ++++++++++++++++++++++++++++++--\n 2 files changed, 58 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f9bdb16..e26c4b2 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -689,8 +689,10 @@ merge.summary::\n \n merge.tool::\n \tControls which merge resolution program is used by\n-\tlinkgit:git-mergetool[1].  Valid values are: \"kdiff3\", \"tkdiff\",\n-\t\"meld\", \"xxdiff\", \"emerge\", \"vimdiff\", \"gvimdiff\", and \"opendiff\".\n+\tlinkgit:git-mergetool[1].  Valid built-in values are: \"kdiff3\",\n+\t\"tkdiff\", \"meld\", \"xxdiff\", \"emerge\", \"vimdiff\", \"gvimdiff\", and\n+\t\"opendiff\".  Any other value is treated is custom merge tool\n+\tand there must be a corresponing mergetool.<tool>.cmd option.\n \n merge.verbosity::\n \tControls the amount of output shown by the recursive merge\n@@ -717,6 +719,30 @@ mergetool.<tool>.path::\n \tOverride the path for the given tool.  This is useful in case\n \tyour tool is not in the PATH.\n \n+mergetool.<tool>.cmd::\n+\tSpecify the command to invoke the specified merge tool.  The\n+\tspecified command is evaluated in shell with the following\n+\tvariables available: 'BASE' is the name of a temporary file\n+\tcontaining the common base of the files to be merged, if available;\n+\t'LOCAL' is the name of a temporary file containing the contents of\n+\tthe file on the current branch; 'REMOTE' is the name of a temporary\n+\tfile containing the contents of the file from the branch being\n+\tmerged; 'path' contains the name of the file to which the merge\n+\ttool should write the results of a successful merge.\n+\n+mergetool.<tool>.trustExitCode::\n+\tFor a custom merge command, specify whether the exit code of\n+\tthe merge command can be used to determine whether the merge was\n+\tsuccessful.  If this is not set to true then the merge target file\n+\ttimestamp is checked and the merge assumed to have been successful\n+\tif the file has been updated, otherwise the user is prompted to\n+\tindicate the success of the merge.\n+\n+mergetool.<tool>.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\n+\tis set to `false` then this file is not preserved.\n+\n pack.window::\n \tThe size of the window used by linkgit:git-pack-objects[1] when no\n \twindow size is given on the command line. Defaults to 10.\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex cbbb707..78c73ca 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -271,6 +271,21 @@ merge_file () {\n \t    status=$?\n \t    save_backup\n \t    ;;\n+\t*)\n+\t    if test -n \"$merge_tool_cmd\"; then\n+\t\ttouch \"$BACKUP\"\n+\t\teval \"$merge_tool_cmd\"\n+\t\tstatus=$?\n+\t\tif test \"$merge_tool_trust_exit_code\" = \"false\"; then\n+\t\t    check_unchanged\n+\t\tfi\n+\t\tif test \"$merge_tool_keep_backup\" = \"true\"; then\n+\t\t    save_backup\n+\t\telse\n+\t\t    remove_backup\n+\t\tfi\n+\t    fi\n+\t    ;;\n     esac\n     if test \"$status\" -ne 0; then\n \techo \"merge of $path failed\" 1>&2\n@@ -309,12 +324,20 @@ do\n     shift\n done\n \n+valid_custom_tool()\n+{\n+    merge_tool_cmd=\"$(git config mergetool.$1.cmd)\"\n+    test -n \"$merge_tool_cmd\"\n+}\n+\n valid_tool() {\n \tcase \"$1\" in\n \t\tkdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)\n \t\t\t;; # happy\n \t\t*)\n-\t\t\treturn 1\n+\t\t\tif ! valid_custom_tool \"$1\"; then\n+\t\t\t\treturn 1\n+\t\t\tfi\n \t\t\t;;\n \tesac\n }\n@@ -380,10 +403,15 @@ else\n \n     init_merge_tool_path \"$merge_tool\"\n \n-    if ! type \"$merge_tool_path\" > /dev/null 2>&1; then\n+    if test -z \"$merge_tool_cmd\" && ! type \"$merge_tool_path\" > /dev/null 2>&1; then\n         echo \"The merge tool $merge_tool is not available as '$merge_tool_path'\"\n         exit 1\n     fi\n+\n+    if ! test -z \"$merge_tool_cmd\"; then\n+        merge_tool_trust_exit_code=\"$(git config --bool merge.$merge_tool.trustExitCode || echo false)\"\n+        merge_tool_keep_backup=\"$(git config --bool merge.$merge_tool.keepBackup || echo true)\"\n+    fi\n fi\n \n \n-- \n1.5.4.1.144.gb4758f\n"},{"id":"68929","messageId":"7vzlu07jn9.fsf@gitster.siamese.dyndns.org","threadId":"12138","inReplyTo":"20080216185349.GA29177@hashpling.org","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-16T20:04:26Z","receivedAt":"2008-02-16T20:04:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> It follows filter-branch's 'eval a user shell snippet' philosophy to\n> provide the flexibility and here in lies an ugliness. It exposes\n> git-mergetool.sh's private variables to the user script. The variables\n> are BASE, REMOTE, LOCAL and path.\n>\n> My feeling is that we should give this consistent and documented\n> names, perhaps GIT_BASE, GIT_REMOTE, GIT_LOCAL, GIT_MERGED or similar.\n\nI like the general idea and you are right that the external\ninterface should not expose the variable names the current\nimplementation happens to use.  I do not think it is necessary\nto rename them to GIT_BASE etc., but I do think we need to list\nthe repertoire of variables that can be expected to be usable by\ncustom script in any future version of mergetool (even after it\nis rewritten in C).  Anybody who uses a variable that is not in\nthe documented set that the current implementation uses can be\nbroken ;-).\n\nAlso perhaps we would want to spawn the eval in a subprocess to\nmake it clear that the custom script cannot affect the caller's\nvariables.\n\n> Also, does anyone know of any reason why the temporary files should\n> not be cleaned up after an unsuccessful merge?\n\nI do not use mergetool myself, but presumably it would be to\nmake the manual inspection easier.\n\n> +mergetool.<tool>.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\n> +\tis set to `false` then this file is not preserved.\n\nI doubt this belongs to this patch, so does ...\n\n> +\t\tif test \"$merge_tool_keep_backup\" = \"true\"; then\n> +\t\t    save_backup\n> +\t\telse\n> +\t\t    remove_backup\n> +\t\tfi\n> +\t    fi\n\n... this part and anything else that deals with the backup\nfiles in the patch.\n\nShouldn't the handling of back-up files be the same across\nbackends?\n\nIf the answer is yes, it makes mergetool.<tool>.keepBackup\nconfiguration a quite bogus variable, as it is not something you\nwould configure per backend.\n\nIn the existing code, I see kdiff3 arm calls remove_backup while\ntkdiff arm and others call save_backup, which seems quite\ninconsistent.  Perhaps mergetool needs a command line option\n(and perhaps a single configuration variable independent from\nwhich backend is used) to tell what to do about them after a\nconflicting merge is resolved and/or resolution is aborted.\n"},{"id":"68932","messageId":"20080216202055.GA30146@hashpling.org","threadId":"12138","inReplyTo":"7vzlu07jn9.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-02-16T20:20:55Z","receivedAt":"2008-02-16T20:20:55Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sat, Feb 16, 2008 at 12:04:26PM -0800, Junio C Hamano wrote:\n> Charles Bailey <charles@hashpling.org> writes:\n> \n> > It follows filter-branch's 'eval a user shell snippet' philosophy to\n> > provide the flexibility and here in lies an ugliness. It exposes\n> > git-mergetool.sh's private variables to the user script. The variables\n> > are BASE, REMOTE, LOCAL and path.\n> >\n> > My feeling is that we should give this consistent and documented\n> > names, perhaps GIT_BASE, GIT_REMOTE, GIT_LOCAL, GIT_MERGED or similar.\n> \n> I like the general idea and you are right that the external\n> interface should not expose the variable names the current\n> implementation happens to use.  I do not think it is necessary\n> to rename them to GIT_BASE etc., but I do think we need to list\n> the repertoire of variables that can be expected to be usable by\n> custom script in any future version of mergetool (even after it\n> is rewritten in C).  Anybody who uses a variable that is not in\n> the documented set that the current implementation uses can be\n> broken ;-).\n\nOK, I'm fine with this, although I think we should use something other\nthan path (in lower case) for the output.  Either MERGED, RESULT or\nOUTPUT, say, I've no strong preferences.\n\n> Also perhaps we would want to spawn the eval in a subprocess to\n> make it clear that the custom script cannot affect the caller's\n> variables.\n\nSub-shell, good plan.\n\n> > Also, does anyone know of any reason why the temporary files should\n> > not be cleaned up after an unsuccessful merge?\n> \n> I do not use mergetool myself, but presumably it would be to\n> make the manual inspection easier.\n\nI don't use it a lot (yet) either, but I think that this sort of\nchange well help git get wider acceptance in the \"but does it work\nwith my favourite 3-way merge tool\" community.\n\nAs far as it goes, though, I find that if I fire up a merge tool and\nit looks complex, I quite often want to abort and look at the easy\nfiles first.  The .BASE/.LOCAL/.REMOTE files don't contain anything\nthat isn't in the index anyway (the merge tool certainly should be\nupdating them with intermediate state).\n\nIt's a separate issue to the main thrust of this patch, though.\n\n> > +mergetool.<tool>.keepBackup::\n> \n--- snip ---\n\n> Shouldn't the handling of back-up files be the same across\n> backends?\n> \n> If the answer is yes, it makes mergetool.<tool>.keepBackup\n> configuration a quite bogus variable, as it is not something you\n> would configure per backend.\n> \n> In the existing code, I see kdiff3 arm calls remove_backup while\n> tkdiff arm and others call save_backup, which seems quite\n> inconsistent.  Perhaps mergetool needs a command line option\n> (and perhaps a single configuration variable independent from\n> which backend is used) to tell what to do about them after a\n> conflicting merge is resolved and/or resolution is aborted.\n\nYes, I think that you're right. I couldn't work out why kdiff3 was\nspecial, but I took the safe/wrong path of assuming that it should be\nconfigurable per backend. I think that it might be something that\nshould be configurable, but the variable should probably me\nmergetool.keepBackup and the patch should be separate from this one.\n\nCharles.\n"},{"id":"68935","messageId":"m3hcg8bo9f.fsf@localhost.localdomain","threadId":"12138","inReplyTo":"20080216185349.GA29177@hashpling.org","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-02-16T21:11:59Z","receivedAt":"2008-02-16T21:11:59Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> Currently git mergetool is restricted to a set of commands defined\n> in the script. You can subvert the mergetool.<tool>.path to force\n> git mergetool to use a different command, but if you have a command\n> whose invocation syntax does not match one of the current tools then\n> you would have to write a wrapper script for it.\n> \n> This patch adds three git config variable patterns which allow a more\n> flexible choice of merge tool.\n\n[...]\n\n> It follows filter-branch's 'eval a user shell snippet' philosophy to\n> provide the flexibility and here in lies an ugliness. It exposes\n> git-mergetool.sh's private variables to the user script. The variables\n> are BASE, REMOTE, LOCAL and path.\n\nAnother solution would be to use StGit merger / i2merge / i3merge\nformat, similar to git-for-each-ref format, namely to expand\n%(branch1), %(branch2), %(ancestor), %(output) (well, StGit uses\nfor some reason %(ancestor)s etc.; git-for-each-ref doesn't).\n\nAlthough for this would be better if git-mergetool was rewritten\nin C, in Perl (or even in Python).\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"68940","messageId":"CD749541-1B3B-4EA7-82A5-0DFC67B953BE@zib.de","threadId":"12138","inReplyTo":"20080216185349.GA29177@hashpling.org","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-02-16T22:37:31Z","receivedAt":"2008-02-16T22:37:31Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Feb 16, 2008, at 7:53 PM, Charles Bailey wrote:\n\n> Currently git mergetool is restricted to a set of commands defined\n> in the script. You can subvert the mergetool.<tool>.path to force\n> git mergetool to use a different command, but if you have a command\n> whose invocation syntax does not match one of the current tools then\n> you would have to write a wrapper script for it.\n>\n> [...]\n>\n> This is a preliminary patch which aims to make it easier to use\n> a.n.other 3-way merge tool with git-mergetool without either hacking\n> the source or writing a wrapper script for the tool.\n\nWhy not just add the tools you have in mind to git mergetool?  If\neveryone did that eventually we would have direct support for a\nrather long list of tools.  This would be the easiest solution\nfor the end user: He could just choose the preferred tool and use\nit.  The invocation of each merge tool would be coded in\nmergetool.  The exact command line could be fine tuned and would\nbe reviewed by other git developers.\n\n...\n\n> git config --global mergetool.tortoise.cmd 'TortoiseMerge.exe \\\n> /base:\"$BASE\" /theirs:\"$REMOTE\" /mine:\"$LOCAL\" /merged:\"$path\"'\n>\n> git config --global mergetool.p4.cmd 'p4merge.exe \"$BASE\" \"$REMOTE\" \\\n> \"$LOCAL\" \"$path\"'\n>\n> git config --global mergetool.p4win.cmd 'P4WinMrg \"$BASE\" \"$REMOTE\" \\\n> \"$LOCAL\" \"$path\"'\n\n\nIf you just integrated these tools directly I could use them\nright away instead of first reading the documentation of the\ncustom mechanism and then looking up how to configure the right\ncommand line for the tool I want to use.  If the tools were\ndirectly integrated I could for example just say \"git mergetool\n--tool=p4\".\n\nThat does not mean I am opposed to adding a mechanism for\nhandling custom tools.  But easier for the end user would be\nif the tools were directly integrated.\n\n\t\tSteffen\n"},{"id":"68942","messageId":"20080217002029.GA504@hashpling.org","threadId":"12138","inReplyTo":"CD749541-1B3B-4EA7-82A5-0DFC67B953BE@zib.de","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-02-17T00:20:29Z","receivedAt":"2008-02-17T00:20:29Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sat, Feb 16, 2008 at 11:37:31PM +0100, Steffen Prohaska wrote:\n> \n> On Feb 16, 2008, at 7:53 PM, Charles Bailey wrote:\n> \n> >Currently git mergetool is restricted to a set of commands defined\n> >in the script. You can subvert the mergetool.<tool>.path to force\n> >git mergetool to use a different command, but if you have a command\n> >whose invocation syntax does not match one of the current tools then\n> >you would have to write a wrapper script for it.\n> >\n> >[...]\n> >\n> >This is a preliminary patch which aims to make it easier to use\n> >a.n.other 3-way merge tool with git-mergetool without either hacking\n> >the source or writing a wrapper script for the tool.\n> \n> Why not just add the tools you have in mind to git mergetool?  If\n> everyone did that eventually we would have direct support for a\n> rather long list of tools.  This would be the easiest solution\n> for the end user: He could just choose the preferred tool and use\n> it.  The invocation of each merge tool would be coded in\n> mergetool.  The exact command line could be fine tuned and would\n> be reviewed by other git developers.\n> \n\nI have to disagree with this approach. I didn't actually have these\ntools in mind when I wrote the patch, I searched around for tools that\nI've used in the past and tools that I know that other people use. I\nhad in mine the questions that I'd want to be able to answer when\npeople ask me about git. It's not that the list will be large, it's\nthat it will never be complete because there will always be tools that\naren't globally available.\n\nWhen I'm asked \"can I use xyz visual merge tool\" with git there will\nalways be some xyz for which the tool isn't yet in the list. It's a\nlot more difficult to sell \"patch the tool, submit the patch so it\ndoesn't get lost in the next upgrade of git, oh and support the patch\nso that it does get integrated\" than it is to sell \"take the command\nline format, massage it and put it in your system/global config\".\n\n> If you just integrated these tools directly I could use them\n> right away instead of first reading the documentation of the\n> custom mechanism and then looking up how to configure the right\n> command line for the tool I want to use.  If the tools were\n> directly integrated I could for example just say \"git mergetool\n> --tool=p4\".\n> \n> That does not mean I am opposed to adding a mechanism for\n> handling custom tools.  But easier for the end user would be\n> if the tools were directly integrated.\n> \n> \t\tSteffen\n\nSure, it's nice when the tool you want is already integrated but the\nlist is never going to be perfect. This patch was designed to support\nthe fallback position: OK, my tool isn't natively supported, how\neasily can I get it working?\n\nI don't believe that git installs a system config by default, but one\nidea I had was to rip out all of the native tools support in git\nmergetool and replace it with a list of predefined custom tools\nconfigs. This would put all merge tools on an equal footing and should\nmake extra tool support patches simpler and easier to integrate. This\ndoesn't have any legs without a system default config, though.\n\nCharles.\n"},{"id":"68943","messageId":"alpine.LSU.1.00.0802170045210.30505@racer.site","threadId":"12138","inReplyTo":"20080217002029.GA504@hashpling.org","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-17T00:46:15Z","receivedAt":"2008-02-17T00:46:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 17 Feb 2008, Charles Bailey wrote:\n\n> On Sat, Feb 16, 2008 at 11:37:31PM +0100, Steffen Prohaska wrote:\n> \n> > Why not just add the tools you have in mind to git mergetool?  If \n> > everyone did that eventually we would have direct support for a rather \n> > long list of tools.  This would be the easiest solution for the end \n> > user: He could just choose the preferred tool and use it.  The \n> > invocation of each merge tool would be coded in mergetool.  The exact \n> > command line could be fine tuned and would be reviewed by other git \n> > developers.\n> > \n> \n> I have to disagree with this approach.\n\nSo you'd rather have the end users do the same work for the same tool over \nand over again?\n\nCiao,\nDscho\n"},{"id":"68944","messageId":"20080217005620.GB504@hashpling.org","threadId":"12138","inReplyTo":"alpine.LSU.1.00.0802170045210.30505@racer.site","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-02-17T00:56:20Z","receivedAt":"2008-02-17T00:56:20Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sun, Feb 17, 2008 at 12:46:15AM +0000, Johannes Schindelin wrote:\n> Hi,\n> \n> On Sun, 17 Feb 2008, Charles Bailey wrote:\n> \n> > On Sat, Feb 16, 2008 at 11:37:31PM +0100, Steffen Prohaska wrote:\n> > \n> > > Why not just add the tools you have in mind to git mergetool?  If \n> > > everyone did that eventually we would have direct support for a rather \n> > > long list of tools.  This would be the easiest solution for the end \n> > > user: He could just choose the preferred tool and use it.  The \n> > > invocation of each merge tool would be coded in mergetool.  The exact \n> > > command line could be fine tuned and would be reviewed by other git \n> > > developers.\n> > > \n> > \n> > I have to disagree with this approach.\n> \n> So you'd rather have the end users do the same work for the same tool over \n> and over again?\n> \n\nI'm sorry, I should have made myself clearer. I disagree that the\napproach of adding new tool support to the source code as and when they\nare encountered is optimal. I believe that it is preferable to have a\nsolution that allows users to configure, rather then code, support for\ntheir own tools that do not to have native support.\n\nI do not disagree that there is benefit to having a wide range of\ntools that are supported natively.\n\nI thought I made a reasonable argument for this in the rest of my\nemail that you took the headline from, but evidently I came across as\nmuddled.\n\nCharles.\n"},{"id":"68946","messageId":"7vbq6g758h.fsf@gitster.siamese.dyndns.org","threadId":"12138","inReplyTo":"20080217005620.GB504@hashpling.org","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-17T01:15:42Z","receivedAt":"2008-02-17T01:15:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> On Sun, Feb 17, 2008 at 12:46:15AM +0000, Johannes Schindelin wrote:\n>> \n>> So you'd rather have the end users do the same work for the same tool over \n>> and over again?\n>\n> I'm sorry, I should have made myself clearer. I disagree that the\n> approach of adding new tool support to the source code as and when they\n> are encountered is optimal. I believe that it is preferable to have a\n> solution that allows users to configure, rather then code, support for\n> their own tools that do not to have native support.\n>\n> I do not disagree that there is benefit to having a wide range of\n> tools that are supported natively.\n>\n> I thought I made a reasonable argument for this in the rest of my\n> email that you took the headline from, but evidently I came across as\n> muddled.\n\nI do not understand why people are so upset about this.  I think\nthe approach Charles's patch takes is reasonable, with example\nconfigurations to coax a few of his tools to be driven by\nmergetool as backends, that demonstrate the customizing\nframework works well.\n\nIt of course would also be good to throw in the native support\nfor the tools he used as examples but I'd say that they are\ntopics of separate patches.\n"},{"id":"68958","messageId":"FBA2E61E-5CAF-49E3-A917-ACDD10586928@zib.de","threadId":"12138","inReplyTo":"7vbq6g758h.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-02-17T07:59:31Z","receivedAt":"2008-02-17T07:59:31Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Feb 17, 2008, at 2:15 AM, Junio C Hamano wrote:\n\n> Charles Bailey <charles@hashpling.org> writes:\n>\n>> On Sun, Feb 17, 2008 at 12:46:15AM +0000, Johannes Schindelin wrote:\n>>>\n>>> So you'd rather have the end users do the same work for the same  \n>>> tool over\n>>> and over again?\n>>\n>> I'm sorry, I should have made myself clearer. I disagree that the\n>> approach of adding new tool support to the source code as and when  \n>> they\n>> are encountered is optimal. I believe that it is preferable to have a\n>> solution that allows users to configure, rather then code, support  \n>> for\n>> their own tools that do not to have native support.\n>>\n>> I do not disagree that there is benefit to having a wide range of\n>> tools that are supported natively.\n>>\n>> I thought I made a reasonable argument for this in the rest of my\n>> email that you took the headline from, but evidently I came across as\n>> muddled.\n>\n> I do not understand why people are so upset about this.  I think\n> the approach Charles's patch takes is reasonable, with example\n> configurations to coax a few of his tools to be driven by\n> mergetool as backends, that demonstrate the customizing\n> framework works well.\n\nI am not upset at all and I really appreciate Charles work for\nadding a generic mechanism.  I was just wondering why not taking\nthe direct way of adding tools to git-mergetool one by one until\nwe eventually have a rather complete list of supported tools.\nThis would be the easiest solution for end users if their\npreferred tool is supported.  It is also easier to add support\nfor a specific tool than a generic mechanism.\n\nMaybe it is sufficient to refactor \"git mergetool\" to make it\nreally easy to add another tool.  Our users are developers who\nshould know how to add a new tool directly to git mergetool if\nthey find some guidance in the source.\n\nI see two benefits of the direct approach\n\n  - the source code of git mergetool could be kept simpler without\n    a generic configuration mechanism for unknown tools.\n\n  - users would be forced to integrate their tool into git mergetool\n    and hopefully they would send patches and eventually we'd have\n    rather complete arge number of tools supported.\n    (I know at least one case where this pressure helps and I expect\n     to see patches that we would not see if a generic mechanism\n     was available.)\n\n\n> It of course would also be good to throw in the native support\n> for the tools he used as examples but I'd say that they are\n> topics of separate patches.\n\n\nI believe this is more important than a generic mechanism.\n\nHowever, I am not opposed to a generic mechanism, ...\n\n\nOn Feb 17, 2008, at 1:20 AM, Charles Bailey wrote:\n>>\n>> I don't believe that git installs a system config by default, but one\n>> idea I had was to rip out all of the native tools support in git\n>> mergetool and replace it with a list of predefined custom tools\n>> configs. This would put all merge tools on an equal footing and  \n>> should\n>> make extra tool support patches simpler and easier to integrate. This\n>> doesn't have any legs without a system default config, though.\n\n... but I am slightly opposed to this idea.  Note that at least\nin one case there is a trick needed to launch the tool.  Such a\ntrick can easily be coded if the tool is directly added in\n\"git mergetool\"; but it would be much harder to capture by a\ngeneric mechanism via config variables.  The example I mean is\nopendiff that needs to be piped to cat (opendiff ... | cat).\nOtherwise opendiff detaches FileMerge and returns immediately\nwithout waiting for the user to complete the merge.\n\n\t\tSteffen\n"},{"id":"68967","messageId":"20080217101538.GA21823@hashpling.org","threadId":"12138","inReplyTo":"FBA2E61E-5CAF-49E3-A917-ACDD10586928@zib.de","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-02-17T10:15:38Z","receivedAt":"2008-02-17T10:15:38Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sun, Feb 17, 2008 at 08:59:31AM +0100, Steffen Prohaska wrote:\n> On Feb 17, 2008, at 1:20 AM, Charles Bailey wrote:\n> >>\n> >>I don't believe that git installs a system config by default, but one\n> >>idea I had was to rip out all of the native tools support in git\n> >>mergetool and replace it with a list of predefined custom tools\n> >>configs. This would put all merge tools on an equal footing and  \n> >>should\n> >>make extra tool support patches simpler and easier to integrate. This\n> >>doesn't have any legs without a system default config, though.\n> \n> ... but I am slightly opposed to this idea.  Note that at least\n> in one case there is a trick needed to launch the tool.  Such a\n> trick can easily be coded if the tool is directly added in\n> \"git mergetool\"; but it would be much harder to capture by a\n> generic mechanism via config variables.  The example I mean is\n> opendiff that needs to be piped to cat (opendiff ... | cat).\n> Otherwise opendiff detaches FileMerge and returns immediately\n> without waiting for the user to complete the merge.\n\nI just wanted to say that in my patch, as the configuration is a\nsub-shell, that it is perfectly possible to do this in a custom\nmergetool as well.\n\nI have different reasons for not liking a default system config file,\nnamely the whole customize vs. updgrade conflict issues.\n\nAn alternative that I have considered is to include a\n$(sharedir)/gitcore/mergetools.gitconfig which could contain the\ndefault native mergetool configs (both the commands and the\n'trustExitCode' settings.\n\nSubmitting a new merge tool patch becomes a simple matter of: \"I've\nused and tested this in my system (or global) gitconfig, please add to\nthe git distribution mergetool.gitconfig.\"\n\nI know that most git developers are just as at ease (if not more so)\nediting a git shell script as they are at using git config but I still\nbelieve that there is a significant group of users and potential users\nfor whom there is an import barrier between configuring software and\n'having' to hack it to get it to work.\n\nCharles.\n"},{"id":"69043","messageId":"20080217214942.GJ8905@mit.edu","threadId":"12138","inReplyTo":"FBA2E61E-5CAF-49E3-A917-ACDD10586928@zib.de","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Theodore Tso","fromEmail":"tytso@mit.edu","sentAt":"2008-02-17T21:49:42Z","receivedAt":"2008-02-17T21:49:42Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Sun, Feb 17, 2008 at 08:59:31AM +0100, Steffen Prohaska wrote:\n>\n> I am not upset at all and I really appreciate Charles work for\n> adding a generic mechanism.  I was just wondering why not taking\n> the direct way of adding tools to git-mergetool one by one until\n> we eventually have a rather complete list of supported tools.\n> This would be the easiest solution for end users if their\n> preferred tool is supported.  It is also easier to add support\n> for a specific tool than a generic mechanism.\n\nI have no objection to a generic mechanism, but I don't see the value\nof Charles suggestion to rip out support for the existing tools\nsupported by git-mergetool.\n\nI think it *would* be better to use %(foo) extrapolation that\nenvironment variables, so that it's not required for users to write\nshell scripts unless absolutely necessary.\n\nWhen we get around to rewriting git-mergetool in C, it might make\nsense to put the tool support in the various shell scripts that are\ninstalled in the git helper binary directory (i.e.,\ngit-mergetool-kdiff3, git-mergetool-meld, etc.)  That would make it\neasier for users to create new shell scripts to support new tools if\nnecessary.\n\n\t\t\t\t\t- Ted\n"},{"id":"69062","messageId":"20080217232858.GA6249@hashpling.org","threadId":"12138","inReplyTo":"20080217214942.GJ8905@mit.edu","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-02-17T23:28:58Z","receivedAt":"2008-02-17T23:28:58Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sun, Feb 17, 2008 at 04:49:42PM -0500, Theodore Tso wrote:\n> I think it *would* be better to use %(foo) extrapolation that\n> environment variables, so that it's not required for users to write\n> shell scripts unless absolutely necessary.\n\nI understand that %(foo) is neater, but this would make the shell\nimplementation significantly more complex, and also potentially less\nflexible (e.g. if you needed to do the opendiff style |cat trick or\nsome similar shell trickery).\n\nThe reason that I first posted a patch for a different config variable\nfor baseless merges was because I was that a significant number of\nmerge tools have significantly different syntaxes for two and three\nway merges and I thought that it might be easier for users to do\nsomething like:\n\ngit config mergetool.mymerge.cmd 'mytool --3way \"$BASE\" \"$LOCAL\" \\\n\"$REMOTE\" \"$MERGED\"'\n\ngit config mergetool.mymerge.cmdNoBase 'mytool --2way \"$LOCAL\" \\\n\"$REMOTE\" \"$MERGED\"'\n\nthan it would be to do something like (totally untested):\n\ngit config mergetool.mymerge.cmd 'if test -f \"$BASE\"; then;\\\n mytool --3way \"$BASE\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"; else; \\\n mytool --2way \"$LOCAL\" \"$REMOTE\" \"$MERGED\"; fi'\n\nSo in the former case you are still using shell syntax, but a simple\nsubset of shell should suffice for most needs.  The later case\nrequires more wizardry.\n\n> When we get around to rewriting git-mergetool in C, it might make\n> sense to put the tool support in the various shell scripts that are\n> installed in the git helper binary directory (i.e.,\n> git-mergetool-kdiff3, git-mergetool-meld, etc.)  That would make it\n> easier for users to create new shell scripts to support new tools if\n> necessary.\n> \n> \t\t\t\t\t- Ted\n\nThis makes sense to me, although I have to say that I'm not really\nsure I see the value of turning git-mergetool into C.  It seems to\nmake a lot of sense as a shell helper, but is it a general principle\nthat all of git's commands should eventually be converted to C?\n\nCharles.\n"},{"id":"69063","messageId":"20080217234150.GB6249@hashpling.org","threadId":"12138","inReplyTo":"20080217214942.GJ8905@mit.edu","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-02-17T23:41:50Z","receivedAt":"2008-02-17T23:41:50Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sun, Feb 17, 2008 at 04:49:42PM -0500, Theodore Tso wrote:\n> I have no objection to a generic mechanism, but I don't see the value\n> of Charles suggestion to rip out support for the existing tools\n> supported by git-mergetool.\n\nApologies for the multiple replies, I just remembered that I didn't\ncomment on this part.\n\nMy suggestion was really just me thinking aloud (\"one idea I had...\").\nI was only stating that it would be possible to do this, there's no\nvalue in actually doing this on its own, but the thought exercise\nhelped me validate my patch (at least to myself). If my patch were\nflexible enough to handle all of the current built-in tools in a\ngeneric fashion then it is a good sign that it should be able to cope\nwith a good portion of (as yet) unknown merge tools which is, after\nall, the main point of my patch.\n\nCharles.\n"},{"id":"69066","messageId":"7vir0nxg0c.fsf@gitster.siamese.dyndns.org","threadId":"12138","inReplyTo":"20080217214942.GJ8905@mit.edu","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-18T00:30:43Z","receivedAt":"2008-02-18T00:30:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Theodore Tso <tytso@MIT.EDU> writes:\n\n> I have no objection to a generic mechanism, but I don't see the value\n> of Charles suggestion to rip out support for the existing tools\n> supported by git-mergetool.\n\nI missed that suggestion but I agree removing existing support\nwould not make much sense.\n\n> I think it *would* be better to use %(foo) extrapolation that\n> environment variables, so that it's not required for users to write\n> shell scripts unless absolutely necessary.\n\nHmm, although I do not have strong opinions either way, I think\nthe necessary interface is narrow enough that we could use\nenvironment variables here.  Charles's implementation does\n\"eval\" but it is easy to replace it to run the custom command\nafter exporting the necessary variables, isn't it?\n\n> When we get around to rewriting git-mergetool in C, it might make\n> sense to put the tool support in the various shell scripts that are\n> installed in the git helper binary directory (i.e.,\n> git-mergetool-kdiff3, git-mergetool-meld, etc.)  That would make it\n> easier for users to create new shell scripts to support new tools if\n> necessary.\n\nYup.\n"},{"id":"69098","messageId":"20080218081446.GB12008@hashpling.org","threadId":"12138","inReplyTo":"7vir0nxg0c.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-02-18T08:14:46Z","receivedAt":"2008-02-18T08:14:46Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sun, Feb 17, 2008 at 04:30:43PM -0800, Junio C Hamano wrote:\n> Theodore Tso <tytso@MIT.EDU> writes:\n> \n> > I have no objection to a generic mechanism, but I don't see the value\n> > of Charles suggestion to rip out support for the existing tools\n> > supported by git-mergetool.\n> \n> I missed that suggestion but I agree removing existing support\n> would not make much sense.\n\nAs I said in an earlier reply this was really more of a thought\nexercise about my patch than a serious suggestion for integration.\n\n> > I think it *would* be better to use %(foo) extrapolation that\n> > environment variables, so that it's not required for users to write\n> > shell scripts unless absolutely necessary.\n> \n> Hmm, although I do not have strong opinions either way, I think\n> the necessary interface is narrow enough that we could use\n> environment variables here.  Charles's implementation does\n> \"eval\" but it is easy to replace it to run the custom command\n> after exporting the necessary variables, isn't it?\n\nDo you mean instead of:\n\n( eval $tool )\n\nsomething like:\nBASE=\"$BASE\" LOCAL=\"$LOCAL\" REMOTE=\"$REMOTE\" MERGED=\"$MERGED\" $tool\n\nIn this case we can skip the whole s/path/MERGED/ patch as it is\nunnecessary, and should we now GIT_ prefix the variables as they will\nintrude on the environment of the spawned command (not just a specific\nsub-shell)? \n\nLet me know what you think and I can integrate it into my next patch\nversion.\n\nCharles.\n"}]}