git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/2] mergetools: vimdiff3: fix regression
@ 2022-08-02 21:41 Felipe Contreras
  2022-08-02 21:41 ` [PATCH 1/2] mergetools: vimdiff3: make it work as intended Felipe Contreras
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Felipe Contreras @ 2022-08-02 21:41 UTC (permalink / raw)
  To: git; +Cc: Fernando Ramos, 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

To get the highlighting the content has to be in a window, and only
*after* the diff mode has done its job can it be hidden. The current
code does nothing of the sort.

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

Here's the patch that restores the intended behavior so vimdiff3
actually does something.

Additionally I noticed that vimdiff3 relied on specific values of
`diffopt`, specifically `closeoff` not being set. This worked fine in my
setup, but vim has `closeoff` enabled by default. So I'm sending a patch
to make it work regardless of the user configuration.

Felipe Contreras (2):
  mergetools: vimdiff3: make it work as intended
  mergetools: vimdiff3: fix diffopt options

 mergetools/vimdiff | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

-- 
2.37.1.313.ge269dbcbc5


^ permalink raw reply	[flat|nested] 15+ messages in thread
* [PATCH 1/2] mergetools: vimdiff3: make it work as intended
  2022-08-02 21:41 [PATCH 0/2] mergetools: vimdiff3: fix regression Felipe Contreras
@ 2022-08-02 21:41 ` Felipe Contreras
  2022-08-02 21:41 ` [PATCH 2/2] mergetools: vimdiff3: fix diffopt options Felipe Contreras
  2022-08-06 16:25 ` [PATCH 0/2] mergetools: vimdiff3: fix regression Fernando Ramos
  2 siblings, 0 replies; 15+ messages in thread
From: Felipe Contreras @ 2022-08-02 21:41 UTC (permalink / raw)
  To: git; +Cc: Fernando Ramos, 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.

We could restore the correct behavior by modifying gen_cmd to open all
the windows, and then hide them, but there's no need to do that when the
-d option of vim (vimdiff) does precisely that.

So let's skip the whole gen_cmd function for vimdiff3, and hide the
windows, therefore restoring the previous intended behavior.

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

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index f770b8fe24..f4c3bf6d11 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -388,26 +388,36 @@ merge_cmd () {
 	layout=$(git config mergetool.vimdiff.layout)
 
 	case "$1" in
-	*vimdiff)
-		if test -z "$layout"
+	*vimdiff3)
+		if $base_present
 		then
-			# Default layout when none is specified
-			layout="(LOCAL,BASE,REMOTE)/MERGED"
+			CMD='hid | hid | hid'
+		else
+			CMD='hid | hid'
 		fi
+		FINAL_CMD="-d -c '$CMD'"
 		;;
-	*vimdiff1)
-		layout="@LOCAL,REMOTE"
-		;;
-	*vimdiff2)
-		layout="LOCAL,MERGED,REMOTE"
-		;;
-	*vimdiff3)
-		layout="MERGED"
+	*)
+		case "$1" in
+		*vimdiff)
+			if test -z "$layout"
+			then
+				# Default layout when none is specified
+				layout="(LOCAL,BASE,REMOTE)/MERGED"
+			fi
+			;;
+		*vimdiff1)
+			layout="@LOCAL,REMOTE"
+			;;
+		*vimdiff2)
+			layout="LOCAL,MERGED,REMOTE"
+			;;
+		esac
+
+		gen_cmd "$layout"
 		;;
 	esac
 
-	gen_cmd "$layout"
-
 	debug_print ""
 	debug_print "FINAL CMD : $FINAL_CMD"
 	debug_print "FINAL TAR : $FINAL_TARGET"
-- 
2.37.1.313.ge269dbcbc5


^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 2/2] mergetools: vimdiff3: fix diffopt options
  2022-08-02 21:41 [PATCH 0/2] mergetools: vimdiff3: fix regression Felipe Contreras
  2022-08-02 21:41 ` [PATCH 1/2] mergetools: vimdiff3: make it work as intended Felipe Contreras
@ 2022-08-02 21:41 ` Felipe Contreras
  2022-08-06 16:25 ` [PATCH 0/2] mergetools: vimdiff3: fix regression Fernando Ramos
  2 siblings, 0 replies; 15+ messages in thread
From: Felipe Contreras @ 2022-08-02 21:41 UTC (permalink / raw)
  To: git; +Cc: Fernando Ramos, Felipe Contreras

If either closeoff or hiddenoff are enabled, vimdiff3 doesn't work as
intended, so turn them off.

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

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index f4c3bf6d11..baccabc403 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -395,7 +395,7 @@ merge_cmd () {
 		else
 			CMD='hid | hid'
 		fi
-		FINAL_CMD="-d -c '$CMD'"
+		FINAL_CMD="-d -c 'setl diffopt-=closeoff diffopt-=hiddenoff | $CMD'"
 		;;
 	*)
 		case "$1" in
-- 
2.37.1.313.ge269dbcbc5


^ permalink raw reply related	[flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] mergetools: vimdiff3: fix regression
  2022-08-02 21:41 [PATCH 0/2] mergetools: vimdiff3: fix regression Felipe Contreras
  2022-08-02 21:41 ` [PATCH 1/2] mergetools: vimdiff3: make it work as intended Felipe Contreras
  2022-08-02 21:41 ` [PATCH 2/2] mergetools: vimdiff3: fix diffopt options Felipe Contreras
@ 2022-08-06 16:25 ` Fernando Ramos
  2022-08-06 16:27   ` Fernando Ramos
  2022-08-06 17:53   ` Felipe Contreras
  2 siblings, 2 replies; 15+ messages in thread
From: Fernando Ramos @ 2022-08-06 16:25 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, greenfoo

On 22/08/02 04:41PM, Felipe Contreras wrote:
> Hello,
> 
> I wrote vimdiff3 to leverage both the power of git's diff3 and vim's
> diff mode, but commit 0041797449 broke that.
> 

Hi Felipe,

This is the command that runs now when using 'git merge -t vimdiff3':

    vim -c "echo | 4b | bufdo diffthis" -c "tabfirst" LOCAL BASE REMOTE MERGED

...and this is the command that runs after your patch:

    vim -d -c "setl diffopt-=closeoff diffopt-=hiddenoff | hid | hid | hid" LOCAL BASE REMOTE MERGED

The new command you suggest is meant to improve two aspects:

    1. Preserves diff colors.
    2. Removes the "Press ENTER" message.

Regarding (1) I never noticed this because in my tests colors were always
shown...  but I just tried to run with '-u NONE' (which prevents .vimrc from
being loaded) and you are right: there are now no colors.

    vim -u NONE -c "echo | 4b | bufdo diffthis" -c "tabfirst" LOCAL BASE REMOTE MERGED
        ^^^^^^^
        `-> Tell vim not to load .vimrc

So... I started looking into my .vimrc and found the "problem":

    set hidden

By default this option is *not* set, which means buffers are discarded when
hidden (and that's why diff colors dissapear). By setting this option colors are
back even with '-u NONE':

    vim -u NONE -c "echo | set hidden | 4b | bufdo diffthis" -c "tabfirst" LOCAL BASE REMOTE MERGED
                           ^^^^^^^^^^

Regarding (2) we can remove the "Press ENTER" message by adding "silent" to both
"4b" and "bufdo", like this:

    vim -u NONE -c "echo | set hidden | silent 4b | silent bufdo diffthis" -c "tabfirst" LOCAL BASE REMOTE MERGED
                                        ^^^^^^      ^^^^^^

So... by making two changes to the current implementation (adding "set hidden"
and "silent") we can make it work. The nice thing is that, this way, "vimdiff3"
does not need to be treated as an exception and thus it will be (hopefully)
easier to maintain.

What do you think? :)

Thanks!

Fernando.

PS: I'll reply to this message with a patch that implements this in case we
decide to go this route.

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] mergetools: vimdiff3: fix regression
  2022-08-06 16:25 ` [PATCH 0/2] mergetools: vimdiff3: fix regression Fernando Ramos
@ 2022-08-06 16:27   ` Fernando Ramos
  2022-08-06 17:53   ` Felipe Contreras
  1 sibling, 0 replies; 15+ messages in thread
From: Fernando Ramos @ 2022-08-06 16:27 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git

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 by adding "set hidden" and "silent" to the
generated command string that is used to run vim.

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

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index f770b8fe24..a64134364c 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -261,19 +261,19 @@ gen_cmd_aux () {
 
 	if test "$target" = "LOCAL"
 	then
-		CMD="$CMD | 1b"
+		CMD="$CMD | silent 1b"
 
 	elif test "$target" = "BASE"
 	then
-		CMD="$CMD | 2b"
+		CMD="$CMD | silent 2b"
 
 	elif test "$target" = "REMOTE"
 	then
-		CMD="$CMD | 3b"
+		CMD="$CMD | silent 3b"
 
 	elif test "$target" = "MERGED"
 	then
-		CMD="$CMD | 4b"
+		CMD="$CMD | silent 4b"
 
 	else
 		CMD="$CMD | ERROR: >$target<"
@@ -310,7 +310,7 @@ gen_cmd () {
 	#
 	#     gen_cmd "@LOCAL , REMOTE"
 	#     |
-	#     `-> FINAL_CMD    == "-c \"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
+	#     `-> FINAL_CMD    == "-c \"echo | leftabove vertical split | silent 1b | wincmd l | silent 3b | silent tabdo windo diffthis\" -c \"tabfirst\""
 	#         FINAL_TARGET == "LOCAL"
 
 	LAYOUT=$1
@@ -341,9 +341,9 @@ gen_cmd () {
 
 	if echo "$LAYOUT" | grep ",\|/" >/dev/null
 	then
-		CMD="$CMD | tabdo windo diffthis"
+		CMD="$CMD | silent tabdo windo diffthis"
 	else
-		CMD="$CMD | bufdo diffthis"
+		CMD="$CMD | set hidden | silent bufdo diffthis"
 	fi
 
 
@@ -555,22 +555,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 \"echo | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 2b | wincmd l | silent 3b | wincmd j | silent 4b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_02="-c \"echo | leftabove vertical split | silent 1b | wincmd l | silent 3b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_03="-c \"echo | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 4b | wincmd l | silent 3b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_04="-c \"echo | silent 4b | set hidden | silent bufdo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_05="-c \"echo | leftabove split | silent 1b | wincmd j | leftabove split | silent 4b | wincmd j | silent 3b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_06="-c \"echo | leftabove vertical split | leftabove split | silent 1b | wincmd j | silent 3b | wincmd l | silent 4b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_07="-c \"echo | leftabove vertical split | silent 4b | wincmd l | leftabove split | silent 1b | wincmd j | silent 3b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_08="-c \"echo | leftabove split | leftabove vertical split | silent 1b | wincmd l | silent 3b | wincmd j | silent 4b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_09="-c \"echo | leftabove split | silent 4b | wincmd j | leftabove vertical split | silent 1b | wincmd l | silent 3b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_10="-c \"echo | leftabove vertical split | leftabove split | silent 1b | wincmd j | leftabove split | silent 2b | wincmd j | silent 3b | wincmd l | silent 4b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_11="-c \"echo | -tabnew | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 2b | wincmd l | silent 3b | wincmd j | silent 4b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 1b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 3b | tabnext | leftabove vertical split | leftabove split | silent 1b | wincmd j | leftabove split | silent 2b | wincmd j | silent 3b | wincmd l | silent 4b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_12="-c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | silent 1b | wincmd l | silent 3b | wincmd j | silent 2b | wincmd l | silent 4b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_13="-c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | silent 1b | wincmd l | silent 3b | wincmd j | silent 2b | wincmd l | leftabove vertical split | leftabove split | silent 1b | wincmd j | silent 3b | wincmd l | silent 4b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_14="-c \"echo | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 3b | tabnext | leftabove vertical split | silent 2b | wincmd l | silent 1b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_15="-c \"echo | -tabnew | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 2b | wincmd l | silent 3b | wincmd j | silent 4b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 1b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 3b | tabnext | leftabove vertical split | leftabove split | silent 1b | wincmd j | leftabove split | silent 2b | wincmd j | silent 3b | wincmd l | silent 4b | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_16="-c \"echo | -tabnew | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 2b | wincmd l | silent 3b | wincmd j | silent 4b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 1b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 3b | tabnext | leftabove vertical split | leftabove split | silent 1b | wincmd j | leftabove split | silent 2b | wincmd j | silent 3b | wincmd l | silent 4b | silent tabdo windo diffthis\" -c \"tabfirst\""
 
 	EXPECTED_TARGET_01="MERGED"
 	EXPECTED_TARGET_02="LOCAL"
@@ -635,7 +635,7 @@ 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
+	echo | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent quit | wincmd l | silent 2b | wincmd j | silent 3b | silent tabdo windo diffthis
 	-c
 	tabfirst
 	lo cal
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] mergetools: vimdiff3: fix regression
  2022-08-06 16:25 ` [PATCH 0/2] mergetools: vimdiff3: fix regression Fernando Ramos
  2022-08-06 16:27   ` Fernando Ramos
@ 2022-08-06 17:53   ` Felipe Contreras
  2022-08-06 18:29     ` Fernando Ramos
  1 sibling, 1 reply; 15+ messages in thread
From: Felipe Contreras @ 2022-08-06 17:53 UTC (permalink / raw)
  To: Fernando Ramos; +Cc: git

Hello,

On Sat, Aug 6, 2022 at 11:25 AM Fernando Ramos <greenfoo@u92.eu> wrote:
> On 22/08/02 04:41PM, Felipe Contreras wrote:

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

> By default this option is *not* set, which means buffers are discarded when
> hidden (and that's why diff colors dissapear). By setting this option colors are
> back even with '-u NONE':
>
>     vim -u NONE -c "echo | set hidden | 4b | bufdo diffthis" -c "tabfirst" LOCAL BASE REMOTE MERGED
>                            ^^^^^^^^^^

Correct.

> Regarding (2) we can remove the "Press ENTER" message by adding "silent" to both
> "4b" and "bufdo", like this:
>
>     vim -u NONE -c "echo | set hidden | silent 4b | silent bufdo diffthis" -c "tabfirst" LOCAL BASE REMOTE MERGED
>                                         ^^^^^^      ^^^^^^

Correct.

> So... by making two changes to the current implementation (adding "set hidden"
> and "silent") we can make it work. The nice thing is that, this way, "vimdiff3"
> does not need to be treated as an exception and thus it will be (hopefully)
> easier to maintain.
>
> What do you think? :)

This could work. The result is not quite the same as with vimdiff, but
the difference is minimal.

Two observations though.

1. The "silent 4b" is ignored, since bufdo makes the last buffer the
current buffer, so if you want a different buffer you have to make the
switch *after* bufdo.

2. You probably want to do "set hidden" on all the modes.

I don't see the need for all this complexity for this simple mode, but
anything that actually works is fine by me.

Cheers.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] mergetools: vimdiff3: fix regression
  2022-08-06 17:53   ` Felipe Contreras
@ 2022-08-06 18:29     ` Fernando Ramos
  2022-08-06 18:37       ` [PATCH] vimdiff: fix 'vimdiff3' behavior (colors + no extra key press) Fernando Ramos
  2022-08-06 19:17       ` [PATCH 0/2] mergetools: vimdiff3: fix regression Felipe Contreras
  0 siblings, 2 replies; 15+ messages in thread
From: Fernando Ramos @ 2022-08-06 18:29 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git

On 22/08/06 12:53PM, Felipe Contreras wrote:
> Two observations though.
> 
> 1. The "silent 4b" is ignored, since bufdo makes the last buffer the
> current buffer, so if you want a different buffer you have to make the
> switch *after* bufdo.
> 

Yes, you are right. For the particular case where there are no windows (only
hidden buffers) it does not have any effect. It's presence there comes from
the fact that the command generation function works in the most "generic" way
(ie. producing output that works for all cases: windows, tabs and buffers).

In order not to have another special case in the generation logic I left it
there, but you are right in that it is not needed (fortunately it also doesn't
make any harm :)


> 2. You probably want to do "set hidden" on all the modes.
> 

You are right. It also makes the logic more symmetric. I'll add it.


> I don't see the need for all this complexity for this simple mode, but
> anything that actually works is fine by me.
> 

...in fact, back in May I just wanted to add a new "vimdiff4" mode and what
originally was a 5 lines patch became the current 1000+ lines patch monster
after all the (very welcomed, I'm not complaining!) suggestions :)


I'll reply to this message with a new version of the patch with your "set
hidden" suggestion. Thanks!



^ permalink raw reply	[flat|nested] 15+ messages in thread
* [PATCH] vimdiff: fix 'vimdiff3' behavior (colors + no extra key press)
  2022-08-06 18:29     ` Fernando Ramos
@ 2022-08-06 18:37       ` Fernando Ramos
  2022-08-06 19:39         ` Felipe Contreras
  2022-08-06 19:17       ` [PATCH 0/2] mergetools: vimdiff3: fix regression Felipe Contreras
  1 sibling, 1 reply; 15+ messages in thread
From: Fernando Ramos @ 2022-08-06 18:37 UTC (permalink / raw)
  To: greenfoo; +Cc: felipe.contreras, git

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 by adding "set hidden" and "silent" to the
generated command string that is used to run vim.

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

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index f770b8fe24..461b8f394f 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -261,19 +261,19 @@ gen_cmd_aux () {
 
 	if test "$target" = "LOCAL"
 	then
-		CMD="$CMD | 1b"
+		CMD="$CMD | silent 1b"
 
 	elif test "$target" = "BASE"
 	then
-		CMD="$CMD | 2b"
+		CMD="$CMD | silent 2b"
 
 	elif test "$target" = "REMOTE"
 	then
-		CMD="$CMD | 3b"
+		CMD="$CMD | silent 3b"
 
 	elif test "$target" = "MERGED"
 	then
-		CMD="$CMD | 4b"
+		CMD="$CMD | silent 4b"
 
 	else
 		CMD="$CMD | ERROR: >$target<"
@@ -310,7 +310,7 @@ gen_cmd () {
 	#
 	#     gen_cmd "@LOCAL , REMOTE"
 	#     |
-	#     `-> FINAL_CMD    == "-c \"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
+	#     `-> FINAL_CMD    == "-c \"echo | leftabove vertical split | silent 1b | wincmd l | silent 3b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
 	#         FINAL_TARGET == "LOCAL"
 
 	LAYOUT=$1
@@ -341,9 +341,9 @@ gen_cmd () {
 
 	if echo "$LAYOUT" | grep ",\|/" >/dev/null
 	then
-		CMD="$CMD | tabdo windo diffthis"
+		CMD="$CMD | set hidden | silent tabdo windo diffthis"
 	else
-		CMD="$CMD | bufdo diffthis"
+		CMD="$CMD | set hidden | silent bufdo diffthis"
 	fi
 
 
@@ -555,22 +555,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 \"echo | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 2b | wincmd l | silent 3b | wincmd j | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_02="-c \"echo | leftabove vertical split | silent 1b | wincmd l | silent 3b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_03="-c \"echo | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 4b | wincmd l | silent 3b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_04="-c \"echo | silent 4b | set hidden | silent bufdo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_05="-c \"echo | leftabove split | silent 1b | wincmd j | leftabove split | silent 4b | wincmd j | silent 3b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_06="-c \"echo | leftabove vertical split | leftabove split | silent 1b | wincmd j | silent 3b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_07="-c \"echo | leftabove vertical split | silent 4b | wincmd l | leftabove split | silent 1b | wincmd j | silent 3b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_08="-c \"echo | leftabove split | leftabove vertical split | silent 1b | wincmd l | silent 3b | wincmd j | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_09="-c \"echo | leftabove split | silent 4b | wincmd j | leftabove vertical split | silent 1b | wincmd l | silent 3b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_10="-c \"echo | leftabove vertical split | leftabove split | silent 1b | wincmd j | leftabove split | silent 2b | wincmd j | silent 3b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_11="-c \"echo | -tabnew | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 2b | wincmd l | silent 3b | wincmd j | silent 4b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 1b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 3b | tabnext | leftabove vertical split | leftabove split | silent 1b | wincmd j | leftabove split | silent 2b | wincmd j | silent 3b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_12="-c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | silent 1b | wincmd l | silent 3b | wincmd j | silent 2b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_13="-c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | silent 1b | wincmd l | silent 3b | wincmd j | silent 2b | wincmd l | leftabove vertical split | leftabove split | silent 1b | wincmd j | silent 3b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_14="-c \"echo | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 3b | tabnext | leftabove vertical split | silent 2b | wincmd l | silent 1b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_15="-c \"echo | -tabnew | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 2b | wincmd l | silent 3b | wincmd j | silent 4b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 1b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 3b | tabnext | leftabove vertical split | leftabove split | silent 1b | wincmd j | leftabove split | silent 2b | wincmd j | silent 3b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_16="-c \"echo | -tabnew | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 2b | wincmd l | silent 3b | wincmd j | silent 4b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 1b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 3b | tabnext | leftabove vertical split | leftabove split | silent 1b | wincmd j | leftabove split | silent 2b | wincmd j | silent 3b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
 
 	EXPECTED_TARGET_01="MERGED"
 	EXPECTED_TARGET_02="LOCAL"
@@ -635,7 +635,7 @@ 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
+	echo | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent quit | wincmd l | silent 2b | wincmd j | silent 3b | set hidden | silent tabdo windo diffthis
 	-c
 	tabfirst
 	lo cal
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] mergetools: vimdiff3: fix regression
  2022-08-06 18:29     ` Fernando Ramos
  2022-08-06 18:37       ` [PATCH] vimdiff: fix 'vimdiff3' behavior (colors + no extra key press) Fernando Ramos
@ 2022-08-06 19:17       ` Felipe Contreras
  2022-08-06 21:23         ` Fernando Ramos
  1 sibling, 1 reply; 15+ messages in thread
From: Felipe Contreras @ 2022-08-06 19:17 UTC (permalink / raw)
  To: Fernando Ramos; +Cc: git

On Sat, Aug 6, 2022 at 1:29 PM Fernando Ramos <greenfoo@u92.eu> wrote:
>
> On 22/08/06 12:53PM, Felipe Contreras wrote:
> > Two observations though.
> >
> > 1. The "silent 4b" is ignored, since bufdo makes the last buffer the
> > current buffer, so if you want a different buffer you have to make the
> > switch *after* bufdo.
> >
>
> Yes, you are right. For the particular case where there are no windows (only
> hidden buffers) it does not have any effect. It's presence there comes from
> the fact that the command generation function works in the most "generic" way
> (ie. producing output that works for all cases: windows, tabs and buffers).
>
> In order not to have another special case in the generation logic I left it
> there, but you are right in that it is not needed (fortunately it also doesn't
> make any harm :)

That's not my point. vimdiff3 is essentially the same as vimdiff with:

    git config --global mergetool.vimdiff.layout MERGED

But the code is written in such a way as to allow:

    git config --global mergetool.vimdiff.layout LOCAL

I don't know why anyone would want to do that, but the code interprets
that as the user wanting '1b', which is completely ignored.

If we are not going to care about these cases, we can just remove all this code:

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

        # 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
-
-       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
+       # If we reach this point, it means there are no separators.

        echo "$CMD"
        return

> > I don't see the need for all this complexity for this simple mode, but
> > anything that actually works is fine by me.
>
> ...in fact, back in May I just wanted to add a new "vimdiff4" mode and what
> originally was a 5 lines patch became the current 1000+ lines patch monster
> after all the (very welcomed, I'm not complaining!) suggestions :)

I understand the need if you want a complex layout, like
"MERGED+LOCAL,BASE,REMOTE", that's very nice, but if you just want
"MERGED", most of the code does nothing, the extra -c "tabfirst" isn't
needed either.

Either way, adding the silent stuff and "set hidden" make vimdiff3
work, which is all I care about.

Cheers.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH] vimdiff: fix 'vimdiff3' behavior (colors + no extra key press)
  2022-08-06 18:37       ` [PATCH] vimdiff: fix 'vimdiff3' behavior (colors + no extra key press) Fernando Ramos
@ 2022-08-06 19:39         ` Felipe Contreras
  2022-08-06 21:26           ` Fernando Ramos
  0 siblings, 1 reply; 15+ messages in thread
From: Felipe Contreras @ 2022-08-06 19:39 UTC (permalink / raw)
  To: Fernando Ramos; +Cc: git

On Sat, Aug 6, 2022 at 1:38 PM 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.
>
> This patch fixes both issues by adding "set hidden" and "silent" to the
> generated command string that is used to run vim.

Although I don't see the point of the extra complexity in the case of
a single window, especially since it doesn't really work for anything
other than "MERGED", this does make vimdiff3 work again for me.

Tested-by: Felipe Contreras <felipe.contreras@gmail.com>

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] mergetools: vimdiff3: fix regression
  2022-08-06 19:17       ` [PATCH 0/2] mergetools: vimdiff3: fix regression Felipe Contreras
@ 2022-08-06 21:23         ` Fernando Ramos
  2022-08-07  0:44           ` Felipe Contreras
  0 siblings, 1 reply; 15+ messages in thread
From: Fernando Ramos @ 2022-08-06 21:23 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git

On 22/08/06 02:17PM, Felipe Contreras wrote:
> 
> I don't know why anyone would want to do that, but the code interprets
> that as the user wanting '1b', which is completely ignored.
> 
> If we are not going to care about these cases, we can just remove all this code:
> 
> ...
> 

Ah! I see now. You are completely right: it wouldn't make sense for anyone to
specify "layout=LOCAL" (or REMOTE or BASE), but if he did *it wouldn't work*
(only works with "layout=MERGED").

That should be fixed. I'll update the patch with a new version to generate this
command string:

     echo | silent 4b | set hidden | let tmp=bufnr('%') | silent bufdo diffthis | exe 'buffer '.tmp
                                     ^^^^^^^^^^^^^^^^^^                           ^^^^^^^^^^^^^^^^^
                                     NEW                                          NEW

Notes:

  - This is "easier" than moving "silent 4b" to the end, due to the way the
    code is structured.

  - I agree that this is absurdly complex for what we want to achieve with
    "vimdiff3" but let's put it this way: now everything can be achieved with
    the "layout" configuration option, even "useless" things such as setting it
    to "LOCAL".


> I understand the need if you want a complex layout, like
> "MERGED+LOCAL,BASE,REMOTE", that's very nice, but if you just want
> "MERGED", most of the code does nothing, 

With the fix above that shouldn't be a problem anymore: even if someone
specifies "LOCAL" it will work, in an absurd way, but it will work :)


> the extra -c "tabfirst" isn't needed either.

Good catch. I'm also removing it.



^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH] vimdiff: fix 'vimdiff3' behavior (colors + no extra key press)
  2022-08-06 19:39         ` Felipe Contreras
@ 2022-08-06 21:26           ` Fernando Ramos
  2022-08-06 21:30             ` Fernando Ramos
  0 siblings, 1 reply; 15+ messages in thread
From: Fernando Ramos @ 2022-08-06 21:26 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git

I will reply to this message with a new patch that, as discussed in the previous
email, will also work with crazy layouts such as "layout=LOCAL" :)

^ permalink raw reply	[flat|nested] 15+ messages in thread
* [PATCH] vimdiff: fix 'vimdiff3' behavior (colors + no extra key press)
  2022-08-06 21:26           ` Fernando Ramos
@ 2022-08-06 21:30             ` Fernando Ramos
  2022-08-07  0:55               ` Felipe Contreras
  0 siblings, 1 reply; 15+ messages in thread
From: Fernando Ramos @ 2022-08-06 21:30 UTC (permalink / raw)
  To: greenfoo; +Cc: felipe.contreras, git

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 by adding "set hidden" and "silent" to the
generated command string that is used to run vim.

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

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index f770b8fe24..238963071a 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 to "true"
 
 	if test -n "$GIT_MERGETOOL_VIMDIFF_DEBUG"
 	then
@@ -261,19 +261,19 @@ gen_cmd_aux () {
 
 	if test "$target" = "LOCAL"
 	then
-		CMD="$CMD | 1b"
+		CMD="$CMD | silent 1b"
 
 	elif test "$target" = "BASE"
 	then
-		CMD="$CMD | 2b"
+		CMD="$CMD | silent 2b"
 
 	elif test "$target" = "REMOTE"
 	then
-		CMD="$CMD | 3b"
+		CMD="$CMD | silent 3b"
 
 	elif test "$target" = "MERGED"
 	then
-		CMD="$CMD | 4b"
+		CMD="$CMD | silent 4b"
 
 	else
 		CMD="$CMD | ERROR: >$target<"
@@ -310,7 +310,7 @@ gen_cmd () {
 	#
 	#     gen_cmd "@LOCAL , REMOTE"
 	#     |
-	#     `-> FINAL_CMD    == "-c \"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\" -c \"tabfirst\""
+	#     `-> FINAL_CMD    == "-c \"echo | leftabove vertical split | silent 1b | wincmd l | silent 3b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
 	#         FINAL_TARGET == "LOCAL"
 
 	LAYOUT=$1
@@ -341,17 +341,18 @@ gen_cmd () {
 
 	if echo "$LAYOUT" | grep ",\|/" >/dev/null
 	then
-		CMD="$CMD | tabdo windo diffthis"
-	else
-		CMD="$CMD | bufdo diffthis"
-	fi
+		CMD="$CMD | set hidden | silent 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
+		# 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\""
+	else
+		CMD="$CMD | set hidden | let tmp=bufnr('%') | silent bufdo diffthis | exe 'buffer '.tmp"
 
-	FINAL_CMD="-c \"$CMD\" -c \"tabfirst\""
+		FINAL_CMD="-c \"$CMD\""
+	fi
 }
 
 
@@ -555,22 +556,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 \"echo | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 2b | wincmd l | silent 3b | wincmd j | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_02="-c \"echo | leftabove vertical split | silent 1b | wincmd l | silent 3b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_03="-c \"echo | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 4b | wincmd l | silent 3b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_04="-c \"echo | silent 4b | set hidden | let tmp=bufnr('%') | silent bufdo diffthis | exe 'buffer '.tmp\""
+	EXPECTED_CMD_05="-c \"echo | leftabove split | silent 1b | wincmd j | leftabove split | silent 4b | wincmd j | silent 3b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_06="-c \"echo | leftabove vertical split | leftabove split | silent 1b | wincmd j | silent 3b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_07="-c \"echo | leftabove vertical split | silent 4b | wincmd l | leftabove split | silent 1b | wincmd j | silent 3b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_08="-c \"echo | leftabove split | leftabove vertical split | silent 1b | wincmd l | silent 3b | wincmd j | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_09="-c \"echo | leftabove split | silent 4b | wincmd j | leftabove vertical split | silent 1b | wincmd l | silent 3b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_10="-c \"echo | leftabove vertical split | leftabove split | silent 1b | wincmd j | leftabove split | silent 2b | wincmd j | silent 3b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_11="-c \"echo | -tabnew | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 2b | wincmd l | silent 3b | wincmd j | silent 4b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 1b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 3b | tabnext | leftabove vertical split | leftabove split | silent 1b | wincmd j | leftabove split | silent 2b | wincmd j | silent 3b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_12="-c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | silent 1b | wincmd l | silent 3b | wincmd j | silent 2b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_13="-c \"echo | leftabove vertical split | leftabove split | leftabove vertical split | silent 1b | wincmd l | silent 3b | wincmd j | silent 2b | wincmd l | leftabove vertical split | leftabove split | silent 1b | wincmd j | silent 3b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_14="-c \"echo | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 3b | tabnext | leftabove vertical split | silent 2b | wincmd l | silent 1b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_15="-c \"echo | -tabnew | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 2b | wincmd l | silent 3b | wincmd j | silent 4b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 1b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 3b | tabnext | leftabove vertical split | leftabove split | silent 1b | wincmd j | leftabove split | silent 2b | wincmd j | silent 3b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
+	EXPECTED_CMD_16="-c \"echo | -tabnew | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent 2b | wincmd l | silent 3b | wincmd j | silent 4b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 1b | tabnext | -tabnew | leftabove vertical split | silent 2b | wincmd l | silent 3b | tabnext | leftabove vertical split | leftabove split | silent 1b | wincmd j | leftabove split | silent 2b | wincmd j | silent 3b | wincmd l | silent 4b | set hidden | silent tabdo windo diffthis\" -c \"tabfirst\""
 
 	EXPECTED_TARGET_01="MERGED"
 	EXPECTED_TARGET_02="LOCAL"
@@ -635,7 +636,7 @@ 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
+	echo | leftabove split | leftabove vertical split | silent 1b | wincmd l | leftabove vertical split | silent quit | wincmd l | silent 2b | wincmd j | silent 3b | set hidden | silent tabdo windo diffthis
 	-c
 	tabfirst
 	lo cal
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] mergetools: vimdiff3: fix regression
  2022-08-06 21:23         ` Fernando Ramos
@ 2022-08-07  0:44           ` Felipe Contreras
  0 siblings, 0 replies; 15+ messages in thread
From: Felipe Contreras @ 2022-08-07  0:44 UTC (permalink / raw)
  To: Fernando Ramos; +Cc: git

On Sat, Aug 6, 2022 at 4:23 PM Fernando Ramos <greenfoo@u92.eu> wrote:
>
> On 22/08/06 02:17PM, Felipe Contreras wrote:
> >
> > I don't know why anyone would want to do that, but the code interprets
> > that as the user wanting '1b', which is completely ignored.
> >
> > If we are not going to care about these cases, we can just remove all this code:
> >
> > ...
> >
>
> Ah! I see now. You are completely right: it wouldn't make sense for anyone to
> specify "layout=LOCAL" (or REMOTE or BASE), but if he did *it wouldn't work*
> (only works with "layout=MERGED").
>
> That should be fixed. I'll update the patch with a new version to generate this
> command string:
>
>      echo | silent 4b | set hidden | let tmp=bufnr('%') | silent bufdo diffthis | exe 'buffer '.tmp
>                                      ^^^^^^^^^^^^^^^^^^                           ^^^^^^^^^^^^^^^^^
>                                      NEW                                          NEW

That's not correct: `exe 'buffer '.tmp` would be executed in every
buffer. To split the commands you need to do the bufdo in a separate
execute command.

> Notes:
>
>   - This is "easier" than moving "silent 4b" to the end, due to the way the
>     code is structured.

Which is a clear hint that the code should be restructured.

>   - I agree that this is absurdly complex for what we want to achieve with
>     "vimdiff3" but let's put it this way: now everything can be achieved with
>     the "layout" configuration option, even "useless" things such as setting it
>     to "LOCAL".

Yes, but even that can be achieved in simpler ways (see the patch below).

> > I understand the need if you want a complex layout, like
> > "MERGED+LOCAL,BASE,REMOTE", that's very nice, but if you just want
> > "MERGED", most of the code does nothing,
>
> With the fix above that shouldn't be a problem anymore: even if someone
> specifies "LOCAL" it will work, in an absurd way, but it will work :)

But the objective isn't to make "everything" work, the objective is to
make everything the user might reasonably want to work. If some
unreasonable use case can be supported with a minimal burden to
maintenance, sure, support that too.

"LOCAL" is not that: it's an unreasonable use case that requires a
bunch of extra code.

Either way, I think you are resisting too much a reshuffling of the
code when it's very clear the single window mode would benefit from
that since it doesn't need gen_cmd_aux() at all.

Here's a patch that shuffles the code around and makes it much easier
to read and maintain (plus it keeps and fixes the support for
unreasonable use cases like "LOCAL"):

(apologies in advance for gmail's possible wrapping)

--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -251,39 +251,34 @@ gen_cmd_aux () {
  return
  fi

+ # Shouldn't happen
+ echo "$CMD | echoerr 'BUG'"
+}

- # 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
-
- target=$(substring "$LAYOUT" "$start" "$(( end - start ))" | sed
's:[ @();|-]::g')
+get_buf () {
+ target=$(echo "$1" | sed 's:[ @();|-]::g')
+ buf="1"

  if test "$target" = "LOCAL"
  then
- CMD="$CMD | 1b"
+ buf="1"

  elif test "$target" = "BASE"
  then
- CMD="$CMD | 2b"
+ buf="2"

  elif test "$target" = "REMOTE"
  then
- CMD="$CMD | 3b"
+ buf="3"

  elif test "$target" = "MERGED"
  then
- CMD="$CMD | 4b"
-
- else
- CMD="$CMD | ERROR: >$target<"
+ buf="4"
  fi

- echo "$CMD"
- return
+ echo "$buf"
 }

-
 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
@@ -315,6 +310,14 @@ gen_cmd () {

  LAYOUT=$1

+ # A single window is handled specially
+
+ if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
+ then
+ buf=$(get_buf "$LAYOUT")
+ FINAL_CMD="-c \"set hidden | silent bufdo diffthis\" -c \"silent ${buf}b\""
+ return
+ fi

  # Search for a "@" in one of the files identifiers ("LOCAL", "BASE",
  # "REMOTE", "MERGED"). If not found, use "MERGE" as the default file
@@ -335,17 +338,7 @@ 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
-
+ 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

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH] vimdiff: fix 'vimdiff3' behavior (colors + no extra key press)
  2022-08-06 21:30             ` Fernando Ramos
@ 2022-08-07  0:55               ` Felipe Contreras
  0 siblings, 0 replies; 15+ messages in thread
From: Felipe Contreras @ 2022-08-07  0:55 UTC (permalink / raw)
  To: Fernando Ramos; +Cc: git

On Sat, Aug 6, 2022 at 4:30 PM Fernando Ramos <greenfoo@u92.eu> wrote:

>
> +               # 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\""
> +       else
> +               CMD="$CMD | set hidden | let tmp=bufnr('%') | silent bufdo diffthis | exe 'buffer '.tmp"

NAK: This runs `exe 'buffer #'` in every buffer. While switching to
the desired buffer 3 extra times isn't a problem, it's also not
correct.

You need something like:

    silent exe 'bufdo diffthis' | exe 'buffer '.tmp"

But at this point it seems like we are working around the current
organization of the code, when we could simply reorganize it.

Cheers.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-08-07  0:55 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-02 21:41 [PATCH 0/2] mergetools: vimdiff3: fix regression Felipe Contreras
2022-08-02 21:41 ` [PATCH 1/2] mergetools: vimdiff3: make it work as intended Felipe Contreras
2022-08-02 21:41 ` [PATCH 2/2] mergetools: vimdiff3: fix diffopt options Felipe Contreras
2022-08-06 16:25 ` [PATCH 0/2] mergetools: vimdiff3: fix regression Fernando Ramos
2022-08-06 16:27   ` Fernando Ramos
2022-08-06 17:53   ` Felipe Contreras
2022-08-06 18:29     ` Fernando Ramos
2022-08-06 18:37       ` [PATCH] vimdiff: fix 'vimdiff3' behavior (colors + no extra key press) Fernando Ramos
2022-08-06 19:39         ` Felipe Contreras
2022-08-06 21:26           ` Fernando Ramos
2022-08-06 21:30             ` Fernando Ramos
2022-08-07  0:55               ` Felipe Contreras
2022-08-06 19:17       ` [PATCH 0/2] mergetools: vimdiff3: fix regression Felipe Contreras
2022-08-06 21:23         ` Fernando Ramos
2022-08-07  0:44           ` Felipe Contreras

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).