{"thread":{"id":"56741","subject":"[RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant","startedAt":"2021-10-19T21:20:30Z","lastAt":"2021-10-27T16:47:00Z","messageCount":6,"participants":["Fernando Ramos","David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"439043","messageId":"20211019212020.25385-1-greenfoo@u92.eu","threadId":"56741","inReplyTo":null,"subject":"[RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2021-10-19T21:20:20Z","receivedAt":"2021-10-19T21:20:30Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"This new vimdiff4 variant of the merge-tool opens three tabs:\n\n - The first one contains the same panes as the standard \"vimdiff\" (ie.\n LOCAL, BASE and REMOTE in the top row and MERGED in the bottom row).\n\n ------------------------------------------\n | | TAB #2 | TAB #3 | |\n ------------------------------------------\n | | | |\n | LOCAL | BASE | REMOTE |\n | | | |\n ------------------------------------------\n | |\n | MERGED |\n | |\n ------------------------------------------\n\n NOTE: This view is enough for 90% of the cases, but when the merge is\n somewhat complex, the three-way differences representation\n end up being messy. That is why two new tabs are added to\n show isolated one-to-one diffs.\n\n - The second one is a vertical diff between BASE and LOCAL\n\n ------------------------------------------\n | TAB #1 | | TAB #3 | |\n ------------------------------------------\n | | |\n | | |\n | | |\n | BASE | LOCAL |\n | | |\n | | |\n | | |\n ------------------------------------------\n\n - The third one is a vertical diff between BASE and REMOTE\n\n ------------------------------------------\n | TAB #1 | TAB #2 | | |\n ------------------------------------------\n | | |\n | | |\n | | |\n | BASE | REMOTE |\n | | |\n | | |\n | | |\n ------------------------------------------\n\nSigned-off-by: Fernando Ramos \n---\n mergetools/vimdiff | 12 +++++++++++-\n t/t7610-mergetool.sh | 1 +\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex 96f6209a04..f830b1ed95 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -40,6 +40,16 @@ merge_cmd () {\n \t\t\t\t\"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n \t\tfi\n \t\t;;\n+\t*vimdiff4)\n+\t\tif $base_present\n+\t\tthen\n+\t\t\t\"$merge_tool_path\" -f -d -c \"4wincmd w | wincmd J | tabnew | edit $LOCAL | vertical diffsplit $BASE | tabnew | edit $REMOTE | vertical diffsplit $BASE | 2tabprevious\" \\\n+\t\t\t\t\"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n+\t\telse\n+\t\t\t\"$merge_tool_path\" -f -d -c 'wincmd l' \\\n+\t\t\t\t\"$LOCAL\" \"$MERGED\" \"$REMOTE\"\n+\t\tfi\n+\t\t;;\n \tesac\n }\n \n@@ -63,7 +73,7 @@ exit_code_trustable () {\n \n list_tool_variants () {\n \tfor prefix in '' g n; do\n-\t\tfor suffix in '' 1 2 3; do\n+\t\tfor suffix in '' 1 2 3 4; do\n \t\t\techo \"${prefix}vimdiff${suffix}\"\n \t\tdone\n \tdone\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 8cc64729ad..755b4c0a4a 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -836,6 +836,7 @@ test_expect_success 'mergetool --tool-help shows recognized tools' '\n \tgit mergetool --tool-help >mergetools &&\n \tgrep vimdiff mergetools &&\n \tgrep vimdiff3 mergetools &&\n+\tgrep vimdiff4 mergetools &&\n \tgrep gvimdiff2 mergetools &&\n \tgrep araxis mergetools &&\n \tgrep xxdiff mergetools &&\n-- \n2.33.1\n\n"},{"id":"439046","messageId":"YW9C4KvsXSQOCX/w@zacax395.localdomain","threadId":"56741","inReplyTo":"20211019212020.25385-1-greenfoo@u92.eu","subject":"Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2021-10-19T22:12:48Z","receivedAt":"2021-10-19T22:12:54Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"On 21/10/19 11:20PM, Fernando Ramos wrote:\n> This new vimdiff4 variant of the merge-tool opens three tabs...\n> \n\nOne reason why this RFC should *not* be merged is that the same functionality\ncan be achieved right now by simply adding these two lines to\n\"~/.config/git/config\":\n\n [mergetool \"vimdiff4\"]\n \tcmd = vim -f -d -c \\\"4wincmd w | wincmd J | tabnew | edit $LOCAL | vertical diffsplit $BASE | tabnew | edit $REMOTE | vertical diffsplit $BASE | 2tabprevious\\\" \\\"$LOCAL\\\" \\\"$BASE\\\" \\\"$REMOTE\\\" \\\"$MERGED\\\"\n \ttrustExitCode = true\n\nOn the other hand, one reason to merge it is that this new windows/tabs\narrangement is so useful when solving complex merges, that it would be nice to\nhave it available by default for easier discoverablility.\n\nLet me know what you think :)\n"},{"id":"439511","messageId":"CAJDDKr5frTgh4_x5yvskJfppew3ntvpgBe9MnUB9CfGQaw1TLQ@mail.gmail.com","threadId":"56741","inReplyTo":"20211019212020.25385-1-greenfoo@u92.eu","subject":"Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-10-24T22:54:49Z","receivedAt":"2021-10-24T22:55:56Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Oct 19, 2021 at 2:22 PM Fernando Ramos wrote:\n>\n> This new vimdiff4 variant of the merge-tool opens three tabs:\n>\n> - The first one contains the same panes as the standard \"vimdiff\" (ie.\n> LOCAL, BASE and REMOTE in the top row and MERGED in the bottom row).\n>\n> ------------------------------------------\n> | | TAB #2 | TAB #3 | |\n> ------------------------------------------\n> | | | |\n> | LOCAL | BASE | REMOTE |\n> | | | |\n> ------------------------------------------\n> | |\n> | MERGED |\n> | |\n> ------------------------------------------\n>\n> NOTE: This view is enough for 90% of the cases, but when the merge is\n> somewhat complex, the three-way differences representation\n> end up being messy. That is why two new tabs are added to\n> show isolated one-to-one diffs.\n>\n> - The second one is a vertical diff between BASE and LOCAL\n>\n> ------------------------------------------\n> | TAB #1 | | TAB #3 | |\n> ------------------------------------------\n> | | |\n> | | |\n> | | |\n> | BASE | LOCAL |\n> | | |\n> | | |\n> | | |\n> ------------------------------------------\n>\n> - The third one is a vertical diff between BASE and REMOTE\n>\n> ------------------------------------------\n> | TAB #1 | TAB #2 | | |\n> ------------------------------------------\n> | | |\n> | | |\n> | | |\n> | BASE | REMOTE |\n> | | |\n> | | |\n> | | |\n> ------------------------------------------\n>\n> Signed-off-by: Fernando Ramos \n> ---\n> mergetools/vimdiff | 12 +++++++++++-\n> t/t7610-mergetool.sh | 1 +\n> 2 files changed, 12 insertions(+), 1 deletion(-)\n\n\nThanks for including the visual diagrams (which I hope gmail doesn't mangle).\nThat makes it much easier to see what's going on.\n\nI'm personally not opposed to the vimdiff4 variants (we already have 3\nothers) but what I think might be missing is a bit of documentation\nthat documents the builtin tools and their variants.\n\nRight now git-mergetool.txt includes config/mergetool.txt for\ndocumenting its config variables. It might be worth having a common\n\"mergetools.txt\" where the builtin tools and variants can be\ndocumented and then we can include that file from both\ngit-mergetool.txt and git-difftool.txt.\n\nThat would be a good place to write up the differences between the\nvariants, and the diagram you included in the commit message would be\nhelpful there as well.\n\n\n\n>\n> diff --git a/mergetools/vimdiff b/mergetools/vimdiff\n> index 96f6209a04..f830b1ed95 100644\n> --- a/mergetools/vimdiff\n> +++ b/mergetools/vimdiff\n> @@ -40,6 +40,16 @@ merge_cmd () {\n> \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n> fi\n> ;;\n> + *vimdiff4)\n> + if $base_present\n> + then\n> + \"$merge_tool_path\" -f -d -c \"4wincmd w | wincmd J | tabnew | edit $LOCAL | vertical diffsplit $BASE | tabnew | edit $REMOTE | vertical diffsplit $BASE | 2tabprevious\" \\\n> + \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n> + else\n> + \"$merge_tool_path\" -f -d -c 'wincmd l' \\\n> + \"$LOCAL\" \"$MERGED\" \"$REMOTE\"\n> + fi\n> + ;;\n> esac\n> }\n\n\nIt's pretty rad how we're able to get that much vim goodness out of\nthis snippet of configuration.\n\nThere seems to be an issue here, though. The $LOCAL values are passed\nto the \"edit $LOCAL\", \"edit $REMOTE\" and \"vertical diffsplit $BASE\"\ncommands as-is. It seems like this would break when the filenames\ncontain spaces. Is that correct?\n\nIf so, does vimscript have a way to quote those arguments? Does\nsurrounding the variable with escaped double-quotes (\"... | edit\n\\\"$LOCAL\\\" | ...\") work? (... for everything except files with\nembedded double-quotes in their name, which might be an acceptable\nlimitation).\n\n\n\n>\n> @@ -63,7 +73,7 @@ exit_code_trustable () {\n>\n> list_tool_variants () {\n> for prefix in '' g n; do\n> - for suffix in '' 1 2 3; do\n> + for suffix in '' 1 2 3 4; do\n\n\nPre-existing, but we typically try to avoid multiple statements on a\nsingle line. It seems worth fixing this up in a preparatory patch\nsince we're touching these lines.\n\nfor prefix in '' g n\ndo\n for suffix in '' 1 2 3 4\n do\n ...\n done\ndone\n\n\n\n> echo \"${prefix}vimdiff${suffix}\"\n> done\n> done\n> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\n> index 8cc64729ad..755b4c0a4a 100755\n> --- a/t/t7610-mergetool.sh\n> +++ b/t/t7610-mergetool.sh\n> @@ -836,6 +836,7 @@ test_expect_success 'mergetool --tool-help shows recognized tools' '\n> git mergetool --tool-help >mergetools &&\n> grep vimdiff mergetools &&\n> grep vimdiff3 mergetools &&\n> + grep vimdiff4 mergetools &&\n> grep gvimdiff2 mergetools &&\n> grep araxis mergetools &&\n> grep xxdiff mergetools &&\n\nLooks good otherwise, thanks for the RFC patch. I'd recommend getting\nthe docs and quoting stuff sorted out as the next step towards getting\nthis merged.\n\nThanks!\n\n--\nDavid\n"},{"id":"439575","messageId":"xmqqk0i10xzt.fsf@gitster.g","threadId":"56741","inReplyTo":"CAJDDKr5frTgh4_x5yvskJfppew3ntvpgBe9MnUB9CfGQaw1TLQ@mail.gmail.com","subject":"Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-25T18:18:14Z","receivedAt":"2021-10-25T18:18:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> I'm personally not opposed to the vimdiff4 variants (we already have 3\n> others) but what I think might be missing is a bit of documentation\n> that documents the builtin tools and their variants.\n\nHmph, are we encouraging everybody to add yet another variant? I\nwonder if we can stop at adding a single \"vimdiffX\" variant that\ntakes the layout information (like the one this vimdiff4 passes to\nthe underlying tool via the command line option) in a configuration\nvariable and stop adding more variants, or is vim's specification of\nthe layout we use here via the command line not flexible enough to\nserve all future needs? I also wonder if all the existing vimdiff\nvariants can be done in terms of such a vimdiffX implementation.\n\n> Right now git-mergetool.txt includes config/mergetool.txt for\n> documenting its config variables. It might be worth having a common\n> \"mergetools.txt\" where the builtin tools and variants can be\n> documented and then we can include that file from both\n> git-mergetool.txt and git-difftool.txt.\n>\n> That would be a good place to write up the differences between the\n> variants, and the diagram you included in the commit message would be\n> helpful there as well.\n\nYup, in any case, I do like the suggestion to document the variants.\n\nThanks, both.\n"},{"id":"439584","messageId":"YXcP6kf3tGr+WFRS@zacax395.localdomain","threadId":"56741","inReplyTo":"xmqqk0i10xzt.fsf@gitster.g","subject":"Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2021-10-25T20:13:30Z","receivedAt":"2021-10-25T20:13:43Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"Thanks David and Juno for your feedback.\n\nI completely agree that adding another vimdiffX variant is ... not elegant.\n\nSo I've been thinking a bit more about how this whole \"vim layout\" mechanism can\nbe made more generic and this is what I came up with:\n\n 1. Let's add a new configuration variable to the \"vimdiff\" merge tool called\n \"layout\":\n\n [mergetool \"vimdiff\"]\n layout = ...\n\n 2. If this new variable is *not* present, vim will behave in the same way it\n does today (ie. a top row with the local, base and remote buffers, and a\n bottom row with the merged buffer).\n\n 3. In all other cases, the contents of the \"layout\" variable will be\n intepreted following these rules:\n\n - \";\" is used to separate \"tab descriptors\"\n - \",\" is used to separate \"row descriptors\"\n - \"|\" is used to separate \"column descriptors\"\n - \"+\" is used to load buffers that won't be displayed by default\n\n This will be better understood with some examples that emulate the behavior\n of the current \"vimdiff\", \"vimdiff1\", \"vimdiff2\" and \"vimdiff3\" variants as\n well as the proposed \"vimdiff4\" one:\n\n\n vimdiff --> layout = \"LOCAL | BASE | REMOTE, MERGED\"\n\n ------------------------------------------\n | | | |\n | LOCAL | BASE | REMOTE |\n | | | |\n ------------------------------------------\n | |\n | MERGED |\n | |\n ------------------------------------------\n\n\n vimdiff1 --> layout = \"LOCAL* | REMOTE\"\n\n ------------------------------------------\n | | |\n | | |\n | | |\n | LOCAL | REMOTE |\n | | |\n | | |\n | | |\n ------------------------------------------\n\n NOTE: In this case (where there is no \"MERGED\"\n buffer specified in the \"layout\" string), a \"*\"\n is needed to indicate which file will be the one\n containing the final version of the file after\n resolving conflicts.\n\n\n vimdiff2 --> layout = \"LOCAL | MERGED | REMOTE\"\n\n ------------------------------------------\n | | | |\n | | | |\n | | | |\n | LOCAL | BASE | REMOTE |\n | | | |\n | | | |\n | | | |\n | | | |\n ------------------------------------------\n\n\n vimdiff3 --> layout = \"LOCAL + REMOTE + BASE + MERGED\"\n\n ------------------------------------------\n | |\n | |\n | |\n | MERGED |\n | |\n | |\n | |\n ------------------------------------------\n\n NOTE: LOCAL, REMOTE and BASE are loaded as hidden\n buffers and you need to recall them explicitely.\n\n\n vimdiff4 --> layout = \"BASE | LOCAL | REMOTE, MERGED; BASE | LOCAL; BASE | REMOTE\"\n \n ------------------------------------------\n | | TAB #2 | TAB #3 | |\n ------------------------------------------\n | | | |\n | LOCAL | BASE | REMOTE |\n | | | |\n ------------------------------------------\n | |\n | MERGED |\n | |\n ------------------------------------------\n\n ------------------------------------------\n | TAB #1 | | TAB #3 | |\n ------------------------------------------\n | | |\n | | |\n | | |\n | BASE | LOCAL |\n | | |\n | | |\n | | |\n ------------------------------------------\n\n ------------------------------------------\n | TAB #1 | TAB #2 | | |\n ------------------------------------------\n | | |\n | | |\n | | |\n | BASE | REMOTE |\n | | |\n | | |\n | | |\n ------------------------------------------\n\n\nThe nice thing about this approach is that, as we have seen, it is generic\nenough to rule all current variants obsolete.\n\nSo, please let me know what you think about this:\n\n * Do you like this approach? Or am I trying to crack a nut with a sledgehammer\n by making the whole thing too complex?\n\n * In case you like it, should we keep the old \"vimdiff1\", \"vimdiff2\" and\n \"vimdiff3\" variants for backwards compatibility?\n If the answer is \"yes\", I'll just alias them to the new \"layout\" mechanism\n so that the amount of extra code needed for supporting them is minimal.\n\nIf you tell me you like this proposal, I'll go ahead and implement a patch for\nall of this, taking into consideration David's suggestions for avoiding problems\nwith file with spaces in their names and also adding new documentation for all\nof this.\n\n NOTE: The only non-trivial thing about implementing this is how to parse the\n \"layout\" variable syntax *in bash* to convert it into a sequence of vim\n commands that achieves the expected outcome... but seems like a funny\n weekend project :)\n\nIf you think it is not worth the effort, let me know if it is OK to just add\n\"vimdiff4\" + documentation instead for now (or something else).\n\nThanks.\n \n\n"},{"id":"439792","messageId":"xmqqy26ewh33.fsf@gitster.g","threadId":"56741","inReplyTo":"YXcP6kf3tGr+WFRS@zacax395.localdomain","subject":"Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-27T16:46:56Z","receivedAt":"2021-10-27T16:47:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fernando Ramos writes:\n\n> The nice thing about this approach is that, as we have seen, it is generic\n> enough to rule all current variants obsolete.\n\nI am not a vimdiff user (or mergetools in general---I should use it\nas appropriately from time to time myself), but it would be great if\nsuch a general \"layout rule language\" can be used to replicate the\nexisting variants.\n\n> So, please let me know what you think about this:\n>\n> * Do you like this approach? Or am I trying to crack a nut with a sledgehammer\n> by making the whole thing too complex?\n>\n> * In case you like it, should we keep the old \"vimdiff1\", \"vimdiff2\" and\n> \"vimdiff3\" variants for backwards compatibility?\n> If the answer is \"yes\", I'll just alias them to the new \"layout\" mechanism\n> so that the amount of extra code needed for supporting them is minimal.\n\nIt is great that the new \"description based layout\" can replicate\nthe existing variants, and it is natural migration path to redo the\nexisting variants with the new mechanism once the dust settles as a\nseparate step. The end-users should be able to rely on their\nexisting configuration to keep working the same way as they are used\nto.\n\nThanks.\n"}]}