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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Aug 8, 2022, 21:00 UTC
Message-ID: <CAMP44s1E6B4gG8nzNJwqAkYGQRns87i7iJFw=0yRadw2QL3-Ng@mail.gmail.com>
In-Reply-To: <YvFSgjK0P5kzoOfg@zacax395.localdomain>

On Mon, Aug 8, 2022 at 1:14 PM Fernando Ramos <greenfoo@u92.eu> wrote:

Show 20 quoted lines

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

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

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

Show 7 quoted lines

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

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

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

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

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

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

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

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

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

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

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

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


-       # Adjust the just obtained script depending on whether more than one
-       # windows are visible or not
-
-       if echo "$LAYOUT" | grep ",\|/" >/dev/null
-       then
-               CMD="$CMD | tabdo windo diffthis"
-       else
-               CMD="$CMD | bufdo diffthis"
-       fi
-
-
        # Add an extra "-c" option to move to the first tab (notice that we
        # can't simply append the command to the previous "-c" string as
        # explained here: https://github.com/vim/vim/issues/9076


> > >         # Add an extra "-c" option to move to the first tab (notice that we
> > >         # can't simply append the command to the previous "-c" string as
> > >         # explained here: https://github.com/vim/vim/issues/9076
> > >
> > > -       FINAL_CMD="-c \"$CMD\" -c \"tabfirst\""
> > > +       FINAL_CMD="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"$CMD\" -c \"tabfirst\""
> > >  }
> >
> > These diffopt settings look awfully familiar.
>
> I would go as far as saying they are the same :)

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

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

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

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

Cheers.

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

-- 
Felipe Contreras