[PATCH v2 0/9] mergetools: vimdiff: regression fix and reorg

[PATCH v2 0/9] mergetools: vimdiff: regression fix and reorg

[Felipe Contreras]

Hello,

I wrote vimdiff3 to leverage both the power of git's diff3 and vim's
diff mode, but commit 0041797449 broke that.

Here you can see how it used to work:

https://i.snipboard.io/hSdfkj.jpg

The added and changed lines are properly highlighted.

After I fix the conflicts vim still properly highlights which lines were
changed, and even what specific characters were modified:

https://i.snipboard.io/HvpULI.jpg

Now I get absolutely nothing:

https://i.snipboard.io/HXMui4.jpg

Additionally, every time I run the command I get an annoying message:

  "./content_LOCAL_8975" 6L, 28B
  "./content_BASE_8975" 6 lines, 29 bytes
  "./content_REMOTE_8975" 6 lines, 29 bytes
  "content" 16 lines, 115 bytes
  Press ENTER or type command to continue

Because that's what `bufdo` does.

After discussing v1 with Fernando Ramos I came up with a different route
to fix the issues by reorganizing the code first, and after the code is
reorganized for the special case of single window mode, it should be
clear that the switch to the old vimdiff mode for vimdiff3 is easy and
trivial.

Felipe Contreras (9):
  mergetools: vimdiff: fix comment
  mergetools: vimdiff: shuffle single window case
  mergetools: vimdiff: add get_buf() helper
  mergetools: vimdiff: make vimdiff3 actually work
  mergetools: vimdiff: silence annoying messages
  mergetools: vimdiff: fix for diffopt
  mergetools: vimdiff: cleanup cruft
  mergetools: vimdiff: fix single window mode
  mergetools: vimdiff: use vimdiff for vimdiff3

 mergetools/vimdiff | 74 ++++++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 36 deletions(-)

Range-diff against v1:
 1:  d0530af49c <  -:  ---------- mergetools: vimdiff3: make it work as intended
 2:  01a229ef5e <  -:  ---------- mergetools: vimdiff3: fix diffopt options
 -:  ---------- >  1:  20c5abdbc8 mergetools: vimdiff: fix comment
 -:  ---------- >  2:  e6c860d2be mergetools: vimdiff: shuffle single window case
 -:  ---------- >  3:  bdf1e919a5 mergetools: vimdiff: add get_buf() helper
 -:  ---------- >  4:  c5e21e3049 mergetools: vimdiff: make vimdiff3 actually work
 -:  ---------- >  5:  2bf45c882d mergetools: vimdiff: silence annoying messages
 -:  ---------- >  6:  77a67628e7 mergetools: vimdiff: fix for diffopt
 -:  ---------- >  7:  adc9d18f2b mergetools: vimdiff: cleanup cruft
 -:  ---------- >  8:  fe7fb1a018 mergetools: vimdiff: fix single window mode
 -:  ---------- >  9:  15765aa9d2 mergetools: vimdiff: use vimdiff for vimdiff3
-- 
2.37.1.378.g3f95da6bac

[PATCH v2 1/9] mergetools: vimdiff: fix comment

[Felipe Contreras]

The name of the variable is wrong, and it can be set to anything, like
1.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index f770b8fe24..ea416adcaa 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -29,8 +29,8 @@
 ################################################################################
 
 debug_print () {
-	# Send message to stderr if global variable GIT_MERGETOOL_VIMDIFF is set
-	# to "true"
+	# Send message to stderr if global variable GIT_MERGETOOL_VIMDIFF_DEBUG
+	# is set.
 
 	if test -n "$GIT_MERGETOOL_VIMDIFF_DEBUG"
 	then
-- 
2.37.1.378.g3f95da6bac

[PATCH v2 2/9] mergetools: vimdiff: shuffle single window case

[Felipe Contreras]

We clearly want to do something different in these use cases.

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index ea416adcaa..9805d139bc 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -315,6 +315,14 @@ gen_cmd () {
 
 	LAYOUT=$1
 
+	# A single window is handled specially
+
+	if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
+	then
+		CMD=$(gen_cmd_aux "$LAYOUT")
+		FINAL_CMD="-c \"$CMD | bufdo diffthis\" -c \"tabfirst\""
+		return
+	fi
 
 	# Search for a "@" in one of the files identifiers ("LOCAL", "BASE",
 	# "REMOTE", "MERGED"). If not found, use "MERGE" as the default file
@@ -334,18 +342,7 @@ gen_cmd () {
 	# 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 echo "$LAYOUT" | grep ",\|/" >/dev/null
-	then
-		CMD="$CMD | tabdo windo diffthis"
-	else
-		CMD="$CMD | bufdo diffthis"
-	fi
-
+	CMD="$CMD | tabdo windo diffthis"
 
 	# 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
-- 
2.37.1.378.g3f95da6bac

[PATCH v2 3/9] mergetools: vimdiff: add get_buf() helper

[Felipe Contreras]

Now the single window mode can avoid gen_cmd_aux() altogether.

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 51 +++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 9805d139bc..103729b6a6 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -55,6 +55,30 @@ substring () {
 	echo "$STRING" | cut -c$(( START + 1 ))-$(( START + $LEN ))
 }
 
+get_buf () {
+	target=$(echo "$1" | sed 's:[ @();|-]::g')
+	buf="1"
+
+	if test "$target" = "LOCAL"
+	then
+		buf="1"
+
+	elif test "$target" = "BASE"
+	then
+		buf="2"
+
+	elif test "$target" = "REMOTE"
+	then
+		buf="3"
+
+	elif test "$target" = "MERGED"
+	then
+		buf="4"
+	fi
+
+	echo "$buf"
+}
+
 gen_cmd_aux () {
 	# Auxiliary function used from "gen_cmd()".
 	# Read that other function documentation for more details.
@@ -257,27 +281,8 @@ gen_cmd_aux () {
 	# If we reach this point, it means there are no separators and we just
 	# need to print the command to display the specified buffer
 
-	target=$(substring "$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
+	buf=$(get_buf $(substring "$LAYOUT" "$start" "$(( end - start ))"))
+	CMD="$CMD | ${buf}b"
 
 	echo "$CMD"
 	return
@@ -319,8 +324,8 @@ gen_cmd () {
 
 	if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
 	then
-		CMD=$(gen_cmd_aux "$LAYOUT")
-		FINAL_CMD="-c \"$CMD | bufdo diffthis\" -c \"tabfirst\""
+		buf=$(get_buf "$LAYOUT")
+		FINAL_CMD="-c \"echo | ${buf}b | bufdo diffthis\" -c \"tabfirst\""
 		return
 	fi
 
-- 
2.37.1.378.g3f95da6bac

[PATCH v2 4/9] mergetools: vimdiff: make vimdiff3 actually work

[Felipe Contreras]

When vimdiff3 was added in 7c147b77d3 (mergetools: add vimdiff3 mode,
2014-04-20), the description made clear the intention:

    It's similar to the default, except that the other windows are
    hidden.  This ensures that removed/added colors are still visible on
    the main merge window, but the other windows not visible.

However, in 0041797449 (vimdiff: new implementation with layout support,
2022-03-30) this was broken by generating a command that never creates
windows, and therefore vim never shows the diff.

In order to show the diff, the windows need to be created first, and
then when they are hidden the diff remains (if hidenoff isn't set).

The layout support implementation broke the whole purpose of vimdiff3,
and simply shows MERGED, which is no different from simply opening the
file with vim.

Setting the `hidden` option makes it work as intended.

Suggested-by: Fernando Ramos <greenfoo@u92.eu>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 103729b6a6..20c61b040b 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -325,7 +325,7 @@ gen_cmd () {
 	if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
 	then
 		buf=$(get_buf "$LAYOUT")
-		FINAL_CMD="-c \"echo | ${buf}b | bufdo diffthis\" -c \"tabfirst\""
+		FINAL_CMD="-c \"echo | set hidden | ${buf}b | bufdo diffthis\" -c \"tabfirst\""
 		return
 	fi
 
@@ -560,7 +560,7 @@ run_unit_tests () {
 	EXPECTED_CMD_01="-c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_02="-c \"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_03="-c \"echo | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_04="-c \"echo | 4b | bufdo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_04="-c \"echo | set hidden | 4b | bufdo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_05="-c \"echo | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_06="-c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_07="-c \"echo | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-- 
2.37.1.378.g3f95da6bac

[PATCH v2 5/9] mergetools: vimdiff: silence annoying messages

[Felipe Contreras]

When using the single window mode we are greeted with the following
warning:

  "./content_LOCAL_8975" 6L, 28B
  "./content_BASE_8975" 6 lines, 29 bytes
  "./content_REMOTE_8975" 6 lines, 29 bytes
  "content" 16 lines, 115 bytes
  Press ENTER or type command to continue

every time.

Silence that.

Suggested-by: Fernando Ramos <greenfoo@u92.eu>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 20c61b040b..fbca6f5c96 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -325,7 +325,7 @@ gen_cmd () {
 	if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
 	then
 		buf=$(get_buf "$LAYOUT")
-		FINAL_CMD="-c \"echo | set hidden | ${buf}b | bufdo diffthis\" -c \"tabfirst\""
+		FINAL_CMD="-c \"echo | set hidden | ${buf}b | silent bufdo diffthis\" -c \"tabfirst\""
 		return
 	fi
 
@@ -560,7 +560,7 @@ run_unit_tests () {
 	EXPECTED_CMD_01="-c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_02="-c \"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_03="-c \"echo | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_04="-c \"echo | set hidden | 4b | bufdo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_04="-c \"echo | set hidden | 4b | silent bufdo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_05="-c \"echo | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_06="-c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_07="-c \"echo | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-- 
2.37.1.378.g3f95da6bac

[PATCH v2 6/9] mergetools: vimdiff: fix for diffopt

[Felipe Contreras]

When diffopt has hiddenoff set and there's only one window (as is the
case in the single window mode) the diff mode is turned off.

We don't want that, so turn that option off.

Cc: Fernando Ramos <greenfoo@u92.eu>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index fbca6f5c96..68e399768c 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -325,7 +325,7 @@ gen_cmd () {
 	if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
 	then
 		buf=$(get_buf "$LAYOUT")
-		FINAL_CMD="-c \"echo | set hidden | ${buf}b | silent bufdo diffthis\" -c \"tabfirst\""
+		FINAL_CMD="-c \"echo | set hidden diffopt-=hiddenoff | ${buf}b | silent bufdo diffthis\" -c \"tabfirst\""
 		return
 	fi
 
@@ -560,7 +560,7 @@ run_unit_tests () {
 	EXPECTED_CMD_01="-c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_02="-c \"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_03="-c \"echo | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_04="-c \"echo | set hidden | 4b | silent bufdo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_04="-c \"echo | set hidden diffopt-=hiddenoff | 4b | silent bufdo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_05="-c \"echo | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_06="-c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_07="-c \"echo | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-- 
2.37.1.378.g3f95da6bac

[PATCH v2 7/9] mergetools: vimdiff: cleanup cruft

[Felipe Contreras]

These unnecessary commands come from when the single window mode was
calling gen_cmd_aux().

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 68e399768c..cdef1dcae3 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -325,7 +325,7 @@ gen_cmd () {
 	if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
 	then
 		buf=$(get_buf "$LAYOUT")
-		FINAL_CMD="-c \"echo | set hidden diffopt-=hiddenoff | ${buf}b | silent bufdo diffthis\" -c \"tabfirst\""
+		FINAL_CMD="-c \"set hidden diffopt-=hiddenoff | ${buf}b | silent bufdo diffthis\""
 		return
 	fi
 
@@ -560,7 +560,7 @@ run_unit_tests () {
 	EXPECTED_CMD_01="-c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_02="-c \"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_03="-c \"echo | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_04="-c \"echo | set hidden diffopt-=hiddenoff | 4b | silent bufdo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_04="-c \"set hidden diffopt-=hiddenoff | 4b | silent bufdo diffthis\""
 	EXPECTED_CMD_05="-c \"echo | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_06="-c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_07="-c \"echo | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-- 
2.37.1.378.g3f95da6bac

[PATCH v2 8/9] mergetools: vimdiff: fix single window mode

[Felipe Contreras]

Right now it doesn't really matter which target is used, vim always goes
for 'MERGED' since that's the last buffer.

As the documentation of bufdo says:

  The last buffer (or where an error occurred) becomes the current
  buffer.

So we have to switch to the desired buffer *after* running the bufdo
command. And we can't simply do `| 1b` because that will be considered
part of the bufdo command, so we have to run it inside an execute
command.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index cdef1dcae3..34373ad92c 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -325,7 +325,7 @@ gen_cmd () {
 	if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
 	then
 		buf=$(get_buf "$LAYOUT")
-		FINAL_CMD="-c \"set hidden diffopt-=hiddenoff | ${buf}b | silent bufdo diffthis\""
+		FINAL_CMD="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | ${buf}b\""
 		return
 	fi
 
@@ -560,7 +560,7 @@ run_unit_tests () {
 	EXPECTED_CMD_01="-c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_02="-c \"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_03="-c \"echo | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_04="-c \"set hidden diffopt-=hiddenoff | 4b | silent bufdo diffthis\""
+	EXPECTED_CMD_04="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | 4b\""
 	EXPECTED_CMD_05="-c \"echo | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_06="-c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_07="-c \"echo | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-- 
2.37.1.378.g3f95da6bac

[PATCH v2 9/9] mergetools: vimdiff: use vimdiff for vimdiff3

[Felipe Contreras]

We are pretty close to actually using vimdiff (-d) so let's just go
ahead and do it: no need for do the same thing vimdiff does.

Also, we need to disable diffopt=closeoff because we are closing all the
other windows.

Cc: Fernando Ramos <greenfoo@u92.eu>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 34373ad92c..daeb3cf1d0 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -325,7 +325,7 @@ gen_cmd () {
 	if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
 	then
 		buf=$(get_buf "$LAYOUT")
-		FINAL_CMD="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | ${buf}b\""
+		FINAL_CMD="-d -c \"set hidden diffopt-=hiddenoff diffopt-=closeoff | ${buf}b | only\""
 		return
 	fi
 
@@ -560,7 +560,7 @@ run_unit_tests () {
 	EXPECTED_CMD_01="-c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_02="-c \"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_03="-c \"echo | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_04="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | 4b\""
+	EXPECTED_CMD_04="-d -c \"set hidden diffopt-=hiddenoff diffopt-=closeoff | 4b | only\""
 	EXPECTED_CMD_05="-c \"echo | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_06="-c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
 	EXPECTED_CMD_07="-c \"echo | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-- 
2.37.1.378.g3f95da6bac

Re: [PATCH v2 0/9] mergetools: vimdiff: regression fix and reorg

[Fernando Ramos]

Thanks Felipe, this new patch is much cleaner than mine.

Just two comments:

    1. Due to the way single windows are now detected, layouts with multiple
       tabs but single windows on each of them do not work. Example:

         layout = LOCAL + BASE + REMOTE + MERGED

       (we should probably add a test case for this)

       I noticed it did also not work in "master" (but it looks like it does in
       the patch I sent yesterday)


    2. Tabs with a single window are not highlighted (this was also a problem in
       "master", I just noticed this when testing your changes)


I'll try to figure out how to fix this (but I won't be able to do so until
tomorrow).

Thanks.

Re: [PATCH v2 9/9] mergetools: vimdiff: use vimdiff for vimdiff3

[Fernando Ramos]

On 22/08/06 09:49PM, Felipe Contreras wrote:
> 
> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
> index 34373ad92c..daeb3cf1d0 100644
> --- a/mergetools/vimdiff
> +++ b/mergetools/vimdiff
> @@ -325,7 +325,7 @@ gen_cmd () {
>  	if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
>  	then
>  		buf=$(get_buf "$LAYOUT")
> -		FINAL_CMD="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | ${buf}b\""
> +		FINAL_CMD="-d -c \"set hidden diffopt-=hiddenoff diffopt-=closeoff | ${buf}b | only\""
>  		return
>  	fi

I found something strange while testing this:

This is the final command that is generated now:

    vim -d -c "set hidden diffopt-=hiddenoff diffopt-=closeoff | 4b | only" LOCAL BASE REMOTE MERGED 

Turns out that now colors "kind of work", but not completely:

    - The buffer that is shown by default ("MERGED") contains colors.
    - If you switch to the "LOCAL" buffer (":buffer 1") it also contains colors.
    - *But* if you switch to any of the other buffers ("BASE" with ":buffer 2"
      or "REMOTE" with ":buffer 3"), there are no colors.

For some reason, the command that was generated by my patch to v1, does work (?)
and all buffers contain diff colors:

    vimdiff -c "echo | silent 4b | set hidden | let tmp=bufnr('%') | silent bufdo diffthis | exe 'buffer '.tmp" LOCAL BASE REMOTE MERGED

Re: [PATCH v2 2/9] mergetools: vimdiff: shuffle single window case

[Fernando Ramos]

On 22/08/06 09:49PM, Felipe Contreras wrote:
>
> +	# A single window is handled specially
> +
> +	if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
> +	then
> +		CMD=$(gen_cmd_aux "$LAYOUT")
> +		FINAL_CMD="-c \"$CMD | bufdo diffthis\" -c \"tabfirst\""
> +		return
> +	fi

If you make this change, it fixes the first issue I was referring to in [1]

-       if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
+       if ! echo "$LAYOUT" | grep ",\|/\|\+" >/dev/null


[1] https://lore.kernel.org/git/Yu9vvAKJzOpoQ5AS@zacax395.localdomain/

Re: [PATCH v2 0/9] mergetools: vimdiff: regression fix and reorg

[Felipe Contreras]

On Sun, Aug 7, 2022 at 2:54 AM Fernando Ramos <greenfoo@u92.eu> wrote:
>
> Thanks Felipe, this new patch is much cleaner than mine.
>
> Just two comments:
>
>     1. Due to the way single windows are now detected, layouts with multiple
>        tabs but single windows on each of them do not work. Example:
>
>          layout = LOCAL + BASE + REMOTE + MERGED
>
>        (we should probably add a test case for this)
>
>        I noticed it did also not work in "master" (but it looks like it does in
>        the patch I sent yesterday)

Yeah, but as you mention that's a problem already, so it has nothing
to do with this patch.

>     2. Tabs with a single window are not highlighted (this was also a problem in
>        "master", I just noticed this when testing your changes)

That's because the diff mode only highlights differences between the
windows in the tab. If you do something like "BASE,MERGED" the diff
won't show colors for LOCAL or REMOTE.

That's why I don't like any mode other than vimdiff3 (and occasionally
vimdiff): because I want to see the diff for all the files, even if I
don't see those files. If I open mergetool with vimdiff and I close
the BASE window I get something better than vimdiff2.

To me if I configure "BASE,MERGED" and I close the first window, I
should end up with the same view as "MERGED", but I don't, which is
why I fundamentally don't like this layout approach.

This could be made to work by opening all the windows and hiding them,
but at the moment it doesn't, only vimdiff (a layout with all the
files in the windows) works correctly for me.

-- 
Felipe Contreras

Re: [PATCH v2 2/9] mergetools: vimdiff: shuffle single window case

[Felipe Contreras]

On Sun, Aug 7, 2022 at 9:46 AM Fernando Ramos <greenfoo@u92.eu> wrote:
>
> On 22/08/06 09:49PM, Felipe Contreras wrote:
> >
> > +     # A single window is handled specially
> > +
> > +     if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
> > +     then
> > +             CMD=$(gen_cmd_aux "$LAYOUT")
> > +             FINAL_CMD="-c \"$CMD | bufdo diffthis\" -c \"tabfirst\""
> > +             return
> > +     fi
>
> If you make this change, it fixes the first issue I was referring to in [1]
>
> -       if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
> +       if ! echo "$LAYOUT" | grep ",\|/\|\+" >/dev/null

Yeah, but 1) that's a problem in master, not with this patch, and 2)
as you already mentioned the colors are not shown so it's debatable if
it's really "fixed".

BTW, much easier to do "[,/|]" instead.

-- 
Felipe Contreras

Re: [PATCH v2 0/9] mergetools: vimdiff: regression fix and reorg

[Fernando Ramos]

On 22/08/07 10:39AM, Felipe Contreras wrote:
>
> That's because the diff mode only highlights differences between the
> windows in the tab. If you do something like "BASE,MERGED" the diff
> won't show colors for LOCAL or REMOTE.

That's right. I've been looking into this in detail today and I think I finally
have a good solution which...

    - Makes vimdiff3 work as any other layout (no special case, not even an
      extra "if" to handle it)

    - Makes colors work *in all cases*: single tab with single window and also
      multiple tabs where one or more of them contain one single window (in that
      case the diff is made agains all buffers)

    - Works even with an empty .vimrc

I'll post the patch as a reply to this message.


> That's why I don't like any mode other than vimdiff3 (and occasionally
> vimdiff): because I want to see the diff for all the files, even if I
> don't see those files. If I open mergetool with vimdiff and I close
> the BASE window I get something better than vimdiff2.

You can keep using vimdiff3 but now, also, after this fix, you can use any
layout you want and append "+ MERGED" at the end (or beginning) and that
particular tab (and only that) will behave the same as "vimdiff3" :)


> To me if I configure "BASE,MERGED" and I close the first window, I
> should end up with the same view as "MERGED", but I don't, which is
> why I fundamentally don't like this layout approach.

This won't work. Not even after the fix. If you want to modify the layout (ex:
by closing a window) vim won't automatically update the list of buffers to
consider for the diff.

You can always manually update the list later *or* use "+ MERGED" as previously
described.

The root cause for this is that, when opening vim, we must decide what to diff
on each tab, and the logic after my patch works like this:

    - If there are more than 1 window, diff among opened windows.
    - If there is only 1 window, diff among all buffers

Seems to be the best of both worlds :)

Let me know what you think.

Thanks.

[PATCH 1/2] vimdiff: fix single tab mode, single window mode and colors

[Fernando Ramos]

vimdiff3 was introduced in 7c147b77d3 (mergetools: add vimdiff3 mode,
2014-04-20) and then partially broken in 0041797449 (vimdiff: new
implementation with layout support, 2022-03-30) in two ways:

    - It does not show colors unless the user has "set hidden" in his
      .vimrc file

    - It prompts the user to "Press ENTER" every time it runs.

This patch fixes both issues and, in adition:

  - Unifies the previously "special" case where LAYOUT contained one single tab
    with one single window.

  - Fixes colors in tabs with just one window.

Cc: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
---
 mergetools/vimdiff | 69 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 20 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index f770b8fe24..ee99a0b03e 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -55,12 +55,40 @@ substring () {
 	echo "$STRING" | cut -c$(( START + 1 ))-$(( START + $LEN ))
 }
 
+enable_diff_mode () {
+	# Auxiliary function that appends extra vim commands at the end of each
+	# tab section to enable diff mode
+
+	NUMBER_OF_WINDOWS_IN_TAB=$1
+
+	if test "$NUMBER_OF_WINDOWS_IN_TAB" -eq 1
+	then
+		# Tabs that only contains one window will "diff"
+		# against all loaded/hidden buffers
+		
+		echo "let tmp=bufnr('%') | execute 'silent 1,4bufdo diffthis' | execute 'buffer '.tmp"
+	else
+		# Tabs that contain more than one window will
+		# only "diff" against those windows
+		
+		echo "execute 'windo diffthis'"
+	fi
+}
+
 gen_cmd_aux () {
 	# Auxiliary function used from "gen_cmd()".
 	# Read that other function documentation for more details.
+	#
+	# This function returns two items
+	#    - STDOUT:  The vim command to use
+        #    - RETCODE: The number of windows opened in the current tab
 
-	LAYOUT=$1
-	CMD=$2  # This is a second (hidden) argument used for recursion
+	WINDOWS_NR=$1 # Number of windows opened in the current tab after
+	              # having parsed the provided "LAYOUT"
+                      # If applicable, this variable will be updated and
+                      # returned in RETCODE
+	LAYOUT=$2     # Substring (from the original LAYOUT) to process
+	CMD=$3        # This is a third (hidden) argument used for recursion
 
 	debug_print
 	debug_print "LAYOUT    : $LAYOUT"
@@ -232,6 +260,7 @@ gen_cmd_aux () {
 		after="wincmd j"
 		index=$index_horizontal_split
 		terminate="true"
+		WINDOWS_NR=$(( WINDOWS_NR + 1 ))
 
 	elif ! test -z "$index_vertical_split"
 	then
@@ -239,16 +268,27 @@ gen_cmd_aux () {
 		after="wincmd l"
 		index=$index_vertical_split
 		terminate="true"
+		WINDOWS_NR=$(( WINDOWS_NR + 1 ))
 	fi
 
 	if  test "$terminate" = "true"
 	then
 		CMD="$CMD | $before"
-		CMD=$(gen_cmd_aux "$(substring "$LAYOUT" "$start" "$(( index - start ))")" "$CMD")
+		CMD=$(gen_cmd_aux $WINDOWS_NR "$(substring "$LAYOUT" "$start" "$(( index - start ))")" "$CMD")
+		WINDOWS_NR=$?
+
+		if ! test -z "$index_new_tab"
+		then
+			CMD="$CMD | $(enable_diff_mode $WINDOWS_NR)"
+			WINDOWS_NR=1
+		fi
+
 		CMD="$CMD | $after"
-		CMD=$(gen_cmd_aux "$(substring "$LAYOUT" "$(( index + 1 ))" "$(( ${#LAYOUT} - index ))")" "$CMD")
+		CMD=$(gen_cmd_aux $WINDOWS_NR "$(substring "$LAYOUT" "$(( index + 1 ))" "$(( ${#LAYOUT} - index ))")" "$CMD")
+		WINDOWS_NR=$?
+
 		echo "$CMD"
-		return
+		return $WINDOWS_NR
 	fi
 
 
@@ -280,10 +320,9 @@ gen_cmd_aux () {
 	fi
 
 	echo "$CMD"
-	return
+	return $WINDOWS_NR
 }
 
-
 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
@@ -333,25 +372,15 @@ gen_cmd () {
 
 	# 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 echo "$LAYOUT" | grep ",\|/" >/dev/null
-	then
-		CMD="$CMD | tabdo windo diffthis"
-	else
-		CMD="$CMD | bufdo diffthis"
-	fi
+	CMD=$(gen_cmd_aux 1 "$LAYOUT")
+	CMD="$CMD | $(enable_diff_mode $?)"
 
 
 	# 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\""
+	FINAL_CMD="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"$CMD\" -c \"tabfirst\""
 }
 
 
-- 
2.37.1

[PATCH 2/2] vimdiff: update unit tests

[Fernando Ramos]

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

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index ee99a0b03e..651f7f99b5 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -584,22 +584,22 @@ run_unit_tests () {
 	TEST_CASE_15="  ((  (LOCAL , BASE , REMOTE) / MERGED))   +(BASE)   , LOCAL+ BASE , REMOTE+ (((LOCAL / BASE / REMOTE)) ,    MERGED   )  "
 	TEST_CASE_16="LOCAL,BASE,REMOTE / MERGED + BASE,LOCAL + BASE,REMOTE + (LOCAL / BASE / REMOTE),MERGED"
 
-	EXPECTED_CMD_01="-c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_02="-c \"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_03="-c \"echo | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_04="-c \"echo | 4b | bufdo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_05="-c \"echo | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_06="-c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_07="-c \"echo | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_08="-c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_09="-c \"echo | leftabove split | 4b | wincmd j | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_10="-c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_11="-c \"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_12="-c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_13="-c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_14="-c \"echo | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnext | leftabove vertical split | 2b | wincmd l | 1b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_15="-c \"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_16="-c \"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_01="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_02="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | 1b | wincmd l | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_03="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_04="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | 4b | let tmp=bufnr('%') | execute 'silent 1,4bufdo diffthis' | execute 'buffer '.tmp\" -c \"tabfirst\""
+	EXPECTED_CMD_05="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_06="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_07="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_08="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_09="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove split | 4b | wincmd j | leftabove vertical split | 1b | wincmd l | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_10="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_11="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis' | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | execute 'windo diffthis' | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | execute 'windo diffthis' | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_12="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_13="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_14="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | execute 'windo diffthis' | tabnext | leftabove vertical split | 2b | wincmd l | 1b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_15="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis' | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | execute 'windo diffthis' | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | execute 'windo diffthis' | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_16="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis' | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | execute 'windo diffthis' | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | execute 'windo diffthis' | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
 
 	EXPECTED_TARGET_01="MERGED"
 	EXPECTED_TARGET_02="LOCAL"
@@ -664,7 +664,9 @@ run_unit_tests () {
 	cat >expect <<-\EOF
 	-f
 	-c
-	echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis
+	set hidden diffopt-=hiddenoff diffopt-=closeoff
+	-c
+	echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | quit | wincmd l | 2b | wincmd j | 3b | execute 'windo diffthis'
 	-c
 	tabfirst
 	lo cal
-- 
2.37.1

Re: [PATCH v2 0/9] mergetools: vimdiff: regression fix and reorg

[Felipe Contreras]

On Sun, Aug 7, 2022 at 1:39 PM Fernando Ramos <greenfoo@u92.eu> wrote:
> On 22/08/07 10:39AM, Felipe Contreras wrote:

> > To me if I configure "BASE,MERGED" and I close the first window, I
> > should end up with the same view as "MERGED", but I don't, which is
> > why I fundamentally don't like this layout approach.
>
> This won't work. Not even after the fix. If you want to modify the layout (ex:
> by closing a window) vim won't automatically update the list of buffers to
> consider for the diff.

Of course it works:

    vim -d -c 'set hidden diffopt=internal' BASE MERGED

Close the first window and the diff colors remain.

vim doesn't need to update the list of buffers.

-- 
Felipe Contreras

[PATCH v3 0/3] mergetools: vimdiff: regression fix (vimdiff3 mode)

[Fernando Ramos]

Felipe detected that color highlighting no longer worked when running "git
mergetools -t vimdiff3".

Deeper inspection of the problem revealed that color highlighting was also not
working in other cases (layouts that contained a single window in one of their
tabs)

The current patch series fixes the issue by doing two things:

  1. Making single tab + single window modes (such as "vimdiff3") use the same
     code path as all other layouts.

  2. Fixing the general "single window" case so that color highlighting also
     works there (and this is achieved by adding all buffers to the diff group
     in single window tabs).

In other words, after this patch:

  A) Tabs with more than one window will only highlight differences between the
     buffers shown in those windows.

  B) Tabs with just one window (and this also covers the more specific case of
     layouts with just one tab containing one window) will highlight differences
     between all buffers (LOCAL, BASE, REMOTE, MERGED).


Felipe Contreras (1):
  mergetools: vimdiff: fix comment

Fernando Ramos (2):
  mergetools: vimdiff: fix single tab mode, single window mode and
    colors
  mergetools: vimdiff: update unit tests

 mergetools/vimdiff | 109 +++++++++++++++++++++++++++++----------------
 1 file changed, 70 insertions(+), 39 deletions(-)

-- 
2.37.1

[PATCH v3 1/3] mergetools: vimdiff: fix comment

[Fernando Ramos]

From: Felipe Contreras <felipe.contreras@gmail.com>

The name of the variable is wrong, and it can be set to anything, like
1.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index f770b8fe24..ea416adcaa 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -29,8 +29,8 @@
 ################################################################################
 
 debug_print () {
-	# Send message to stderr if global variable GIT_MERGETOOL_VIMDIFF is set
-	# to "true"
+	# Send message to stderr if global variable GIT_MERGETOOL_VIMDIFF_DEBUG
+	# is set.
 
 	if test -n "$GIT_MERGETOOL_VIMDIFF_DEBUG"
 	then
-- 
2.37.1

[PATCH v3 2/3] mergetools: vimdiff: fix single tab mode, single window mode and colors

[Fernando Ramos]

vimdiff3 was introduced in 7c147b77d3 (mergetools: add vimdiff3 mode,
2014-04-20) and then partially broken in 0041797449 (vimdiff: new
implementation with layout support, 2022-03-30) in two ways:

    - It does not show colors unless the user has "set hidden" in his
      .vimrc file

    - It prompts the user to "Press ENTER" every time it runs.

This patch fixes both issues and, in adition:

  - Unifies the previously "special" case where LAYOUT contained one single tab
    with one single window.

  - Fixes colors in tabs with just one window.

Cc: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
---
 mergetools/vimdiff | 69 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 20 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index ea416adcaa..f8cd7a83f0 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -55,12 +55,40 @@ substring () {
 	echo "$STRING" | cut -c$(( START + 1 ))-$(( START + $LEN ))
 }
 
+enable_diff_mode () {
+	# Auxiliary function that appends extra vim commands at the end of each
+	# tab section to enable diff mode
+
+	NUMBER_OF_WINDOWS_IN_TAB=$1
+
+	if test "$NUMBER_OF_WINDOWS_IN_TAB" -eq 1
+	then
+		# Tabs that only contains one window will "diff"
+		# against all loaded/hidden buffers
+
+		echo "let tmp=bufnr('%') | execute 'silent 1,4bufdo diffthis' | execute 'buffer '.tmp"
+	else
+		# Tabs that contain more than one window will
+		# only "diff" against those windows
+
+		echo "execute 'windo diffthis'"
+	fi
+}
+
 gen_cmd_aux () {
 	# Auxiliary function used from "gen_cmd()".
 	# Read that other function documentation for more details.
+	#
+	# This function returns two items
+	#    - STDOUT:  The vim command to use
+	#    - RETCODE: The number of windows opened in the current tab
 
-	LAYOUT=$1
-	CMD=$2  # This is a second (hidden) argument used for recursion
+	WINDOWS_NR=$1 # Number of windows opened in the current tab after
+	              # having parsed the provided "LAYOUT"
+	              # If applicable, this variable will be updated and
+	              # returned in RETCODE
+	LAYOUT=$2     # Substring (from the original LAYOUT) to process
+	CMD=$3        # This is a third (hidden) argument used for recursion
 
 	debug_print
 	debug_print "LAYOUT    : $LAYOUT"
@@ -232,6 +260,7 @@ gen_cmd_aux () {
 		after="wincmd j"
 		index=$index_horizontal_split
 		terminate="true"
+		WINDOWS_NR=$(( WINDOWS_NR + 1 ))
 
 	elif ! test -z "$index_vertical_split"
 	then
@@ -239,16 +268,27 @@ gen_cmd_aux () {
 		after="wincmd l"
 		index=$index_vertical_split
 		terminate="true"
+		WINDOWS_NR=$(( WINDOWS_NR + 1 ))
 	fi
 
 	if  test "$terminate" = "true"
 	then
 		CMD="$CMD | $before"
-		CMD=$(gen_cmd_aux "$(substring "$LAYOUT" "$start" "$(( index - start ))")" "$CMD")
+		CMD=$(gen_cmd_aux $WINDOWS_NR "$(substring "$LAYOUT" "$start" "$(( index - start ))")" "$CMD")
+		WINDOWS_NR=$?
+
+		if ! test -z "$index_new_tab"
+		then
+			CMD="$CMD | $(enable_diff_mode $WINDOWS_NR)"
+			WINDOWS_NR=1
+		fi
+
 		CMD="$CMD | $after"
-		CMD=$(gen_cmd_aux "$(substring "$LAYOUT" "$(( index + 1 ))" "$(( ${#LAYOUT} - index ))")" "$CMD")
+		CMD=$(gen_cmd_aux $WINDOWS_NR "$(substring "$LAYOUT" "$(( index + 1 ))" "$(( ${#LAYOUT} - index ))")" "$CMD")
+		WINDOWS_NR=$?
+
 		echo "$CMD"
-		return
+		return $WINDOWS_NR
 	fi
 
 
@@ -280,10 +320,9 @@ gen_cmd_aux () {
 	fi
 
 	echo "$CMD"
-	return
+	return $WINDOWS_NR
 }
 
-
 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
@@ -333,25 +372,15 @@ gen_cmd () {
 
 	# 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 echo "$LAYOUT" | grep ",\|/" >/dev/null
-	then
-		CMD="$CMD | tabdo windo diffthis"
-	else
-		CMD="$CMD | bufdo diffthis"
-	fi
+	CMD=$(gen_cmd_aux 1 "$LAYOUT")
+	CMD="$CMD | $(enable_diff_mode $?)"
 
 
 	# 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\""
+	FINAL_CMD="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"$CMD\" -c \"tabfirst\""
 }
 
 
-- 
2.37.1

[PATCH v3 3/3] mergetools: vimdiff: update unit tests

[Fernando Ramos]

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

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index f8cd7a83f0..f04dad24ce 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -584,22 +584,22 @@ run_unit_tests () {
 	TEST_CASE_15="  ((  (LOCAL , BASE , REMOTE) / MERGED))   +(BASE)   , LOCAL+ BASE , REMOTE+ (((LOCAL / BASE / REMOTE)) ,    MERGED   )  "
 	TEST_CASE_16="LOCAL,BASE,REMOTE / MERGED + BASE,LOCAL + BASE,REMOTE + (LOCAL / BASE / REMOTE),MERGED"
 
-	EXPECTED_CMD_01="-c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_02="-c \"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_03="-c \"echo | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_04="-c \"echo | 4b | bufdo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_05="-c \"echo | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_06="-c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_07="-c \"echo | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_08="-c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_09="-c \"echo | leftabove split | 4b | wincmd j | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_10="-c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_11="-c \"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_12="-c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_13="-c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_14="-c \"echo | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnext | leftabove vertical split | 2b | wincmd l | 1b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_15="-c \"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
-	EXPECTED_CMD_16="-c \"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_01="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_02="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | 1b | wincmd l | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_03="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_04="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | 4b | let tmp=bufnr('%') | execute 'silent 1,4bufdo diffthis' | execute 'buffer '.tmp\" -c \"tabfirst\""
+	EXPECTED_CMD_05="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_06="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_07="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_08="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_09="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove split | 4b | wincmd j | leftabove vertical split | 1b | wincmd l | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_10="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_11="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis' | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | execute 'windo diffthis' | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | execute 'windo diffthis' | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_12="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_13="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_14="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | execute 'windo diffthis' | tabnext | leftabove vertical split | 2b | wincmd l | 1b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_15="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis' | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | execute 'windo diffthis' | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | execute 'windo diffthis' | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_16="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis' | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | execute 'windo diffthis' | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | execute 'windo diffthis' | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
 
 	EXPECTED_TARGET_01="MERGED"
 	EXPECTED_TARGET_02="LOCAL"
@@ -664,7 +664,9 @@ run_unit_tests () {
 	cat >expect <<-\EOF
 	-f
 	-c
-	echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis
+	set hidden diffopt-=hiddenoff diffopt-=closeoff
+	-c
+	echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | quit | wincmd l | 2b | wincmd j | 3b | execute 'windo diffthis'
 	-c
 	tabfirst
 	lo cal
-- 
2.37.1

Re: [PATCH v3 2/3] mergetools: vimdiff: fix single tab mode, single window mode and colors

[Felipe Contreras]

On Mon, Aug 8, 2022 at 12:35 AM Fernando Ramos <greenfoo@u92.eu> wrote:
>
> vimdiff3 was introduced in 7c147b77d3 (mergetools: add vimdiff3 mode,
> 2014-04-20) and then partially broken in 0041797449 (vimdiff: new
> implementation with layout support, 2022-03-30) in two ways:
>
>     - It does not show colors unless the user has "set hidden" in his
>       .vimrc file
>
>     - It prompts the user to "Press ENTER" every time it runs.

For the record, in my version these two issues are fixed in a much simpler way:

--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -325,7 +325,7 @@ gen_cmd () {
        if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
        then
                buf=$(get_buf "$LAYOUT")
-               FINAL_CMD="-c \"echo | ${buf}b | bufdo diffthis\" -c
\"tabfirst\""
+               FINAL_CMD="-c \"echo | set hidden | ${buf}b | silent
bufdo diffthis\" -c \"tabfirst\""
                return
        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\""
> +       FINAL_CMD="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"$CMD\" -c \"tabfirst\""
>  }

These diffopt settings look awfully familiar.

https://lore.kernel.org/git/20220807024941.222018-7-felipe.contreras@gmail.com/

-- 
Felipe Contreras

Re: [PATCH v3 2/3] mergetools: vimdiff: fix single tab mode, single window mode and colors

[Fernando Ramos]

On 22/08/08 01:37AM, Felipe Contreras wrote:
> On Mon, Aug 8, 2022 at 12:35 AM Fernando Ramos <greenfoo@u92.eu> wrote:
> >
> > vimdiff3 was introduced in 7c147b77d3 (mergetools: add vimdiff3 mode,
> > 2014-04-20) and then partially broken in 0041797449 (vimdiff: new
> > implementation with layout support, 2022-03-30) in two ways:
> >
> >     - It does not show colors unless the user has "set hidden" in his
> >       .vimrc file
> >
> >     - It prompts the user to "Press ENTER" every time it runs.
> 
> For the record, in my version these two issues are fixed in a much simpler way:
> 

Yes, it was simpler but remember it had two small issues:

  1. In "vimdiff3" mode, if you switch to buffers #2 or #3, highlighting
     disappears.

  2. It treats a single tab with a single window as a special case, when in
     fact it is just a subcase of a layout with many tabs where one of them
     contains just one window.
     The new patch series makes no distinction between them by keeping track
     of the number of windows opened on each tab which, as you noted, adds
     some extra complexity (but needed complexity nevertheless if we want to
     have highlighting enabled in all cases)


> >         # 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\""
> > +       FINAL_CMD="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"$CMD\" -c \"tabfirst\""
> >  }
> 
> These diffopt settings look awfully familiar.
> 

I would go as far as saying they are the same :)

As you explained, it is better to keep these options explicitly set so that
buffer diff'ing works in all cases.

Notice that in this new patch series, however, these options apply to all
layouts (and not just to "vimdiff3"), as we want highlighting to also be
enabled in multi-tab single window layouts.


PS: I have been testing many layouts today with and without an empty .vimrc and
everything seems to work. But it would be great if others reading this did the
same to make sure there are no other strange vim configuration options that
affect the way diffs are displayed (as, unfortunately, we have found out in the
past more than once!)

Thanks!

Re: [PATCH v3 2/3] mergetools: vimdiff: fix single tab mode, single window mode and colors

[Felipe Contreras]

On Mon, Aug 8, 2022 at 1:14 PM Fernando Ramos <greenfoo@u92.eu> wrote:
>
> On 22/08/08 01:37AM, Felipe Contreras wrote:
> > On Mon, Aug 8, 2022 at 12:35 AM Fernando Ramos <greenfoo@u92.eu> wrote:
> > >
> > > vimdiff3 was introduced in 7c147b77d3 (mergetools: add vimdiff3 mode,
> > > 2014-04-20) and then partially broken in 0041797449 (vimdiff: new
> > > implementation with layout support, 2022-03-30) in two ways:
> > >
> > >     - It does not show colors unless the user has "set hidden" in his
> > >       .vimrc file
> > >
> > >     - It prompts the user to "Press ENTER" every time it runs.
> >
> > For the record, in my version these two issues are fixed in a much simpler way:
> >
>
> Yes, it was simpler but remember it had two small issues:
>
>   1. In "vimdiff3" mode, if you switch to buffers #2 or #3, highlighting
>      disappears.

No. That only happens in patch 9. In patch 5--which is where those two
bugs are resolved--that problem doesn't exist yet.

Also, I'm pretty sure that's a bug in vim (of which there are many).

>   2. It treats a single tab with a single window as a special case, when in
>      fact it is just a subcase of a layout with many tabs where one of them
>      contains just one window.
>      The new patch series makes no distinction between them by keeping track
>      of the number of windows opened on each tab which, as you noted, adds
>      some extra complexity (but needed complexity nevertheless if we want to
>      have highlighting enabled in all cases)

That's not necessarily true. You are assuming that is the only
solution possible.

Even supposing your solution was the only solution possible, nothing
prevents us from applying your patch on top of mine. In git (and in
many other endeavors) it behooves us to do one thing at a time for
many reasons.

There's no reason to try to do two things at the same time. We can fix
the specific case now (which is urgent and needed), and explore the
generic case later on (which few people would care about anyway).

For the generic case, I took a look at your solution and noticed most
of the complexity comes from trying to guess the number of windows per
tab. There is no need to do that.

I experimented with doing "bufdo diffthis" even in cases with multiple
windows *before* doing anything else, and it works. There's no need to
do "bufdo diffthis" afterwards, and there's no need for "windo
diffthis" either. There's also no need to store the current buffer in
a variable.

It *also* has the added benefit that now multi-window tabs now show
the diff colors for all the buffers, not just the visible ones (which
is what I would have expected anyway).

This solution is not only simpler than your solution, it's actually
simpler than the current code.

--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -68,7 +68,7 @@ gen_cmd_aux () {

        if test -z "$CMD"
        then
-               CMD="echo" # vim "nop" operator
+               CMD="silent execute 'bufdo diffthis'"
        fi

        start=0
@@ -221,7 +221,7 @@ gen_cmd_aux () {

        if ! test -z "$index_new_tab"
        then
-               before="-tabnew"
+               before="-tabnew | silent execute 'bufdo diffthis'"
                after="tabnext"
                index=$index_new_tab
                terminate="true"
@@ -336,17 +336,6 @@ gen_cmd () {
        CMD=$(gen_cmd_aux "$LAYOUT")


-       # Adjust the just obtained script depending on whether more than one
-       # windows are visible or not
-
-       if echo "$LAYOUT" | grep ",\|/" >/dev/null
-       then
-               CMD="$CMD | tabdo windo diffthis"
-       else
-               CMD="$CMD | bufdo 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


> > >         # 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\""
> > > +       FINAL_CMD="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"$CMD\" -c \"tabfirst\""
> > >  }
> >
> > These diffopt settings look awfully familiar.
>
> I would go as far as saying they are the same :)

That's actually my point. You copied one of my fixes without
mentioning it. Not only is it not nice to copy code without
attribution, it's not a good practice to sneak in unrelated changes.
If later on it turns out the diffopt stuff introduce a regression,
people will have a harder time figuring out what in this patch
triggered the issue especially since it's not mentioned.

The diffopt fix is completely separate from what you are trying to do
in this patch. It's good practice to do those kinds of fixes in a
separate patch. My patch [1] does only one thing, and it explains why
in the commit message.

> As you explained, it is better to keep these options explicitly set so that
> buffer diff'ing works in all cases.
>
> Notice that in this new patch series, however, these options apply to all
> layouts (and not just to "vimdiff3"), as we want highlighting to also be
> enabled in multi-tab single window layouts.

Yes, but still: it should be done in a separate patch and explained why.

Cheers.

[1] https://lore.kernel.org/git/20220807024941.222018-7-felipe.contreras@gmail.com/

-- 
Felipe Contreras

Re: [PATCH v3 2/3] mergetools: vimdiff: fix single tab mode, single window mode and colors

[Fernando Ramos]

On 22/08/08 04:00PM, Felipe Contreras wrote:
> 
> No. That only happens in patch 9. In patch 5--which is where those two
> bugs are resolved--that problem doesn't exist yet.
> 

Sure. I was referring to your whole patch series (which is what I was testing)
not to a particular commit.

If I only apply some of your patches it is true that (1) is no longer an issue,
but we still have to deal with (2).


> Also, I'm pretty sure that's a bug in vim (of which there are many).
> 

In any case it doesn't happen with v3 :)


> >   2. It treats a single tab with a single window as a special case, when in
> >      fact it is just a subcase of a layout with many tabs where one of them
> >      contains just one window.
> >      The new patch series makes no distinction between them by keeping track
> >      of the number of windows opened on each tab which, as you noted, adds
> >      some extra complexity (but needed complexity nevertheless if we want to
> >      have highlighting enabled in all cases)
> 
> That's not necessarily true. You are assuming that is the only
> solution possible.

Only solution possible? Of course not! (I'm sorry if you got that impression)

I'm just presenting one solution that works, but I'm sure there are many others
(we could use "vim -d" in all cases or even implement a completely different
parsing logic).

Do you think we should try a different approach from the currently proposed one?


> Even supposing your solution was the only solution possible, nothing
> prevents us from applying your patch on top of mine. In git (and in
> many other endeavors) it behooves us to do one thing at a time for
> many reasons.
> 
> There's no reason to try to do two things at the same time. We can fix
> the specific case now (which is urgent and needed), and explore the
> generic case later on (which few people would care about anyway).
> 

You mean to apply your patch series and then apply mine? Sure, why not? But what
do get get from that? (I'm just curious)

I mean... v3 already works in all cases, right? Or am I missing something?


> For the generic case, I took a look at your solution and noticed most
> of the complexity comes from trying to guess the number of windows per
> tab. There is no need to do that.
> 
> I experimented with doing "bufdo diffthis" even in cases with multiple
> windows *before* doing anything else, and it works. There's no need to
> do "bufdo diffthis" afterwards, and there's no need for "windo
> diffthis" either. There's also no need to store the current buffer in
> a variable.
> 
> It *also* has the added benefit that now multi-window tabs now show
> the diff colors for all the buffers, not just the visible ones (which
> is what I would have expected anyway).
> 

Oh! Ok, now I see where the confusion comes from: showing the diff colors only
for the visible buffers is *a desired property* from the original design and not
something we want to avoid (except for the case where there is just one window,
which is what we are fixing now).

This is actually why all of this work started: we want to see, in one tab, only
differences between BASE and LOCAL and, in another tab, only differences between
BASE and REMOTE *without extra diff noise*.

If we enable ":bufdo diffthis" in those tabs we get a mess (at least for complex
merges).

Imagine this:

  - Tab #1: classical four windows layout (LOCAL, BASE, REMOTE on top and MERGED
    on the bottom).

  - Tab #2: two windows: left BASE, right REMOTE

  - Tab #3: two windows: left BASE, right LOCAL

We already see all highgliting and colors in the 4-way diff in tab #1, but we
are only interested in the sepecific BASE vs REMOTE and BASE vs LOCAL in tabs #2
and #3.


> > I would go as far as saying they are the same :)
> 
> That's actually my point. You copied one of my fixes without
> mentioning it. Not only is it not nice to copy code without
> attribution, 
>

You must excuse me here... I still find it extremely confusing to figure out
which field to use for attribution each time.

I added you to the "CC:" footer of the commit. Is this not the right procedure? 
Should I have done something different?


> ...it's not a good practice to sneak in unrelated changes.

Should we split the patch series in more commits, then?
Something like this?

  1. Comment fix
  2. Keep track of windows opened per tab
  3. Update generated string
  4. Add "set" options
  5. Update unit tests

I mean... I'm right with that sure :) Note, however that 2, 3 and 4 are closely
related (ie. "vimdiff3" won't work until the 3 of them are merged)

Re: [PATCH v3 2/3] mergetools: vimdiff: fix single tab mode, single window mode and colors

[Felipe Contreras]

On Mon, Aug 8, 2022 at 4:51 PM Fernando Ramos <greenfoo@u92.eu> wrote:
>
> On 22/08/08 04:00PM, Felipe Contreras wrote:
> >
> > No. That only happens in patch 9. In patch 5--which is where those two
> > bugs are resolved--that problem doesn't exist yet.
> >
>
> Sure. I was referring to your whole patch series (which is what I was testing)
> not to a particular commit.

And I was referring to these changes:

--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -325,7 +325,7 @@ gen_cmd () {
        if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
        then
                buf=$(get_buf "$LAYOUT")
-               FINAL_CMD="-c \"echo | ${buf}b | bufdo diffthis\" -c
\"tabfirst\""
+               FINAL_CMD="-c \"echo | set hidden | ${buf}b | silent
bufdo diffthis\" -c \"tabfirst\""
                return
        fi

Which do fix the two issues in a much simpler way. As I said.

> If I only apply some of your patches it is true that (1) is no longer an issue,
> but we still have to deal with (2).

Do we? Fixing regressions should take precedence over adding new
features, especially if nobody asked for them.

> > Also, I'm pretty sure that's a bug in vim (of which there are many).
>
> In any case it doesn't happen with v3 :)

Neither does it happen in my v2 patch series minus the last patch.

> > >   2. It treats a single tab with a single window as a special case, when in
> > >      fact it is just a subcase of a layout with many tabs where one of them
> > >      contains just one window.
> > >      The new patch series makes no distinction between them by keeping track
> > >      of the number of windows opened on each tab which, as you noted, adds
> > >      some extra complexity (but needed complexity nevertheless if we want to
> > >      have highlighting enabled in all cases)
> >
> > That's not necessarily true. You are assuming that is the only
> > solution possible.
>
> Only solution possible? Of course not! (I'm sorry if you got that impression)
>
> I'm just presenting one solution that works, but I'm sure there are many others
> (we could use "vim -d" in all cases or even implement a completely different
> parsing logic).
>
> Do you think we should try a different approach from the currently proposed one?

Yes. The proposed solution is too complex, and it seems to be trying
to workaround the current logic rather than fixing it.

It should be pretty clear at this point that we want to do something
either before, or after a tab is populated. So it would make perfect
sense to have a function that receives all the string that defines a
tab (text between "+"s).

Instead, the logic treats "+" as other separators, which is not the
case, for example, this clearly doesn't make sense:

  MERGED/(LOCAL+BASE+REMOTE)

Yet the code acts as if it does.

I wrote a patch that splits the tab parsing logic outside of
gen_cmd_aux() and the result is much simpler:

https://lore.kernel.org/git/20220809004549.123020-7-felipe.contreras@gmail.com/

> > Even supposing your solution was the only solution possible, nothing
> > prevents us from applying your patch on top of mine. In git (and in
> > many other endeavors) it behooves us to do one thing at a time for
> > many reasons.
> >
> > There's no reason to try to do two things at the same time. We can fix
> > the specific case now (which is urgent and needed), and explore the
> > generic case later on (which few people would care about anyway).
>
> You mean to apply your patch series and then apply mine? Sure, why not? But what
> do get get from that? (I'm just curious)

We get a simple regression fix decoupled from complex new feature
code. Additionally we get a little more readable code thanks to
get_buf(), which still does make sense even in your patch.

> I mean... v3 already works in all cases, right? Or am I missing something?

"Work" is not the only metric.

> > For the generic case, I took a look at your solution and noticed most
> > of the complexity comes from trying to guess the number of windows per
> > tab. There is no need to do that.
> >
> > I experimented with doing "bufdo diffthis" even in cases with multiple
> > windows *before* doing anything else, and it works. There's no need to
> > do "bufdo diffthis" afterwards, and there's no need for "windo
> > diffthis" either. There's also no need to store the current buffer in
> > a variable.
> >
> > It *also* has the added benefit that now multi-window tabs now show
> > the diff colors for all the buffers, not just the visible ones (which
> > is what I would have expected anyway).
>
> Oh! Ok, now I see where the confusion comes from: showing the diff colors only
> for the visible buffers is *a desired property* from the original design and not
> something we want to avoid (except for the case where there is just one window,
> which is what we are fixing now).

Who is "we"? I certainly don't want to avoid that, that's precisely
what prevents me from using this layout feature.

> This is actually why all of this work started: we want to see, in one tab, only
> differences between BASE and LOCAL and, in another tab, only differences between
> BASE and REMOTE

You still will be able to see that.

> *without extra diff noise*.

I don't consider color hints due to a very real difference in the code
"noise". This seems like a preference rather than something
fundamental to the tool.

> If we enable ":bufdo diffthis" in those tabs we get a mess (at least for complex
> merges).
>
> Imagine this:
>
>   - Tab #1: classical four windows layout (LOCAL, BASE, REMOTE on top and MERGED
>     on the bottom).
>
>   - Tab #2: two windows: left BASE, right REMOTE
>
>   - Tab #3: two windows: left BASE, right LOCAL
>
> We already see all highgliting and colors in the 4-way diff in tab #1, but we
> are only interested in the sepecific BASE vs REMOTE and BASE vs LOCAL in tabs #2
> and #3.

If "we" is you (plural), then sure, but *I* (and presumably other
users) am not interested only in that.

If we want to support both preferences the current code would not be
amenable to that. If we move the tab parsing logic outside of
gen_cmd_aux() as I proposed in my v3 patch, the logic is much simpler.

> > > I would go as far as saying they are the same :)
> >
> > That's actually my point. You copied one of my fixes without
> > mentioning it. Not only is it not nice to copy code without
> > attribution,
>
> You must excuse me here... I still find it extremely confusing to figure out
> which field to use for attribution each time.
>
> I added you to the "CC:" footer of the commit. Is this not the right procedure?
> Should I have done something different?

CC is just carbon copy, doesn't mean anything. You would have to ask
the maintainer for what would be the "right procedure" but some people
add "Helped-by".

> > ...it's not a good practice to sneak in unrelated changes.
>
> Should we split the patch series in more commits, then?
> Something like this?
>
>   1. Comment fix
>   2. Keep track of windows opened per tab
>   3. Update generated string
>   4. Add "set" options
>   5. Update unit tests
>
> I mean... I'm right with that sure :) Note, however that 2, 3 and 4 are closely
> related (ie. "vimdiff3" won't work until the 3 of them are merged)

Something like that, yeah. I've sent a new version of my patch series
with a proposal. Tests generally should be updated along with the
changes, it's tedious, but it forces you to rethink the changes you
are making. It also helps other reviewers see what would be the actual
result of your changes.

Cheers.

--
Felipe Contreras