[PATCH 0/3] vimdiff: new layout option + docs

[PATCH 0/3] vimdiff: new layout option + docs

[Fernando Ramos]

A few weeks ago I presented this RFC [1] where I introduced a new variant of the
vimdiff merge tool ("vimdiff4") that creates three tabs (instead of just one)
that look like this:

    ------------------------------------------
    | <TAB #1> |  TAB #2  |  TAB #3  |       |
    ------------------------------------------
    |             |           |              |
    |   LOCAL     |   BASE    |   REMOTE     |
    |             |           |              |   <---- Same information
    ------------------------------------------         presented by the
    |                                        |         "standard" vimdiff
    |                MERGED                  |         merge tool
    |                                        |
    ------------------------------------------
    
    ------------------------------------------
    |  TAB #1  | <TAB #2> |  TAB #3  |       |
    ------------------------------------------
    |                   |                    |
    |                   |                    |
    |                   |                    |
    |     BASE          |    LOCAL           |   <---- Only differences
    |                   |                    |         between BASE and
    |                   |                    |         LOCAL are shown
    |                   |                    |
    ------------------------------------------
    
    ------------------------------------------
    |  TAB #1  |  TAB #2  | <TAB #3> |       |
    ------------------------------------------
    |                   |                    |
    |                   |                    |
    |                   |                    |
    |     BASE          |    REMOTE          |   <---- Only differences
    |                   |                    |         between BASE and
    |                   |                    |         REMOTE are shown
    |                   |                    |
    ------------------------------------------


The motivation behind this was that, for non-trivial merges, the three way diff
presented in the first tab tends to be very confusing and in these cases
indivial diffs between BASE and LOCAL and between BASE and REMOTE are very
useful.

I have been using a "custom" merge tool for months to achieve this same result
by adding these lines to my .gitconfig file:

  [mergetool "supermerge"]
        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\"
        trustExitCode = true

...and, because I found this "trick" very useful, I thought it would be a good
idea to add it as a git built-in merge tool (called "vimdiff4" because  1, 2 and
3 had already been taken) for everyone to use... and that's exactly what the RFC
I published did.

Now... as you can see in the RFC thread [1], David and Juno suggested that
maybe, instead of creating *yet another vimdiff variant*, we should take this
opportunity to:

  * Come up with a more general way of defining arbitrary vim layouts.
  
  * Re-implement "vimdiff1", "vimdiff2" and "vimdiff3" using this new mechanism
    (after all, the only difference among them is that they present different
    layouts to the user)

  * Add documentation to all of this.

And the result of that work is what I'm presenting today :)

Some things I would like to mention:

  1. There are three commits in this patch series:

     - The first one implements the logic to generate new arbitrary layouts and
       also re-defines "vimdiff1", "vimdiff2" and "vimdiff3" on top of it.

     - The second one adds documentation. It is probably a good idea to start
       reviewing this commit before the first one!

     - The last commit *is not meant to be merged now*. It removes "vimdiff1",
       "vimdiff2" and "vimdiff3", which is something that should only be done
       after one or two releases with a deprecation notice and only if everyone
       agrees to do so :)

  2. "mergetools/vimdiff" is now a ~800 lines bash script, but most of it is
     documentation (which is embedded in the tool itself for easier maintenance)
     and unit tests.
     I have only tested it with bash, but I've tried not to use any command not
     already being used somewhere else, so I expect it to work in the same
     places it was working before (however, let me know if there are some shell
     compatibility requirements and I'll try to check them).

  3. Regarding unit tests, "mergetool/vimdiff" contains instructions on how to
     run them (just call the script without arguments after making changes, to
     make sure you didn't break anything).
     Right now it prints "OK" on all test cases (obviously) [2]

  3. The "git {diff,merge}tool --tool-help" command now also prints the
     documentation for each tool (instead of just its name, as before).
     You can see an example of the output here ([3] and [4])

Finally, let me say that, while I like what this patch series achieves, I would
also *completely* understand if you decide not to merge it due to being a
complex solution to a simple problem that can be solved (as I had been doing up
until today) by just adding three line to one's .gitconfig.

  [mergetool "supermerge"]
        cmd = vim -f -d -c ...(custom complex sequence of vim commands)...
        trustExitCode = true

Let me know what you think.

Thanks.


References:

  [1] https://lore.kernel.org/git/20211019212020.25385-1-greenfoo@u92.eu/#r
  [2] https://pastebin.com/kuQ5pETG
  [3] https://pastebin.com/yvLWxeiM
  [4] https://pastebin.com/qNc7qymp


Fernando Ramos (3):
  vimdiff: new implementation with layout support
  vimdiff: add tool documentation
  vimdiff: remove deprecated {,g,n}vimdiff{1,2,3} variants

 git-mergetool--lib.sh |  12 +
 mergetools/vimdiff    | 697 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 677 insertions(+), 32 deletions(-)


base-commit: 876b1423317071f43c99666f3fc3db3642dfbe14
-- 
2.33.1

[PATCH 1/3] vimdiff: new implementation with layout support

[Fernando Ramos]

When running 'git mergetool -t vimdiff', a new configuration option
('mergetool.vimdiff.layout') can now be used to select how the user
wants the different windows, tabs and buffers to be displayed.

If the option is not provided, the layout will be the same one that was
being used before this commit (ie. two rows with LOCAL, BASE and COMMIT
in the top one and MERGED in the bottom one).

The 'vimdiff' variants ('vimdiff{1,2,3}') still work but, because they
represented nothing else than different layouts, are now internally
implemented as a subcase of 'vimdiff' with the corresponding
pre-configured 'layout'.

Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
---
 mergetools/vimdiff | 530 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 504 insertions(+), 26 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 96f6209a04..1f2e88777e 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -1,48 +1,509 @@
+#!/bin/bash
+
+# This script can be run in two different contexts:
+#
+#   - From git, when the user invokes the "vimdiff" merge tool. In this context
+#     this script expects the following environment variables (among others) to
+#     be defined (which is something "git" takes care of):
+#
+#       - $BASE
+#       - $LOCAL
+#       - $REMOTE
+#       - $MERGED
+#
+#     In this mode, all this script does is to run the next command:
+#
+#         vim -f -c ... $LOCAL $BASE $REMOTE $MERGED
+#
+#     ...where the "..." string depends on the value of the
+#     "mergetool.vimdiff.layout" configuration variable and is used to open vim
+#     with a certain layout of buffers, windows and tabs.
+#
+#   - From the shell, manually. This is only expected to be done by developers
+#     who are editing this script. When this happens, the script runs a battery
+#     of unit tests to make sure nothing breaks.
+#     In this context this script does not expect any particular environment
+#     variable to be set.
+#
+
+
+# Set to "true" to print debug messages to stderr
+DEBUG=false
+#DEBUG=true
+
+
+################################################################################
+## Internal functions (not meant to be used outside this script)
+################################################################################
+
+debug_print() { 
+	# Send message to stderr if global variable DEBUG is set to "true"
+        
+	if test "$DEBUG" = "true"
+	then
+		>&2 echo "$@";
+	fi
+}
+
+
+gen_cmd_aux() {
+	# Auxiliary function used from "gen_cmd()".
+	# Read that other function documentation for more details.
+
+	local LAYOUT=$1 
+	local CMD=$2  # This is a second (hidden) argument used for recursion  
+
+	debug_print
+	debug_print "LAYOUT    : $LAYOUT"
+	debug_print "CMD       : $CMD"
+
+	if test -z "$CMD"
+	then
+		CMD="echo" # vim "nop" operator
+	fi
+
+	local start=0
+	local end=${#LAYOUT}
+
+	local nested=0
+	local nested_min=100
+
+
+	# Step 1:
+	#
+	# Increase/decrease "start"/"end" indices respectively to get rid of
+	# outer parenthesis.
+	#
+	# Example:
+	#
+	#   - BEFORE: (( LOCAL | BASE ) - MERGED )
+	#   - AFTER :  ( LOCAL | BASE ) - MERGED 
+
+	for (( i=$start; i<$end; i++ )); do
+		if test "${LAYOUT:$i:1}" = " "
+		then
+			continue
+		fi
+
+		if test "${LAYOUT:$i:1}" = "("
+		then
+			nested=$(( nested + 1 ))
+			continue
+		fi
+
+		if test "${LAYOUT:$i:1}" = ")"
+		then
+			nested=$(( nested - 1 ))
+			continue
+		fi
+
+		if test "$nested" -lt "$nested_min"
+		then
+			nested_min=$nested
+		fi
+	done
+
+	debug_print "NESTED MIN: $nested_min"
+
+	while test "$nested_min" -gt "0"
+	do
+		start=$(( start + 1 ))
+		end=$(( end - 1 ))
+
+		start_minus_one=$(( start - 1 ))
+
+		while ! test "${LAYOUT:$start_minus_one:1}" = "("
+		do
+			start=$(( start + 1 ))
+			start_minus_one=$(( start_minus_one + 1 ))
+		done
+
+		while ! test "${LAYOUT:$end:1}" = ")"
+		do
+			end=$(( end - 1 ))
+		done
+
+		nested_min=$(( nested_min - 1 ))
+	done
+
+        debug_print "CLEAN     : ${LAYOUT:$start:$(( end - start ))}"
+
+
+	# Step 2:
+	#
+	# Search for all valid separators (";", "-" or "|") which are *not*
+	# inside parenthesis. Save the index at which each of them makes the
+	# first appearance.
+
+	local index_semicolon=""
+	local index_minus=""
+	local index_pipe=""
+
+	nested=0
+	for (( i=$start; i<$end; i++ )); do
+		if test "${LAYOUT:$i:1}" = " "
+		then
+			continue
+		fi
+
+		if test "${LAYOUT:$i:1}" = "("
+		then
+			nested=$(( nested + 1 ))
+			continue
+		fi
+
+		if test "${LAYOUT:$i:1}" = ")"
+		then
+			nested=$(( nested - 1 ))
+			continue
+		fi
+
+		if test "$nested" -eq "0"
+		then
+			current=${LAYOUT:$i:1}
+			
+			if test "$current" = ";"
+			then
+				if test -z "$index_semicolon"
+				then
+					index_semicolon=$i
+				fi
+
+			elif test "$current" = "-"
+			then
+				if test -z "$index_minus"
+				then
+					index_minus=$i
+				fi
+
+			elif test "$current" = "|"
+			then
+				if test -z "$index_pipe"
+				then
+					index_pipe=$i
+				fi
+			fi
+		fi
+	done
+
+
+	# Step 3:
+	#
+	# Process the separator with the highest order of precedence
+	# (";" has the highest precedence and "|" the lowest one).
+	#
+	# By "process" I mean recursively call this function twice: the first
+	# one with the substring at the left of the separator and the second one
+	# with the one at its right.
+
+	local terminate="false"
+
+	if ! test -z "$index_semicolon"
+	then
+		before="-tabnew"
+		after="tabnext"
+		index=$index_semicolon
+		terminate="true"
+
+	elif ! test -z "$index_minus"
+	then
+		before="split"
+		after="wincmd j"
+		index=$index_minus
+		terminate="true"
+
+	elif ! test -z "$index_pipe"
+	then
+		before="vertical split"
+		after="wincmd l"
+		index=$index_pipe
+		terminate="true"
+	fi
+
+	if  test "$terminate" = "true"
+	then
+		CMD="$CMD | $before"
+		CMD=$(gen_cmd_aux "${LAYOUT:$start:$(( index - start ))}" "$CMD")
+		CMD="$CMD | $after"
+		CMD=$(gen_cmd_aux "${LAYOUT:$(( index + 1 )):$(( ${#LAYOUT} - index ))}" "$CMD")
+		echo $CMD
+		return
+	fi
+
+
+	# Step 4:
+	#
+	# If we reach this point, it means there are no separators and we just
+	# need to print the command to display the specified buffer
+
+	local target=$(echo "${LAYOUT:$start:$(( end - start ))}" | sed 's:[ *();|-]::g')
+
+	if test "$target" = "LOCAL"
+	then
+		CMD="$CMD | 1b"
+
+	elif test "$target" = "BASE"
+	then
+		CMD="$CMD | 2b"
+
+	elif test "$target" = "REMOTE"
+	then
+		CMD="$CMD | 3b"
+
+	elif test "$target" = "MERGED"
+	then
+		CMD="$CMD | 4b"
+
+	else
+		CMD="$CMD | ERROR: >$target<"
+	fi
+
+	echo $CMD
+	return
+}
+
+
+gen_cmd() {
+	# This function returns (in global variable FINAL_CMD) the string that
+	# you can use when invoking "vim" (as shown next) to obtain a given
+	# layout:
+	#
+	#   $ vim -f $FINAL_CMD "$LOCAL" "$BASE" "$REMOTE" "$MERGED"
+	#
+	# It takes one single argument: a string containing the desired layout
+	# definition.
+	#
+	# The syntax of the "layout definitions" is explained in ... (TODO)...
+	# but you can already intuitively understand how it works by knowing
+	# that...
+	#
+	#   * ";" means "a new vim tab"
+	#   * "-" means "a new vim horizontal split"
+	#   * "|" means "a new vim vertical split"
+	#
+	# It also returns (in global variable FINAL_TARGET) the name ("LOCAL",
+	# "BASE", "REMOTE" or "MERGED") of the file that is marked with an "*",
+	# or "MERGED" if none of them is.
+	#
+	# Example:
+	#
+	#     gen_cmd "LOCAL* | REMOTE"
+	#     |
+	#     `-> FINAL_CMD    == "-c \"echo | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
+	#         FINAL_TARGET == "LOCAL"
+
+	local LAYOUT=$1 
+
+
+	# Search for a "*" in one of the files identifiers ("LOCAL", "BASE",
+	# "REMOTE", "MERGED"). If not found, use "MERGE" as the default file
+	# where changes will be saved.
+
+	AUX=$(echo "$LAYOUT" | grep -oe "[A-Z]\+\*")
+
+	if ! test -z "$AUX"
+	then
+	        FINAL_TARGET="${AUX:0:-1}"
+	else
+	        FINAL_TARGET="MERGED"
+	fi
+
+
+	# Obtain the first part of vim "-c" option to obtain the desired layout
+
+	CMD=$(gen_cmd_aux "$LAYOUT")
+
+
+	# Adjust the just obtained script depending on whether more than one
+	# windows are visible or not
+
+	if test $(echo $LAYOUT | wc -w) == "1"
+	then
+		CMD="$CMD | bufdo diffthis"
+        else
+		CMD="$CMD | tabdo windo diffthis"
+	fi
+
+
+	# Add an extra "-c" option to move to the first tab (notice that we
+	# can't simply append the command to the previous "-c" string as
+	# explained here: https://github.com/vim/vim/issues/9076
+
+	FINAL_CMD="-c \"$CMD\" -c \"tabfirst\""
+}
+
+
+run_unit_tests() {
+	# Function to make sure that we don't break anything when modifying this
+	# script.
+	#
+	# This function is automatically executed when you execute this script
+	# from the shell with environment variable "DEBUG_GIT_VIMDIFF" set (to
+	# any value).
+
+	local test_cases=(
+		`#Test case 00` "LOCAL | MERGED | REMOTE"
+		`#Test case 01` "LOCAL - MERGED - REMOTE"
+		`#Test case 02` "(LOCAL - REMOTE) | MERGED"
+		`#Test case 03` "MERGED | (LOCAL - REMOTE)"
+		`#Test case 04` "(LOCAL | REMOTE) - MERGED"
+		`#Test case 05` "MERGED - (LOCAL | REMOTE)"
+		`#Test case 06` "(LOCAL | BASE | REMOTE) - MERGED"
+		`#Test case 07` "(LOCAL - BASE - REMOTE) | MERGED"
+		`#Test case 08` "LOCAL* | REMOTE"
+		`#Test case 09` "MERGED"
+		`#Test case 10` "(LOCAL | BASE | REMOTE) - MERGED; BASE | LOCAL; BASE | REMOTE; (LOCAL - BASE - REMOTE) | MERGED"
+		`#Test case 11` "((LOCAL | REMOTE) - BASE) | MERGED"
+		`#Test case 12` "((LOCAL | REMOTE) - BASE) | ((LOCAL - REMOTE) | MERGED)"
+		`#Test case 13` "BASE | REMOTE ; BASE | LOCAL"
+	)
+
+	local expected_cmd=(
+		`#Test case 00` "-c \"echo | vertical split | 1b | wincmd l | vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
+		`#Test case 01` "-c \"echo | split | 1b | wincmd j | split | 4b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
+		`#Test case 02` "-c \"echo | vertical split | split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
+		`#Test case 03` "-c \"echo | vertical split | 4b | wincmd l | split | 1b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
+		`#Test case 04` "-c \"echo | split | vertical split | 1b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\" -c \"tabfirst\""
+		`#Test case 05` "-c \"echo | split | 4b | wincmd j | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
+		`#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\""
+		`#Test case 07` "-c \"echo | vertical split | split | 1b | wincmd j | split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
+		`#Test case 08` "-c \"echo | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
+		`#Test case 09` "-c \"echo | 4b | bufdo diffthis\" -c \"tabfirst\""
+		`#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\""
+		`#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\""
+		`#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\""
+		`#Test case 13` "-c \"echo | -tabnew | vertical split | 2b | wincmd l | 3b | tabnext | vertical split | 2b | wincmd l | 1b | tabdo windo diffthis\" -c \"tabfirst\""
+	)
+
+	local expected_target=(
+		`#Test case 00` "MERGED"
+		`#Test case 01` "MERGED"
+		`#Test case 02` "MERGED"
+		`#Test case 03` "MERGED"
+		`#Test case 04` "MERGED"
+		`#Test case 05` "MERGED"
+		`#Test case 06` "MERGED"
+		`#Test case 07` "MERGED"
+		`#Test case 08` "LOCAL"
+		`#Test case 09` "MERGED"
+		`#Test case 10` "MERGED"
+		`#Test case 11` "MERGED"
+		`#Test case 12` "MERGED"
+		`#Test case 13` "MERGED"
+	)
+
+	local at_least_one_ko="false"
+
+	for i in ${!test_cases[@]}; do
+		gen_cmd "${test_cases[$i]}"
+	
+		if test "$FINAL_CMD" = "${expected_cmd[$i]}" && test "$FINAL_TARGET" = "${expected_target[$i]}"
+		then
+			printf "Test Case #%02d: OK\n" $i
+		else
+			printf "Test Case #%02d: KO !!!!\n" $i
+			echo "  FINAL_CMD              : $FINAL_CMD"
+                        echo "  FINAL_CMD (expected)   : ${expected_cmd[$i]}"
+			echo "  FINAL_TARGET           : $FINAL_TARGET"
+                        echo "  FINAL_TARGET (expected): ${expected_target[$i]}"
+			at_least_one_ko="true"
+		fi
+	done
+	
+	if test "$at_least_one_ko" = "true"
+	then
+		return -1
+	else
+		return 0
+	fi
+}
+
+
+################################################################################
+## API functions (called from "git-mergetool--lib.sh")
+################################################################################
+
 diff_cmd () {
 	"$merge_tool_path" -R -f -d \
 		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
 }
 
+
 merge_cmd () {
+	layout=$(git config mergetool.$merge_tool.layout)
+	print_warning="false"
+
 	case "$1" in
 	*vimdiff)
-		if $base_present
+		if test -z "$layout"
 		then
-			"$merge_tool_path" -f -d -c '4wincmd w | wincmd J' \
-				"$LOCAL" "$BASE" "$REMOTE" "$MERGED"
-		else
-			"$merge_tool_path" -f -d -c 'wincmd l' \
-				"$LOCAL" "$MERGED" "$REMOTE"
+			# Default layout when none is specified
+			layout="(LOCAL | BASE | REMOTE) - MERGED"
 		fi
 		;;
 	*vimdiff1)
-		"$merge_tool_path" -f -d \
-			-c 'echon "Resolve conflicts leftward then save. Use :cq to abort."' \
-			"$LOCAL" "$REMOTE"
-		ret="$?"
-		if test "$ret" -eq 0
-		then
-			cp -- "$LOCAL" "$MERGED"
-		fi
-		return "$ret"
+		layout="LOCAL* | MERGED"
+		print_warning="true"
 		;;
 	*vimdiff2)
-		"$merge_tool_path" -f -d -c 'wincmd l' \
-			"$LOCAL" "$MERGED" "$REMOTE"
+		layout="LOCAL | MERGED | REMOTE"
+		print_warning="true"
 		;;
 	*vimdiff3)
-		if $base_present
-		then
-			"$merge_tool_path" -f -d -c 'hid | hid | hid' \
-				"$LOCAL" "$REMOTE" "$BASE" "$MERGED"
-		else
-			"$merge_tool_path" -f -d -c 'hid | hid' \
-				"$LOCAL" "$REMOTE" "$MERGED"
-		fi
+		layout="MERGED"
+		print_warning="true"
 		;;
 	esac
+
+	if test "$print_warning" = "true"
+	then
+		echo "WARNING:"
+		echo "WARNING: '$1' is going to be removed in a future version. You will be"
+		echo "WARNING: able to obtain the same result by selecting 'vimdiff' as the merge"
+		echo "WARNING: tool and setting configuration variable 'mergetool.vimdiff.layout'"
+		echo "WARNING: to the following value:"
+		echo "WARNING:"
+		echo "WARNING:     layout = \"$layout\""
+		echo "WARNING:"
+		echo "Press ENTER to continue..."
+		read
+	fi
+
+	gen_cmd "$layout"
+
+	debug_print ""
+	debug_print "FINAL CMD : $FINAL_CMD"
+	debug_print "FINAL TAR : $FINAL_TARGET"
+
+	if $base_present
+	then
+		eval "$merge_tool_path" \
+			-f $FINAL_CMD "$LOCAL" "$BASE" "$REMOTE" "$MERGED"
+	else
+		# If there is no BASE (example: a merge conflict in a new file
+		# with the same name created in both braches which didn't exist
+		# before), close all BASE windows using vim's "quit" command
+
+		FINAL_CMD=$(echo $FINAL_CMD | \
+			sed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g')
+
+		eval "$merge_tool_path" \
+			-f $FINAL_CMD "$LOCAL" "$REMOTE" "$MERGED"
+	fi
+
+
+	ret="$?"
+	if test "$ret" -eq 0
+	then
+		if test "$FINAL_TARGET" != "MERGED"
+		then
+			eval cp -- \$"$FINAL_TARGET" "$MERGED"
+		fi
+	fi
+	return "$ret"
 }
 
+
 translate_merge_tool_path() {
 	case "$1" in
 	nvimdiff*)
@@ -57,14 +518,31 @@ translate_merge_tool_path() {
 	esac
 }
 
+
 exit_code_trustable () {
 	true
 }
 
+
 list_tool_variants () {
 	for prefix in '' g n; do
-		for suffix in '' 1 2 3; do
+		for suffix in '' 1 2 3
+		do
 			echo "${prefix}vimdiff${suffix}"
 		done
 	done
 }
+
+
+################################################################################
+## Run unit tests when calling this script from a shell
+################################################################################
+
+if test $(ps -o stat= -p $PPID) = "Ss" && test $(ps -o stat= -p $$) = "S+"
+then
+	# Script is being manually run from command line (see
+	# https://stackoverflow.com/questions/4261876/check-if-bash-script-was-invoked-from-a-shell-or-another-script-application)
+
+	run_unit_tests
+fi
+
-- 
2.33.1

Re: [PATCH 1/3] vimdiff: new implementation with layout support

[David Aguilar]

This is an early review. We're still discussing the docs but there's a
few things worth mentioning now before we go too far.

On Thu, Nov 4, 2021 at 9:10 AM Fernando Ramos <greenfoo@u92.eu> wrote:
>
> When running 'git mergetool -t vimdiff', a new configuration option
> ('mergetool.vimdiff.layout') can now be used to select how the user
> wants the different windows, tabs and buffers to be displayed.
>
> If the option is not provided, the layout will be the same one that was
> being used before this commit (ie. two rows with LOCAL, BASE and COMMIT
> in the top one and MERGED in the bottom one).
>
> The 'vimdiff' variants ('vimdiff{1,2,3}') still work but, because they
> represented nothing else than different layouts, are now internally
> implemented as a subcase of 'vimdiff' with the corresponding
> pre-configured 'layout'.
>
> Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
> ---
>  mergetools/vimdiff | 530 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 504 insertions(+), 26 deletions(-)
>
> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
> index 96f6209a04..1f2e88777e 100644
> --- a/mergetools/vimdiff
> +++ b/mergetools/vimdiff
> @@ -1,48 +1,509 @@
> +#!/bin/bash
> +
> +# This script can be run in two different contexts:
> +#
> +#   - From git, when the user invokes the "vimdiff" merge tool. In this context
> +#     this script expects the following environment variables (among others) to
> +#     be defined (which is something "git" takes care of):
> +#
> +#       - $BASE
> +#       - $LOCAL
> +#       - $REMOTE
> +#       - $MERGED
> +#
> +#     In this mode, all this script does is to run the next command:
> +#
> +#         vim -f -c ... $LOCAL $BASE $REMOTE $MERGED
> +#
> +#     ...where the "..." string depends on the value of the
> +#     "mergetool.vimdiff.layout" configuration variable and is used to open vim
> +#     with a certain layout of buffers, windows and tabs.
> +#
> +#   - From the shell, manually. This is only expected to be done by developers
> +#     who are editing this script. When this happens, the script runs a battery
> +#     of unit tests to make sure nothing breaks.
> +#     In this context this script does not expect any particular environment
> +#     variable to be set.
> +#
> +
> +
> +# Set to "true" to print debug messages to stderr
> +DEBUG=false
> +#DEBUG=true


It might be better to omit "DEBUG=false" and call it
GIT_MERGETOOL_VIMDIFF_DEBUG.


> +
> +
> +################################################################################
> +## Internal functions (not meant to be used outside this script)
> +################################################################################
> +
> +debug_print() {
> +       # Send message to stderr if global variable DEBUG is set to "true"
> +
> +       if test "$DEBUG" = "true"
> +       then
> +               >&2 echo "$@";
> +       fi


... and then in here we can just check:

    if test -n "$GIT_MERGETOOL_VIMDIFF_DEBUG"
    then
        ....
    fi

and we won't ever have to edit the script to activate the debug mode
because it'll get inherited from the environment.



> +}
> +
> +
> +gen_cmd_aux() {
> +       # Auxiliary function used from "gen_cmd()".
> +       # Read that other function documentation for more details.
> +
> +       local LAYOUT=$1
> +       local CMD=$2  # This is a second (hidden) argument used for recursion


I believe "local" and other features used in this script are bash-isms.

We have to stick to a strict portable posix shell subset, so there's
some stuff here that will need adjustment for maximal portability.

We can't use any bashisms and we avoid "local" in scripted porcelains.

We also avoid shell arrays and "Substring Expansion" ${parameter:offset:length}.

https://github.com/git/git/blob/master/Documentation/CodingGuidelines#L41

That's going to require some rework of the implementation below to avoid these.


> +
> +       debug_print
> +       debug_print "LAYOUT    : $LAYOUT"
> +       debug_print "CMD       : $CMD"
> +
> +       if test -z "$CMD"
> +       then
> +               CMD="echo" # vim "nop" operator
> +       fi
> +
> +       local start=0
> +       local end=${#LAYOUT}
> +
> +       local nested=0
> +       local nested_min=100
> +
> +
> +       # Step 1:
> +       #
> +       # Increase/decrease "start"/"end" indices respectively to get rid of
> +       # outer parenthesis.
> +       #
> +       # Example:
> +       #
> +       #   - BEFORE: (( LOCAL | BASE ) - MERGED )
> +       #   - AFTER :  ( LOCAL | BASE ) - MERGED
> +
> +       for (( i=$start; i<$end; i++ )); do

Please avoid multiple statements on a single line (the ";" should be a
line break instead).

The for loop is a bash-ism. An alternative might be...

for i in $(seq $start $end)
do
    ...
done

but that is off-by-one because "seq" includes the $end value, so it'll
need to be decremented by 1.



> +               if test "${LAYOUT:$i:1}" = " "
> +               then
> +                       continue
> +               fi

This is going to need rework because we can't use "${LAYOUT:$i:1}".


> +
> +               if test "${LAYOUT:$i:1}" = "("
> +               then
> +                       nested=$(( nested + 1 ))


I mentioned this in the documentation commit as a comment about splitting a
long line, but we do not use Process Substitution <(list) or >(list) either
so that's another reason to break up the pipeline I mentioned in the previous
email.


Arithmetic substitution is something we do use, though, so this would be fine.


> +                       continue
> +               fi
> +
> +               if test "${LAYOUT:$i:1}" = ")"
> +               then
> +                       nested=$(( nested - 1 ))
> +                       continue
> +               fi
> +
> +               if test "$nested" -lt "$nested_min"
> +               then
> +                       nested_min=$nested
> +               fi
> +       done
> +
> +       debug_print "NESTED MIN: $nested_min"
> +
> +       while test "$nested_min" -gt "0"
> +       do
> +               start=$(( start + 1 ))
> +               end=$(( end - 1 ))
> +
> +               start_minus_one=$(( start - 1 ))
> +
> +               while ! test "${LAYOUT:$start_minus_one:1}" = "("
> +               do
> +                       start=$(( start + 1 ))
> +                       start_minus_one=$(( start_minus_one + 1 ))
> +               done
> +
> +               while ! test "${LAYOUT:$end:1}" = ")"
> +               do
> +                       end=$(( end - 1 ))
> +               done
> +
> +               nested_min=$(( nested_min - 1 ))
> +       done
> +
> +        debug_print "CLEAN     : ${LAYOUT:$start:$(( end - start ))}"
> +
> +
> +       # Step 2:
> +       #
> +       # Search for all valid separators (";", "-" or "|") which are *not*
> +       # inside parenthesis. Save the index at which each of them makes the
> +       # first appearance.


I now understand why the parens are helpful. They seem to make the
implementation simpler/possible.

> +
> +       local index_semicolon=""
> +       local index_minus=""
> +       local index_pipe=""

Drop "local".

Semantic names for these might be helpful. "index_new_tab",
"index_vertical_split" and "index_horizontal_split" might be easier to
understand and would be resilient to sugs about using different
tokens.


> +
> +       nested=0
> +       for (( i=$start; i<$end; i++ )); do
> +               if test "${LAYOUT:$i:1}" = " "
> +               then
> +                       continue
> +               fi
> +
> +               if test "${LAYOUT:$i:1}" = "("
> +               then
> +                       nested=$(( nested + 1 ))


Here and below -- should that be nested=$(( $nested + 1 )) ?

It seems to be missing a '$' prefix on the inner $nested variable.

My shell accepts both but the predominant style in the git test suite
is the use $nested, so let's do that.



> +                       continue
> +               fi
> +
> +               if test "${LAYOUT:$i:1}" = ")"
> +               then
> +                       nested=$(( nested - 1 ))
> +                       continue
> +               fi
> +
> +               if test "$nested" -eq "0"
> +               then
> +                       current=${LAYOUT:$i:1}
> +
> +                       if test "$current" = ";"
> +                       then
> +                               if test -z "$index_semicolon"
> +                               then
> +                                       index_semicolon=$i
> +                               fi
> +
> +                       elif test "$current" = "-"
> +                       then
> +                               if test -z "$index_minus"
> +                               then
> +                                       index_minus=$i
> +                               fi
> +
> +                       elif test "$current" = "|"
> +                       then
> +                               if test -z "$index_pipe"
> +                               then
> +                                       index_pipe=$i
> +                               fi
> +                       fi
> +               fi
> +       done
> +
> +
> +       # Step 3:
> +       #
> +       # Process the separator with the highest order of precedence
> +       # (";" has the highest precedence and "|" the lowest one).
> +       #
> +       # By "process" I mean recursively call this function twice: the first
> +       # one with the substring at the left of the separator and the second one
> +       # with the one at its right.
> +
> +       local terminate="false"
> +
> +       if ! test -z "$index_semicolon"
> +       then
> +               before="-tabnew"
> +               after="tabnext"
> +               index=$index_semicolon
> +               terminate="true"
> +
> +       elif ! test -z "$index_minus"
> +       then
> +               before="split"
> +               after="wincmd j"
> +               index=$index_minus
> +               terminate="true"
> +
> +       elif ! test -z "$index_pipe"
> +       then
> +               before="vertical split"
> +               after="wincmd l"
> +               index=$index_pipe
> +               terminate="true"
> +       fi
> +
> +       if  test "$terminate" = "true"
> +       then
> +               CMD="$CMD | $before"
> +               CMD=$(gen_cmd_aux "${LAYOUT:$start:$(( index - start ))}" "$CMD")
> +               CMD="$CMD | $after"
> +               CMD=$(gen_cmd_aux "${LAYOUT:$(( index + 1 )):$(( ${#LAYOUT} - index ))}" "$CMD")
> +               echo $CMD
> +               return
> +       fi
> +
> +
> +       # Step 4:
> +       #
> +       # If we reach this point, it means there are no separators and we just
> +       # need to print the command to display the specified buffer
> +
> +       local target=$(echo "${LAYOUT:$start:$(( end - start ))}" | sed 's:[ *();|-]::g')
> +
> +       if test "$target" = "LOCAL"
> +       then
> +               CMD="$CMD | 1b"
> +
> +       elif test "$target" = "BASE"
> +       then
> +               CMD="$CMD | 2b"
> +
> +       elif test "$target" = "REMOTE"
> +       then
> +               CMD="$CMD | 3b"
> +
> +       elif test "$target" = "MERGED"
> +       then
> +               CMD="$CMD | 4b"
> +
> +       else
> +               CMD="$CMD | ERROR: >$target<"
> +       fi
> +
> +       echo $CMD
> +       return
> +}
> +
> +
> +gen_cmd() {
> +       # This function returns (in global variable FINAL_CMD) the string that
> +       # you can use when invoking "vim" (as shown next) to obtain a given
> +       # layout:
> +       #
> +       #   $ vim -f $FINAL_CMD "$LOCAL" "$BASE" "$REMOTE" "$MERGED"
> +       #
> +       # It takes one single argument: a string containing the desired layout
> +       # definition.
> +       #
> +       # The syntax of the "layout definitions" is explained in ... (TODO)...
> +       # but you can already intuitively understand how it works by knowing
> +       # that...
> +       #
> +       #   * ";" means "a new vim tab"
> +       #   * "-" means "a new vim horizontal split"
> +       #   * "|" means "a new vim vertical split"
> +       #
> +       # It also returns (in global variable FINAL_TARGET) the name ("LOCAL",
> +       # "BASE", "REMOTE" or "MERGED") of the file that is marked with an "*",
> +       # or "MERGED" if none of them is.
> +       #
> +       # Example:
> +       #
> +       #     gen_cmd "LOCAL* | REMOTE"
> +       #     |
> +       #     `-> FINAL_CMD    == "-c \"echo | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
> +       #         FINAL_TARGET == "LOCAL"
> +
> +       local LAYOUT=$1
> +
> +
> +       # Search for a "*" in one of the files identifiers ("LOCAL", "BASE",
> +       # "REMOTE", "MERGED"). If not found, use "MERGE" as the default file
> +       # where changes will be saved.
> +
> +       AUX=$(echo "$LAYOUT" | grep -oe "[A-Z]\+\*")


From Documentatin/CodingGuidelines:

As to use of grep, stick to a subset of BRE (namely, no {m,n},
   [::], [==], or [..]) for portability.



> +
> +       if ! test -z "$AUX"
> +       then
> +               FINAL_TARGET="${AUX:0:-1}"
> +       else
> +               FINAL_TARGET="MERGED"
> +       fi


The conditional above is better written as:

    if test -n "$AUX"
    then
        ...
    else
        ...
    fi

"test -n" is the logical opposite of "test -z", so "! test -z" can be
replaced with "test -n".




> +
> +
> +       # Obtain the first part of vim "-c" option to obtain the desired layout
> +
> +       CMD=$(gen_cmd_aux "$LAYOUT")
> +
> +
> +       # Adjust the just obtained script depending on whether more than one
> +       # windows are visible or not
> +
> +       if test $(echo $LAYOUT | wc -w) == "1"
> +       then
> +               CMD="$CMD | bufdo diffthis"
> +        else
> +               CMD="$CMD | tabdo windo diffthis"
> +       fi


The output of "wc -c" is non-portable. It contains leading whitespace
on some platforms.

The test expression should be:

   test "$value" = 1

with a single "=" rather than "==".



> +
> +
> +       # Add an extra "-c" option to move to the first tab (notice that we
> +       # can't simply append the command to the previous "-c" string as
> +       # explained here: https://github.com/vim/vim/issues/9076
> +
> +       FINAL_CMD="-c \"$CMD\" -c \"tabfirst\""
> +}
> +
> +
> +run_unit_tests() {
> +       # Function to make sure that we don't break anything when modifying this
> +       # script.
> +       #
> +       # This function is automatically executed when you execute this script
> +       # from the shell with environment variable "DEBUG_GIT_VIMDIFF" set (to
> +       # any value).
> +
> +       local test_cases=(
> +               `#Test case 00` "LOCAL | MERGED | REMOTE"
> +               `#Test case 01` "LOCAL - MERGED - REMOTE"
> +               `#Test case 02` "(LOCAL - REMOTE) | MERGED"
> +               `#Test case 03` "MERGED | (LOCAL - REMOTE)"
> +               `#Test case 04` "(LOCAL | REMOTE) - MERGED"
> +               `#Test case 05` "MERGED - (LOCAL | REMOTE)"
> +               `#Test case 06` "(LOCAL | BASE | REMOTE) - MERGED"
> +               `#Test case 07` "(LOCAL - BASE - REMOTE) | MERGED"
> +               `#Test case 08` "LOCAL* | REMOTE"
> +               `#Test case 09` "MERGED"
> +               `#Test case 10` "(LOCAL | BASE | REMOTE) - MERGED; BASE | LOCAL; BASE | REMOTE; (LOCAL - BASE - REMOTE) | MERGED"
> +               `#Test case 11` "((LOCAL | REMOTE) - BASE) | MERGED"
> +               `#Test case 12` "((LOCAL | REMOTE) - BASE) | ((LOCAL - REMOTE) | MERGED)"
> +               `#Test case 13` "BASE | REMOTE ; BASE | LOCAL"
> +       )
> +
> +       local expected_cmd=(
> +               `#Test case 00` "-c \"echo | vertical split | 1b | wincmd l | vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
> +               `#Test case 01` "-c \"echo | split | 1b | wincmd j | split | 4b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
> +               `#Test case 02` "-c \"echo | vertical split | split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
> +               `#Test case 03` "-c \"echo | vertical split | 4b | wincmd l | split | 1b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
> +               `#Test case 04` "-c \"echo | split | vertical split | 1b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\" -c \"tabfirst\""
> +               `#Test case 05` "-c \"echo | split | 4b | wincmd j | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
> +               `#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\""
> +               `#Test case 07` "-c \"echo | vertical split | split | 1b | wincmd j | split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
> +               `#Test case 08` "-c \"echo | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
> +               `#Test case 09` "-c \"echo | 4b | bufdo diffthis\" -c \"tabfirst\""
> +               `#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\""
> +               `#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\""
> +               `#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\""
> +               `#Test case 13` "-c \"echo | -tabnew | vertical split | 2b | wincmd l | 3b | tabnext | vertical split | 2b | wincmd l | 1b | tabdo windo diffthis\" -c \"tabfirst\""
> +       )
> +
> +       local expected_target=(
> +               `#Test case 00` "MERGED"
> +               `#Test case 01` "MERGED"
> +               `#Test case 02` "MERGED"
> +               `#Test case 03` "MERGED"
> +               `#Test case 04` "MERGED"
> +               `#Test case 05` "MERGED"
> +               `#Test case 06` "MERGED"
> +               `#Test case 07` "MERGED"
> +               `#Test case 08` "LOCAL"
> +               `#Test case 09` "MERGED"
> +               `#Test case 10` "MERGED"
> +               `#Test case 11` "MERGED"
> +               `#Test case 12` "MERGED"
> +               `#Test case 13` "MERGED"
> +       )


We can't use shell arrays. This part really tells me that I don't
understand bash at all.

I don't understand what the backticks are doing.. is it actually
running something?

That's just a rhetorical question.. I don't actually know, but it's okay if we
ignore this question since we already indicated that we'll have to do
w/out arrays.




> +
> +       local at_least_one_ko="false"
> +
> +       for i in ${!test_cases[@]}; do


I suspect ${!test_cases[@]} is some bash-ism that we can't use.

For the final patch, I think it would be good if we had a way to run this
through the test suite if possible rather than needing to run the script
directly.

It might have more leeway to setup the environment for testing that way too.


> +               gen_cmd "${test_cases[$i]}"
> +
> +               if test "$FINAL_CMD" = "${expected_cmd[$i]}" && test "$FINAL_TARGET" = "${expected_target[$i]}"
> +               then
> +                       printf "Test Case #%02d: OK\n" $i
> +               else
> +                       printf "Test Case #%02d: KO !!!!\n" $i
> +                       echo "  FINAL_CMD              : $FINAL_CMD"
> +                        echo "  FINAL_CMD (expected)   : ${expected_cmd[$i]}"
> +                       echo "  FINAL_TARGET           : $FINAL_TARGET"
> +                        echo "  FINAL_TARGET (expected): ${expected_target[$i]}"
> +                       at_least_one_ko="true"
> +               fi
> +       done
> +
> +       if test "$at_least_one_ko" = "true"
> +       then
> +               return -1
> +       else
> +               return 0
> +       fi
> +}
> +
> +
> +################################################################################
> +## API functions (called from "git-mergetool--lib.sh")
> +################################################################################
> +
>  diff_cmd () {
>         "$merge_tool_path" -R -f -d \
>                 -c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
>  }
>
> +
>  merge_cmd () {
> +       layout=$(git config mergetool.$merge_tool.layout)
> +       print_warning="false"
> +
>         case "$1" in
>         *vimdiff)
> -               if $base_present
> +               if test -z "$layout"
>                 then
> -                       "$merge_tool_path" -f -d -c '4wincmd w | wincmd J' \
> -                               "$LOCAL" "$BASE" "$REMOTE" "$MERGED"
> -               else
> -                       "$merge_tool_path" -f -d -c 'wincmd l' \
> -                               "$LOCAL" "$MERGED" "$REMOTE"
> +                       # Default layout when none is specified
> +                       layout="(LOCAL | BASE | REMOTE) - MERGED"
>                 fi
>                 ;;
>         *vimdiff1)
> -               "$merge_tool_path" -f -d \
> -                       -c 'echon "Resolve conflicts leftward then save. Use :cq to abort."' \
> -                       "$LOCAL" "$REMOTE"
> -               ret="$?"
> -               if test "$ret" -eq 0
> -               then
> -                       cp -- "$LOCAL" "$MERGED"
> -               fi
> -               return "$ret"
> +               layout="LOCAL* | MERGED"
> +               print_warning="true"
>                 ;;
>         *vimdiff2)
> -               "$merge_tool_path" -f -d -c 'wincmd l' \
> -                       "$LOCAL" "$MERGED" "$REMOTE"
> +               layout="LOCAL | MERGED | REMOTE"
> +               print_warning="true"
>                 ;;
>         *vimdiff3)
> -               if $base_present
> -               then
> -                       "$merge_tool_path" -f -d -c 'hid | hid | hid' \
> -                               "$LOCAL" "$REMOTE" "$BASE" "$MERGED"
> -               else
> -                       "$merge_tool_path" -f -d -c 'hid | hid' \
> -                               "$LOCAL" "$REMOTE" "$MERGED"
> -               fi
> +               layout="MERGED"
> +               print_warning="true"
>                 ;;
>         esac
> +
> +       if test "$print_warning" = "true"
> +       then
> +               echo "WARNING:"
> +               echo "WARNING: '$1' is going to be removed in a future version. You will be"
> +               echo "WARNING: able to obtain the same result by selecting 'vimdiff' as the merge"
> +               echo "WARNING: tool and setting configuration variable 'mergetool.vimdiff.layout'"
> +               echo "WARNING: to the following value:"
> +               echo "WARNING:"
> +               echo "WARNING:     layout = \"$layout\""
> +               echo "WARNING:"
> +               echo "Press ENTER to continue..."
> +               read
> +       fi

I wonder if we should ever remove the old variants.

We could just keep them around forever, and then users won't ever be bothered.

The bulk of the improvement here is to improve the implementation so
that we don't
ever have to add any new variants, and to fold the implementations
together into a
common approach so that there's less code to maintain.

It seems like there's really no need to burden users with our
implementation choices.

I personally would be in favor of dropping the deprecation angle and treating
these patches more-so as an refactoring of the implementation.


> +
> +       gen_cmd "$layout"
> +
> +       debug_print ""
> +       debug_print "FINAL CMD : $FINAL_CMD"
> +       debug_print "FINAL TAR : $FINAL_TARGET"
> +
> +       if $base_present
> +       then
> +               eval "$merge_tool_path" \
> +                       -f $FINAL_CMD "$LOCAL" "$BASE" "$REMOTE" "$MERGED"
> +       else
> +               # If there is no BASE (example: a merge conflict in a new file
> +               # with the same name created in both braches which didn't exist
> +               # before), close all BASE windows using vim's "quit" command
> +
> +               FINAL_CMD=$(echo $FINAL_CMD | \
> +                       sed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g')
> +
> +               eval "$merge_tool_path" \
> +                       -f $FINAL_CMD "$LOCAL" "$REMOTE" "$MERGED"
> +       fi
> +
> +
> +       ret="$?"
> +       if test "$ret" -eq 0

This should be:

    if test "$ret" = 0

> +       then
> +               if test "$FINAL_TARGET" != "MERGED"
> +               then
> +                       eval cp -- \$"$FINAL_TARGET" "$MERGED"

This eval may not be safe when the value contains whitespace or shell
metacharacters.

I think it might be better to just spell it out and be explicit.

It's more code but it'll be easier to follow:

case "$FINAL_TARGET" in
LOCAL)
    source_path="$LOCAL"
    ;;
REMOTE)
    source_path="$REMOTE"
    ;;
MERGED|*)
    # Do nothing
    source_path=
    ;;
esac

if test -n "$source_path"
then
    cp -- "$source_path" "$MERGED"
fi



> +               fi
> +       fi
> +       return "$ret"
>  }
>
> +
>  translate_merge_tool_path() {
>         case "$1" in
>         nvimdiff*)
> @@ -57,14 +518,31 @@ translate_merge_tool_path() {
>         esac
>  }
>
> +
>  exit_code_trustable () {
>         true
>  }
>
> +
>  list_tool_variants () {
>         for prefix in '' g n; do
> -               for suffix in '' 1 2 3; do
> +               for suffix in '' 1 2 3
> +               do
>                         echo "${prefix}vimdiff${suffix}"
>                 done
>         done
>  }
> +
> +
> +################################################################################
> +## Run unit tests when calling this script from a shell
> +################################################################################
> +
> +if test $(ps -o stat= -p $PPID) = "Ss" && test $(ps -o stat= -p $$) = "S+"
> +then
> +       # Script is being manually run from command line (see
> +       # https://stackoverflow.com/questions/4261876/check-if-bash-script-was-invoked-from-a-shell-or-another-script-application)
> +
> +       run_unit_tests
> +fi


I'm not 100% sure, but I suspect this is probably non-portable and
will have some issues.

This is another vote for setting something up to go through the test suite.

The test suite can set some GIT_MERGETOOL_VIMDIFF_TESTING variable
that the scriplet
can key off of explicitly rather than having the script embed this stuff in it.

A test could then source the scriptlet directly and exercise functions
provided by it.

If the implementation details get too complex for shell, I wonder if
it's time to split
off a small helper command in C that does the layout -> vim script
translation. TBD.

The parsing code is pretty complex but if there's a way to do it in our portable
posix shell subset then we'll take that. There might be a point where having a
small dedicated helper command might be easier.

You'll probably have the best sense of that, though.

Thanks for diving into making this happen.
-- 
David

Re: [PATCH 1/3] vimdiff: new implementation with layout support

[Eric Sunshine]

On Sun, Nov 7, 2021 at 5:42 PM David Aguilar <davvid@gmail.com> wrote:
> On Thu, Nov 4, 2021 at 9:10 AM Fernando Ramos <greenfoo@u92.eu> wrote:
> > +       AUX=$(echo "$LAYOUT" | grep -oe "[A-Z]\+\*")
>
> From Documentatin/CodingGuidelines:
>
> As to use of grep, stick to a subset of BRE (namely, no {m,n},
>    [::], [==], or [..]) for portability.

Also, `grep -o` isn't POSIX.

> > +       if test $(echo $LAYOUT | wc -w) == "1"
> > +       then
> > +               CMD="$CMD | bufdo diffthis"
> > +        else
> > +               CMD="$CMD | tabdo windo diffthis"
> > +       fi
>
> The output of "wc -c" is non-portable. It contains leading whitespace
> on some platforms.
>
> The test expression should be:
>
>    test "$value" = 1
>
> with a single "=" rather than "==".

For clarification, the leading whitespace emitted by some `wc`
implementations is only a problem when encapsulated in a string. For
instance, like this:

    if test "$(... | wc -w)" = "1"

in which case "  1"  won't equal "1". The usage here, however, should
be okay since the output is not quoted.

Quite correct about using "=" (or even "-eq") here rather than "==", though.

> > +       if $base_present
> > +       then
> > +               eval "$merge_tool_path" \
> > +                       -f $FINAL_CMD "$LOCAL" "$BASE" "$REMOTE" "$MERGED"
> > +       else
> > +               [...]
> > +               eval "$merge_tool_path" \
> > +                       -f $FINAL_CMD "$LOCAL" "$REMOTE" "$MERGED"
> > +       fi
> > +
> > +       ret="$?"
> > +       if test "$ret" -eq 0
>
> This should be:
>
>     if test "$ret" = 0

Or simpler, no need for `ret` at all:

    if test $? -eq 0

(or `if test $? = 0` -- either works)

Another (perhaps better) alternative is to assign the result of `eval`
to `ret` at the point of invocation, which lessens the cognitive load
a bit since you don't have to scan backward through the code trying to
figure out what $? refers to.

Also, why is `eval` needed here? Is there something non-obvious going
on? (Genuine question; I didn't trace the code thoroughly to
understand.)

> > +                       eval cp -- \$"$FINAL_TARGET" "$MERGED"
>
> This eval may not be safe when the value contains whitespace or shell
> metacharacters.
>
> I think it might be better to just spell it out and be explicit.
>
> It's more code but it'll be easier to follow:
> [...]
> if test -n "$source_path"
> then
>     cp -- "$source_path" "$MERGED"
> fi

I suspect `--` also needs to be avoided since it is not POSIX.

[PATCH 2/3] vimdiff: add tool documentation

[Fernando Ramos]

Running 'git {merge,diff}tool --tool-help' now also prints usage
information about the vimdiff tool (and its variantes) instead of just
its name.

Two new functions ('diff_cmd_help()' and 'merge_cmd_help()') have been
added to the set of functions that each merge tool (ie. scripts found
inside "mergetools/") can overwrite to provided tool specific
information.

Right now, only 'mergetools/vimdiff' implements these functions, but
other tools are encouraged to do so in the future, specially if they
take configuration options not explained anywhere else (as it is the
case with the 'vimdiff' tool and the new 'layout' option)

Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
---
 git-mergetool--lib.sh |  12 ++
 mergetools/vimdiff    | 272 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 284 insertions(+)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 542a6a75eb..3964cd8f99 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -64,6 +64,10 @@ $(list_tool_variants)"
 				fi
 				shown_any=yes
 				printf "%s%s\n" "$per_line_prefix" "$toolname"
+				while IFS= read -r line
+				do
+					printf "%s\t%s\n" "$per_line_prefix" "$line"
+                                done < <(diff_mode && diff_cmd_help "$toolname" || merge_cmd_help "$toolname")
 			fi
 		done
 
@@ -162,10 +166,18 @@ setup_tool () {
 		return 1
 	}
 
+	diff_cmd_help () {
+		return 0
+	}
+
 	merge_cmd () {
 		return 1
 	}
 
+	merge_cmd_help () {
+		return 0
+	}
+
 	hide_resolved_enabled () {
 		return 0
 	}
diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 1f2e88777e..aa8fbc0a19 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -428,6 +428,46 @@ diff_cmd () {
 		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
 }
 
+diff_cmd_help() {
+	case "$1" in
+	vimdiff)
+		cat <<-ENDOFMESSAGE
+			Opens vim with two vertical windows: LOCAL changes will be placed in the left
+			window and REMOTE changes in the right one.
+		ENDOFMESSAGE
+		;;
+	vimdiff*)
+		cat <<-ENDOFMESSAGE
+			Same as 'vimdiff'
+		ENDOFMESSAGE
+		;;
+	gvimdiff)
+		cat <<-ENDOFMESSAGE
+			Same as 'vimdiff' but opens 'gvim' instead (which uses a graphical toolkit for
+			opening its own window)
+		ENDOFMESSAGE
+		;;
+	gvimdiff*)
+		cat <<-ENDOFMESSAGE
+			Same as 'gvimdiff'
+		ENDOFMESSAGE
+		;;
+	nvimdiff)
+		cat <<-ENDOFMESSAGE
+			Same as 'vimdiff' but opens 'neovim' instead (which is a fork of the original
+			'vim' 'focused on extensibility and usability' according to their authors)
+		ENDOFMESSAGE
+		;;
+	nvimdiff*)
+		cat <<-ENDOFMESSAGE
+			Same as 'nvimdiff'
+		ENDOFMESSAGE
+		;;
+	esac
+
+	return 0
+}
+
 
 merge_cmd () {
 	layout=$(git config mergetool.$merge_tool.layout)
@@ -503,6 +543,238 @@ merge_cmd () {
 	return "$ret"
 }
 
+merge_cmd_help() {
+	case "$1" in
+	vimdiff)
+		cat <<-ENDOFMESSAGE
+			Opens vim with a 4 windows layout distributed in the following way:
+			
+			    ------------------------------------------
+			    |             |           |              |
+			    |   LOCAL     |   BASE    |   REMOTE     |
+			    |             |           |              |
+			    ------------------------------------------
+			    |                                        |
+			    |                MERGED                  |
+			    |                                        |
+			    ------------------------------------------
+			
+			"LOCAL", "BASE" and "REMOTE" are read-only buffers showing the contents of the
+			conflicting file in a specific git commit ("commit you are merging into",
+			"common ancestor commit" and "commit you are merging from" respectively)
+			
+			"MERGED" is a writable buffer where you have to resolve the conflicts (using the
+			other read-only buffers as a reference). Once you are done, save and exit "vim"
+			as usual (":wq") or, if you want to abort, exit using ":cq".
+			
+			You can change the windows layout used by vim by setting configuration variable
+			"mergetool.vimdiff.layout" which accepts a string where these separators have
+			special meaning:
+			
+			  - ";" is used to "open a new tab"
+			  - "-" is used to "open a new vertical split"
+			  - "|" is used to "open a new horizontal split"
+			
+			Let's see some examples to undertand how it works:
+			
+			  * layout = "(LOCAL | BASE | REMOTE) - MERGED"
+			
+			    This is exactly the same as the default layout we have already seen.
+			
+			  * layout = "LOCAL | MERGED | REMOTE"
+			
+			    If, for some reason, we are not interested in the "BASE" buffer.
+			
+			           ------------------------------------------
+			           |             |           |              |
+			           |             |           |              |
+			           |   LOCAL     |   MERGED  |   REMOTE     |
+			           |             |           |              |
+			           |             |           |              |
+			           ------------------------------------------
+			
+			  * layout = "MERGED"
+			
+			    Only the "MERGED" buffer will be shown. Note, however, that all the other
+			    ones are still loaded in vim, and you can access them with the "buffers"
+			    command. 
+			
+			           ------------------------------------------
+			           |                                        |
+			           |                                        |
+			           |                 MERGED                 |
+			           |                                        |
+			           |                                        |
+			           ------------------------------------------
+			
+			  * layout = "LOCAL* | REMOTE"
+			
+			    When "MERGED" is not present in the layout, you must "mark" one of the
+			    buffers with an asterisk. That will become the buffer you need to edit and
+			    save after resolving the conflicts.
+			
+			           ------------------------------------------
+			           |                   |                    |
+			           |                   |                    |
+			           |                   |                    |
+			           |     LOCAL         |    REMOTE          |
+			           |                   |                    |
+			           |                   |                    |
+			           |                   |                    |
+			           ------------------------------------------
+			
+                          * layout = "(LOCAL | BASE | REMOTE) - MERGED; BASE | LOCAL; BASE | REMOTE"
+			
+			    Three tabs will open: the first one is a copy of the default layout, while
+			    the other two only show the differences between "BASE" and "LOCAL" and
+			    "BASE" and "REMOTE" respectively.
+			 
+			           ------------------------------------------
+			           | <TAB #1> |  TAB #2  |  TAB #3  |       |
+			           ------------------------------------------
+			           |             |           |              |
+			           |   LOCAL     |   BASE    |   REMOTE     |
+			           |             |           |              |
+			           ------------------------------------------
+			           |                                        |
+			           |                MERGED                  |
+			           |                                        |
+			           ------------------------------------------
+			
+			           ------------------------------------------
+			           |  TAB #1  | <TAB #2> |  TAB #3  |       |
+			           ------------------------------------------
+			           |                   |                    |
+			           |                   |                    |
+			           |                   |                    |
+			           |     BASE          |    LOCAL           |
+			           |                   |                    |
+			           |                   |                    |
+			           |                   |                    |
+			           ------------------------------------------
+			
+			           ------------------------------------------
+			           |  TAB #1  |  TAB #2  | <TAB #3> |       |
+			           ------------------------------------------
+			           |                   |                    |
+			           |                   |                    |
+			           |                   |                    |
+			           |     BASE          |    REMOTE          |
+			           |                   |                    |
+			           |                   |                    |
+			           |                   |                    |
+			           ------------------------------------------
+			
+                          * layout = "(LOCAL | BASE | REMOTE) - MERGED; BASE | LOCAL; BASE | REMOTE; (LOCAL - BASE - REMOTE) | MERGED"
+			
+			    Same as the previous example, but adds a fourth tab with the same
+			    information as the first tab, with a different layout.
+			 
+			           ---------------------------------------------
+			           |  TAB #1  |  TAB #2  |  TAB #3  | <TAB #4> |
+			           ---------------------------------------------
+			           |       LOCAL         |                     |
+			           |---------------------|                     |
+			           |       BASE          |        MERGED       |
+			           |---------------------|                     |
+			           |       REMOTE        |                     |
+			           ---------------------------------------------
+
+		ENDOFMESSAGE
+		;;
+	vimdiff1)
+		cat <<-ENDOFMESSAGE
+			Same as 'vimdiff' using this layout: "LOCAL* | REMOTE"
+			
+			This will probably be deprecated in the future. Please use "vimdiff" and
+			manually set the "mergetool.vimdiff.layout" configuration variable instead.
+		ENDOFMESSAGE
+		;;
+	vimdiff2)
+		cat <<-ENDOFMESSAGE
+			Same as 'vimdiff' using this layout: "LOCAL | MERGED | REMOTE"
+			
+			This will probably be deprecated in the future. Please use "vimdiff" and
+			manually set the "mergetool.vimdiff.layout" configuration variable instead.
+		ENDOFMESSAGE
+		;;
+	vimdiff3)
+		cat <<-ENDOFMESSAGE
+			Same as 'vimdiff' using this layout: "MERGED"
+			
+			This will probably be deprecated in the future. Please use "vimdiff" and
+			manually set the "mergetool.vimdiff.layout" configuration variable instead.
+		ENDOFMESSAGE
+		;;
+	gvimdiff)
+		cat <<-ENDOFMESSAGE
+			Same as 'vimdiff' but opens 'gvim' instead (which uses a graphical toolkit for
+			opening its own window).
+			The desired layout can be set with configuration variable
+			"mergetool.gvimdiff.layout"
+		ENDOFMESSAGE
+		;;
+	gvimdiff1)
+		cat <<-ENDOFMESSAGE
+			Same as 'gvimdiff' using this layout: "LOCAL* | REMOTE"
+			
+			This will probably be deprecated in the future. Please use "gvimdiff" and
+			manually set the "mergetool.gvimdiff.layout" configuration variable instead.
+		ENDOFMESSAGE
+		;;
+	gvimdiff2)
+		cat <<-ENDOFMESSAGE
+			Same as 'gvimdiff' using this layout: "LOCAL | MERGED | REMOTE"
+			
+			This will probably be deprecated in the future. Please use "gvimdiff" and
+			manually set the "mergetool.gvimdiff.layout" configuration variable instead.
+		ENDOFMESSAGE
+		;;
+	gvimdiff3)
+		cat <<-ENDOFMESSAGE
+			Same as 'gvimdiff' using this layout: "MERGED"
+			
+			This will probably be deprecated in the future. Please use "gvimdiff" and
+			manually set the "mergetool.gvimdiff.layout" configuration variable instead.
+		ENDOFMESSAGE
+		;;
+	nvimdiff)
+		cat <<-ENDOFMESSAGE
+			Same as 'vimdiff' but opens 'neovim' instead (which is a fork of the original
+			'vim' 'focused on extensibility and usability' according to their authors)
+			The desired layout can be set with configuration variable
+			"mergetool.nvimdiff.layout"
+		ENDOFMESSAGE
+		;;
+	nvimdiff1)
+		cat <<-ENDOFMESSAGE
+			Same as 'nvimdiff' using this layout: "LOCAL* | REMOTE"
+			
+			This will probably be deprecated in the future. Please use "nvimdiff" and
+			manually set the "mergetool.nvimdiff.layout" configuration variable instead.
+		ENDOFMESSAGE
+		;;
+	nvimdiff2)
+		cat <<-ENDOFMESSAGE
+			Same as 'nvimdiff' using this layout: "LOCAL | MERGED | REMOTE"
+			
+			This will probably be deprecated in the future. Please use "nvimdiff" and
+			manually set the "mergetool.nvimdiff.layout" configuration variable instead.
+		ENDOFMESSAGE
+		;;
+	nvimdiff3)
+		cat <<-ENDOFMESSAGE
+			Same as 'nvimdiff' using this layout: "MERGED"
+			
+			This will probably be deprecated in the future. Please use "nvimdiff" and
+			manually set the "mergetool.nvimdiff.layout" configuration variable instead.
+		ENDOFMESSAGE
+		;;
+	esac
+
+	return 0
+}
+
 
 translate_merge_tool_path() {
 	case "$1" in
-- 
2.33.1

Re: [PATCH 2/3] vimdiff: add tool documentation

[David Aguilar]

Thanks for writing this up. Notes inline below.


On Thu, Nov 4, 2021 at 9:10 AM Fernando Ramos <greenfoo@u92.eu> wrote:
>
> Running 'git {merge,diff}tool --tool-help' now also prints usage
> information about the vimdiff tool (and its variantes) instead of just
> its name.
>
> Two new functions ('diff_cmd_help()' and 'merge_cmd_help()') have been
> added to the set of functions that each merge tool (ie. scripts found
> inside "mergetools/") can overwrite to provided tool specific
> information.
>
> Right now, only 'mergetools/vimdiff' implements these functions, but
> other tools are encouraged to do so in the future, specially if they
> take configuration options not explained anywhere else (as it is the
> case with the 'vimdiff' tool and the new 'layout' option)
>
> Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
> ---
>  git-mergetool--lib.sh |  12 ++
>  mergetools/vimdiff    | 272 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 284 insertions(+)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 542a6a75eb..3964cd8f99 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -64,6 +64,10 @@ $(list_tool_variants)"
>                                 fi
>                                 shown_any=yes
>                                 printf "%s%s\n" "$per_line_prefix" "$toolname"
> +                               while IFS= read -r line
> +                               do
> +                                       printf "%s\t%s\n" "$per_line_prefix" "$line"
> +                                done < <(diff_mode && diff_cmd_help "$toolname" || merge_cmd_help "$toolname")


If we wanted to shorten this line a bit, would it work to run the pipeline
first and pipe the result?

    (diff_mode && ... || merge_cmd_help ...) |
    while IFS= read -r line
    do
       ...
    done

How come we have to unset IFS here?
We often do:

IFS='
'

so that we have just \n newline as the separator.

I wonder if that would be better along with a save/restore of the IFS value?


>                         fi
>                 done
>
> @@ -162,10 +166,18 @@ setup_tool () {
>                 return 1
>         }
>
> +       diff_cmd_help () {
> +               return 0
> +       }
> +
>         merge_cmd () {
>                 return 1
>         }
>
> +       merge_cmd_help () {
> +               return 0
> +       }
> +
>         hide_resolved_enabled () {
>                 return 0
>         }
> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
> index 1f2e88777e..aa8fbc0a19 100644
> --- a/mergetools/vimdiff
> +++ b/mergetools/vimdiff
> @@ -428,6 +428,46 @@ diff_cmd () {
>                 -c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
>  }
>
> +diff_cmd_help() {
> +       case "$1" in
> +       vimdiff)
> +               cat <<-ENDOFMESSAGE
> +                       Opens vim with two vertical windows: LOCAL changes will be placed in the left
> +                       window and REMOTE changes in the right one.
> +               ENDOFMESSAGE


Tiny nit: we call this EOF in the other places
(git-mergetool--helper.sh) where we do the same.

ENDOFMESSAGE is a bit verbose so it might be worth sticking to the
conventional EOF marker.



> +               ;;
> +       vimdiff*)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'vimdiff'
> +               ENDOFMESSAGE
> +               ;;
> +       gvimdiff)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'vimdiff' but opens 'gvim' instead (which uses a graphical toolkit for
> +                       opening its own window)
> +               ENDOFMESSAGE
> +               ;;
> +       gvimdiff*)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'gvimdiff'
> +               ENDOFMESSAGE
> +               ;;
> +       nvimdiff)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'vimdiff' but opens 'neovim' instead (which is a fork of the original
> +                       'vim' 'focused on extensibility and usability' according to their authors)
> +               ENDOFMESSAGE
> +               ;;
> +       nvimdiff*)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'nvimdiff'
> +               ENDOFMESSAGE
> +               ;;
> +       esac
> +
> +       return 0
> +}
> +
>
>  merge_cmd () {
>         layout=$(git config mergetool.$merge_tool.layout)
> @@ -503,6 +543,238 @@ merge_cmd () {
>         return "$ret"
>  }
>
> +merge_cmd_help() {
> +       case "$1" in
> +       vimdiff)
> +               cat <<-ENDOFMESSAGE
> +                       Opens vim with a 4 windows layout distributed in the following way:
> +
> +                           ------------------------------------------
> +                           |             |           |              |
> +                           |   LOCAL     |   BASE    |   REMOTE     |
> +                           |             |           |              |
> +                           ------------------------------------------
> +                           |                                        |
> +                           |                MERGED                  |
> +                           |                                        |
> +                           ------------------------------------------
> +
> +                       "LOCAL", "BASE" and "REMOTE" are read-only buffers showing the contents of the
> +                       conflicting file in a specific git commit ("commit you are merging into",
> +                       "common ancestor commit" and "commit you are merging from" respectively)
> +
> +                       "MERGED" is a writable buffer where you have to resolve the conflicts (using the
> +                       other read-only buffers as a reference). Once you are done, save and exit "vim"
> +                       as usual (":wq") or, if you want to abort, exit using ":cq".
> +
> +                       You can change the windows layout used by vim by setting configuration variable
> +                       "mergetool.vimdiff.layout" which accepts a string where these separators have
> +                       special meaning:
> +
> +                         - ";" is used to "open a new tab"
> +                         - "-" is used to "open a new vertical split"
> +                         - "|" is used to "open a new horizontal split"


This would probably be a good place to mention the "*" current marker as well.

I wonder how good these special symbols are for shell friendliness.

Instead of a "*" suffix I would suggest a "@" prefix eg. "@LOCAL" for
the "use this tab" stuff.



The "|" is a nice choice visually, as is ";" but I wonder if we can
avoid shell metacharacters.

Instead of ";" would "+" or "T" work for "open a new tab". I kinda
like "+", so my examples below will use that.



Instead of "|" would "," (comma) work for a vertical split? Ditto --
the examples below will use "," comma.

Note that I said "vertical", which is the opposite of what is written
above (horizontal split) for "|".

I believe these are called "vertical" splits:

   LOCAL | BASE | REMOTE

because the splits are vertical... that might just be a small
switcheroo in the docs that needs updating.



"-" is nice for horizontal splits. It visually matches what happens,
but in the context of "," and "+" I think "/" would be a better
choice.

"a / b" means "a over b" in some math interpretations, and visually
would make sense for horizontal splits where "a" is on the top and "b"
is on the bottom.


Anyways, just some sugs to try and avoid shell syntax. My examples
below will use "/" for horizontal splits.


> +
> +                       Let's see some examples to undertand how it works:
> +
> +                         * layout = "(LOCAL | BASE | REMOTE) - MERGED"


My sug would be to write this as either: "LOCAL,BASE,REMOTE - MERGED"
or "(LOCAL, BASE, REMOTE) - MERGED".

Are the (parens) necessary, or can we infer grouping because
"LOCAL,BASE,REMOTE" are grouped together in the first example? Maybe
that's too subtle, but it's an idea.


> +
> +                           This is exactly the same as the default layout we have already seen.
> +
> +                         * layout = "LOCAL | MERGED | REMOTE"


My sug would be: "LOCAL, MERGED, REMOTE".

> +
> +                           If, for some reason, we are not interested in the "BASE" buffer.
> +
> +                                  ------------------------------------------
> +                                  |             |           |              |
> +                                  |             |           |              |
> +                                  |   LOCAL     |   MERGED  |   REMOTE     |
> +                                  |             |           |              |
> +                                  |             |           |              |
> +                                  ------------------------------------------
> +
> +                         * layout = "MERGED"
> +
> +                           Only the "MERGED" buffer will be shown. Note, however, that all the other
> +                           ones are still loaded in vim, and you can access them with the "buffers"
> +                           command.
> +
> +                                  ------------------------------------------
> +                                  |                                        |
> +                                  |                                        |
> +                                  |                 MERGED                 |
> +                                  |                                        |
> +                                  |                                        |
> +                                  ------------------------------------------
> +
> +                         * layout = "LOCAL* | REMOTE"
> +
> +                           When "MERGED" is not present in the layout, you must "mark" one of the
> +                           buffers with an asterisk. That will become the buffer you need to edit and
> +                           save after resolving the conflicts.


My sug is that this would be written as: "@LOCAL, REMOTE"


> +
> +                                  ------------------------------------------
> +                                  |                   |                    |
> +                                  |                   |                    |
> +                                  |                   |                    |
> +                                  |     LOCAL         |    REMOTE          |
> +                                  |                   |                    |
> +                                  |                   |                    |
> +                                  |                   |                    |
> +                                  ------------------------------------------
> +
> +                          * layout = "(LOCAL | BASE | REMOTE) - MERGED; BASE | LOCAL; BASE | REMOTE"


If we eschewed parens then this would look like:

    "LOCAL,BASE,REMOTE / MERGED + BASE,LOCAL + BASE,REMOTE"

With parens it would be:

    "(LOCAL, BASE, REMOTE) / MERGED + BASE, LOCAL + BASE, REMOTE"

I personally like the idea of not needing the parens if we can do without them,
but I haven't looked at the implementation at all yet.

I suspect the more complicated examples below require them, perhaps?

The upside of parens is that it avoids having significant whitespace,
so a grouping syntax might be warranted.


> +
> +                           Three tabs will open: the first one is a copy of the default layout, while
> +                           the other two only show the differences between "BASE" and "LOCAL" and
> +                           "BASE" and "REMOTE" respectively.
> +
> +                                  ------------------------------------------
> +                                  | <TAB #1> |  TAB #2  |  TAB #3  |       |
> +                                  ------------------------------------------
> +                                  |             |           |              |
> +                                  |   LOCAL     |   BASE    |   REMOTE     |
> +                                  |             |           |              |
> +                                  ------------------------------------------
> +                                  |                                        |
> +                                  |                MERGED                  |
> +                                  |                                        |
> +                                  ------------------------------------------
> +
> +                                  ------------------------------------------
> +                                  |  TAB #1  | <TAB #2> |  TAB #3  |       |
> +                                  ------------------------------------------
> +                                  |                   |                    |
> +                                  |                   |                    |
> +                                  |                   |                    |
> +                                  |     BASE          |    LOCAL           |
> +                                  |                   |                    |
> +                                  |                   |                    |
> +                                  |                   |                    |
> +                                  ------------------------------------------
> +
> +                                  ------------------------------------------
> +                                  |  TAB #1  |  TAB #2  | <TAB #3> |       |
> +                                  ------------------------------------------
> +                                  |                   |                    |
> +                                  |                   |                    |
> +                                  |                   |                    |
> +                                  |     BASE          |    REMOTE          |
> +                                  |                   |                    |
> +                                  |                   |                    |
> +                                  |                   |                    |
> +                                  ------------------------------------------
> +
> +                          * layout = "(LOCAL | BASE | REMOTE) - MERGED; BASE | LOCAL; BASE | REMOTE; (LOCAL - BASE - REMOTE) | MERGED"


This would be:

"(LOCAL, BASE, REMOTE) / MERGED + BASE, LOCAL + BASE, REMOTE + (LOCAL
/ BASE / REMOTE), MERGED"

Without parents it would be:

"LOCAL,BASE,REMOTE / MERGED + BASE, LOCAL + BASE, REMOTE +
LOCAL/BASE/REMOTE, MERGED"


That does seem a bit subtle... it could work, but maybe we should
avoid making whitespace have special meaning and just go with parens
(or [] or {}) for grouping.

I'd like to hear some opinions on that.


> +
> +                           Same as the previous example, but adds a fourth tab with the same
> +                           information as the first tab, with a different layout.
> +
> +                                  ---------------------------------------------
> +                                  |  TAB #1  |  TAB #2  |  TAB #3  | <TAB #4> |
> +                                  ---------------------------------------------
> +                                  |       LOCAL         |                     |
> +                                  |---------------------|                     |
> +                                  |       BASE          |        MERGED       |
> +                                  |---------------------|                     |
> +                                  |       REMOTE        |                     |
> +                                  ---------------------------------------------
> +
> +               ENDOFMESSAGE
> +               ;;


This example really shows off how nice and flexible this approach is.
This is really cool, by the way.

My only other sug is that maybe this documentation can be in the
Documentation/ area and these messages can be reduced to something
along the lines of:

    Please see the "vimdiff" section in "git help mergetool" or "git
help difftool" for more details.


Not sure. It is kinda nice that the help message is maximally helpful as-is,
but it probably should be in Documentation/ so that we don't have to duplicate
the information in multiple places.



> +       vimdiff1)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'vimdiff' using this layout: "LOCAL* | REMOTE"
> +
> +                       This will probably be deprecated in the future. Please use "vimdiff" and
> +                       manually set the "mergetool.vimdiff.layout" configuration variable instead.
> +               ENDOFMESSAGE
> +               ;;
> +       vimdiff2)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'vimdiff' using this layout: "LOCAL | MERGED | REMOTE"
> +
> +                       This will probably be deprecated in the future. Please use "vimdiff" and
> +                       manually set the "mergetool.vimdiff.layout" configuration variable instead.
> +               ENDOFMESSAGE
> +               ;;
> +       vimdiff3)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'vimdiff' using this layout: "MERGED"
> +
> +                       This will probably be deprecated in the future. Please use "vimdiff" and
> +                       manually set the "mergetool.vimdiff.layout" configuration variable instead.
> +               ENDOFMESSAGE
> +               ;;
> +       gvimdiff)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'vimdiff' but opens 'gvim' instead (which uses a graphical toolkit for
> +                       opening its own window).
> +                       The desired layout can be set with configuration variable
> +                       "mergetool.gvimdiff.layout"
> +               ENDOFMESSAGE
> +               ;;
> +       gvimdiff1)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'gvimdiff' using this layout: "LOCAL* | REMOTE"
> +
> +                       This will probably be deprecated in the future. Please use "gvimdiff" and
> +                       manually set the "mergetool.gvimdiff.layout" configuration variable instead.
> +               ENDOFMESSAGE
> +               ;;
> +       gvimdiff2)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'gvimdiff' using this layout: "LOCAL | MERGED | REMOTE"
> +
> +                       This will probably be deprecated in the future. Please use "gvimdiff" and
> +                       manually set the "mergetool.gvimdiff.layout" configuration variable instead.
> +               ENDOFMESSAGE
> +               ;;
> +       gvimdiff3)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'gvimdiff' using this layout: "MERGED"
> +
> +                       This will probably be deprecated in the future. Please use "gvimdiff" and
> +                       manually set the "mergetool.gvimdiff.layout" configuration variable instead.
> +               ENDOFMESSAGE
> +               ;;
> +       nvimdiff)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'vimdiff' but opens 'neovim' instead (which is a fork of the original
> +                       'vim' 'focused on extensibility and usability' according to their authors)
> +                       The desired layout can be set with configuration variable
> +                       "mergetool.nvimdiff.layout"
> +               ENDOFMESSAGE
> +               ;;
> +       nvimdiff1)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'nvimdiff' using this layout: "LOCAL* | REMOTE"
> +
> +                       This will probably be deprecated in the future. Please use "nvimdiff" and
> +                       manually set the "mergetool.nvimdiff.layout" configuration variable instead.
> +               ENDOFMESSAGE
> +               ;;
> +       nvimdiff2)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'nvimdiff' using this layout: "LOCAL | MERGED | REMOTE"
> +
> +                       This will probably be deprecated in the future. Please use "nvimdiff" and
> +                       manually set the "mergetool.nvimdiff.layout" configuration variable instead.
> +               ENDOFMESSAGE
> +               ;;
> +       nvimdiff3)
> +               cat <<-ENDOFMESSAGE
> +                       Same as 'nvimdiff' using this layout: "MERGED"
> +
> +                       This will probably be deprecated in the future. Please use "nvimdiff" and
> +                       manually set the "mergetool.nvimdiff.layout" configuration variable instead.
> +               ENDOFMESSAGE
> +               ;;
> +       esac
> +
> +       return 0
> +}
> +
>
>  translate_merge_tool_path() {
>         case "$1" in
> --
> 2.33.1
>

Thanks for taking it this far! I personally like the direction this is
going, but I'd very much appreciate hearing others' opinions.
-- 
David

Re: [PATCH 2/3] vimdiff: add tool documentation

[Eric Sunshine]

On Sun, Nov 7, 2021 at 4:25 PM David Aguilar <davvid@gmail.com> wrote:
> On Thu, Nov 4, 2021 at 9:10 AM Fernando Ramos <greenfoo@u92.eu> wrote:
> > +                               while IFS= read -r line
> > +                               do
> > +                                       printf "%s\t%s\n" "$per_line_prefix" "$line"
> > +                                done < <(diff_mode && diff_cmd_help "$toolname" || merge_cmd_help "$toolname")
>
> If we wanted to shorten this line a bit, would it work to run the pipeline
> first and pipe the result?
>
>     (diff_mode && ... || merge_cmd_help ...) |
>     while IFS= read -r line
>     do
>        ...
>     done

The additional benefit is that this avoids the `<(...)` Bashism (which
you mentioned in your other review).

> > +               cat <<-ENDOFMESSAGE
> > +                       Opens vim with two vertical windows: LOCAL changes will be placed in the left
> > +                       window and REMOTE changes in the right one.
> > +               ENDOFMESSAGE
>
> Tiny nit: we call this EOF in the other places
> (git-mergetool--helper.sh) where we do the same.
>
> ENDOFMESSAGE is a bit verbose so it might be worth sticking to the
> conventional EOF marker.

A couple additional really micro nits (take them or leave them)...

We normally don't add extra indentation to the here-doc body, and we
use `<<-\EOF` to reduce cognitive load if there's nothing in the body
which requires interpolation or expansion. Thus:

    cat <<-\EOF
    Opens vim with two...
    EOF

Re: [PATCH 2/3] vimdiff: add tool documentation

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

>     (diff_mode && ... || merge_cmd_help ...) |
>     while IFS= read -r line
>     do
>        ...
>     done
>
> How come we have to unset IFS here?

We are taking the input into a single variable "line", so what are
we splitting with IFS anyway?

In any case, thanks for spotting bash-isms to avoid.  We started
allowing "local", I think, at least in the tests, but otherwise many
bash-only things like "<(process)" redirection and "${substring:4}"
substitution are simply no-no.

[PATCH 3/3] vimdiff: remove deprecated {,g,n}vimdiff{1,2,3} variants

[Fernando Ramos]

After this commit is merged, users of "{,g,n}vimdiff{1,2,3}" will need to
set their merge tool to "{,g,v}vimdiff" (without the number suffix) and
the "mergetool.{,g,n}vimdiff.layout" configuration option to one of
these:

  * For "1" variant: "LOCAL* | REMOTE"
  * For "2" variant: "LOCAL | MERGED | REMOTE"
  * For "3" variant: "MERGED"

Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
---
 mergetools/vimdiff | 125 ++-------------------------------------------
 1 file changed, 4 insertions(+), 121 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index aa8fbc0a19..9d469c0553 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -436,33 +436,18 @@ diff_cmd_help() {
 			window and REMOTE changes in the right one.
 		ENDOFMESSAGE
 		;;
-	vimdiff*)
-		cat <<-ENDOFMESSAGE
-			Same as 'vimdiff'
-		ENDOFMESSAGE
-		;;
 	gvimdiff)
 		cat <<-ENDOFMESSAGE
 			Same as 'vimdiff' but opens 'gvim' instead (which uses a graphical toolkit for
 			opening its own window)
 		ENDOFMESSAGE
 		;;
-	gvimdiff*)
-		cat <<-ENDOFMESSAGE
-			Same as 'gvimdiff'
-		ENDOFMESSAGE
-		;;
 	nvimdiff)
 		cat <<-ENDOFMESSAGE
 			Same as 'vimdiff' but opens 'neovim' instead (which is a fork of the original
 			'vim' 'focused on extensibility and usability' according to their authors)
 		ENDOFMESSAGE
 		;;
-	nvimdiff*)
-		cat <<-ENDOFMESSAGE
-			Same as 'nvimdiff'
-		ENDOFMESSAGE
-		;;
 	esac
 
 	return 0
@@ -471,7 +456,6 @@ diff_cmd_help() {
 
 merge_cmd () {
 	layout=$(git config mergetool.$merge_tool.layout)
-	print_warning="false"
 
 	case "$1" in
 	*vimdiff)
@@ -481,34 +465,8 @@ merge_cmd () {
 			layout="(LOCAL | BASE | REMOTE) - MERGED"
 		fi
 		;;
-	*vimdiff1)
-		layout="LOCAL* | MERGED"
-		print_warning="true"
-		;;
-	*vimdiff2)
-		layout="LOCAL | MERGED | REMOTE"
-		print_warning="true"
-		;;
-	*vimdiff3)
-		layout="MERGED"
-		print_warning="true"
-		;;
 	esac
 
-	if test "$print_warning" = "true"
-	then
-		echo "WARNING:"
-		echo "WARNING: '$1' is going to be removed in a future version. You will be"
-		echo "WARNING: able to obtain the same result by selecting 'vimdiff' as the merge"
-		echo "WARNING: tool and setting configuration variable 'mergetool.vimdiff.layout'"
-		echo "WARNING: to the following value:"
-		echo "WARNING:"
-		echo "WARNING:     layout = \"$layout\""
-		echo "WARNING:"
-		echo "Press ENTER to continue..."
-		read
-	fi
-
 	gen_cmd "$layout"
 
 	debug_print ""
@@ -682,30 +640,6 @@ merge_cmd_help() {
 
 		ENDOFMESSAGE
 		;;
-	vimdiff1)
-		cat <<-ENDOFMESSAGE
-			Same as 'vimdiff' using this layout: "LOCAL* | REMOTE"
-			
-			This will probably be deprecated in the future. Please use "vimdiff" and
-			manually set the "mergetool.vimdiff.layout" configuration variable instead.
-		ENDOFMESSAGE
-		;;
-	vimdiff2)
-		cat <<-ENDOFMESSAGE
-			Same as 'vimdiff' using this layout: "LOCAL | MERGED | REMOTE"
-			
-			This will probably be deprecated in the future. Please use "vimdiff" and
-			manually set the "mergetool.vimdiff.layout" configuration variable instead.
-		ENDOFMESSAGE
-		;;
-	vimdiff3)
-		cat <<-ENDOFMESSAGE
-			Same as 'vimdiff' using this layout: "MERGED"
-			
-			This will probably be deprecated in the future. Please use "vimdiff" and
-			manually set the "mergetool.vimdiff.layout" configuration variable instead.
-		ENDOFMESSAGE
-		;;
 	gvimdiff)
 		cat <<-ENDOFMESSAGE
 			Same as 'vimdiff' but opens 'gvim' instead (which uses a graphical toolkit for
@@ -714,30 +648,6 @@ merge_cmd_help() {
 			"mergetool.gvimdiff.layout"
 		ENDOFMESSAGE
 		;;
-	gvimdiff1)
-		cat <<-ENDOFMESSAGE
-			Same as 'gvimdiff' using this layout: "LOCAL* | REMOTE"
-			
-			This will probably be deprecated in the future. Please use "gvimdiff" and
-			manually set the "mergetool.gvimdiff.layout" configuration variable instead.
-		ENDOFMESSAGE
-		;;
-	gvimdiff2)
-		cat <<-ENDOFMESSAGE
-			Same as 'gvimdiff' using this layout: "LOCAL | MERGED | REMOTE"
-			
-			This will probably be deprecated in the future. Please use "gvimdiff" and
-			manually set the "mergetool.gvimdiff.layout" configuration variable instead.
-		ENDOFMESSAGE
-		;;
-	gvimdiff3)
-		cat <<-ENDOFMESSAGE
-			Same as 'gvimdiff' using this layout: "MERGED"
-			
-			This will probably be deprecated in the future. Please use "gvimdiff" and
-			manually set the "mergetool.gvimdiff.layout" configuration variable instead.
-		ENDOFMESSAGE
-		;;
 	nvimdiff)
 		cat <<-ENDOFMESSAGE
 			Same as 'vimdiff' but opens 'neovim' instead (which is a fork of the original
@@ -746,30 +656,6 @@ merge_cmd_help() {
 			"mergetool.nvimdiff.layout"
 		ENDOFMESSAGE
 		;;
-	nvimdiff1)
-		cat <<-ENDOFMESSAGE
-			Same as 'nvimdiff' using this layout: "LOCAL* | REMOTE"
-			
-			This will probably be deprecated in the future. Please use "nvimdiff" and
-			manually set the "mergetool.nvimdiff.layout" configuration variable instead.
-		ENDOFMESSAGE
-		;;
-	nvimdiff2)
-		cat <<-ENDOFMESSAGE
-			Same as 'nvimdiff' using this layout: "LOCAL | MERGED | REMOTE"
-			
-			This will probably be deprecated in the future. Please use "nvimdiff" and
-			manually set the "mergetool.nvimdiff.layout" configuration variable instead.
-		ENDOFMESSAGE
-		;;
-	nvimdiff3)
-		cat <<-ENDOFMESSAGE
-			Same as 'nvimdiff' using this layout: "MERGED"
-			
-			This will probably be deprecated in the future. Please use "nvimdiff" and
-			manually set the "mergetool.nvimdiff.layout" configuration variable instead.
-		ENDOFMESSAGE
-		;;
 	esac
 
 	return 0
@@ -778,13 +664,13 @@ merge_cmd_help() {
 
 translate_merge_tool_path() {
 	case "$1" in
-	nvimdiff*)
+	nvimdiff)
 		echo nvim
 		;;
-	gvimdiff*)
+	gvimdiff)
 		echo gvim
 		;;
-	vimdiff*)
+	vimdiff)
 		echo vim
 		;;
 	esac
@@ -798,10 +684,7 @@ exit_code_trustable () {
 
 list_tool_variants () {
 	for prefix in '' g n; do
-		for suffix in '' 1 2 3
-		do
-			echo "${prefix}vimdiff${suffix}"
-		done
+		echo "${prefix}vimdiff"
 	done
 }
 
-- 
2.33.1