threads / patch / 41282

patchmergetool: reorder vim/gvim buffers in three-way diffs

Subject: [PATCH] mergetool: reorder vim/gvim buffers in three-way diffs

## tl;dr

12 messages between Jan 29, 2016 and Feb 12, 2016. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Dickson Wong· Jan 29, 2016, 02:18 UTC · lore

When invoking default (g)vimdiff three-way merge, the merged file is loaded as the first buffer but moved to the bottom as the fourth window. This causes a disconnect between vim commands that operate on window positions (e.g. CTRL-W_w) and those that operate on buffer index (e.g. do/dp).

This change reorders the buffers to have the same index as windows while keeping the cursor default to the merged result as the bottom window.

Signed-off-by: Dickson Wong <dicksonwong@gmail.com>
---
 mergetools/vimdiff | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to mergetools/vimdiff +2 −2
diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 1ddfbfc..74ea6d5 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -2,22 +2,22 @@ diff_cmd () {
 	"$merge_tool_path" -R -f -d \
 		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
 }
 
 merge_cmd () {
 	touch "$BACKUP"
 	case "$1" in
 	gvimdiff|vimdiff)
 		if $base_present
 		then
-			"$merge_tool_path" -f -d -c 'wincmd J' \
-				"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
+			"$merge_tool_path" -f -d -c '4wincmd w | wincmd J' \
+				"$LOCAL" "$BASE" "$REMOTE" "$MERGED"
 		else
 			"$merge_tool_path" -f -d -c 'wincmd l' \
 				"$LOCAL" "$MERGED" "$REMOTE"
 		fi
 		;;
 	gvimdiff2|vimdiff2)
 		"$merge_tool_path" -f -d -c 'wincmd l' \
 			"$LOCAL" "$MERGED" "$REMOTE"
 		;;
 	gvimdiff3|vimdiff3)

-- 
2.6.2
Junio C Hamano· Jan 29, 2016, 18:45 UTC · re: Dickson Wong · lore

Re: [PATCH] mergetool: reorder vim/gvim buffers in three-way diffs

Dickson Wong <dicksonwong@gmail.com> writes:
Show 11 quoted lines
> When invoking default (g)vimdiff three-way merge, the merged file is
> loaded as the first buffer but moved to the bottom as the fourth window.
> This causes a disconnect between vim commands that operate on window
> positions (e.g. CTRL-W_w) and those that operate on buffer index (e.g.
> do/dp).
>
> This change reorders the buffers to have the same index as windows while
> keeping the cursor default to the merged result as the bottom window.
>
> Signed-off-by: Dickson Wong <dicksonwong@gmail.com>
> ---

David, I unfortunately do not use 'mergetools' myself and certainly not vimdiff.

Opinions?
Show 32 quoted lines
>  mergetools/vimdiff | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
> index 1ddfbfc..74ea6d5 100644
> --- a/mergetools/vimdiff
> +++ b/mergetools/vimdiff
> @@ -2,22 +2,22 @@ diff_cmd () {
>  	"$merge_tool_path" -R -f -d \
>  		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
>  }
>  
>  merge_cmd () {
>  	touch "$BACKUP"
>  	case "$1" in
>  	gvimdiff|vimdiff)
>  		if $base_present
>  		then
> -			"$merge_tool_path" -f -d -c 'wincmd J' \
> -				"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
> +			"$merge_tool_path" -f -d -c '4wincmd w | wincmd J' \
> +				"$LOCAL" "$BASE" "$REMOTE" "$MERGED"
>  		else
>  			"$merge_tool_path" -f -d -c 'wincmd l' \
>  				"$LOCAL" "$MERGED" "$REMOTE"
>  		fi
>  		;;
>  	gvimdiff2|vimdiff2)
>  		"$merge_tool_path" -f -d -c 'wincmd l' \
>  			"$LOCAL" "$MERGED" "$REMOTE"
>  		;;
>  	gvimdiff3|vimdiff3)
Junio C Hamano· Feb 9, 2016, 22:25 UTC · re: Junio C Hamano · lore

Re: [PATCH] mergetool: reorder vim/gvim buffers in three-way diffs

Junio C Hamano <gitster@pobox.com> writes:
Show 18 quoted lines
> Dickson Wong <dicksonwong@gmail.com> writes:
>
>> When invoking default (g)vimdiff three-way merge, the merged file is
>> loaded as the first buffer but moved to the bottom as the fourth window.
>> This causes a disconnect between vim commands that operate on window
>> positions (e.g. CTRL-W_w) and those that operate on buffer index (e.g.
>> do/dp).
>>
>> This change reorders the buffers to have the same index as windows while
>> keeping the cursor default to the merged result as the bottom window.
>>
>> Signed-off-by: Dickson Wong <dicksonwong@gmail.com>
>> ---
>
> David, I unfortunately do not use 'mergetools' myself and certainly
> not vimdiff.
>
> Opinions?

Second call for help. Any comments on this from anybody other than the author that I missed to support this change?

Show 33 quoted lines
>
>>  mergetools/vimdiff | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
>> index 1ddfbfc..74ea6d5 100644
>> --- a/mergetools/vimdiff
>> +++ b/mergetools/vimdiff
>> @@ -2,22 +2,22 @@ diff_cmd () {
>>  	"$merge_tool_path" -R -f -d \
>>  		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
>>  }
>>  
>>  merge_cmd () {
>>  	touch "$BACKUP"
>>  	case "$1" in
>>  	gvimdiff|vimdiff)
>>  		if $base_present
>>  		then
>> -			"$merge_tool_path" -f -d -c 'wincmd J' \
>> -				"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
>> +			"$merge_tool_path" -f -d -c '4wincmd w | wincmd J' \
>> +				"$LOCAL" "$BASE" "$REMOTE" "$MERGED"
>>  		else
>>  			"$merge_tool_path" -f -d -c 'wincmd l' \
>>  				"$LOCAL" "$MERGED" "$REMOTE"
>>  		fi
>>  		;;
>>  	gvimdiff2|vimdiff2)
>>  		"$merge_tool_path" -f -d -c 'wincmd l' \
>>  			"$LOCAL" "$MERGED" "$REMOTE"
>>  		;;
>>  	gvimdiff3|vimdiff3)
Michael J Gruber· Feb 10, 2016, 14:40 UTC · re: Junio C Hamano · lore

Re: [PATCH] mergetool: reorder vim/gvim buffers in three-way diffs

Junio C Hamano venit, vidit, dixit 09.02.2016 23:25:
Show 24 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Dickson Wong <dicksonwong@gmail.com> writes:
>>
>>> When invoking default (g)vimdiff three-way merge, the merged file is
>>> loaded as the first buffer but moved to the bottom as the fourth window.
>>> This causes a disconnect between vim commands that operate on window
>>> positions (e.g. CTRL-W_w) and those that operate on buffer index (e.g.
>>> do/dp).
>>>
>>> This change reorders the buffers to have the same index as windows while
>>> keeping the cursor default to the merged result as the bottom window.
>>>
>>> Signed-off-by: Dickson Wong <dicksonwong@gmail.com>
>>> ---
>>
>> David, I unfortunately do not use 'mergetools' myself and certainly
>> not vimdiff.
>>
>> Opinions?
> 
> Second call for help.  Any comments on this from anybody other than
> the author that I missed to support this change?
>

OK, applied it (on top of next), looks sane and improves the situation for the majority of people who read left to right, then top down and assign buffers 1 2 3 4 "mentally" to local base remote merge windows based on that. Their expectation is met now. Thanks!

(Also, the other vim variants don't need a corresponding change.)
Michael
Show 33 quoted lines
>>
>>>  mergetools/vimdiff | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
>>> index 1ddfbfc..74ea6d5 100644
>>> --- a/mergetools/vimdiff
>>> +++ b/mergetools/vimdiff
>>> @@ -2,22 +2,22 @@ diff_cmd () {
>>>  	"$merge_tool_path" -R -f -d \
>>>  		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
>>>  }
>>>  
>>>  merge_cmd () {
>>>  	touch "$BACKUP"
>>>  	case "$1" in
>>>  	gvimdiff|vimdiff)
>>>  		if $base_present
>>>  		then
>>> -			"$merge_tool_path" -f -d -c 'wincmd J' \
>>> -				"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
>>> +			"$merge_tool_path" -f -d -c '4wincmd w | wincmd J' \
>>> +				"$LOCAL" "$BASE" "$REMOTE" "$MERGED"
>>>  		else
>>>  			"$merge_tool_path" -f -d -c 'wincmd l' \
>>>  				"$LOCAL" "$MERGED" "$REMOTE"
>>>  		fi
>>>  		;;
>>>  	gvimdiff2|vimdiff2)
>>>  		"$merge_tool_path" -f -d -c 'wincmd l' \
>>>  			"$LOCAL" "$MERGED" "$REMOTE"
>>>  		;;
>>>  	gvimdiff3|vimdiff3)
Junio C Hamano· Feb 10, 2016, 17:45 UTC · re: Michael J Gruber · lore

Re: [PATCH] mergetool: reorder vim/gvim buffers in three-way diffs

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 7 quoted lines
>> Second call for help.  Any comments on this from anybody other than
>> the author that I missed to support this change?
>
> OK, applied it (on top of next), looks sane and improves the situation
> for the majority of people who read left to right, then top down and
> assign buffers 1 2 3 4 "mentally" to local base remote merge windows
> based on that. Their expectation is met now. Thanks!
Thanks.

Does this mean that I should warn in the release notes that some existing users might get their expectation broken but we are going ahead anyway because we think most people read left to right and then top down? I am OK with saying that--I just wanted to make sure we know that it is what we are doing.

> (Also, the other vim variants don't need a corresponding change.)

A stupid question while I am here. What are these different variants? When reviewing this patch for the first time I tried to find where they are documented, but didn't spot anything.

I can see from the code that vimdiff2 variant does not do anything special when it is doing a 3-way merge, but vimdiff3 variant does behave differently when it has $BASE. It does not need a change like this because it already arranges and numbers the windows sensibly (in other words, we can label this patch as aligning the behaviour of vimdiff to that of vimdiff3)?

Thanks.
Show 35 quoted lines
> Michael
>
>>>
>>>>  mergetools/vimdiff | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
>>>> index 1ddfbfc..74ea6d5 100644
>>>> --- a/mergetools/vimdiff
>>>> +++ b/mergetools/vimdiff
>>>> @@ -2,22 +2,22 @@ diff_cmd () {
>>>>  	"$merge_tool_path" -R -f -d \
>>>>  		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
>>>>  }
>>>>  
>>>>  merge_cmd () {
>>>>  	touch "$BACKUP"
>>>>  	case "$1" in
>>>>  	gvimdiff|vimdiff)
>>>>  		if $base_present
>>>>  		then
>>>> -			"$merge_tool_path" -f -d -c 'wincmd J' \
>>>> -				"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
>>>> +			"$merge_tool_path" -f -d -c '4wincmd w | wincmd J' \
>>>> +				"$LOCAL" "$BASE" "$REMOTE" "$MERGED"
>>>>  		else
>>>>  			"$merge_tool_path" -f -d -c 'wincmd l' \
>>>>  				"$LOCAL" "$MERGED" "$REMOTE"
>>>>  		fi
>>>>  		;;
>>>>  	gvimdiff2|vimdiff2)
>>>>  		"$merge_tool_path" -f -d -c 'wincmd l' \
>>>>  			"$LOCAL" "$MERGED" "$REMOTE"
>>>>  		;;
>>>>  	gvimdiff3|vimdiff3)
Michael J Gruber· Feb 11, 2016, 15:44 UTC · re: Junio C Hamano · lore

Re: [PATCH] mergetool: reorder vim/gvim buffers in three-way diffs

Junio C Hamano venit, vidit, dixit 10.02.2016 18:45:
Show 17 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>>> Second call for help.  Any comments on this from anybody other than
>>> the author that I missed to support this change?
>>
>> OK, applied it (on top of next), looks sane and improves the situation
>> for the majority of people who read left to right, then top down and
>> assign buffers 1 2 3 4 "mentally" to local base remote merge windows
>> based on that. Their expectation is met now. Thanks!
> 
> Thanks.
> 
> Does this mean that I should warn in the release notes that some
> existing users might get their expectation broken but we are going
> ahead anyway because we think most people read left to right and
> then top down?  I am OK with saying that--I just wanted to make sure
> we know that it is what we are doing.

I would claim that anyone who notices the difference in buffer numbering would be positively surprised.

Show 8 quoted lines
>> (Also, the other vim variants don't need a corresponding change.)
> 
> A stupid question while I am here.  What are these different
> variants?  When reviewing this patch for the first time I tried to
> find where they are documented, but didn't spot anything.
> 
> I can see from the code that vimdiff2 variant does not do anything
> special when it is doing a 3-way merge,
It is "vimdiff" without the base window.
Show 5 quoted lines
> but vimdiff3 variant does
> behave differently when it has $BASE.  It does not need a change
> like this because it already arranges and numbers the windows
> sensibly (in other words, we can label this patch as aligning the
> behaviour of vimdiff to that of vimdiff3)?
git log mergetools/vimdiff3
commit 7c147b77d34f072c40b912fafba499727921fa6e
Author: Felipe Contreras <felipe.contreras@gmail.com>
Date:   Sun Apr 20 19:24:20 2014 -0500
    mergetools: add vimdiff3 mode
    It's similar to the default, except that the other windows are hidden.
    This ensures that removed/added colors are still visible on the main
    merge window, but the other windows not visible.
    Specially useful with merge.conflictstyle=diff3.

I have to say I'm still not sure what it is about (even after trying it out, even with the conflictstyle config)).

In any case, the buffer numbering is not the same (it is local remote base merge) but it doesn't matter in this case because only one window is displayed, so there is no visual association.

Michael
Junio C Hamano· Feb 11, 2016, 16:03 UTC · re: Michael J Gruber · lore

Re: [PATCH] mergetool: reorder vim/gvim buffers in three-way diffs

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 8 quoted lines
>> Does this mean that I should warn in the release notes that some
>> existing users might get their expectation broken but we are going
>> ahead anyway because we think most people read left to right and
>> then top down?  I am OK with saying that--I just wanted to make sure
>> we know that it is what we are doing.
>
> I would claim that anyone who notices the difference in buffer numbering
> would be positively surprised.

Thanks. I, being a non-user of vim, was wondering if people who had their own user-defined commands (macros? and possibly short-cut keys to invoke them) built around the old (and odd) numbering need to adjust--in which case we may need to forewarn.

> In any case, the buffer numbering is not the same (it is local remote
> base merge) but it doesn't matter in this case because only one window
> is displayed, so there is no visual association.
OK, thanks.
Dickson Wong· Feb 11, 2016, 19:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] mergetool: reorder vim/gvim buffers in three-way diffs

Thanks for looking into the patch.
Show 5 quoted lines
> Thanks. I, being a non-user of vim, was wondering if people who had
> their own user-defined commands (macros? and possibly short-cut keys
> to invoke them) built around the old (and odd) numbering need to
> adjust--in which case we may need to forewarn.
>

Vim buffers are not reorderable and windows are always numbered from top-left to bottom-right. However, I can see someone who is familiar with the old behavior issue a "CTRL-W_K" command to move the merged window from the bottom to the top, making the merged window the first window and consistent with buffer indexes. If they use this command after the patch, the indexes would no longer be consistent. The command would most likely be issued manually and not through a vim startup script.

Junio C Hamano· Feb 11, 2016, 19:31 UTC · re: Dickson Wong · lore

Re: [PATCH] mergetool: reorder vim/gvim buffers in three-way diffs

Dickson Wong <dicksonwong@gmail.com> writes:
> Thanks for looking into the patch.
No.  Thank _you_ for contributing ;-)
Show 7 quoted lines
> Vim buffers are not reorderable and windows are always numbered from
> top-left to bottom-right. However, I can see someone who is familiar with
> the old behavior issue a "CTRL-W_K" command to move the merged window from
> the bottom to the top, making the merged window the first window and
> consistent with buffer indexes. If they use this command after the patch,
> the indexes would no longer be consistent. The command would most likely be
> issued manually and not through a vim startup script.

So it should suffice to mention it in the release notes as one bullet item that describes one positive change, among all other updates described in a simlar way. And there is no special "warnings for existing users" necessary, I would say.

Thanks.
Dickson Wong· Feb 11, 2016, 20:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] mergetool: reorder vim/gvim buffers in three-way diffs

On 11 February 2016 at 11:31, Junio C Hamano <gitster@pobox.com> wrote:
> So it should suffice to mention it in the release notes as one
> bullet item that describes one positive change, among all other
> updates described in a simlar way.  And there is no special
> "warnings for existing users" necessary, I would say.

It's unlikely anyone would move the merged window to the top as a preference only without being aware of the original inconsistency. If someone has been using a workaround, it would be fairly obvious as to what has changed, and definitely more so than when they first encountered the original behavior. Any adjustments to the new behavior would probably be a workflow change rather than a system configuration or scripting change.

David Aguilar· Feb 11, 2016, 21:15 UTC · re: Junio C Hamano · lore

Re: [PATCH] mergetool: reorder vim/gvim buffers in three-way diffs

On Thu, Feb 11, 2016 at 08:03:57AM -0800, Junio C Hamano wrote:
Show 21 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
> >> Does this mean that I should warn in the release notes that some
> >> existing users might get their expectation broken but we are going
> >> ahead anyway because we think most people read left to right and
> >> then top down?  I am OK with saying that--I just wanted to make sure
> >> we know that it is what we are doing.
> >
> > I would claim that anyone who notices the difference in buffer numbering
> > would be positively surprised.
> 
> Thanks. I, being a non-user of vim, was wondering if people who had
> their own user-defined commands (macros? and possibly short-cut keys
> to invoke them) built around the old (and odd) numbering need to
> adjust--in which case we may need to forewarn.
> 
> > In any case, the buffer numbering is not the same (it is local remote
> > base merge) but it doesn't matter in this case because only one window
> > is displayed, so there is no visual association.
> 
> OK, thanks.

Sorry for not noticing this thread earlier. The change and the rationale sound good to me.

FWIW,
Acked-by: David Aguilar <davvid@gmail.com>
ciao,
-- 
David
Junio C Hamano· Feb 12, 2016, 18:15 UTC · re: David Aguilar · lore

Re: [PATCH] mergetool: reorder vim/gvim buffers in three-way diffs

David Aguilar <davvid@gmail.com> writes:
Show 29 quoted lines
> On Thu, Feb 11, 2016 at 08:03:57AM -0800, Junio C Hamano wrote:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> 
>> >> Does this mean that I should warn in the release notes that some
>> >> existing users might get their expectation broken but we are going
>> >> ahead anyway because we think most people read left to right and
>> >> then top down?  I am OK with saying that--I just wanted to make sure
>> >> we know that it is what we are doing.
>> >
>> > I would claim that anyone who notices the difference in buffer numbering
>> > would be positively surprised.
>> 
>> Thanks. I, being a non-user of vim, was wondering if people who had
>> their own user-defined commands (macros? and possibly short-cut keys
>> to invoke them) built around the old (and odd) numbering need to
>> adjust--in which case we may need to forewarn.
>> 
>> > In any case, the buffer numbering is not the same (it is local remote
>> > base merge) but it doesn't matter in this case because only one window
>> > is displayed, so there is no visual association.
>> 
>> OK, thanks.
>
> Sorry for not noticing this thread earlier.
> The change and the rationale sound good to me.
>
> FWIW,
>
> Acked-by: David Aguilar <davvid@gmail.com>
Thanks all.  Will merge to 'next' and then to 'master' shortly.

← back to recent threads