diff mbox series

[v3,7/7] mergetools: vimdiff: restore selective diff mode

Message ID 20220809004549.123020-8-felipe.contreras@gmail.com (mailing list archive)
State Superseded
Headers show
Series mergetools: vimdiff: regression fixes and improvements | expand

Commit Message

Felipe Contreras Aug. 9, 2022, 12:45 a.m. UTC 
Apparently some people want the diff mode to show differences only on
the visible windows, so turn this on only when the tab has more than one
window.

This should probably be configurable.

Cc: Fernando Ramos <greenfoo@u92.eu>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 50 ++++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 19 deletions(-)

Comments

Felipe Contreras Aug. 9, 2022, 1:08 a.m. UTC | #1 
On Mon, Aug 8, 2022 at 7:46 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Apparently some people want the diff mode to show differences only on
> the visible windows, so turn this on only when the tab has more than one
> window.

> @@ -323,8 +323,20 @@ gen_cmd () {
>         IFS=+
>         for tab in $LAYOUT
>         do
> -               test -n "$CMD" && CMD="$CMD | tabnew | silent execute 'bufdo diffthis'"
> -               CMD=$(gen_cmd_aux "$tab" "$CMD")
> +               if echo "$tab" | grep ",\|/" >/dev/null
> +               then
> +                       test -n "$CMD" && CMD="$CMD | tabnew"
> +                       CMD=$(gen_cmd_aux "$tab" "$CMD")
> +                       CMD="$CMD | execute 'windo diffthis'"
> +               else
> +                       if test -z "$CMD"
> +                       then
> +                               CMD="silent execute 'bufdo diffthis'"
> +                       else
> +                               CMD="$CMD | tabnew | silent execute 'bufdo diffthis'"
> +                       fi
> +                       CMD=$(gen_cmd_aux "$tab" "$CMD")
> +               fi
>         done
>         IFS=$oldIFS
>

Notice that after reorganizing the tab handling my layout becomes really simple:

  set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | 4b

And your layout becomes:

  set hidden diffopt-=hiddenoff | echo | leftabove vertical split | 2b
| wincmd l | 1b | execute 'windo diffthis' | tabnew | leftabove
vertical split | 2b | wincmd l | 3b | execute 'windo diffthis'

So this "works" too, right?

Cheers.
Fernando Ramos Aug. 9, 2022, 5:26 a.m. UTC | #2 
On 22/08/08 08:08PM, Felipe Contreras wrote:
> 
> Notice that after reorganizing the tab handling my layout becomes really simple:
> 
>   set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | 4b
>

Thanks! I really like this new approach.

I also manually tested your new series (with and without .vimrc) and it works as
expected in all listed test cases.

This last series gets the best of both worlds:

  - Highglihting works in single window tabs even when there is just one tab
  
  - Single tab layouts are not treated differently

  - Generates shorter (easier to understand!) vim command strings (nice!)

  - Opens the gate for a future configuration option that lets you enable "all
    buffers diff mode" even if they are not visible in a given tab (maybe this
    could be a new syntax token, *, that "marks" a tab to work in this mode?)

So, definitely a great work. Thanks for the deeper look at the problem and this
brilliant solution :)
Felipe Contreras Aug. 9, 2022, 8:07 p.m. UTC | #3 
On Tue, Aug 9, 2022 at 12:26 AM Fernando Ramos <greenfoo@u92.eu> wrote:
>
> On 22/08/08 08:08PM, Felipe Contreras wrote:
> >
> > Notice that after reorganizing the tab handling my layout becomes really simple:
> >
> >   set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | 4b
>
> Thanks! I really like this new approach.
>
> I also manually tested your new series (with and without .vimrc) and it works as
> expected in all listed test cases.
>
> This last series gets the best of both worlds:
>
>   - Highglihting works in single window tabs even when there is just one tab
>
>   - Single tab layouts are not treated differently
>
>   - Generates shorter (easier to understand!) vim command strings (nice!)
>
>   - Opens the gate for a future configuration option that lets you enable "all
>     buffers diff mode" even if they are not visible in a given tab (maybe this
>     could be a new syntax token, *, that "marks" a tab to work in this mode?)
>
> So, definitely a great work. Thanks for the deeper look at the problem and this
> brilliant solution :)

It probably can be cleaned up a bit more, but the important thing is
the idea: parse tabs especially (which they are anyway).

Now if only the maintainer cared about fixing the regression that
would be great.
Junio C Hamano Aug. 10, 2022, 8:45 a.m. UTC | #4 
Fernando Ramos <greenfoo@u92.eu> writes:

> On 22/08/08 08:08PM, Felipe Contreras wrote:
>> 
>> Notice that after reorganizing the tab handling my layout becomes really simple:
>> 
>>   set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | 4b
>>
>
> Thanks! I really like this new approach.
>
> I also manually tested your new series (with and without .vimrc) and it works as
> expected in all listed test cases.
>
> This last series gets the best of both worlds:
>
>   - Highglihting works in single window tabs even when there is just one tab
>   
>   - Single tab layouts are not treated differently
>
>   - Generates shorter (easier to understand!) vim command strings (nice!)
>
>   - Opens the gate for a future configuration option that lets you enable "all
>     buffers diff mode" even if they are not visible in a given tab (maybe this
>     could be a new syntax token, *, that "marks" a tab to work in this mode?)
>
> So, definitely a great work. Thanks for the deeper look at the problem and this
> brilliant solution :)

Is that a "Reviewed-by:" I should add while queuing these 7 patches?

Thanks, both.
Junio C Hamano Aug. 10, 2022, 6:29 p.m. UTC | #5 
Junio C Hamano <gitster@pobox.com> writes:

>> So, definitely a great work. Thanks for the deeper look at the problem and this
>> brilliant solution :)
>
> Is that a "Reviewed-by:" I should add while queuing these 7 patches?

Ah, sorry, scratch that.  There is a newer round, and that is the
one that can use your input.

> Thanks, both.
diff mbox series

Patch

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 8029be0975..17921b6ba9 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -68,7 +68,7 @@  gen_cmd_aux () {
 
 	if test -z "$CMD"
 	then
-		CMD="silent execute 'bufdo diffthis'"
+		CMD="echo"
 	fi
 
 	start=0
@@ -323,8 +323,20 @@  gen_cmd () {
 	IFS=+
 	for tab in $LAYOUT
 	do
-		test -n "$CMD" && CMD="$CMD | tabnew | silent execute 'bufdo diffthis'"
-		CMD=$(gen_cmd_aux "$tab" "$CMD")
+		if echo "$tab" | grep ",\|/" >/dev/null
+		then
+			test -n "$CMD" && CMD="$CMD | tabnew"
+			CMD=$(gen_cmd_aux "$tab" "$CMD")
+			CMD="$CMD | execute 'windo diffthis'"
+		else
+			if test -z "$CMD"
+			then
+				CMD="silent execute 'bufdo diffthis'"
+			else
+				CMD="$CMD | tabnew | silent execute 'bufdo diffthis'"
+			fi
+			CMD=$(gen_cmd_aux "$tab" "$CMD")
+		fi
 	done
 	IFS=$oldIFS
 
@@ -537,22 +549,22 @@  run_unit_tests () {
 	TEST_CASE_15="  ((  (LOCAL , BASE , REMOTE) / MERGED))   +(BASE)   , LOCAL+ BASE , REMOTE+ (((LOCAL / BASE / REMOTE)) ,    MERGED   )  "
 	TEST_CASE_16="LOCAL,BASE,REMOTE / MERGED + BASE,LOCAL + BASE,REMOTE + (LOCAL / BASE / REMOTE),MERGED"
 
-	EXPECTED_CMD_01="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b\" -c \"tabfirst\""
-	EXPECTED_CMD_02="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove vertical split | 1b | wincmd l | 3b\" -c \"tabfirst\""
-	EXPECTED_CMD_03="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b\" -c \"tabfirst\""
+	EXPECTED_CMD_01="-c \"set hidden diffopt-=hiddenoff | echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_02="-c \"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | 1b | wincmd l | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_03="-c \"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
 	EXPECTED_CMD_04="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | 4b\" -c \"tabfirst\""
-	EXPECTED_CMD_05="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b\" -c \"tabfirst\""
-	EXPECTED_CMD_06="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b\" -c \"tabfirst\""
-	EXPECTED_CMD_07="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b\" -c \"tabfirst\""
-	EXPECTED_CMD_08="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 4b\" -c \"tabfirst\""
-	EXPECTED_CMD_09="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove split | 4b | wincmd j | leftabove vertical split | 1b | wincmd l | 3b\" -c \"tabfirst\""
-	EXPECTED_CMD_10="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b\" -c \"tabfirst\""
-	EXPECTED_CMD_11="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnew | silent execute 'bufdo diffthis' | leftabove vertical split | 2b | wincmd l | 1b | tabnew | silent execute 'bufdo diffthis' | leftabove vertical split | 2b | wincmd l | 3b | tabnew | silent execute 'bufdo diffthis' | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b\" -c \"tabfirst\""
-	EXPECTED_CMD_12="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | 4b\" -c \"tabfirst\""
-	EXPECTED_CMD_13="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b\" -c \"tabfirst\""
-	EXPECTED_CMD_14="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove vertical split | 2b | wincmd l | 3b | tabnew | silent execute 'bufdo diffthis' | leftabove vertical split | 2b | wincmd l | 1b\" -c \"tabfirst\""
-	EXPECTED_CMD_15="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnew | silent execute 'bufdo diffthis' | leftabove vertical split | 2b | wincmd l | 1b | tabnew | silent execute 'bufdo diffthis' | leftabove vertical split | 2b | wincmd l | 3b | tabnew | silent execute 'bufdo diffthis' | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b\" -c \"tabfirst\""
-	EXPECTED_CMD_16="-c \"set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnew | silent execute 'bufdo diffthis' | leftabove vertical split | 2b | wincmd l | 1b | tabnew | silent execute 'bufdo diffthis' | leftabove vertical split | 2b | wincmd l | 3b | tabnew | silent execute 'bufdo diffthis' | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b\" -c \"tabfirst\""
+	EXPECTED_CMD_05="-c \"set hidden diffopt-=hiddenoff | echo | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_06="-c \"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_07="-c \"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_08="-c \"set hidden diffopt-=hiddenoff | echo | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_09="-c \"set hidden diffopt-=hiddenoff | echo | leftabove split | 4b | wincmd j | leftabove vertical split | 1b | wincmd l | 3b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_10="-c \"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_11="-c \"set hidden diffopt-=hiddenoff | echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis' | tabnew | leftabove vertical split | 2b | wincmd l | 1b | execute 'windo diffthis' | tabnew | leftabove vertical split | 2b | wincmd l | 3b | execute 'windo diffthis' | tabnew | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_12="-c \"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_13="-c \"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_14="-c \"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | 2b | wincmd l | 3b | execute 'windo diffthis' | tabnew | leftabove vertical split | 2b | wincmd l | 1b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_15="-c \"set hidden diffopt-=hiddenoff | echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis' | tabnew | leftabove vertical split | 2b | wincmd l | 1b | execute 'windo diffthis' | tabnew | leftabove vertical split | 2b | wincmd l | 3b | execute 'windo diffthis' | tabnew | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
+	EXPECTED_CMD_16="-c \"set hidden diffopt-=hiddenoff | echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'windo diffthis' | tabnew | leftabove vertical split | 2b | wincmd l | 1b | execute 'windo diffthis' | tabnew | leftabove vertical split | 2b | wincmd l | 3b | execute 'windo diffthis' | tabnew | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'windo diffthis'\" -c \"tabfirst\""
 
 	EXPECTED_TARGET_01="MERGED"
 	EXPECTED_TARGET_02="LOCAL"
@@ -617,7 +629,7 @@  run_unit_tests () {
 	cat >expect <<-\EOF
 	-f
 	-c
-	set hidden diffopt-=hiddenoff | silent execute 'bufdo diffthis' | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | quit | wincmd l | 2b | wincmd j | 3b
+	set hidden diffopt-=hiddenoff | echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | quit | wincmd l | 2b | wincmd j | 3b | execute 'windo diffthis'
 	-c
 	tabfirst
 	lo cal