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

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

[Dickson Wong]

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

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

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

[Junio C Hamano]

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?

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

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

[Junio C Hamano]

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?

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

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 09.02.2016 23:25:
> 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

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

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

[Junio C Hamano]

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.

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

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

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 10.02.2016 18:45:
> 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.

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

> 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

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

[Junio C Hamano]

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.

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

[Dickson Wong]

Thanks for looking into the patch.

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

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

[Junio C Hamano]

Dickson Wong <dicksonwong@gmail.com> writes:

> Thanks for looking into the patch.

No.  Thank _you_ for contributing ;-)

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

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

[Dickson Wong]

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.

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

[David Aguilar]

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>


ciao,
-- 
David

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

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