threads / patch / 25106

patch, 2 partsmergetool-lib: combine vimdiff and gvimdiff run blocks

Subject: [PATCH 1/2] mergetool-lib: combine vimdiff and gvimdiff run blocks

## tl;dr

10 messages between Sep 15, 2010 and Sep 27, 2010. Diffs are folded; open one to read it.

replies: 9people: 5as markdown or json

Dan McGee· Sep 15, 2010, 02:21 UTC · lore

They are nearly identical outside of the foreground flag, which can safely be passed to both vim and gvim. The merge tool itself is named in $merge_tool_path.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 git-mergetool--lib.sh |   17 +++--------------
 1 files changed, 3 insertions(+), 14 deletions(-)

Show changes to git-mergetool--lib.sh +3 −14

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index b5e1943..f9a51ba 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -169,25 +169,14 @@ run_merge_tool () {
 			"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
 		fi
 		;;
-	vimdiff)
-		if merge_mode; then
-			touch "$BACKUP"
-			"$merge_tool_path" -d -c "wincmd l" \
-				"$LOCAL" "$MERGED" "$REMOTE"
-			check_unchanged
-		else
-			"$merge_tool_path" -d -c "wincmd l" \
-				"$LOCAL" "$REMOTE"
-		fi
-		;;
-	gvimdiff)
+	vimdiff|gvimdiff)
 		if merge_mode; then
 			touch "$BACKUP"
-			"$merge_tool_path" -d -c "wincmd l" -f \
+			"$merge_tool_path" -f -d -c "wincmd l" \
 				"$LOCAL" "$MERGED" "$REMOTE"
 			check_unchanged
 		else
-			"$merge_tool_path" -d -c "wincmd l" -f \
+			"$merge_tool_path" -f -d -c "wincmd l" \
 				"$LOCAL" "$REMOTE"
 		fi
 		;;

-- 
1.7.2.3

Dan McGee· Sep 15, 2010, 02:21 UTC · re: Dan McGee · lore

[PATCH 2/2] mergetool-lib: add a three-way diff view for vim/gvim

When the base version is available, use a three-way, four panel view by default. This shows the (local, base, remote) revisions up top and the merged result by itself in the lower pane. All revisions will still scroll together by default, and the cursor still defaults to the merged result edit pane.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---

Vim was one of the few diff commands to not support a three-way merge showing the base revision, so this is a stab at resolving that shortfall. The biggest objection I can see to this is making the interface a bit more cumbersome and bloated.

An example screenshot of what this produces: http://www.toofishes.net/media/extra/vim_three_way.png

-Dan

 git-mergetool--lib.sh |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

Show changes to git-mergetool--lib.sh +7 −2

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index f9a51ba..b84ac58 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -172,8 +172,13 @@ run_merge_tool () {
 	vimdiff|gvimdiff)
 		if merge_mode; then
 			touch "$BACKUP"
-			"$merge_tool_path" -f -d -c "wincmd l" \
-				"$LOCAL" "$MERGED" "$REMOTE"
+			if $base_present; then
+				"$merge_tool_path" -f -d -c "wincmd J" \
+					"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
+			else
+				"$merge_tool_path" -f -d -c "wincmd l" \
+					"$LOCAL" "$MERGED" "$REMOTE"
+			fi
 			check_unchanged
 		else
 			"$merge_tool_path" -f -d -c "wincmd l" \

-- 
1.7.2.3

David Aguilar· Sep 18, 2010, 07:34 UTC · re: Dan McGee · lore

Re: [PATCH 2/2] mergetool-lib: add a three-way diff view for vim/gvim

On Tue, Sep 14, 2010 at 09:21:43PM -0500, Dan McGee wrote:

Show 18 quoted lines

> When the base version is available, use a three-way, four panel view by
> default. This shows the (local, base, remote) revisions up top and the
> merged result by itself in the lower pane. All revisions will still scroll
> together by default, and the cursor still defaults to the merged result edit
> pane.
> 
> Signed-off-by: Dan McGee <dpmcgee@gmail.com>
> ---
> 
> Vim was one of the few diff commands to not support a three-way merge showing
> the base revision, so this is a stab at resolving that shortfall. The biggest
> objection I can see to this is making the interface a bit more cumbersome and
> bloated.
> 
> An example screenshot of what this produces:
> http://www.toofishes.net/media/extra/vim_three_way.png
> 
> -Dan

Patch 1/2 of this series looks good to me.

Is it worth keeping the old behavior and calling this new mode "vimdiff3" or something along those lines?

I'm not a vimdiff user so I'm not be the best person to judge the merits of this change. I like what it's trying to accomplish, though. Are there any vimdiff users with strong feelings either way?

-- 
		David

Felipe Contreras· Sep 19, 2010, 09:48 UTC · re: David Aguilar · lore

Re: [PATCH 2/2] mergetool-lib: add a three-way diff view for vim/gvim

On Sat, Sep 18, 2010 at 10:34 AM, David Aguilar <davvid@gmail.com> wrote:

Show 30 quoted lines

> On Tue, Sep 14, 2010 at 09:21:43PM -0500, Dan McGee wrote:
>> When the base version is available, use a three-way, four panel view by
>> default. This shows the (local, base, remote) revisions up top and the
>> merged result by itself in the lower pane. All revisions will still scroll
>> together by default, and the cursor still defaults to the merged result edit
>> pane.
>>
>> Signed-off-by: Dan McGee <dpmcgee@gmail.com>
>> ---
>>
>> Vim was one of the few diff commands to not support a three-way merge showing
>> the base revision, so this is a stab at resolving that shortfall. The biggest
>> objection I can see to this is making the interface a bit more cumbersome and
>> bloated.
>>
>> An example screenshot of what this produces:
>> http://www.toofishes.net/media/extra/vim_three_way.png
>>
>> -Dan
>
>
> Patch 1/2 of this series looks good to me.
>
> Is it worth keeping the old behavior and calling this new
> mode "vimdiff3" or something along those lines?
>
> I'm not a vimdiff user so I'm not be the best person to
> judge the merits of this change.  I like what it's trying
> to accomplish, though.  Are there any vimdiff users
> with strong feelings either way?

I think this is a definite improvement; the old mode wasn't really useful for me.

-- 
Felipe Contreras

Dan McGee· Sep 24, 2010, 19:01 UTC · re: Felipe Contreras · lore

Re: [PATCH 2/2] mergetool-lib: add a three-way diff view for vim/gvim

On Sun, Sep 19, 2010 at 4:48 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 34 quoted lines

> On Sat, Sep 18, 2010 at 10:34 AM, David Aguilar <davvid@gmail.com> wrote:
>> On Tue, Sep 14, 2010 at 09:21:43PM -0500, Dan McGee wrote:
>>> When the base version is available, use a three-way, four panel view by
>>> default. This shows the (local, base, remote) revisions up top and the
>>> merged result by itself in the lower pane. All revisions will still scroll
>>> together by default, and the cursor still defaults to the merged result edit
>>> pane.
>>>
>>> Signed-off-by: Dan McGee <dpmcgee@gmail.com>
>>> ---
>>>
>>> Vim was one of the few diff commands to not support a three-way merge showing
>>> the base revision, so this is a stab at resolving that shortfall. The biggest
>>> objection I can see to this is making the interface a bit more cumbersome and
>>> bloated.
>>>
>>> An example screenshot of what this produces:
>>> http://www.toofishes.net/media/extra/vim_three_way.png
>>>
>>> -Dan
>>
>>
>> Patch 1/2 of this series looks good to me.
>>
>> Is it worth keeping the old behavior and calling this new
>> mode "vimdiff3" or something along those lines?
>>
>> I'm not a vimdiff user so I'm not be the best person to
>> judge the merits of this change.  I like what it's trying
>> to accomplish, though.  Are there any vimdiff users
>> with strong feelings either way?
>
> I think this is a definite improvement; the old mode wasn't really
> useful for me.

Not as much feedback as I had hoped, but thanks to those that did speak up. I was thinking of adding a separate mode, but I think it would then get under-used and as I said, every other merge tool was already doing this anyway.

So are these patches good to go forward with? No major objections in a over a week's time.

-Dan

Jacob Helwig· Sep 24, 2010, 19:09 UTC · re: Dan McGee · lore

Re: [PATCH 2/2] mergetool-lib: add a three-way diff view for vim/gvim

On Fri, 24 Sep 2010 14:01:01 -0500, Dan McGee wrote:

Show 47 quoted lines

> 
> On Sun, Sep 19, 2010 at 4:48 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> > On Sat, Sep 18, 2010 at 10:34 AM, David Aguilar <davvid@gmail.com> wrote:
> >> On Tue, Sep 14, 2010 at 09:21:43PM -0500, Dan McGee wrote:
> >>> When the base version is available, use a three-way, four panel view by
> >>> default. This shows the (local, base, remote) revisions up top and the
> >>> merged result by itself in the lower pane. All revisions will still scroll
> >>> together by default, and the cursor still defaults to the merged result edit
> >>> pane.
> >>>
> >>> Signed-off-by: Dan McGee <dpmcgee@gmail.com>
> >>> ---
> >>>
> >>> Vim was one of the few diff commands to not support a three-way merge showing
> >>> the base revision, so this is a stab at resolving that shortfall. The biggest
> >>> objection I can see to this is making the interface a bit more cumbersome and
> >>> bloated.
> >>>
> >>> An example screenshot of what this produces:
> >>> http://www.toofishes.net/media/extra/vim_three_way.png
> >>>
> >>> -Dan
> >>
> >>
> >> Patch 1/2 of this series looks good to me.
> >>
> >> Is it worth keeping the old behavior and calling this new
> >> mode "vimdiff3" or something along those lines?
> >>
> >> I'm not a vimdiff user so I'm not be the best person to
> >> judge the merits of this change.  I like what it's trying
> >> to accomplish, though.  Are there any vimdiff users
> >> with strong feelings either way?
> >
> > I think this is a definite improvement; the old mode wasn't really
> > useful for me.
> 
> Not as much feedback as I had hoped, but thanks to those that did
> speak up. I was thinking of adding a separate mode, but I think it
> would then get under-used and as I said, every other merge tool was
> already doing this anyway.
> 
> So are these patches good to go forward with? No major objections in a
> over a week's time.
> 
> -Dan

I'd +1 David's suggestion of calling this "vimdiff3", I'd like to still be able to access the current behavior, since I have merge.conflictstyle = diff3, and already see the merge base when I use (g)vimdiff with mergetool.

-- 
Jacob Helwig

Jeff King· Sep 24, 2010, 21:38 UTC · re: Jacob Helwig · lore

Re: [PATCH 2/2] mergetool-lib: add a three-way diff view for vim/gvim

On Fri, Sep 24, 2010 at 12:09:28PM -0700, Jacob Helwig wrote:

Show 9 quoted lines

> > So are these patches good to go forward with? No major objections in a
> > over a week's time.
> > 
> > -Dan
> 
> I'd +1 David's suggestion of calling this "vimdiff3", I'd like to still
> be able to access the current behavior, since I have merge.conflictstyle
> = diff3, and already see the merge base when I use (g)vimdiff with
> mergetool.

Of course as soon as I say "nobody objected" in my other email, this arrives. :)

Can we provide both, but make the vimdiff3 behavior the preferred default? It better matches the default merge.conflictstyle, and people who are using diff3 obviously understand how to tweak config.

-Peff

David Aguilar· Sep 25, 2010, 03:17 UTC · re: Jeff King · lore

Re: [PATCH 2/2] mergetool-lib: add a three-way diff view for vim/gvim

On Fri, Sep 24, 2010 at 05:38:52PM -0400, Jeff King wrote:

Show 20 quoted lines

> On Fri, Sep 24, 2010 at 12:09:28PM -0700, Jacob Helwig wrote:
> 
> > > So are these patches good to go forward with? No major objections in a
> > > over a week's time.
> > > 
> > > -Dan
> > 
> > I'd +1 David's suggestion of calling this "vimdiff3", I'd like to still
> > be able to access the current behavior, since I have merge.conflictstyle
> > = diff3, and already see the merge base when I use (g)vimdiff with
> > mergetool.
> 
> Of course as soon as I say "nobody objected" in my other email, this
> arrives. :)
> 
> Can we provide both, but make the vimdiff3 behavior the preferred
> default? It better matches the default merge.conflictstyle, and people
> who are using diff3 obviously understand how to tweak config.
> 
> -Peff

+1 to Peff's suggestion.

Dan, can you reroll the patch so that the new behavior is "(g)vimdiff" and the old behavior is available as "(g)vimdiff2"?

I do slightly dislike having both from the maintenance POV. But, it's better to keep it around than to rip it away from happy users' hands. Thanks for speaking up Jacob.

Cheers,

-- 
		David

Dan McGee· Sep 27, 2010, 15:19 UTC · re: David Aguilar · lore

[PATCH 2/2] mergetool-lib: add a three-way diff view for vim/gvim

The original vimdiff/gvimdiff configuration is now available by using 'vimdiff2' or 'gvimdiff2' as the preferred merge tool.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---

This should address the comments I got once I pestered people. The new behavior is the default, but it at least becomes possible to use the previous behavior without much hassle (just set your mergetool appropriately).

-Dan

 git-mergetool--lib.sh |   23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)

Show changes to git-mergetool--lib.sh +20 −3

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index f9a51ba..77d4aee 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -10,10 +10,10 @@ merge_mode() {
 
 translate_merge_tool_path () {
 	case "$1" in
-	vimdiff)
+	vimdiff|vimdiff2)
 		echo vim
 		;;
-	gvimdiff)
+	gvimdiff|gvimdiff2)
 		echo gvim
 		;;
 	emerge)
@@ -47,7 +47,8 @@ check_unchanged () {
 valid_tool () {
 	case "$1" in
 	kdiff3 | tkdiff | xxdiff | meld | opendiff | \
-	emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge)
+	vimdiff | gvimdiff | vimdiff2 | gvimdiff2 | \
+	emerge | ecmerge | diffuse | araxis | p4merge)
 		;; # happy
 	tortoisemerge)
 		if ! merge_mode; then
@@ -172,6 +173,22 @@ run_merge_tool () {
 	vimdiff|gvimdiff)
 		if merge_mode; then
 			touch "$BACKUP"
+			if $base_present; then
+				"$merge_tool_path" -f -d -c "wincmd J" \
+					"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
+			else
+				"$merge_tool_path" -f -d -c "wincmd l" \
+					"$LOCAL" "$MERGED" "$REMOTE"
+			fi
+			check_unchanged
+		else
+			"$merge_tool_path" -f -d -c "wincmd l" \
+				"$LOCAL" "$REMOTE"
+		fi
+		;;
+	vimdiff2|gvimdiff2)
+		if merge_mode; then
+			touch "$BACKUP"
 			"$merge_tool_path" -f -d -c "wincmd l" \
 				"$LOCAL" "$MERGED" "$REMOTE"
 			check_unchanged

-- 
1.7.3

Jeff King· Sep 24, 2010, 21:31 UTC · re: Dan McGee · lore

Re: [PATCH 2/2] mergetool-lib: add a three-way diff view for vim/gvim

On Fri, Sep 24, 2010 at 02:01:01PM -0500, Dan McGee wrote:

Show 41 quoted lines

> On Sun, Sep 19, 2010 at 4:48 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> > On Sat, Sep 18, 2010 at 10:34 AM, David Aguilar <davvid@gmail.com> wrote:
> >> On Tue, Sep 14, 2010 at 09:21:43PM -0500, Dan McGee wrote:
> >>> When the base version is available, use a three-way, four panel view by
> >>> default. This shows the (local, base, remote) revisions up top and the
> >>> merged result by itself in the lower pane. All revisions will still scroll
> >>> together by default, and the cursor still defaults to the merged result edit
> >>> pane.
> >>>
> >>> Signed-off-by: Dan McGee <dpmcgee@gmail.com>
> >>> ---
> >>>
> >>> Vim was one of the few diff commands to not support a three-way merge showing
> >>> the base revision, so this is a stab at resolving that shortfall. The biggest
> >>> objection I can see to this is making the interface a bit more cumbersome and
> >>> bloated.
> >>>
> >>> An example screenshot of what this produces:
> >>> http://www.toofishes.net/media/extra/vim_three_way.png
> >>>
> >>> -Dan
> >>
> >>
> >> Patch 1/2 of this series looks good to me.
> >>
> >> Is it worth keeping the old behavior and calling this new
> >> mode "vimdiff3" or something along those lines?
> >>
> >> I'm not a vimdiff user so I'm not be the best person to
> >> judge the merits of this change.  I like what it's trying
> >> to accomplish, though.  Are there any vimdiff users
> >> with strong feelings either way?
> >
> > I think this is a definite improvement; the old mode wasn't really
> > useful for me.
> 
> Not as much feedback as I had hoped, but thanks to those that did
> speak up. I was thinking of adding a separate mode, but I think it
> would then get under-used and as I said, every other merge tool was
> already doing this anyway.

A little more feedback:

I use vim but don't use vimdiff, because the original mode seemed useless to me. Your change makes it much better. I haven't actually had to do any merging lately, though, so I can't comment in practice.

Given that nobody has objected, you have a few comments in support, and the fact that it makes it similar to every other mergetool driver, I think it should probably be the default. If somebody really finds it objectionable, it is not hard for them to configure the old behavior.

-Peff