[0/2] mergetools: vimdiff3: fix regression

Message ID	20220802214134.681300-1-felipe.contreras@gmail.com (mailing list archive)
Headers	show Return-Path: <git-owner@kernel.org> From: Felipe Contreras <felipe.contreras@gmail.com> To: git@vger.kernel.org Cc: Fernando Ramos <greenfoo@u92.eu>, Felipe Contreras <felipe.contreras@gmail.com> Subject: [PATCH 0/2] mergetools: vimdiff3: fix regression Date: Tue, 2 Aug 2022 16:41:32 -0500 Message-Id: <20220802214134.681300-1-felipe.contreras@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	mergetools: vimdiff3: fix regression \| expand [0/2] mergetools: vimdiff3: fix regression [1/2] mergetools: vimdiff3: make it work as intended [2/2] mergetools: vimdiff3: fix diffopt options

Felipe Contreras Aug. 2, 2022, 9:41 p.m. UTC

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(-)

Fernando Ramos Aug. 6, 2022, 4:25 p.m. UTC | #1

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.

Fernando Ramos Aug. 6, 2022, 4:27 p.m. UTC | #2

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

Felipe Contreras Aug. 6, 2022, 5:53 p.m. UTC | #3

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.

Fernando Ramos Aug. 6, 2022, 6:29 p.m. UTC | #4

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!

Felipe Contreras Aug. 6, 2022, 7:17 p.m. UTC | #5

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.

Fernando Ramos Aug. 6, 2022, 9:23 p.m. UTC | #6

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.

Felipe Contreras Aug. 7, 2022, 12:44 a.m. UTC | #7

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

[0/2] mergetools: vimdiff3: fix regression

Message

Comments