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

[Felipe Contreras <felipe.contreras@gmail.com>]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


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


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

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

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

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

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

Cheers.

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

-- 
Felipe Contreras