{"thread":{"id":"56843","subject":"[PATCH 0/3] vimdiff: new layout option + docs","startedAt":"2021-11-04T16:10:10Z","lastAt":"2021-11-08T19:09:00Z","messageCount":9,"participants":["Fernando Ramos","David Aguilar","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"440460","messageId":"20211104160959.183402-1-greenfoo@u92.eu","threadId":"56843","inReplyTo":null,"subject":"[PATCH 0/3] vimdiff: new layout option + docs","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2021-11-04T16:09:56Z","receivedAt":"2021-11-04T16:10:10Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"A few weeks ago I presented this RFC [1] where I introduced a new variant of the\nvimdiff merge tool (\"vimdiff4\") that creates three tabs (instead of just one)\nthat look like this:\n\n ------------------------------------------\n | | TAB #2 | TAB #3 | |\n ------------------------------------------\n | | | |\n | LOCAL | BASE | REMOTE |\n | | | | <---- Same information\n ------------------------------------------ presented by the\n | | \"standard\" vimdiff\n | MERGED | merge tool\n | |\n ------------------------------------------\n \n ------------------------------------------\n | TAB #1 | | TAB #3 | |\n ------------------------------------------\n | | |\n | | |\n | | |\n | BASE | LOCAL | <---- Only differences\n | | | between BASE and\n | | | LOCAL are shown\n | | |\n ------------------------------------------\n \n ------------------------------------------\n | TAB #1 | TAB #2 | | |\n ------------------------------------------\n | | |\n | | |\n | | |\n | BASE | REMOTE | <---- Only differences\n | | | between BASE and\n | | | REMOTE are shown\n | | |\n ------------------------------------------\n\n\nThe motivation behind this was that, for non-trivial merges, the three way diff\npresented in the first tab tends to be very confusing and in these cases\nindivial diffs between BASE and LOCAL and between BASE and REMOTE are very\nuseful.\n\nI have been using a \"custom\" merge tool for months to achieve this same result\nby adding these lines to my .gitconfig file:\n\n [mergetool \"supermerge\"]\n cmd = 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 trustExitCode = true\n\n...and, because I found this \"trick\" very useful, I thought it would be a good\nidea to add it as a git built-in merge tool (called \"vimdiff4\" because 1, 2 and\n3 had already been taken) for everyone to use... and that's exactly what the RFC\nI published did.\n\nNow... as you can see in the RFC thread [1], David and Juno suggested that\nmaybe, instead of creating *yet another vimdiff variant*, we should take this\nopportunity to:\n\n * Come up with a more general way of defining arbitrary vim layouts.\n \n * Re-implement \"vimdiff1\", \"vimdiff2\" and \"vimdiff3\" using this new mechanism\n (after all, the only difference among them is that they present different\n layouts to the user)\n\n * Add documentation to all of this.\n\nAnd the result of that work is what I'm presenting today :)\n\nSome things I would like to mention:\n\n 1. There are three commits in this patch series:\n\n - The first one implements the logic to generate new arbitrary layouts and\n also re-defines \"vimdiff1\", \"vimdiff2\" and \"vimdiff3\" on top of it.\n\n - The second one adds documentation. It is probably a good idea to start\n reviewing this commit before the first one!\n\n - The last commit *is not meant to be merged now*. It removes \"vimdiff1\",\n \"vimdiff2\" and \"vimdiff3\", which is something that should only be done\n after one or two releases with a deprecation notice and only if everyone\n agrees to do so :)\n\n 2. \"mergetools/vimdiff\" is now a ~800 lines bash script, but most of it is\n documentation (which is embedded in the tool itself for easier maintenance)\n and unit tests.\n I have only tested it with bash, but I've tried not to use any command not\n already being used somewhere else, so I expect it to work in the same\n places it was working before (however, let me know if there are some shell\n compatibility requirements and I'll try to check them).\n\n 3. Regarding unit tests, \"mergetool/vimdiff\" contains instructions on how to\n run them (just call the script without arguments after making changes, to\n make sure you didn't break anything).\n Right now it prints \"OK\" on all test cases (obviously) [2]\n\n 3. The \"git {diff,merge}tool --tool-help\" command now also prints the\n documentation for each tool (instead of just its name, as before).\n You can see an example of the output here ([3] and [4])\n\nFinally, let me say that, while I like what this patch series achieves, I would\nalso *completely* understand if you decide not to merge it due to being a\ncomplex solution to a simple problem that can be solved (as I had been doing up\nuntil today) by just adding three line to one's .gitconfig.\n\n [mergetool \"supermerge\"]\n cmd = vim -f -d -c ...(custom complex sequence of vim commands)...\n trustExitCode = true\n\nLet me know what you think.\n\nThanks.\n\n\nReferences:\n\n [1] https://lore.kernel.org/git/20211019212020.25385-1-greenfoo@u92.eu/#r\n [2] https://pastebin.com/kuQ5pETG\n [3] https://pastebin.com/yvLWxeiM\n [4] https://pastebin.com/qNc7qymp\n\n\nFernando Ramos (3):\n vimdiff: new implementation with layout support\n vimdiff: add tool documentation\n vimdiff: remove deprecated {,g,n}vimdiff{1,2,3} variants\n\n git-mergetool--lib.sh | 12 +\n mergetools/vimdiff | 697 ++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 677 insertions(+), 32 deletions(-)\n\n\nbase-commit: 876b1423317071f43c99666f3fc3db3642dfbe14\n-- \n2.33.1\n\n"},{"id":"440461","messageId":"20211104160959.183402-2-greenfoo@u92.eu","threadId":"56843","inReplyTo":"20211104160959.183402-1-greenfoo@u92.eu","subject":"[PATCH 1/3] vimdiff: new implementation with layout support","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2021-11-04T16:09:57Z","receivedAt":"2021-11-04T16:10:13Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"When running 'git mergetool -t vimdiff', a new configuration option\n('mergetool.vimdiff.layout') can now be used to select how the user\nwants the different windows, tabs and buffers to be displayed.\n\nIf the option is not provided, the layout will be the same one that was\nbeing used before this commit (ie. two rows with LOCAL, BASE and COMMIT\nin the top one and MERGED in the bottom one).\n\nThe 'vimdiff' variants ('vimdiff{1,2,3}') still work but, because they\nrepresented nothing else than different layouts, are now internally\nimplemented as a subcase of 'vimdiff' with the corresponding\npre-configured 'layout'.\n\nSigned-off-by: Fernando Ramos \n---\n mergetools/vimdiff | 530 ++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 504 insertions(+), 26 deletions(-)\n\ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex 96f6209a04..1f2e88777e 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -1,48 +1,509 @@\n+#!/bin/bash\n+\n+# This script can be run in two different contexts:\n+#\n+# - From git, when the user invokes the \"vimdiff\" merge tool. In this context\n+# this script expects the following environment variables (among others) to\n+# be defined (which is something \"git\" takes care of):\n+#\n+# - $BASE\n+# - $LOCAL\n+# - $REMOTE\n+# - $MERGED\n+#\n+# In this mode, all this script does is to run the next command:\n+#\n+# vim -f -c ... $LOCAL $BASE $REMOTE $MERGED\n+#\n+# ...where the \"...\" string depends on the value of the\n+# \"mergetool.vimdiff.layout\" configuration variable and is used to open vim\n+# with a certain layout of buffers, windows and tabs.\n+#\n+# - From the shell, manually. This is only expected to be done by developers\n+# who are editing this script. When this happens, the script runs a battery\n+# of unit tests to make sure nothing breaks.\n+# In this context this script does not expect any particular environment\n+# variable to be set.\n+#\n+\n+\n+# Set to \"true\" to print debug messages to stderr\n+DEBUG=false\n+#DEBUG=true\n+\n+\n+################################################################################\n+## Internal functions (not meant to be used outside this script)\n+################################################################################\n+\n+debug_print() { \n+\t# Send message to stderr if global variable DEBUG is set to \"true\"\n+ \n+\tif test \"$DEBUG\" = \"true\"\n+\tthen\n+\t\t>&2 echo \"$@\";\n+\tfi\n+}\n+\n+\n+gen_cmd_aux() {\n+\t# Auxiliary function used from \"gen_cmd()\".\n+\t# Read that other function documentation for more details.\n+\n+\tlocal LAYOUT=$1 \n+\tlocal CMD=$2 # This is a second (hidden) argument used for recursion \n+\n+\tdebug_print\n+\tdebug_print \"LAYOUT : $LAYOUT\"\n+\tdebug_print \"CMD : $CMD\"\n+\n+\tif test -z \"$CMD\"\n+\tthen\n+\t\tCMD=\"echo\" # vim \"nop\" operator\n+\tfi\n+\n+\tlocal start=0\n+\tlocal end=${#LAYOUT}\n+\n+\tlocal nested=0\n+\tlocal nested_min=100\n+\n+\n+\t# Step 1:\n+\t#\n+\t# Increase/decrease \"start\"/\"end\" indices respectively to get rid of\n+\t# outer parenthesis.\n+\t#\n+\t# Example:\n+\t#\n+\t# - BEFORE: (( LOCAL | BASE ) - MERGED )\n+\t# - AFTER : ( LOCAL | BASE ) - MERGED \n+\n+\tfor (( i=$start; i<$end; i++ )); do\n+\t\tif test \"${LAYOUT:$i:1}\" = \" \"\n+\t\tthen\n+\t\t\tcontinue\n+\t\tfi\n+\n+\t\tif test \"${LAYOUT:$i:1}\" = \"(\"\n+\t\tthen\n+\t\t\tnested=$(( nested + 1 ))\n+\t\t\tcontinue\n+\t\tfi\n+\n+\t\tif test \"${LAYOUT:$i:1}\" = \")\"\n+\t\tthen\n+\t\t\tnested=$(( nested - 1 ))\n+\t\t\tcontinue\n+\t\tfi\n+\n+\t\tif test \"$nested\" -lt \"$nested_min\"\n+\t\tthen\n+\t\t\tnested_min=$nested\n+\t\tfi\n+\tdone\n+\n+\tdebug_print \"NESTED MIN: $nested_min\"\n+\n+\twhile test \"$nested_min\" -gt \"0\"\n+\tdo\n+\t\tstart=$(( start + 1 ))\n+\t\tend=$(( end - 1 ))\n+\n+\t\tstart_minus_one=$(( start - 1 ))\n+\n+\t\twhile ! test \"${LAYOUT:$start_minus_one:1}\" = \"(\"\n+\t\tdo\n+\t\t\tstart=$(( start + 1 ))\n+\t\t\tstart_minus_one=$(( start_minus_one + 1 ))\n+\t\tdone\n+\n+\t\twhile ! test \"${LAYOUT:$end:1}\" = \")\"\n+\t\tdo\n+\t\t\tend=$(( end - 1 ))\n+\t\tdone\n+\n+\t\tnested_min=$(( nested_min - 1 ))\n+\tdone\n+\n+ debug_print \"CLEAN : ${LAYOUT:$start:$(( end - start ))}\"\n+\n+\n+\t# Step 2:\n+\t#\n+\t# Search for all valid separators (\";\", \"-\" or \"|\") which are *not*\n+\t# inside parenthesis. Save the index at which each of them makes the\n+\t# first appearance.\n+\n+\tlocal index_semicolon=\"\"\n+\tlocal index_minus=\"\"\n+\tlocal index_pipe=\"\"\n+\n+\tnested=0\n+\tfor (( i=$start; i<$end; i++ )); do\n+\t\tif test \"${LAYOUT:$i:1}\" = \" \"\n+\t\tthen\n+\t\t\tcontinue\n+\t\tfi\n+\n+\t\tif test \"${LAYOUT:$i:1}\" = \"(\"\n+\t\tthen\n+\t\t\tnested=$(( nested + 1 ))\n+\t\t\tcontinue\n+\t\tfi\n+\n+\t\tif test \"${LAYOUT:$i:1}\" = \")\"\n+\t\tthen\n+\t\t\tnested=$(( nested - 1 ))\n+\t\t\tcontinue\n+\t\tfi\n+\n+\t\tif test \"$nested\" -eq \"0\"\n+\t\tthen\n+\t\t\tcurrent=${LAYOUT:$i:1}\n+\t\t\t\n+\t\t\tif test \"$current\" = \";\"\n+\t\t\tthen\n+\t\t\t\tif test -z \"$index_semicolon\"\n+\t\t\t\tthen\n+\t\t\t\t\tindex_semicolon=$i\n+\t\t\t\tfi\n+\n+\t\t\telif test \"$current\" = \"-\"\n+\t\t\tthen\n+\t\t\t\tif test -z \"$index_minus\"\n+\t\t\t\tthen\n+\t\t\t\t\tindex_minus=$i\n+\t\t\t\tfi\n+\n+\t\t\telif test \"$current\" = \"|\"\n+\t\t\tthen\n+\t\t\t\tif test -z \"$index_pipe\"\n+\t\t\t\tthen\n+\t\t\t\t\tindex_pipe=$i\n+\t\t\t\tfi\n+\t\t\tfi\n+\t\tfi\n+\tdone\n+\n+\n+\t# Step 3:\n+\t#\n+\t# Process the separator with the highest order of precedence\n+\t# (\";\" has the highest precedence and \"|\" the lowest one).\n+\t#\n+\t# By \"process\" I mean recursively call this function twice: the first\n+\t# one with the substring at the left of the separator and the second one\n+\t# with the one at its right.\n+\n+\tlocal terminate=\"false\"\n+\n+\tif ! test -z \"$index_semicolon\"\n+\tthen\n+\t\tbefore=\"-tabnew\"\n+\t\tafter=\"tabnext\"\n+\t\tindex=$index_semicolon\n+\t\tterminate=\"true\"\n+\n+\telif ! test -z \"$index_minus\"\n+\tthen\n+\t\tbefore=\"split\"\n+\t\tafter=\"wincmd j\"\n+\t\tindex=$index_minus\n+\t\tterminate=\"true\"\n+\n+\telif ! test -z \"$index_pipe\"\n+\tthen\n+\t\tbefore=\"vertical split\"\n+\t\tafter=\"wincmd l\"\n+\t\tindex=$index_pipe\n+\t\tterminate=\"true\"\n+\tfi\n+\n+\tif test \"$terminate\" = \"true\"\n+\tthen\n+\t\tCMD=\"$CMD | $before\"\n+\t\tCMD=$(gen_cmd_aux \"${LAYOUT:$start:$(( index - start ))}\" \"$CMD\")\n+\t\tCMD=\"$CMD | $after\"\n+\t\tCMD=$(gen_cmd_aux \"${LAYOUT:$(( index + 1 )):$(( ${#LAYOUT} - index ))}\" \"$CMD\")\n+\t\techo $CMD\n+\t\treturn\n+\tfi\n+\n+\n+\t# Step 4:\n+\t#\n+\t# If we reach this point, it means there are no separators and we just\n+\t# need to print the command to display the specified buffer\n+\n+\tlocal target=$(echo \"${LAYOUT:$start:$(( end - start ))}\" | sed 's:[ *();|-]::g')\n+\n+\tif test \"$target\" = \"LOCAL\"\n+\tthen\n+\t\tCMD=\"$CMD | 1b\"\n+\n+\telif test \"$target\" = \"BASE\"\n+\tthen\n+\t\tCMD=\"$CMD | 2b\"\n+\n+\telif test \"$target\" = \"REMOTE\"\n+\tthen\n+\t\tCMD=\"$CMD | 3b\"\n+\n+\telif test \"$target\" = \"MERGED\"\n+\tthen\n+\t\tCMD=\"$CMD | 4b\"\n+\n+\telse\n+\t\tCMD=\"$CMD | ERROR: >$target<\"\n+\tfi\n+\n+\techo $CMD\n+\treturn\n+}\n+\n+\n+gen_cmd() {\n+\t# This function returns (in global variable FINAL_CMD) the string that\n+\t# you can use when invoking \"vim\" (as shown next) to obtain a given\n+\t# layout:\n+\t#\n+\t# $ vim -f $FINAL_CMD \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n+\t#\n+\t# It takes one single argument: a string containing the desired layout\n+\t# definition.\n+\t#\n+\t# The syntax of the \"layout definitions\" is explained in ... (TODO)...\n+\t# but you can already intuitively understand how it works by knowing\n+\t# that...\n+\t#\n+\t# * \";\" means \"a new vim tab\"\n+\t# * \"-\" means \"a new vim horizontal split\"\n+\t# * \"|\" means \"a new vim vertical split\"\n+\t#\n+\t# It also returns (in global variable FINAL_TARGET) the name (\"LOCAL\",\n+\t# \"BASE\", \"REMOTE\" or \"MERGED\") of the file that is marked with an \"*\",\n+\t# or \"MERGED\" if none of them is.\n+\t#\n+\t# Example:\n+\t#\n+\t# gen_cmd \"LOCAL* | REMOTE\"\n+\t# |\n+\t# `-> FINAL_CMD == \"-c \\\"echo | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t# FINAL_TARGET == \"LOCAL\"\n+\n+\tlocal LAYOUT=$1 \n+\n+\n+\t# Search for a \"*\" in one of the files identifiers (\"LOCAL\", \"BASE\",\n+\t# \"REMOTE\", \"MERGED\"). If not found, use \"MERGE\" as the default file\n+\t# where changes will be saved.\n+\n+\tAUX=$(echo \"$LAYOUT\" | grep -oe \"[A-Z]\\+\\*\")\n+\n+\tif ! test -z \"$AUX\"\n+\tthen\n+\t FINAL_TARGET=\"${AUX:0:-1}\"\n+\telse\n+\t FINAL_TARGET=\"MERGED\"\n+\tfi\n+\n+\n+\t# Obtain the first part of vim \"-c\" option to obtain the desired layout\n+\n+\tCMD=$(gen_cmd_aux \"$LAYOUT\")\n+\n+\n+\t# Adjust the just obtained script depending on whether more than one\n+\t# windows are visible or not\n+\n+\tif test $(echo $LAYOUT | wc -w) == \"1\"\n+\tthen\n+\t\tCMD=\"$CMD | bufdo diffthis\"\n+ else\n+\t\tCMD=\"$CMD | tabdo windo diffthis\"\n+\tfi\n+\n+\n+\t# Add an extra \"-c\" option to move to the first tab (notice that we\n+\t# can't simply append the command to the previous \"-c\" string as\n+\t# explained here: https://github.com/vim/vim/issues/9076\n+\n+\tFINAL_CMD=\"-c \\\"$CMD\\\" -c \\\"tabfirst\\\"\"\n+}\n+\n+\n+run_unit_tests() {\n+\t# Function to make sure that we don't break anything when modifying this\n+\t# script.\n+\t#\n+\t# This function is automatically executed when you execute this script\n+\t# from the shell with environment variable \"DEBUG_GIT_VIMDIFF\" set (to\n+\t# any value).\n+\n+\tlocal test_cases=(\n+\t\t`#Test case 00` \"LOCAL | MERGED | REMOTE\"\n+\t\t`#Test case 01` \"LOCAL - MERGED - REMOTE\"\n+\t\t`#Test case 02` \"(LOCAL - REMOTE) | MERGED\"\n+\t\t`#Test case 03` \"MERGED | (LOCAL - REMOTE)\"\n+\t\t`#Test case 04` \"(LOCAL | REMOTE) - MERGED\"\n+\t\t`#Test case 05` \"MERGED - (LOCAL | REMOTE)\"\n+\t\t`#Test case 06` \"(LOCAL | BASE | REMOTE) - MERGED\"\n+\t\t`#Test case 07` \"(LOCAL - BASE - REMOTE) | MERGED\"\n+\t\t`#Test case 08` \"LOCAL* | REMOTE\"\n+\t\t`#Test case 09` \"MERGED\"\n+\t\t`#Test case 10` \"(LOCAL | BASE | REMOTE) - MERGED; BASE | LOCAL; BASE | REMOTE; (LOCAL - BASE - REMOTE) | MERGED\"\n+\t\t`#Test case 11` \"((LOCAL | REMOTE) - BASE) | MERGED\"\n+\t\t`#Test case 12` \"((LOCAL | REMOTE) - BASE) | ((LOCAL - REMOTE) | MERGED)\"\n+\t\t`#Test case 13` \"BASE | REMOTE ; BASE | LOCAL\"\n+\t)\n+\n+\tlocal expected_cmd=(\n+\t\t`#Test case 00` \"-c \\\"echo | vertical split | 1b | wincmd l | vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 01` \"-c \\\"echo | split | 1b | wincmd j | split | 4b | wincmd j | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 02` \"-c \\\"echo | vertical split | split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 03` \"-c \\\"echo | vertical split | 4b | wincmd l | split | 1b | wincmd j | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 04` \"-c \\\"echo | split | vertical split | 1b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 05` \"-c \\\"echo | split | 4b | wincmd j | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 06` \"-c \\\"echo | split | vertical split | 1b | wincmd l | vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 07` \"-c \\\"echo | vertical split | split | 1b | wincmd j | split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 08` \"-c \\\"echo | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 09` \"-c \\\"echo | 4b | bufdo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 10` \"-c \\\"echo | -tabnew | split | vertical split | 1b | wincmd l | vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | vertical split | 2b | wincmd l | 3b | tabnext | vertical split | split | 1b | wincmd j | split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 11` \"-c \\\"echo | vertical split | split | vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 12` \"-c \\\"echo | vertical split | split | vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | vertical split | split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t\t`#Test case 13` \"-c \\\"echo | -tabnew | vertical split | 2b | wincmd l | 3b | tabnext | vertical split | 2b | wincmd l | 1b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t)\n+\n+\tlocal expected_target=(\n+\t\t`#Test case 00` \"MERGED\"\n+\t\t`#Test case 01` \"MERGED\"\n+\t\t`#Test case 02` \"MERGED\"\n+\t\t`#Test case 03` \"MERGED\"\n+\t\t`#Test case 04` \"MERGED\"\n+\t\t`#Test case 05` \"MERGED\"\n+\t\t`#Test case 06` \"MERGED\"\n+\t\t`#Test case 07` \"MERGED\"\n+\t\t`#Test case 08` \"LOCAL\"\n+\t\t`#Test case 09` \"MERGED\"\n+\t\t`#Test case 10` \"MERGED\"\n+\t\t`#Test case 11` \"MERGED\"\n+\t\t`#Test case 12` \"MERGED\"\n+\t\t`#Test case 13` \"MERGED\"\n+\t)\n+\n+\tlocal at_least_one_ko=\"false\"\n+\n+\tfor i in ${!test_cases[@]}; do\n+\t\tgen_cmd \"${test_cases[$i]}\"\n+\t\n+\t\tif test \"$FINAL_CMD\" = \"${expected_cmd[$i]}\" && test \"$FINAL_TARGET\" = \"${expected_target[$i]}\"\n+\t\tthen\n+\t\t\tprintf \"Test Case #%02d: OK\\n\" $i\n+\t\telse\n+\t\t\tprintf \"Test Case #%02d: KO !!!!\\n\" $i\n+\t\t\techo \" FINAL_CMD : $FINAL_CMD\"\n+ echo \" FINAL_CMD (expected) : ${expected_cmd[$i]}\"\n+\t\t\techo \" FINAL_TARGET : $FINAL_TARGET\"\n+ echo \" FINAL_TARGET (expected): ${expected_target[$i]}\"\n+\t\t\tat_least_one_ko=\"true\"\n+\t\tfi\n+\tdone\n+\t\n+\tif test \"$at_least_one_ko\" = \"true\"\n+\tthen\n+\t\treturn -1\n+\telse\n+\t\treturn 0\n+\tfi\n+}\n+\n+\n+################################################################################\n+## API functions (called from \"git-mergetool--lib.sh\")\n+################################################################################\n+\n diff_cmd () {\n \t\"$merge_tool_path\" -R -f -d \\\n \t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n }\n \n+\n merge_cmd () {\n+\tlayout=$(git config mergetool.$merge_tool.layout)\n+\tprint_warning=\"false\"\n+\n \tcase \"$1\" in\n \t*vimdiff)\n-\t\tif $base_present\n+\t\tif test -z \"$layout\"\n \t\tthen\n-\t\t\t\"$merge_tool_path\" -f -d -c '4wincmd w | wincmd J' \\\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\t\t# Default layout when none is specified\n+\t\t\tlayout=\"(LOCAL | BASE | REMOTE) - MERGED\"\n \t\tfi\n \t\t;;\n \t*vimdiff1)\n-\t\t\"$merge_tool_path\" -f -d \\\n-\t\t\t-c 'echon \"Resolve conflicts leftward then save. Use :cq to abort.\"' \\\n-\t\t\t\"$LOCAL\" \"$REMOTE\"\n-\t\tret=\"$?\"\n-\t\tif test \"$ret\" -eq 0\n-\t\tthen\n-\t\t\tcp -- \"$LOCAL\" \"$MERGED\"\n-\t\tfi\n-\t\treturn \"$ret\"\n+\t\tlayout=\"LOCAL* | MERGED\"\n+\t\tprint_warning=\"true\"\n \t\t;;\n \t*vimdiff2)\n-\t\t\"$merge_tool_path\" -f -d -c 'wincmd l' \\\n-\t\t\t\"$LOCAL\" \"$MERGED\" \"$REMOTE\"\n+\t\tlayout=\"LOCAL | MERGED | REMOTE\"\n+\t\tprint_warning=\"true\"\n \t\t;;\n \t*vimdiff3)\n-\t\tif $base_present\n-\t\tthen\n-\t\t\t\"$merge_tool_path\" -f -d -c 'hid | hid | hid' \\\n-\t\t\t\t\"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$MERGED\"\n-\t\telse\n-\t\t\t\"$merge_tool_path\" -f -d -c 'hid | hid' \\\n-\t\t\t\t\"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n-\t\tfi\n+\t\tlayout=\"MERGED\"\n+\t\tprint_warning=\"true\"\n \t\t;;\n \tesac\n+\n+\tif test \"$print_warning\" = \"true\"\n+\tthen\n+\t\techo \"WARNING:\"\n+\t\techo \"WARNING: '$1' is going to be removed in a future version. You will be\"\n+\t\techo \"WARNING: able to obtain the same result by selecting 'vimdiff' as the merge\"\n+\t\techo \"WARNING: tool and setting configuration variable 'mergetool.vimdiff.layout'\"\n+\t\techo \"WARNING: to the following value:\"\n+\t\techo \"WARNING:\"\n+\t\techo \"WARNING: layout = \\\"$layout\\\"\"\n+\t\techo \"WARNING:\"\n+\t\techo \"Press ENTER to continue...\"\n+\t\tread\n+\tfi\n+\n+\tgen_cmd \"$layout\"\n+\n+\tdebug_print \"\"\n+\tdebug_print \"FINAL CMD : $FINAL_CMD\"\n+\tdebug_print \"FINAL TAR : $FINAL_TARGET\"\n+\n+\tif $base_present\n+\tthen\n+\t\teval \"$merge_tool_path\" \\\n+\t\t\t-f $FINAL_CMD \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n+\telse\n+\t\t# If there is no BASE (example: a merge conflict in a new file\n+\t\t# with the same name created in both braches which didn't exist\n+\t\t# before), close all BASE windows using vim's \"quit\" command\n+\n+\t\tFINAL_CMD=$(echo $FINAL_CMD | \\\n+\t\t\tsed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g')\n+\n+\t\teval \"$merge_tool_path\" \\\n+\t\t\t-f $FINAL_CMD \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\tfi\n+\n+\n+\tret=\"$?\"\n+\tif test \"$ret\" -eq 0\n+\tthen\n+\t\tif test \"$FINAL_TARGET\" != \"MERGED\"\n+\t\tthen\n+\t\t\teval cp -- \\$\"$FINAL_TARGET\" \"$MERGED\"\n+\t\tfi\n+\tfi\n+\treturn \"$ret\"\n }\n \n+\n translate_merge_tool_path() {\n \tcase \"$1\" in\n \tnvimdiff*)\n@@ -57,14 +518,31 @@ translate_merge_tool_path() {\n \tesac\n }\n \n+\n exit_code_trustable () {\n \ttrue\n }\n \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\n+\t\tdo\n \t\t\techo \"${prefix}vimdiff${suffix}\"\n \t\tdone\n \tdone\n }\n+\n+\n+################################################################################\n+## Run unit tests when calling this script from a shell\n+################################################################################\n+\n+if test $(ps -o stat= -p $PPID) = \"Ss\" && test $(ps -o stat= -p $$) = \"S+\"\n+then\n+\t# Script is being manually run from command line (see\n+\t# https://stackoverflow.com/questions/4261876/check-if-bash-script-was-invoked-from-a-shell-or-another-script-application)\n+\n+\trun_unit_tests\n+fi\n+\n-- \n2.33.1\n\n"},{"id":"440462","messageId":"20211104160959.183402-3-greenfoo@u92.eu","threadId":"56843","inReplyTo":"20211104160959.183402-1-greenfoo@u92.eu","subject":"[PATCH 2/3] vimdiff: add tool documentation","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2021-11-04T16:09:58Z","receivedAt":"2021-11-04T16:10:18Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"Running 'git {merge,diff}tool --tool-help' now also prints usage\ninformation about the vimdiff tool (and its variantes) instead of just\nits name.\n\nTwo new functions ('diff_cmd_help()' and 'merge_cmd_help()') have been\nadded to the set of functions that each merge tool (ie. scripts found\ninside \"mergetools/\") can overwrite to provided tool specific\ninformation.\n\nRight now, only 'mergetools/vimdiff' implements these functions, but\nother tools are encouraged to do so in the future, specially if they\ntake configuration options not explained anywhere else (as it is the\ncase with the 'vimdiff' tool and the new 'layout' option)\n\nSigned-off-by: Fernando Ramos \n---\n git-mergetool--lib.sh | 12 ++\n mergetools/vimdiff | 272 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 284 insertions(+)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 542a6a75eb..3964cd8f99 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -64,6 +64,10 @@ $(list_tool_variants)\"\n \t\t\t\tfi\n \t\t\t\tshown_any=yes\n \t\t\t\tprintf \"%s%s\\n\" \"$per_line_prefix\" \"$toolname\"\n+\t\t\t\twhile IFS= read -r line\n+\t\t\t\tdo\n+\t\t\t\t\tprintf \"%s\\t%s\\n\" \"$per_line_prefix\" \"$line\"\n+ done < <(diff_mode && diff_cmd_help \"$toolname\" || merge_cmd_help \"$toolname\")\n \t\t\tfi\n \t\tdone\n \n@@ -162,10 +166,18 @@ setup_tool () {\n \t\treturn 1\n \t}\n \n+\tdiff_cmd_help () {\n+\t\treturn 0\n+\t}\n+\n \tmerge_cmd () {\n \t\treturn 1\n \t}\n \n+\tmerge_cmd_help () {\n+\t\treturn 0\n+\t}\n+\n \thide_resolved_enabled () {\n \t\treturn 0\n \t}\ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex 1f2e88777e..aa8fbc0a19 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -428,6 +428,46 @@ diff_cmd () {\n \t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n }\n \n+diff_cmd_help() {\n+\tcase \"$1\" in\n+\tvimdiff)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tOpens vim with two vertical windows: LOCAL changes will be placed in the left\n+\t\t\twindow and REMOTE changes in the right one.\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tvimdiff*)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'vimdiff'\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tgvimdiff)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'vimdiff' but opens 'gvim' instead (which uses a graphical toolkit for\n+\t\t\topening its own window)\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tgvimdiff*)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'gvimdiff'\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tnvimdiff)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'vimdiff' but opens 'neovim' instead (which is a fork of the original\n+\t\t\t'vim' 'focused on extensibility and usability' according to their authors)\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tnvimdiff*)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'nvimdiff'\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tesac\n+\n+\treturn 0\n+}\n+\n \n merge_cmd () {\n \tlayout=$(git config mergetool.$merge_tool.layout)\n@@ -503,6 +543,238 @@ merge_cmd () {\n \treturn \"$ret\"\n }\n \n+merge_cmd_help() {\n+\tcase \"$1\" in\n+\tvimdiff)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tOpens vim with a 4 windows layout distributed in the following way:\n+\t\t\t\n+\t\t\t ------------------------------------------\n+\t\t\t | | | |\n+\t\t\t | LOCAL | BASE | REMOTE |\n+\t\t\t | | | |\n+\t\t\t ------------------------------------------\n+\t\t\t | |\n+\t\t\t | MERGED |\n+\t\t\t | |\n+\t\t\t ------------------------------------------\n+\t\t\t\n+\t\t\t\"LOCAL\", \"BASE\" and \"REMOTE\" are read-only buffers showing the contents of the\n+\t\t\tconflicting file in a specific git commit (\"commit you are merging into\",\n+\t\t\t\"common ancestor commit\" and \"commit you are merging from\" respectively)\n+\t\t\t\n+\t\t\t\"MERGED\" is a writable buffer where you have to resolve the conflicts (using the\n+\t\t\tother read-only buffers as a reference). Once you are done, save and exit \"vim\"\n+\t\t\tas usual (\":wq\") or, if you want to abort, exit using \":cq\".\n+\t\t\t\n+\t\t\tYou can change the windows layout used by vim by setting configuration variable\n+\t\t\t\"mergetool.vimdiff.layout\" which accepts a string where these separators have\n+\t\t\tspecial meaning:\n+\t\t\t\n+\t\t\t - \";\" is used to \"open a new tab\"\n+\t\t\t - \"-\" is used to \"open a new vertical split\"\n+\t\t\t - \"|\" is used to \"open a new horizontal split\"\n+\t\t\t\n+\t\t\tLet's see some examples to undertand how it works:\n+\t\t\t\n+\t\t\t * layout = \"(LOCAL | BASE | REMOTE) - MERGED\"\n+\t\t\t\n+\t\t\t This is exactly the same as the default layout we have already seen.\n+\t\t\t\n+\t\t\t * layout = \"LOCAL | MERGED | REMOTE\"\n+\t\t\t\n+\t\t\t If, for some reason, we are not interested in the \"BASE\" buffer.\n+\t\t\t\n+\t\t\t ------------------------------------------\n+\t\t\t | | | |\n+\t\t\t | | | |\n+\t\t\t | LOCAL | MERGED | REMOTE |\n+\t\t\t | | | |\n+\t\t\t | | | |\n+\t\t\t ------------------------------------------\n+\t\t\t\n+\t\t\t * layout = \"MERGED\"\n+\t\t\t\n+\t\t\t Only the \"MERGED\" buffer will be shown. Note, however, that all the other\n+\t\t\t ones are still loaded in vim, and you can access them with the \"buffers\"\n+\t\t\t command. \n+\t\t\t\n+\t\t\t ------------------------------------------\n+\t\t\t | |\n+\t\t\t | |\n+\t\t\t | MERGED |\n+\t\t\t | |\n+\t\t\t | |\n+\t\t\t ------------------------------------------\n+\t\t\t\n+\t\t\t * layout = \"LOCAL* | REMOTE\"\n+\t\t\t\n+\t\t\t When \"MERGED\" is not present in the layout, you must \"mark\" one of the\n+\t\t\t buffers with an asterisk. That will become the buffer you need to edit and\n+\t\t\t save after resolving the conflicts.\n+\t\t\t\n+\t\t\t ------------------------------------------\n+\t\t\t | | |\n+\t\t\t | | |\n+\t\t\t | | |\n+\t\t\t | LOCAL | REMOTE |\n+\t\t\t | | |\n+\t\t\t | | |\n+\t\t\t | | |\n+\t\t\t ------------------------------------------\n+\t\t\t\n+ * layout = \"(LOCAL | BASE | REMOTE) - MERGED; BASE | LOCAL; BASE | REMOTE\"\n+\t\t\t\n+\t\t\t Three tabs will open: the first one is a copy of the default layout, while\n+\t\t\t the other two only show the differences between \"BASE\" and \"LOCAL\" and\n+\t\t\t \"BASE\" and \"REMOTE\" respectively.\n+\t\t\t \n+\t\t\t ------------------------------------------\n+\t\t\t | | TAB #2 | TAB #3 | |\n+\t\t\t ------------------------------------------\n+\t\t\t | | | |\n+\t\t\t | LOCAL | BASE | REMOTE |\n+\t\t\t | | | |\n+\t\t\t ------------------------------------------\n+\t\t\t | |\n+\t\t\t | MERGED |\n+\t\t\t | |\n+\t\t\t ------------------------------------------\n+\t\t\t\n+\t\t\t ------------------------------------------\n+\t\t\t | TAB #1 | | TAB #3 | |\n+\t\t\t ------------------------------------------\n+\t\t\t | | |\n+\t\t\t | | |\n+\t\t\t | | |\n+\t\t\t | BASE | LOCAL |\n+\t\t\t | | |\n+\t\t\t | | |\n+\t\t\t | | |\n+\t\t\t ------------------------------------------\n+\t\t\t\n+\t\t\t ------------------------------------------\n+\t\t\t | TAB #1 | TAB #2 | | |\n+\t\t\t ------------------------------------------\n+\t\t\t | | |\n+\t\t\t | | |\n+\t\t\t | | |\n+\t\t\t | BASE | REMOTE |\n+\t\t\t | | |\n+\t\t\t | | |\n+\t\t\t | | |\n+\t\t\t ------------------------------------------\n+\t\t\t\n+ * layout = \"(LOCAL | BASE | REMOTE) - MERGED; BASE | LOCAL; BASE | REMOTE; (LOCAL - BASE - REMOTE) | MERGED\"\n+\t\t\t\n+\t\t\t Same as the previous example, but adds a fourth tab with the same\n+\t\t\t information as the first tab, with a different layout.\n+\t\t\t \n+\t\t\t ---------------------------------------------\n+\t\t\t | TAB #1 | TAB #2 | TAB #3 | |\n+\t\t\t ---------------------------------------------\n+\t\t\t | LOCAL | |\n+\t\t\t |---------------------| |\n+\t\t\t | BASE | MERGED |\n+\t\t\t |---------------------| |\n+\t\t\t | REMOTE | |\n+\t\t\t ---------------------------------------------\n+\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tvimdiff1)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'vimdiff' using this layout: \"LOCAL* | REMOTE\"\n+\t\t\t\n+\t\t\tThis will probably be deprecated in the future. Please use \"vimdiff\" and\n+\t\t\tmanually set the \"mergetool.vimdiff.layout\" configuration variable instead.\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tvimdiff2)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'vimdiff' using this layout: \"LOCAL | MERGED | REMOTE\"\n+\t\t\t\n+\t\t\tThis will probably be deprecated in the future. Please use \"vimdiff\" and\n+\t\t\tmanually set the \"mergetool.vimdiff.layout\" configuration variable instead.\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tvimdiff3)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'vimdiff' using this layout: \"MERGED\"\n+\t\t\t\n+\t\t\tThis will probably be deprecated in the future. Please use \"vimdiff\" and\n+\t\t\tmanually set the \"mergetool.vimdiff.layout\" configuration variable instead.\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tgvimdiff)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'vimdiff' but opens 'gvim' instead (which uses a graphical toolkit for\n+\t\t\topening its own window).\n+\t\t\tThe desired layout can be set with configuration variable\n+\t\t\t\"mergetool.gvimdiff.layout\"\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tgvimdiff1)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'gvimdiff' using this layout: \"LOCAL* | REMOTE\"\n+\t\t\t\n+\t\t\tThis will probably be deprecated in the future. Please use \"gvimdiff\" and\n+\t\t\tmanually set the \"mergetool.gvimdiff.layout\" configuration variable instead.\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tgvimdiff2)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'gvimdiff' using this layout: \"LOCAL | MERGED | REMOTE\"\n+\t\t\t\n+\t\t\tThis will probably be deprecated in the future. Please use \"gvimdiff\" and\n+\t\t\tmanually set the \"mergetool.gvimdiff.layout\" configuration variable instead.\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tgvimdiff3)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'gvimdiff' using this layout: \"MERGED\"\n+\t\t\t\n+\t\t\tThis will probably be deprecated in the future. Please use \"gvimdiff\" and\n+\t\t\tmanually set the \"mergetool.gvimdiff.layout\" configuration variable instead.\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tnvimdiff)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'vimdiff' but opens 'neovim' instead (which is a fork of the original\n+\t\t\t'vim' 'focused on extensibility and usability' according to their authors)\n+\t\t\tThe desired layout can be set with configuration variable\n+\t\t\t\"mergetool.nvimdiff.layout\"\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tnvimdiff1)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'nvimdiff' using this layout: \"LOCAL* | REMOTE\"\n+\t\t\t\n+\t\t\tThis will probably be deprecated in the future. Please use \"nvimdiff\" and\n+\t\t\tmanually set the \"mergetool.nvimdiff.layout\" configuration variable instead.\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tnvimdiff2)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'nvimdiff' using this layout: \"LOCAL | MERGED | REMOTE\"\n+\t\t\t\n+\t\t\tThis will probably be deprecated in the future. Please use \"nvimdiff\" and\n+\t\t\tmanually set the \"mergetool.nvimdiff.layout\" configuration variable instead.\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tnvimdiff3)\n+\t\tcat <<-ENDOFMESSAGE\n+\t\t\tSame as 'nvimdiff' using this layout: \"MERGED\"\n+\t\t\t\n+\t\t\tThis will probably be deprecated in the future. Please use \"nvimdiff\" and\n+\t\t\tmanually set the \"mergetool.nvimdiff.layout\" configuration variable instead.\n+\t\tENDOFMESSAGE\n+\t\t;;\n+\tesac\n+\n+\treturn 0\n+}\n+\n \n translate_merge_tool_path() {\n \tcase \"$1\" in\n-- \n2.33.1\n\n"},{"id":"440463","messageId":"20211104160959.183402-4-greenfoo@u92.eu","threadId":"56843","inReplyTo":"20211104160959.183402-1-greenfoo@u92.eu","subject":"[PATCH 3/3] vimdiff: remove deprecated {,g,n}vimdiff{1,2,3} variants","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2021-11-04T16:09:59Z","receivedAt":"2021-11-04T16:10:19Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"After this commit is merged, users of \"{,g,n}vimdiff{1,2,3}\" will need to\nset their merge tool to \"{,g,v}vimdiff\" (without the number suffix) and\nthe \"mergetool.{,g,n}vimdiff.layout\" configuration option to one of\nthese:\n\n * For \"1\" variant: \"LOCAL* | REMOTE\"\n * For \"2\" variant: \"LOCAL | MERGED | REMOTE\"\n * For \"3\" variant: \"MERGED\"\n\nSigned-off-by: Fernando Ramos \n---\n mergetools/vimdiff | 125 ++-------------------------------------------\n 1 file changed, 4 insertions(+), 121 deletions(-)\n\ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex aa8fbc0a19..9d469c0553 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -436,33 +436,18 @@ diff_cmd_help() {\n \t\t\twindow and REMOTE changes in the right one.\n \t\tENDOFMESSAGE\n \t\t;;\n-\tvimdiff*)\n-\t\tcat <<-ENDOFMESSAGE\n-\t\t\tSame as 'vimdiff'\n-\t\tENDOFMESSAGE\n-\t\t;;\n \tgvimdiff)\n \t\tcat <<-ENDOFMESSAGE\n \t\t\tSame as 'vimdiff' but opens 'gvim' instead (which uses a graphical toolkit for\n \t\t\topening its own window)\n \t\tENDOFMESSAGE\n \t\t;;\n-\tgvimdiff*)\n-\t\tcat <<-ENDOFMESSAGE\n-\t\t\tSame as 'gvimdiff'\n-\t\tENDOFMESSAGE\n-\t\t;;\n \tnvimdiff)\n \t\tcat <<-ENDOFMESSAGE\n \t\t\tSame as 'vimdiff' but opens 'neovim' instead (which is a fork of the original\n \t\t\t'vim' 'focused on extensibility and usability' according to their authors)\n \t\tENDOFMESSAGE\n \t\t;;\n-\tnvimdiff*)\n-\t\tcat <<-ENDOFMESSAGE\n-\t\t\tSame as 'nvimdiff'\n-\t\tENDOFMESSAGE\n-\t\t;;\n \tesac\n \n \treturn 0\n@@ -471,7 +456,6 @@ diff_cmd_help() {\n \n merge_cmd () {\n \tlayout=$(git config mergetool.$merge_tool.layout)\n-\tprint_warning=\"false\"\n \n \tcase \"$1\" in\n \t*vimdiff)\n@@ -481,34 +465,8 @@ merge_cmd () {\n \t\t\tlayout=\"(LOCAL | BASE | REMOTE) - MERGED\"\n \t\tfi\n \t\t;;\n-\t*vimdiff1)\n-\t\tlayout=\"LOCAL* | MERGED\"\n-\t\tprint_warning=\"true\"\n-\t\t;;\n-\t*vimdiff2)\n-\t\tlayout=\"LOCAL | MERGED | REMOTE\"\n-\t\tprint_warning=\"true\"\n-\t\t;;\n-\t*vimdiff3)\n-\t\tlayout=\"MERGED\"\n-\t\tprint_warning=\"true\"\n-\t\t;;\n \tesac\n \n-\tif test \"$print_warning\" = \"true\"\n-\tthen\n-\t\techo \"WARNING:\"\n-\t\techo \"WARNING: '$1' is going to be removed in a future version. You will be\"\n-\t\techo \"WARNING: able to obtain the same result by selecting 'vimdiff' as the merge\"\n-\t\techo \"WARNING: tool and setting configuration variable 'mergetool.vimdiff.layout'\"\n-\t\techo \"WARNING: to the following value:\"\n-\t\techo \"WARNING:\"\n-\t\techo \"WARNING: layout = \\\"$layout\\\"\"\n-\t\techo \"WARNING:\"\n-\t\techo \"Press ENTER to continue...\"\n-\t\tread\n-\tfi\n-\n \tgen_cmd \"$layout\"\n \n \tdebug_print \"\"\n@@ -682,30 +640,6 @@ merge_cmd_help() {\n \n \t\tENDOFMESSAGE\n \t\t;;\n-\tvimdiff1)\n-\t\tcat <<-ENDOFMESSAGE\n-\t\t\tSame as 'vimdiff' using this layout: \"LOCAL* | REMOTE\"\n-\t\t\t\n-\t\t\tThis will probably be deprecated in the future. Please use \"vimdiff\" and\n-\t\t\tmanually set the \"mergetool.vimdiff.layout\" configuration variable instead.\n-\t\tENDOFMESSAGE\n-\t\t;;\n-\tvimdiff2)\n-\t\tcat <<-ENDOFMESSAGE\n-\t\t\tSame as 'vimdiff' using this layout: \"LOCAL | MERGED | REMOTE\"\n-\t\t\t\n-\t\t\tThis will probably be deprecated in the future. Please use \"vimdiff\" and\n-\t\t\tmanually set the \"mergetool.vimdiff.layout\" configuration variable instead.\n-\t\tENDOFMESSAGE\n-\t\t;;\n-\tvimdiff3)\n-\t\tcat <<-ENDOFMESSAGE\n-\t\t\tSame as 'vimdiff' using this layout: \"MERGED\"\n-\t\t\t\n-\t\t\tThis will probably be deprecated in the future. Please use \"vimdiff\" and\n-\t\t\tmanually set the \"mergetool.vimdiff.layout\" configuration variable instead.\n-\t\tENDOFMESSAGE\n-\t\t;;\n \tgvimdiff)\n \t\tcat <<-ENDOFMESSAGE\n \t\t\tSame as 'vimdiff' but opens 'gvim' instead (which uses a graphical toolkit for\n@@ -714,30 +648,6 @@ merge_cmd_help() {\n \t\t\t\"mergetool.gvimdiff.layout\"\n \t\tENDOFMESSAGE\n \t\t;;\n-\tgvimdiff1)\n-\t\tcat <<-ENDOFMESSAGE\n-\t\t\tSame as 'gvimdiff' using this layout: \"LOCAL* | REMOTE\"\n-\t\t\t\n-\t\t\tThis will probably be deprecated in the future. Please use \"gvimdiff\" and\n-\t\t\tmanually set the \"mergetool.gvimdiff.layout\" configuration variable instead.\n-\t\tENDOFMESSAGE\n-\t\t;;\n-\tgvimdiff2)\n-\t\tcat <<-ENDOFMESSAGE\n-\t\t\tSame as 'gvimdiff' using this layout: \"LOCAL | MERGED | REMOTE\"\n-\t\t\t\n-\t\t\tThis will probably be deprecated in the future. Please use \"gvimdiff\" and\n-\t\t\tmanually set the \"mergetool.gvimdiff.layout\" configuration variable instead.\n-\t\tENDOFMESSAGE\n-\t\t;;\n-\tgvimdiff3)\n-\t\tcat <<-ENDOFMESSAGE\n-\t\t\tSame as 'gvimdiff' using this layout: \"MERGED\"\n-\t\t\t\n-\t\t\tThis will probably be deprecated in the future. Please use \"gvimdiff\" and\n-\t\t\tmanually set the \"mergetool.gvimdiff.layout\" configuration variable instead.\n-\t\tENDOFMESSAGE\n-\t\t;;\n \tnvimdiff)\n \t\tcat <<-ENDOFMESSAGE\n \t\t\tSame as 'vimdiff' but opens 'neovim' instead (which is a fork of the original\n@@ -746,30 +656,6 @@ merge_cmd_help() {\n \t\t\t\"mergetool.nvimdiff.layout\"\n \t\tENDOFMESSAGE\n \t\t;;\n-\tnvimdiff1)\n-\t\tcat <<-ENDOFMESSAGE\n-\t\t\tSame as 'nvimdiff' using this layout: \"LOCAL* | REMOTE\"\n-\t\t\t\n-\t\t\tThis will probably be deprecated in the future. Please use \"nvimdiff\" and\n-\t\t\tmanually set the \"mergetool.nvimdiff.layout\" configuration variable instead.\n-\t\tENDOFMESSAGE\n-\t\t;;\n-\tnvimdiff2)\n-\t\tcat <<-ENDOFMESSAGE\n-\t\t\tSame as 'nvimdiff' using this layout: \"LOCAL | MERGED | REMOTE\"\n-\t\t\t\n-\t\t\tThis will probably be deprecated in the future. Please use \"nvimdiff\" and\n-\t\t\tmanually set the \"mergetool.nvimdiff.layout\" configuration variable instead.\n-\t\tENDOFMESSAGE\n-\t\t;;\n-\tnvimdiff3)\n-\t\tcat <<-ENDOFMESSAGE\n-\t\t\tSame as 'nvimdiff' using this layout: \"MERGED\"\n-\t\t\t\n-\t\t\tThis will probably be deprecated in the future. Please use \"nvimdiff\" and\n-\t\t\tmanually set the \"mergetool.nvimdiff.layout\" configuration variable instead.\n-\t\tENDOFMESSAGE\n-\t\t;;\n \tesac\n \n \treturn 0\n@@ -778,13 +664,13 @@ merge_cmd_help() {\n \n translate_merge_tool_path() {\n \tcase \"$1\" in\n-\tnvimdiff*)\n+\tnvimdiff)\n \t\techo nvim\n \t\t;;\n-\tgvimdiff*)\n+\tgvimdiff)\n \t\techo gvim\n \t\t;;\n-\tvimdiff*)\n+\tvimdiff)\n \t\techo vim\n \t\t;;\n \tesac\n@@ -798,10 +684,7 @@ exit_code_trustable () {\n \n list_tool_variants () {\n \tfor prefix in '' g n; do\n-\t\tfor suffix in '' 1 2 3\n-\t\tdo\n-\t\t\techo \"${prefix}vimdiff${suffix}\"\n-\t\tdone\n+\t\techo \"${prefix}vimdiff\"\n \tdone\n }\n \n-- \n2.33.1\n\n"},{"id":"440643","messageId":"CAJDDKr4icrigRO-2S_HVdY7+W0EGJ8XBn5dD=O-qy1tXym4u8w@mail.gmail.com","threadId":"56843","inReplyTo":"20211104160959.183402-3-greenfoo@u92.eu","subject":"Re: [PATCH 2/3] vimdiff: add tool documentation","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-11-07T21:24:47Z","receivedAt":"2021-11-07T21:25:30Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Thanks for writing this up. Notes inline below.\n\n\nOn Thu, Nov 4, 2021 at 9:10 AM Fernando Ramos wrote:\n>\n> Running 'git {merge,diff}tool --tool-help' now also prints usage\n> information about the vimdiff tool (and its variantes) instead of just\n> its name.\n>\n> Two new functions ('diff_cmd_help()' and 'merge_cmd_help()') have been\n> added to the set of functions that each merge tool (ie. scripts found\n> inside \"mergetools/\") can overwrite to provided tool specific\n> information.\n>\n> Right now, only 'mergetools/vimdiff' implements these functions, but\n> other tools are encouraged to do so in the future, specially if they\n> take configuration options not explained anywhere else (as it is the\n> case with the 'vimdiff' tool and the new 'layout' option)\n>\n> Signed-off-by: Fernando Ramos \n> ---\n> git-mergetool--lib.sh | 12 ++\n> mergetools/vimdiff | 272 ++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 284 insertions(+)\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 542a6a75eb..3964cd8f99 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -64,6 +64,10 @@ $(list_tool_variants)\"\n> fi\n> shown_any=yes\n> printf \"%s%s\\n\" \"$per_line_prefix\" \"$toolname\"\n> + while IFS= read -r line\n> + do\n> + printf \"%s\\t%s\\n\" \"$per_line_prefix\" \"$line\"\n> + done < <(diff_mode && diff_cmd_help \"$toolname\" || merge_cmd_help \"$toolname\")\n\n\nIf we wanted to shorten this line a bit, would it work to run the pipeline\nfirst and pipe the result?\n\n (diff_mode && ... || merge_cmd_help ...) |\n while IFS= read -r line\n do\n ...\n done\n\nHow come we have to unset IFS here?\nWe often do:\n\nIFS='\n'\n\nso that we have just \\n newline as the separator.\n\nI wonder if that would be better along with a save/restore of the IFS value?\n\n\n> fi\n> done\n>\n> @@ -162,10 +166,18 @@ setup_tool () {\n> return 1\n> }\n>\n> + diff_cmd_help () {\n> + return 0\n> + }\n> +\n> merge_cmd () {\n> return 1\n> }\n>\n> + merge_cmd_help () {\n> + return 0\n> + }\n> +\n> hide_resolved_enabled () {\n> return 0\n> }\n> diff --git a/mergetools/vimdiff b/mergetools/vimdiff\n> index 1f2e88777e..aa8fbc0a19 100644\n> --- a/mergetools/vimdiff\n> +++ b/mergetools/vimdiff\n> @@ -428,6 +428,46 @@ diff_cmd () {\n> -c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n> }\n>\n> +diff_cmd_help() {\n> + case \"$1\" in\n> + vimdiff)\n> + cat <<-ENDOFMESSAGE\n> + Opens vim with two vertical windows: LOCAL changes will be placed in the left\n> + window and REMOTE changes in the right one.\n> + ENDOFMESSAGE\n\n\nTiny nit: we call this EOF in the other places\n(git-mergetool--helper.sh) where we do the same.\n\nENDOFMESSAGE is a bit verbose so it might be worth sticking to the\nconventional EOF marker.\n\n\n\n> + ;;\n> + vimdiff*)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'vimdiff'\n> + ENDOFMESSAGE\n> + ;;\n> + gvimdiff)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'vimdiff' but opens 'gvim' instead (which uses a graphical toolkit for\n> + opening its own window)\n> + ENDOFMESSAGE\n> + ;;\n> + gvimdiff*)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'gvimdiff'\n> + ENDOFMESSAGE\n> + ;;\n> + nvimdiff)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'vimdiff' but opens 'neovim' instead (which is a fork of the original\n> + 'vim' 'focused on extensibility and usability' according to their authors)\n> + ENDOFMESSAGE\n> + ;;\n> + nvimdiff*)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'nvimdiff'\n> + ENDOFMESSAGE\n> + ;;\n> + esac\n> +\n> + return 0\n> +}\n> +\n>\n> merge_cmd () {\n> layout=$(git config mergetool.$merge_tool.layout)\n> @@ -503,6 +543,238 @@ merge_cmd () {\n> return \"$ret\"\n> }\n>\n> +merge_cmd_help() {\n> + case \"$1\" in\n> + vimdiff)\n> + cat <<-ENDOFMESSAGE\n> + Opens vim with a 4 windows layout distributed in the following way:\n> +\n> + ------------------------------------------\n> + | | | |\n> + | LOCAL | BASE | REMOTE |\n> + | | | |\n> + ------------------------------------------\n> + | |\n> + | MERGED |\n> + | |\n> + ------------------------------------------\n> +\n> + \"LOCAL\", \"BASE\" and \"REMOTE\" are read-only buffers showing the contents of the\n> + conflicting file in a specific git commit (\"commit you are merging into\",\n> + \"common ancestor commit\" and \"commit you are merging from\" respectively)\n> +\n> + \"MERGED\" is a writable buffer where you have to resolve the conflicts (using the\n> + other read-only buffers as a reference). Once you are done, save and exit \"vim\"\n> + as usual (\":wq\") or, if you want to abort, exit using \":cq\".\n> +\n> + You can change the windows layout used by vim by setting configuration variable\n> + \"mergetool.vimdiff.layout\" which accepts a string where these separators have\n> + special meaning:\n> +\n> + - \";\" is used to \"open a new tab\"\n> + - \"-\" is used to \"open a new vertical split\"\n> + - \"|\" is used to \"open a new horizontal split\"\n\n\nThis would probably be a good place to mention the \"*\" current marker as well.\n\nI wonder how good these special symbols are for shell friendliness.\n\nInstead of a \"*\" suffix I would suggest a \"@\" prefix eg. \"@LOCAL\" for\nthe \"use this tab\" stuff.\n\n\n\nThe \"|\" is a nice choice visually, as is \";\" but I wonder if we can\navoid shell metacharacters.\n\nInstead of \";\" would \"+\" or \"T\" work for \"open a new tab\". I kinda\nlike \"+\", so my examples below will use that.\n\n\n\nInstead of \"|\" would \",\" (comma) work for a vertical split? Ditto --\nthe examples below will use \",\" comma.\n\nNote that I said \"vertical\", which is the opposite of what is written\nabove (horizontal split) for \"|\".\n\nI believe these are called \"vertical\" splits:\n\n LOCAL | BASE | REMOTE\n\nbecause the splits are vertical... that might just be a small\nswitcheroo in the docs that needs updating.\n\n\n\n\"-\" is nice for horizontal splits. It visually matches what happens,\nbut in the context of \",\" and \"+\" I think \"/\" would be a better\nchoice.\n\n\"a / b\" means \"a over b\" in some math interpretations, and visually\nwould make sense for horizontal splits where \"a\" is on the top and \"b\"\nis on the bottom.\n\n\nAnyways, just some sugs to try and avoid shell syntax. My examples\nbelow will use \"/\" for horizontal splits.\n\n\n> +\n> + Let's see some examples to undertand how it works:\n> +\n> + * layout = \"(LOCAL | BASE | REMOTE) - MERGED\"\n\n\nMy sug would be to write this as either: \"LOCAL,BASE,REMOTE - MERGED\"\nor \"(LOCAL, BASE, REMOTE) - MERGED\".\n\nAre the (parens) necessary, or can we infer grouping because\n\"LOCAL,BASE,REMOTE\" are grouped together in the first example? Maybe\nthat's too subtle, but it's an idea.\n\n\n> +\n> + This is exactly the same as the default layout we have already seen.\n> +\n> + * layout = \"LOCAL | MERGED | REMOTE\"\n\n\nMy sug would be: \"LOCAL, MERGED, REMOTE\".\n\n> +\n> + If, for some reason, we are not interested in the \"BASE\" buffer.\n> +\n> + ------------------------------------------\n> + | | | |\n> + | | | |\n> + | LOCAL | MERGED | REMOTE |\n> + | | | |\n> + | | | |\n> + ------------------------------------------\n> +\n> + * layout = \"MERGED\"\n> +\n> + Only the \"MERGED\" buffer will be shown. Note, however, that all the other\n> + ones are still loaded in vim, and you can access them with the \"buffers\"\n> + command.\n> +\n> + ------------------------------------------\n> + | |\n> + | |\n> + | MERGED |\n> + | |\n> + | |\n> + ------------------------------------------\n> +\n> + * layout = \"LOCAL* | REMOTE\"\n> +\n> + When \"MERGED\" is not present in the layout, you must \"mark\" one of the\n> + buffers with an asterisk. That will become the buffer you need to edit and\n> + save after resolving the conflicts.\n\n\nMy sug is that this would be written as: \"@LOCAL, REMOTE\"\n\n\n> +\n> + ------------------------------------------\n> + | | |\n> + | | |\n> + | | |\n> + | LOCAL | REMOTE |\n> + | | |\n> + | | |\n> + | | |\n> + ------------------------------------------\n> +\n> + * layout = \"(LOCAL | BASE | REMOTE) - MERGED; BASE | LOCAL; BASE | REMOTE\"\n\n\nIf we eschewed parens then this would look like:\n\n \"LOCAL,BASE,REMOTE / MERGED + BASE,LOCAL + BASE,REMOTE\"\n\nWith parens it would be:\n\n \"(LOCAL, BASE, REMOTE) / MERGED + BASE, LOCAL + BASE, REMOTE\"\n\nI personally like the idea of not needing the parens if we can do without them,\nbut I haven't looked at the implementation at all yet.\n\nI suspect the more complicated examples below require them, perhaps?\n\nThe upside of parens is that it avoids having significant whitespace,\nso a grouping syntax might be warranted.\n\n\n> +\n> + Three tabs will open: the first one is a copy of the default layout, while\n> + the other two only show the differences between \"BASE\" and \"LOCAL\" and\n> + \"BASE\" and \"REMOTE\" respectively.\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> + * layout = \"(LOCAL | BASE | REMOTE) - MERGED; BASE | LOCAL; BASE | REMOTE; (LOCAL - BASE - REMOTE) | MERGED\"\n\n\nThis would be:\n\n\"(LOCAL, BASE, REMOTE) / MERGED + BASE, LOCAL + BASE, REMOTE + (LOCAL\n/ BASE / REMOTE), MERGED\"\n\nWithout parents it would be:\n\n\"LOCAL,BASE,REMOTE / MERGED + BASE, LOCAL + BASE, REMOTE +\nLOCAL/BASE/REMOTE, MERGED\"\n\n\nThat does seem a bit subtle... it could work, but maybe we should\navoid making whitespace have special meaning and just go with parens\n(or [] or {}) for grouping.\n\nI'd like to hear some opinions on that.\n\n\n> +\n> + Same as the previous example, but adds a fourth tab with the same\n> + information as the first tab, with a different layout.\n> +\n> + ---------------------------------------------\n> + | TAB #1 | TAB #2 | TAB #3 | |\n> + ---------------------------------------------\n> + | LOCAL | |\n> + |---------------------| |\n> + | BASE | MERGED |\n> + |---------------------| |\n> + | REMOTE | |\n> + ---------------------------------------------\n> +\n> + ENDOFMESSAGE\n> + ;;\n\n\nThis example really shows off how nice and flexible this approach is.\nThis is really cool, by the way.\n\nMy only other sug is that maybe this documentation can be in the\nDocumentation/ area and these messages can be reduced to something\nalong the lines of:\n\n Please see the \"vimdiff\" section in \"git help mergetool\" or \"git\nhelp difftool\" for more details.\n\n\nNot sure. It is kinda nice that the help message is maximally helpful as-is,\nbut it probably should be in Documentation/ so that we don't have to duplicate\nthe information in multiple places.\n\n\n\n> + vimdiff1)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'vimdiff' using this layout: \"LOCAL* | REMOTE\"\n> +\n> + This will probably be deprecated in the future. Please use \"vimdiff\" and\n> + manually set the \"mergetool.vimdiff.layout\" configuration variable instead.\n> + ENDOFMESSAGE\n> + ;;\n> + vimdiff2)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'vimdiff' using this layout: \"LOCAL | MERGED | REMOTE\"\n> +\n> + This will probably be deprecated in the future. Please use \"vimdiff\" and\n> + manually set the \"mergetool.vimdiff.layout\" configuration variable instead.\n> + ENDOFMESSAGE\n> + ;;\n> + vimdiff3)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'vimdiff' using this layout: \"MERGED\"\n> +\n> + This will probably be deprecated in the future. Please use \"vimdiff\" and\n> + manually set the \"mergetool.vimdiff.layout\" configuration variable instead.\n> + ENDOFMESSAGE\n> + ;;\n> + gvimdiff)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'vimdiff' but opens 'gvim' instead (which uses a graphical toolkit for\n> + opening its own window).\n> + The desired layout can be set with configuration variable\n> + \"mergetool.gvimdiff.layout\"\n> + ENDOFMESSAGE\n> + ;;\n> + gvimdiff1)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'gvimdiff' using this layout: \"LOCAL* | REMOTE\"\n> +\n> + This will probably be deprecated in the future. Please use \"gvimdiff\" and\n> + manually set the \"mergetool.gvimdiff.layout\" configuration variable instead.\n> + ENDOFMESSAGE\n> + ;;\n> + gvimdiff2)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'gvimdiff' using this layout: \"LOCAL | MERGED | REMOTE\"\n> +\n> + This will probably be deprecated in the future. Please use \"gvimdiff\" and\n> + manually set the \"mergetool.gvimdiff.layout\" configuration variable instead.\n> + ENDOFMESSAGE\n> + ;;\n> + gvimdiff3)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'gvimdiff' using this layout: \"MERGED\"\n> +\n> + This will probably be deprecated in the future. Please use \"gvimdiff\" and\n> + manually set the \"mergetool.gvimdiff.layout\" configuration variable instead.\n> + ENDOFMESSAGE\n> + ;;\n> + nvimdiff)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'vimdiff' but opens 'neovim' instead (which is a fork of the original\n> + 'vim' 'focused on extensibility and usability' according to their authors)\n> + The desired layout can be set with configuration variable\n> + \"mergetool.nvimdiff.layout\"\n> + ENDOFMESSAGE\n> + ;;\n> + nvimdiff1)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'nvimdiff' using this layout: \"LOCAL* | REMOTE\"\n> +\n> + This will probably be deprecated in the future. Please use \"nvimdiff\" and\n> + manually set the \"mergetool.nvimdiff.layout\" configuration variable instead.\n> + ENDOFMESSAGE\n> + ;;\n> + nvimdiff2)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'nvimdiff' using this layout: \"LOCAL | MERGED | REMOTE\"\n> +\n> + This will probably be deprecated in the future. Please use \"nvimdiff\" and\n> + manually set the \"mergetool.nvimdiff.layout\" configuration variable instead.\n> + ENDOFMESSAGE\n> + ;;\n> + nvimdiff3)\n> + cat <<-ENDOFMESSAGE\n> + Same as 'nvimdiff' using this layout: \"MERGED\"\n> +\n> + This will probably be deprecated in the future. Please use \"nvimdiff\" and\n> + manually set the \"mergetool.nvimdiff.layout\" configuration variable instead.\n> + ENDOFMESSAGE\n> + ;;\n> + esac\n> +\n> + return 0\n> +}\n> +\n>\n> translate_merge_tool_path() {\n> case \"$1\" in\n> --\n> 2.33.1\n>\n\nThanks for taking it this far! I personally like the direction this is\ngoing, but I'd very much appreciate hearing others' opinions.\n-- \nDavid\n"},{"id":"440645","messageId":"CAJDDKr5KZK3wpox6FJPOL9koPr=k5+qTzZtPPFEmg4iZsvwoog@mail.gmail.com","threadId":"56843","inReplyTo":"20211104160959.183402-2-greenfoo@u92.eu","subject":"Re: [PATCH 1/3] vimdiff: new implementation with layout support","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-11-07T22:41:36Z","receivedAt":"2021-11-07T22:42:16Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"This is an early review. We're still discussing the docs but there's a\nfew things worth mentioning now before we go too far.\n\nOn Thu, Nov 4, 2021 at 9:10 AM Fernando Ramos wrote:\n>\n> When running 'git mergetool -t vimdiff', a new configuration option\n> ('mergetool.vimdiff.layout') can now be used to select how the user\n> wants the different windows, tabs and buffers to be displayed.\n>\n> If the option is not provided, the layout will be the same one that was\n> being used before this commit (ie. two rows with LOCAL, BASE and COMMIT\n> in the top one and MERGED in the bottom one).\n>\n> The 'vimdiff' variants ('vimdiff{1,2,3}') still work but, because they\n> represented nothing else than different layouts, are now internally\n> implemented as a subcase of 'vimdiff' with the corresponding\n> pre-configured 'layout'.\n>\n> Signed-off-by: Fernando Ramos \n> ---\n> mergetools/vimdiff | 530 ++++++++++++++++++++++++++++++++++++++++++---\n> 1 file changed, 504 insertions(+), 26 deletions(-)\n>\n> diff --git a/mergetools/vimdiff b/mergetools/vimdiff\n> index 96f6209a04..1f2e88777e 100644\n> --- a/mergetools/vimdiff\n> +++ b/mergetools/vimdiff\n> @@ -1,48 +1,509 @@\n> +#!/bin/bash\n> +\n> +# This script can be run in two different contexts:\n> +#\n> +# - From git, when the user invokes the \"vimdiff\" merge tool. In this context\n> +# this script expects the following environment variables (among others) to\n> +# be defined (which is something \"git\" takes care of):\n> +#\n> +# - $BASE\n> +# - $LOCAL\n> +# - $REMOTE\n> +# - $MERGED\n> +#\n> +# In this mode, all this script does is to run the next command:\n> +#\n> +# vim -f -c ... $LOCAL $BASE $REMOTE $MERGED\n> +#\n> +# ...where the \"...\" string depends on the value of the\n> +# \"mergetool.vimdiff.layout\" configuration variable and is used to open vim\n> +# with a certain layout of buffers, windows and tabs.\n> +#\n> +# - From the shell, manually. This is only expected to be done by developers\n> +# who are editing this script. When this happens, the script runs a battery\n> +# of unit tests to make sure nothing breaks.\n> +# In this context this script does not expect any particular environment\n> +# variable to be set.\n> +#\n> +\n> +\n> +# Set to \"true\" to print debug messages to stderr\n> +DEBUG=false\n> +#DEBUG=true\n\n\nIt might be better to omit \"DEBUG=false\" and call it\nGIT_MERGETOOL_VIMDIFF_DEBUG.\n\n\n> +\n> +\n> +################################################################################\n> +## Internal functions (not meant to be used outside this script)\n> +################################################################################\n> +\n> +debug_print() {\n> + # Send message to stderr if global variable DEBUG is set to \"true\"\n> +\n> + if test \"$DEBUG\" = \"true\"\n> + then\n> + >&2 echo \"$@\";\n> + fi\n\n\n... and then in here we can just check:\n\n if test -n \"$GIT_MERGETOOL_VIMDIFF_DEBUG\"\n then\n ....\n fi\n\nand we won't ever have to edit the script to activate the debug mode\nbecause it'll get inherited from the environment.\n\n\n\n> +}\n> +\n> +\n> +gen_cmd_aux() {\n> + # Auxiliary function used from \"gen_cmd()\".\n> + # Read that other function documentation for more details.\n> +\n> + local LAYOUT=$1\n> + local CMD=$2 # This is a second (hidden) argument used for recursion\n\n\nI believe \"local\" and other features used in this script are bash-isms.\n\nWe have to stick to a strict portable posix shell subset, so there's\nsome stuff here that will need adjustment for maximal portability.\n\nWe can't use any bashisms and we avoid \"local\" in scripted porcelains.\n\nWe also avoid shell arrays and \"Substring Expansion\" ${parameter:offset:length}.\n\nhttps://github.com/git/git/blob/master/Documentation/CodingGuidelines#L41\n\nThat's going to require some rework of the implementation below to avoid these.\n\n\n> +\n> + debug_print\n> + debug_print \"LAYOUT : $LAYOUT\"\n> + debug_print \"CMD : $CMD\"\n> +\n> + if test -z \"$CMD\"\n> + then\n> + CMD=\"echo\" # vim \"nop\" operator\n> + fi\n> +\n> + local start=0\n> + local end=${#LAYOUT}\n> +\n> + local nested=0\n> + local nested_min=100\n> +\n> +\n> + # Step 1:\n> + #\n> + # Increase/decrease \"start\"/\"end\" indices respectively to get rid of\n> + # outer parenthesis.\n> + #\n> + # Example:\n> + #\n> + # - BEFORE: (( LOCAL | BASE ) - MERGED )\n> + # - AFTER : ( LOCAL | BASE ) - MERGED\n> +\n> + for (( i=$start; i<$end; i++ )); do\n\nPlease avoid multiple statements on a single line (the \";\" should be a\nline break instead).\n\nThe for loop is a bash-ism. An alternative might be...\n\nfor i in $(seq $start $end)\ndo\n ...\ndone\n\nbut that is off-by-one because \"seq\" includes the $end value, so it'll\nneed to be decremented by 1.\n\n\n\n> + if test \"${LAYOUT:$i:1}\" = \" \"\n> + then\n> + continue\n> + fi\n\nThis is going to need rework because we can't use \"${LAYOUT:$i:1}\".\n\n\n> +\n> + if test \"${LAYOUT:$i:1}\" = \"(\"\n> + then\n> + nested=$(( nested + 1 ))\n\n\nI mentioned this in the documentation commit as a comment about splitting a\nlong line, but we do not use Process Substitution <(list) or >(list) either\nso that's another reason to break up the pipeline I mentioned in the previous\nemail.\n\n\nArithmetic substitution is something we do use, though, so this would be fine.\n\n\n> + continue\n> + fi\n> +\n> + if test \"${LAYOUT:$i:1}\" = \")\"\n> + then\n> + nested=$(( nested - 1 ))\n> + continue\n> + fi\n> +\n> + if test \"$nested\" -lt \"$nested_min\"\n> + then\n> + nested_min=$nested\n> + fi\n> + done\n> +\n> + debug_print \"NESTED MIN: $nested_min\"\n> +\n> + while test \"$nested_min\" -gt \"0\"\n> + do\n> + start=$(( start + 1 ))\n> + end=$(( end - 1 ))\n> +\n> + start_minus_one=$(( start - 1 ))\n> +\n> + while ! test \"${LAYOUT:$start_minus_one:1}\" = \"(\"\n> + do\n> + start=$(( start + 1 ))\n> + start_minus_one=$(( start_minus_one + 1 ))\n> + done\n> +\n> + while ! test \"${LAYOUT:$end:1}\" = \")\"\n> + do\n> + end=$(( end - 1 ))\n> + done\n> +\n> + nested_min=$(( nested_min - 1 ))\n> + done\n> +\n> + debug_print \"CLEAN : ${LAYOUT:$start:$(( end - start ))}\"\n> +\n> +\n> + # Step 2:\n> + #\n> + # Search for all valid separators (\";\", \"-\" or \"|\") which are *not*\n> + # inside parenthesis. Save the index at which each of them makes the\n> + # first appearance.\n\n\nI now understand why the parens are helpful. They seem to make the\nimplementation simpler/possible.\n\n> +\n> + local index_semicolon=\"\"\n> + local index_minus=\"\"\n> + local index_pipe=\"\"\n\nDrop \"local\".\n\nSemantic names for these might be helpful. \"index_new_tab\",\n\"index_vertical_split\" and \"index_horizontal_split\" might be easier to\nunderstand and would be resilient to sugs about using different\ntokens.\n\n\n> +\n> + nested=0\n> + for (( i=$start; i<$end; i++ )); do\n> + if test \"${LAYOUT:$i:1}\" = \" \"\n> + then\n> + continue\n> + fi\n> +\n> + if test \"${LAYOUT:$i:1}\" = \"(\"\n> + then\n> + nested=$(( nested + 1 ))\n\n\nHere and below -- should that be nested=$(( $nested + 1 )) ?\n\nIt seems to be missing a '$' prefix on the inner $nested variable.\n\nMy shell accepts both but the predominant style in the git test suite\nis the use $nested, so let's do that.\n\n\n\n> + continue\n> + fi\n> +\n> + if test \"${LAYOUT:$i:1}\" = \")\"\n> + then\n> + nested=$(( nested - 1 ))\n> + continue\n> + fi\n> +\n> + if test \"$nested\" -eq \"0\"\n> + then\n> + current=${LAYOUT:$i:1}\n> +\n> + if test \"$current\" = \";\"\n> + then\n> + if test -z \"$index_semicolon\"\n> + then\n> + index_semicolon=$i\n> + fi\n> +\n> + elif test \"$current\" = \"-\"\n> + then\n> + if test -z \"$index_minus\"\n> + then\n> + index_minus=$i\n> + fi\n> +\n> + elif test \"$current\" = \"|\"\n> + then\n> + if test -z \"$index_pipe\"\n> + then\n> + index_pipe=$i\n> + fi\n> + fi\n> + fi\n> + done\n> +\n> +\n> + # Step 3:\n> + #\n> + # Process the separator with the highest order of precedence\n> + # (\";\" has the highest precedence and \"|\" the lowest one).\n> + #\n> + # By \"process\" I mean recursively call this function twice: the first\n> + # one with the substring at the left of the separator and the second one\n> + # with the one at its right.\n> +\n> + local terminate=\"false\"\n> +\n> + if ! test -z \"$index_semicolon\"\n> + then\n> + before=\"-tabnew\"\n> + after=\"tabnext\"\n> + index=$index_semicolon\n> + terminate=\"true\"\n> +\n> + elif ! test -z \"$index_minus\"\n> + then\n> + before=\"split\"\n> + after=\"wincmd j\"\n> + index=$index_minus\n> + terminate=\"true\"\n> +\n> + elif ! test -z \"$index_pipe\"\n> + then\n> + before=\"vertical split\"\n> + after=\"wincmd l\"\n> + index=$index_pipe\n> + terminate=\"true\"\n> + fi\n> +\n> + if test \"$terminate\" = \"true\"\n> + then\n> + CMD=\"$CMD | $before\"\n> + CMD=$(gen_cmd_aux \"${LAYOUT:$start:$(( index - start ))}\" \"$CMD\")\n> + CMD=\"$CMD | $after\"\n> + CMD=$(gen_cmd_aux \"${LAYOUT:$(( index + 1 )):$(( ${#LAYOUT} - index ))}\" \"$CMD\")\n> + echo $CMD\n> + return\n> + fi\n> +\n> +\n> + # Step 4:\n> + #\n> + # If we reach this point, it means there are no separators and we just\n> + # need to print the command to display the specified buffer\n> +\n> + local target=$(echo \"${LAYOUT:$start:$(( end - start ))}\" | sed 's:[ *();|-]::g')\n> +\n> + if test \"$target\" = \"LOCAL\"\n> + then\n> + CMD=\"$CMD | 1b\"\n> +\n> + elif test \"$target\" = \"BASE\"\n> + then\n> + CMD=\"$CMD | 2b\"\n> +\n> + elif test \"$target\" = \"REMOTE\"\n> + then\n> + CMD=\"$CMD | 3b\"\n> +\n> + elif test \"$target\" = \"MERGED\"\n> + then\n> + CMD=\"$CMD | 4b\"\n> +\n> + else\n> + CMD=\"$CMD | ERROR: >$target<\"\n> + fi\n> +\n> + echo $CMD\n> + return\n> +}\n> +\n> +\n> +gen_cmd() {\n> + # This function returns (in global variable FINAL_CMD) the string that\n> + # you can use when invoking \"vim\" (as shown next) to obtain a given\n> + # layout:\n> + #\n> + # $ vim -f $FINAL_CMD \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n> + #\n> + # It takes one single argument: a string containing the desired layout\n> + # definition.\n> + #\n> + # The syntax of the \"layout definitions\" is explained in ... (TODO)...\n> + # but you can already intuitively understand how it works by knowing\n> + # that...\n> + #\n> + # * \";\" means \"a new vim tab\"\n> + # * \"-\" means \"a new vim horizontal split\"\n> + # * \"|\" means \"a new vim vertical split\"\n> + #\n> + # It also returns (in global variable FINAL_TARGET) the name (\"LOCAL\",\n> + # \"BASE\", \"REMOTE\" or \"MERGED\") of the file that is marked with an \"*\",\n> + # or \"MERGED\" if none of them is.\n> + #\n> + # Example:\n> + #\n> + # gen_cmd \"LOCAL* | REMOTE\"\n> + # |\n> + # `-> FINAL_CMD == \"-c \\\"echo | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + # FINAL_TARGET == \"LOCAL\"\n> +\n> + local LAYOUT=$1\n> +\n> +\n> + # Search for a \"*\" in one of the files identifiers (\"LOCAL\", \"BASE\",\n> + # \"REMOTE\", \"MERGED\"). If not found, use \"MERGE\" as the default file\n> + # where changes will be saved.\n> +\n> + AUX=$(echo \"$LAYOUT\" | grep -oe \"[A-Z]\\+\\*\")\n\n\nFrom Documentatin/CodingGuidelines:\n\nAs to use of grep, stick to a subset of BRE (namely, no {m,n},\n [::], [==], or [..]) for portability.\n\n\n\n> +\n> + if ! test -z \"$AUX\"\n> + then\n> + FINAL_TARGET=\"${AUX:0:-1}\"\n> + else\n> + FINAL_TARGET=\"MERGED\"\n> + fi\n\n\nThe conditional above is better written as:\n\n if test -n \"$AUX\"\n then\n ...\n else\n ...\n fi\n\n\"test -n\" is the logical opposite of \"test -z\", so \"! test -z\" can be\nreplaced with \"test -n\".\n\n\n\n\n> +\n> +\n> + # Obtain the first part of vim \"-c\" option to obtain the desired layout\n> +\n> + CMD=$(gen_cmd_aux \"$LAYOUT\")\n> +\n> +\n> + # Adjust the just obtained script depending on whether more than one\n> + # windows are visible or not\n> +\n> + if test $(echo $LAYOUT | wc -w) == \"1\"\n> + then\n> + CMD=\"$CMD | bufdo diffthis\"\n> + else\n> + CMD=\"$CMD | tabdo windo diffthis\"\n> + fi\n\n\nThe output of \"wc -c\" is non-portable. It contains leading whitespace\non some platforms.\n\nThe test expression should be:\n\n test \"$value\" = 1\n\nwith a single \"=\" rather than \"==\".\n\n\n\n> +\n> +\n> + # Add an extra \"-c\" option to move to the first tab (notice that we\n> + # can't simply append the command to the previous \"-c\" string as\n> + # explained here: https://github.com/vim/vim/issues/9076\n> +\n> + FINAL_CMD=\"-c \\\"$CMD\\\" -c \\\"tabfirst\\\"\"\n> +}\n> +\n> +\n> +run_unit_tests() {\n> + # Function to make sure that we don't break anything when modifying this\n> + # script.\n> + #\n> + # This function is automatically executed when you execute this script\n> + # from the shell with environment variable \"DEBUG_GIT_VIMDIFF\" set (to\n> + # any value).\n> +\n> + local test_cases=(\n> + `#Test case 00` \"LOCAL | MERGED | REMOTE\"\n> + `#Test case 01` \"LOCAL - MERGED - REMOTE\"\n> + `#Test case 02` \"(LOCAL - REMOTE) | MERGED\"\n> + `#Test case 03` \"MERGED | (LOCAL - REMOTE)\"\n> + `#Test case 04` \"(LOCAL | REMOTE) - MERGED\"\n> + `#Test case 05` \"MERGED - (LOCAL | REMOTE)\"\n> + `#Test case 06` \"(LOCAL | BASE | REMOTE) - MERGED\"\n> + `#Test case 07` \"(LOCAL - BASE - REMOTE) | MERGED\"\n> + `#Test case 08` \"LOCAL* | REMOTE\"\n> + `#Test case 09` \"MERGED\"\n> + `#Test case 10` \"(LOCAL | BASE | REMOTE) - MERGED; BASE | LOCAL; BASE | REMOTE; (LOCAL - BASE - REMOTE) | MERGED\"\n> + `#Test case 11` \"((LOCAL | REMOTE) - BASE) | MERGED\"\n> + `#Test case 12` \"((LOCAL | REMOTE) - BASE) | ((LOCAL - REMOTE) | MERGED)\"\n> + `#Test case 13` \"BASE | REMOTE ; BASE | LOCAL\"\n> + )\n> +\n> + local expected_cmd=(\n> + `#Test case 00` \"-c \\\"echo | vertical split | 1b | wincmd l | vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 01` \"-c \\\"echo | split | 1b | wincmd j | split | 4b | wincmd j | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 02` \"-c \\\"echo | vertical split | split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 03` \"-c \\\"echo | vertical split | 4b | wincmd l | split | 1b | wincmd j | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 04` \"-c \\\"echo | split | vertical split | 1b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 05` \"-c \\\"echo | split | 4b | wincmd j | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 06` \"-c \\\"echo | split | vertical split | 1b | wincmd l | vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 07` \"-c \\\"echo | vertical split | split | 1b | wincmd j | split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 08` \"-c \\\"echo | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 09` \"-c \\\"echo | 4b | bufdo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 10` \"-c \\\"echo | -tabnew | split | vertical split | 1b | wincmd l | vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | vertical split | 2b | wincmd l | 3b | tabnext | vertical split | split | 1b | wincmd j | split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 11` \"-c \\\"echo | vertical split | split | vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 12` \"-c \\\"echo | vertical split | split | vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | vertical split | split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + `#Test case 13` \"-c \\\"echo | -tabnew | vertical split | 2b | wincmd l | 3b | tabnext | vertical split | 2b | wincmd l | 1b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n> + )\n> +\n> + local expected_target=(\n> + `#Test case 00` \"MERGED\"\n> + `#Test case 01` \"MERGED\"\n> + `#Test case 02` \"MERGED\"\n> + `#Test case 03` \"MERGED\"\n> + `#Test case 04` \"MERGED\"\n> + `#Test case 05` \"MERGED\"\n> + `#Test case 06` \"MERGED\"\n> + `#Test case 07` \"MERGED\"\n> + `#Test case 08` \"LOCAL\"\n> + `#Test case 09` \"MERGED\"\n> + `#Test case 10` \"MERGED\"\n> + `#Test case 11` \"MERGED\"\n> + `#Test case 12` \"MERGED\"\n> + `#Test case 13` \"MERGED\"\n> + )\n\n\nWe can't use shell arrays. This part really tells me that I don't\nunderstand bash at all.\n\nI don't understand what the backticks are doing.. is it actually\nrunning something?\n\nThat's just a rhetorical question.. I don't actually know, but it's okay if we\nignore this question since we already indicated that we'll have to do\nw/out arrays.\n\n\n\n\n> +\n> + local at_least_one_ko=\"false\"\n> +\n> + for i in ${!test_cases[@]}; do\n\n\nI suspect ${!test_cases[@]} is some bash-ism that we can't use.\n\nFor the final patch, I think it would be good if we had a way to run this\nthrough the test suite if possible rather than needing to run the script\ndirectly.\n\nIt might have more leeway to setup the environment for testing that way too.\n\n\n> + gen_cmd \"${test_cases[$i]}\"\n> +\n> + if test \"$FINAL_CMD\" = \"${expected_cmd[$i]}\" && test \"$FINAL_TARGET\" = \"${expected_target[$i]}\"\n> + then\n> + printf \"Test Case #%02d: OK\\n\" $i\n> + else\n> + printf \"Test Case #%02d: KO !!!!\\n\" $i\n> + echo \" FINAL_CMD : $FINAL_CMD\"\n> + echo \" FINAL_CMD (expected) : ${expected_cmd[$i]}\"\n> + echo \" FINAL_TARGET : $FINAL_TARGET\"\n> + echo \" FINAL_TARGET (expected): ${expected_target[$i]}\"\n> + at_least_one_ko=\"true\"\n> + fi\n> + done\n> +\n> + if test \"$at_least_one_ko\" = \"true\"\n> + then\n> + return -1\n> + else\n> + return 0\n> + fi\n> +}\n> +\n> +\n> +################################################################################\n> +## API functions (called from \"git-mergetool--lib.sh\")\n> +################################################################################\n> +\n> diff_cmd () {\n> \"$merge_tool_path\" -R -f -d \\\n> -c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n> }\n>\n> +\n> merge_cmd () {\n> + layout=$(git config mergetool.$merge_tool.layout)\n> + print_warning=\"false\"\n> +\n> case \"$1\" in\n> *vimdiff)\n> - if $base_present\n> + if test -z \"$layout\"\n> then\n> - \"$merge_tool_path\" -f -d -c '4wincmd w | wincmd J' \\\n> - \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n> - else\n> - \"$merge_tool_path\" -f -d -c 'wincmd l' \\\n> - \"$LOCAL\" \"$MERGED\" \"$REMOTE\"\n> + # Default layout when none is specified\n> + layout=\"(LOCAL | BASE | REMOTE) - MERGED\"\n> fi\n> ;;\n> *vimdiff1)\n> - \"$merge_tool_path\" -f -d \\\n> - -c 'echon \"Resolve conflicts leftward then save. Use :cq to abort.\"' \\\n> - \"$LOCAL\" \"$REMOTE\"\n> - ret=\"$?\"\n> - if test \"$ret\" -eq 0\n> - then\n> - cp -- \"$LOCAL\" \"$MERGED\"\n> - fi\n> - return \"$ret\"\n> + layout=\"LOCAL* | MERGED\"\n> + print_warning=\"true\"\n> ;;\n> *vimdiff2)\n> - \"$merge_tool_path\" -f -d -c 'wincmd l' \\\n> - \"$LOCAL\" \"$MERGED\" \"$REMOTE\"\n> + layout=\"LOCAL | MERGED | REMOTE\"\n> + print_warning=\"true\"\n> ;;\n> *vimdiff3)\n> - if $base_present\n> - then\n> - \"$merge_tool_path\" -f -d -c 'hid | hid | hid' \\\n> - \"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$MERGED\"\n> - else\n> - \"$merge_tool_path\" -f -d -c 'hid | hid' \\\n> - \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n> - fi\n> + layout=\"MERGED\"\n> + print_warning=\"true\"\n> ;;\n> esac\n> +\n> + if test \"$print_warning\" = \"true\"\n> + then\n> + echo \"WARNING:\"\n> + echo \"WARNING: '$1' is going to be removed in a future version. You will be\"\n> + echo \"WARNING: able to obtain the same result by selecting 'vimdiff' as the merge\"\n> + echo \"WARNING: tool and setting configuration variable 'mergetool.vimdiff.layout'\"\n> + echo \"WARNING: to the following value:\"\n> + echo \"WARNING:\"\n> + echo \"WARNING: layout = \\\"$layout\\\"\"\n> + echo \"WARNING:\"\n> + echo \"Press ENTER to continue...\"\n> + read\n> + fi\n\nI wonder if we should ever remove the old variants.\n\nWe could just keep them around forever, and then users won't ever be bothered.\n\nThe bulk of the improvement here is to improve the implementation so\nthat we don't\never have to add any new variants, and to fold the implementations\ntogether into a\ncommon approach so that there's less code to maintain.\n\nIt seems like there's really no need to burden users with our\nimplementation choices.\n\nI personally would be in favor of dropping the deprecation angle and treating\nthese patches more-so as an refactoring of the implementation.\n\n\n> +\n> + gen_cmd \"$layout\"\n> +\n> + debug_print \"\"\n> + debug_print \"FINAL CMD : $FINAL_CMD\"\n> + debug_print \"FINAL TAR : $FINAL_TARGET\"\n> +\n> + if $base_present\n> + then\n> + eval \"$merge_tool_path\" \\\n> + -f $FINAL_CMD \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n> + else\n> + # If there is no BASE (example: a merge conflict in a new file\n> + # with the same name created in both braches which didn't exist\n> + # before), close all BASE windows using vim's \"quit\" command\n> +\n> + FINAL_CMD=$(echo $FINAL_CMD | \\\n> + sed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g')\n> +\n> + eval \"$merge_tool_path\" \\\n> + -f $FINAL_CMD \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n> + fi\n> +\n> +\n> + ret=\"$?\"\n> + if test \"$ret\" -eq 0\n\nThis should be:\n\n if test \"$ret\" = 0\n\n> + then\n> + if test \"$FINAL_TARGET\" != \"MERGED\"\n> + then\n> + eval cp -- \\$\"$FINAL_TARGET\" \"$MERGED\"\n\nThis eval may not be safe when the value contains whitespace or shell\nmetacharacters.\n\nI think it might be better to just spell it out and be explicit.\n\nIt's more code but it'll be easier to follow:\n\ncase \"$FINAL_TARGET\" in\nLOCAL)\n source_path=\"$LOCAL\"\n ;;\nREMOTE)\n source_path=\"$REMOTE\"\n ;;\nMERGED|*)\n # Do nothing\n source_path=\n ;;\nesac\n\nif test -n \"$source_path\"\nthen\n cp -- \"$source_path\" \"$MERGED\"\nfi\n\n\n\n> + fi\n> + fi\n> + return \"$ret\"\n> }\n>\n> +\n> translate_merge_tool_path() {\n> case \"$1\" in\n> nvimdiff*)\n> @@ -57,14 +518,31 @@ translate_merge_tool_path() {\n> esac\n> }\n>\n> +\n> exit_code_trustable () {\n> true\n> }\n>\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\n> + do\n> echo \"${prefix}vimdiff${suffix}\"\n> done\n> done\n> }\n> +\n> +\n> +################################################################################\n> +## Run unit tests when calling this script from a shell\n> +################################################################################\n> +\n> +if test $(ps -o stat= -p $PPID) = \"Ss\" && test $(ps -o stat= -p $$) = \"S+\"\n> +then\n> + # Script is being manually run from command line (see\n> + # https://stackoverflow.com/questions/4261876/check-if-bash-script-was-invoked-from-a-shell-or-another-script-application)\n> +\n> + run_unit_tests\n> +fi\n\n\nI'm not 100% sure, but I suspect this is probably non-portable and\nwill have some issues.\n\nThis is another vote for setting something up to go through the test suite.\n\nThe test suite can set some GIT_MERGETOOL_VIMDIFF_TESTING variable\nthat the scriplet\ncan key off of explicitly rather than having the script embed this stuff in it.\n\nA test could then source the scriptlet directly and exercise functions\nprovided by it.\n\nIf the implementation details get too complex for shell, I wonder if\nit's time to split\noff a small helper command in C that does the layout -> vim script\ntranslation. TBD.\n\nThe parsing code is pretty complex but if there's a way to do it in our portable\nposix shell subset then we'll take that. There might be a point where having a\nsmall dedicated helper command might be easier.\n\nYou'll probably have the best sense of that, though.\n\nThanks for diving into making this happen.\n-- \nDavid\n"},{"id":"440654","messageId":"CAPig+cRiMVzMd5yEZkRvwdAjf-tawGGrSsCBUAVs+pTTw5KunA@mail.gmail.com","threadId":"56843","inReplyTo":"CAJDDKr5KZK3wpox6FJPOL9koPr=k5+qTzZtPPFEmg4iZsvwoog@mail.gmail.com","subject":"Re: [PATCH 1/3] vimdiff: new implementation with layout support","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-11-08T00:47:17Z","receivedAt":"2021-11-08T00:47:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 7, 2021 at 5:42 PM David Aguilar wrote:\n> On Thu, Nov 4, 2021 at 9:10 AM Fernando Ramos wrote:\n> > + AUX=$(echo \"$LAYOUT\" | grep -oe \"[A-Z]\\+\\*\")\n>\n> From Documentatin/CodingGuidelines:\n>\n> As to use of grep, stick to a subset of BRE (namely, no {m,n},\n> [::], [==], or [..]) for portability.\n\nAlso, `grep -o` isn't POSIX.\n\n> > + if test $(echo $LAYOUT | wc -w) == \"1\"\n> > + then\n> > + CMD=\"$CMD | bufdo diffthis\"\n> > + else\n> > + CMD=\"$CMD | tabdo windo diffthis\"\n> > + fi\n>\n> The output of \"wc -c\" is non-portable. It contains leading whitespace\n> on some platforms.\n>\n> The test expression should be:\n>\n> test \"$value\" = 1\n>\n> with a single \"=\" rather than \"==\".\n\nFor clarification, the leading whitespace emitted by some `wc`\nimplementations is only a problem when encapsulated in a string. For\ninstance, like this:\n\n if test \"$(... | wc -w)\" = \"1\"\n\nin which case \" 1\" won't equal \"1\". The usage here, however, should\nbe okay since the output is not quoted.\n\nQuite correct about using \"=\" (or even \"-eq\") here rather than \"==\", though.\n\n> > + if $base_present\n> > + then\n> > + eval \"$merge_tool_path\" \\\n> > + -f $FINAL_CMD \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n> > + else\n> > + [...]\n> > + eval \"$merge_tool_path\" \\\n> > + -f $FINAL_CMD \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n> > + fi\n> > +\n> > + ret=\"$?\"\n> > + if test \"$ret\" -eq 0\n>\n> This should be:\n>\n> if test \"$ret\" = 0\n\nOr simpler, no need for `ret` at all:\n\n if test $? -eq 0\n\n(or `if test $? = 0` -- either works)\n\nAnother (perhaps better) alternative is to assign the result of `eval`\nto `ret` at the point of invocation, which lessens the cognitive load\na bit since you don't have to scan backward through the code trying to\nfigure out what $? refers to.\n\nAlso, why is `eval` needed here? Is there something non-obvious going\non? (Genuine question; I didn't trace the code thoroughly to\nunderstand.)\n\n> > + eval cp -- \\$\"$FINAL_TARGET\" \"$MERGED\"\n>\n> This eval may not be safe when the value contains whitespace or shell\n> metacharacters.\n>\n> I think it might be better to just spell it out and be explicit.\n>\n> It's more code but it'll be easier to follow:\n> [...]\n> if test -n \"$source_path\"\n> then\n> cp -- \"$source_path\" \"$MERGED\"\n> fi\n\nI suspect `--` also needs to be avoided since it is not POSIX.\n"},{"id":"440655","messageId":"CAPig+cSi9S+EdqKLNWDGA76B3kUbvxQHEh2csyZYqX2sf0gztQ@mail.gmail.com","threadId":"56843","inReplyTo":"CAJDDKr4icrigRO-2S_HVdY7+W0EGJ8XBn5dD=O-qy1tXym4u8w@mail.gmail.com","subject":"Re: [PATCH 2/3] vimdiff: add tool documentation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-11-08T01:02:22Z","receivedAt":"2021-11-08T01:02:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 7, 2021 at 4:25 PM David Aguilar wrote:\n> On Thu, Nov 4, 2021 at 9:10 AM Fernando Ramos wrote:\n> > + while IFS= read -r line\n> > + do\n> > + printf \"%s\\t%s\\n\" \"$per_line_prefix\" \"$line\"\n> > + done < <(diff_mode && diff_cmd_help \"$toolname\" || merge_cmd_help \"$toolname\")\n>\n> If we wanted to shorten this line a bit, would it work to run the pipeline\n> first and pipe the result?\n>\n> (diff_mode && ... || merge_cmd_help ...) |\n> while IFS= read -r line\n> do\n> ...\n> done\n\nThe additional benefit is that this avoids the `<(...)` Bashism (which\nyou mentioned in your other review).\n\n> > + cat <<-ENDOFMESSAGE\n> > + Opens vim with two vertical windows: LOCAL changes will be placed in the left\n> > + window and REMOTE changes in the right one.\n> > + ENDOFMESSAGE\n>\n> Tiny nit: we call this EOF in the other places\n> (git-mergetool--helper.sh) where we do the same.\n>\n> ENDOFMESSAGE is a bit verbose so it might be worth sticking to the\n> conventional EOF marker.\n\nA couple additional really micro nits (take them or leave them)...\n\nWe normally don't add extra indentation to the here-doc body, and we\nuse `<<-\\EOF` to reduce cognitive load if there's nothing in the body\nwhich requires interpolation or expansion. Thus:\n\n cat <<-\\EOF\n Opens vim with two...\n EOF\n"},{"id":"440672","messageId":"xmqqfss6v51a.fsf@gitster.g","threadId":"56843","inReplyTo":"CAJDDKr4icrigRO-2S_HVdY7+W0EGJ8XBn5dD=O-qy1tXym4u8w@mail.gmail.com","subject":"Re: [PATCH 2/3] vimdiff: add tool documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-08T19:08:49Z","receivedAt":"2021-11-08T19:09:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> (diff_mode && ... || merge_cmd_help ...) |\n> while IFS= read -r line\n> do\n> ...\n> done\n>\n> How come we have to unset IFS here?\n\nWe are taking the input into a single variable \"line\", so what are\nwe splitting with IFS anyway?\n\nIn any case, thanks for spotting bash-isms to avoid. We started\nallowing \"local\", I think, at least in the tests, but otherwise many\nbash-only things like \"<(process)\" redirection and \"${substring:4}\"\nsubstitution are simply no-no.\n\n\n"}]}