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

Message

Felipe Contreras Aug. 7, 2022, 2:49 a.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

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

Comments

Fernando Ramos Aug. 7, 2022, 7:54 a.m. UTC | #1

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.

Felipe Contreras Aug. 7, 2022, 3:39 p.m. UTC | #2

On Sun, Aug 7, 2022 at 2:54 AM Fernando Ramos <[e-mail]> 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.

Fernando Ramos Aug. 7, 2022, 6:39 p.m. UTC | #3

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.

Felipe Contreras Aug. 7, 2022, 10:27 p.m. UTC | #4

On Sun, Aug 7, 2022 at 1:39 PM Fernando Ramos <[e-mail]> 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.