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

[Fernando Ramos <greenfoo@u92.eu>]

On 22/08/08 04:00PM, Felipe Contreras wrote:
> 
> No. That only happens in patch 9. In patch 5--which is where those two
> bugs are resolved--that problem doesn't exist yet.
> 

Sure. I was referring to your whole patch series (which is what I was testing)
not to a particular commit.

If I only apply some of your patches it is true that (1) is no longer an issue,
but we still have to deal with (2).


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

In any case it doesn't happen with v3 :)


> >   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.

Only solution possible? Of course not! (I'm sorry if you got that impression)

I'm just presenting one solution that works, but I'm sure there are many others
(we could use "vim -d" in all cases or even implement a completely different
parsing logic).

Do you think we should try a different approach from the currently proposed one?


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

You mean to apply your patch series and then apply mine? Sure, why not? But what
do get get from that? (I'm just curious)

I mean... v3 already works in all cases, right? Or am I missing something?


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

Oh! Ok, now I see where the confusion comes from: showing the diff colors only
for the visible buffers is *a desired property* from the original design and not
something we want to avoid (except for the case where there is just one window,
which is what we are fixing now).

This is actually why all of this work started: we want to see, in one tab, only
differences between BASE and LOCAL and, in another tab, only differences between
BASE and REMOTE *without extra diff noise*.

If we enable ":bufdo diffthis" in those tabs we get a mess (at least for complex
merges).

Imagine this:

  - Tab #1: classical four windows layout (LOCAL, BASE, REMOTE on top and MERGED
    on the bottom).

  - Tab #2: two windows: left BASE, right REMOTE

  - Tab #3: two windows: left BASE, right LOCAL

We already see all highgliting and colors in the 4-way diff in tab #1, but we
are only interested in the sepecific BASE vs REMOTE and BASE vs LOCAL in tabs #2
and #3.


> > 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, 
>

You must excuse me here... I still find it extremely confusing to figure out
which field to use for attribution each time.

I added you to the "CC:" footer of the commit. Is this not the right procedure? 
Should I have done something different?


> ...it's not a good practice to sneak in unrelated changes.

Should we split the patch series in more commits, then?
Something like this?

  1. Comment fix
  2. Keep track of windows opened per tab
  3. Update generated string
  4. Add "set" options
  5. Update unit tests

I mean... I'm right with that sure :) Note, however that 2, 3 and 4 are closely
related (ie. "vimdiff3" won't work until the 3 of them are merged)