threads / rfc / 56741

RFC patchmergetools/vimdiff: add vimdiff4 merge tool variant

Subject: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant

## tl;dr

6 messages between Oct 19, 2021 and Oct 27, 2021. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Fernando Ramos· Oct 19, 2021, 21:20 UTC · lore
This new vimdiff4 variant of the merge-tool opens three tabs:
  - The first one contains the same panes as the standard "vimdiff" (ie.
    LOCAL, BASE and REMOTE in the top row and MERGED in the bottom row).
      ------------------------------------------
      | <TAB #1> |  TAB #2  |  TAB #3  |       |
      ------------------------------------------
      |             |           |              |
      |   LOCAL     |   BASE    |   REMOTE     |
      |             |           |              |
      ------------------------------------------
      |                                        |
      |                MERGED                  |
      |                                        |
      ------------------------------------------
      NOTE: This view is enough for 90% of the cases, but when the merge is
            somewhat complex, the three-way differences representation
            end up being messy. That is why two new tabs are added to
            show isolated one-to-one diffs.
  - The second one is a vertical diff between BASE and LOCAL
      ------------------------------------------
      |  TAB #1  | <TAB #2> |  TAB #3  |       |
      ------------------------------------------
      |                   |                    |
      |                   |                    |
      |                   |                    |
      |     BASE          |    LOCAL           |
      |                   |                    |
      |                   |                    |
      |                   |                    |
      ------------------------------------------
  - The third one is a vertical diff between BASE and REMOTE
      ------------------------------------------
      |  TAB #1  |  TAB #2  | <TAB #3> |       |
      ------------------------------------------
      |                   |                    |
      |                   |                    |
      |                   |                    |
      |     BASE          |    REMOTE          |
      |                   |                    |
      |                   |                    |
      |                   |                    |
      ------------------------------------------
Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
---
 mergetools/vimdiff   | 12 +++++++++++-
 t/t7610-mergetool.sh |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)
Show changes to 2 files +12 −1

mergetools/vimdiff, t/t7610-mergetool.sh

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 96f6209a04..f830b1ed95 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -40,6 +40,16 @@ merge_cmd () {
 				"$LOCAL" "$REMOTE" "$MERGED"
 		fi
 		;;
+	*vimdiff4)
+		if $base_present
+		then
+			"$merge_tool_path" -f -d -c "4wincmd w | wincmd J | tabnew | edit $LOCAL | vertical diffsplit $BASE | tabnew | edit $REMOTE | vertical diffsplit $BASE | 2tabprevious" \
+				"$LOCAL" "$BASE" "$REMOTE" "$MERGED"
+		else
+			"$merge_tool_path" -f -d -c 'wincmd l' \
+				"$LOCAL" "$MERGED" "$REMOTE"
+		fi
+		;;
 	esac
 }
 
@@ -63,7 +73,7 @@ exit_code_trustable () {
 
 list_tool_variants () {
 	for prefix in '' g n; do
-		for suffix in '' 1 2 3; do
+		for suffix in '' 1 2 3 4; do
 			echo "${prefix}vimdiff${suffix}"
 		done
 	done
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 8cc64729ad..755b4c0a4a 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -836,6 +836,7 @@ test_expect_success 'mergetool --tool-help shows recognized tools' '
 	git mergetool --tool-help >mergetools &&
 	grep vimdiff mergetools &&
 	grep vimdiff3 mergetools &&
+	grep vimdiff4 mergetools &&
 	grep gvimdiff2 mergetools &&
 	grep araxis mergetools &&
 	grep xxdiff mergetools &&
-- 
2.33.1
Fernando Ramos· Oct 19, 2021, 22:12 UTC · re: Fernando Ramos · lore

Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant

On 21/10/19 11:20PM, Fernando Ramos wrote:
> This new vimdiff4 variant of the merge-tool opens three tabs...
> 

One reason why this RFC should *not* be merged is that the same functionality can be achieved right now by simply adding these two lines to "~/.config/git/config":

  [mergetool "vimdiff4"]
  	cmd = vim -f -d -c \"4wincmd w | wincmd J | tabnew | edit $LOCAL | vertical diffsplit $BASE | tabnew | edit $REMOTE | vertical diffsplit $BASE | 2tabprevious\" \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"
  	trustExitCode = true

On the other hand, one reason to merge it is that this new windows/tabs arrangement is so useful when solving complex merges, that it would be nice to have it available by default for easier discoverablility.

Let me know what you think :)
David Aguilar· Oct 24, 2021, 22:54 UTC · re: Fernando Ramos · lore

Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant

On Tue, Oct 19, 2021 at 2:22 PM Fernando Ramos <greenfoo@u92.eu> wrote:
Show 56 quoted lines
>
> This new vimdiff4 variant of the merge-tool opens three tabs:
>
>   - The first one contains the same panes as the standard "vimdiff" (ie.
>     LOCAL, BASE and REMOTE in the top row and MERGED in the bottom row).
>
>       ------------------------------------------
>       | <TAB #1> |  TAB #2  |  TAB #3  |       |
>       ------------------------------------------
>       |             |           |              |
>       |   LOCAL     |   BASE    |   REMOTE     |
>       |             |           |              |
>       ------------------------------------------
>       |                                        |
>       |                MERGED                  |
>       |                                        |
>       ------------------------------------------
>
>       NOTE: This view is enough for 90% of the cases, but when the merge is
>             somewhat complex, the three-way differences representation
>             end up being messy. That is why two new tabs are added to
>             show isolated one-to-one diffs.
>
>   - The second one is a vertical diff between BASE and LOCAL
>
>       ------------------------------------------
>       |  TAB #1  | <TAB #2> |  TAB #3  |       |
>       ------------------------------------------
>       |                   |                    |
>       |                   |                    |
>       |                   |                    |
>       |     BASE          |    LOCAL           |
>       |                   |                    |
>       |                   |                    |
>       |                   |                    |
>       ------------------------------------------
>
>   - The third one is a vertical diff between BASE and REMOTE
>
>       ------------------------------------------
>       |  TAB #1  |  TAB #2  | <TAB #3> |       |
>       ------------------------------------------
>       |                   |                    |
>       |                   |                    |
>       |                   |                    |
>       |     BASE          |    REMOTE          |
>       |                   |                    |
>       |                   |                    |
>       |                   |                    |
>       ------------------------------------------
>
> Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
> ---
>  mergetools/vimdiff   | 12 +++++++++++-
>  t/t7610-mergetool.sh |  1 +
>  2 files changed, 12 insertions(+), 1 deletion(-)

Thanks for including the visual diagrams (which I hope gmail doesn't mangle). That makes it much easier to see what's going on.

I'm personally not opposed to the vimdiff4 variants (we already have 3 others) but what I think might be missing is a bit of documentation that documents the builtin tools and their variants.

Right now git-mergetool.txt includes config/mergetool.txt for documenting its config variables. It might be worth having a common "mergetools.txt" where the builtin tools and variants can be documented and then we can include that file from both git-mergetool.txt and git-difftool.txt.

That would be a good place to write up the differences between the variants, and the diagram you included in the commit message would be helpful there as well.

Show 21 quoted lines
>
> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
> index 96f6209a04..f830b1ed95 100644
> --- a/mergetools/vimdiff
> +++ b/mergetools/vimdiff
> @@ -40,6 +40,16 @@ merge_cmd () {
>                                 "$LOCAL" "$REMOTE" "$MERGED"
>                 fi
>                 ;;
> +       *vimdiff4)
> +               if $base_present
> +               then
> +                       "$merge_tool_path" -f -d -c "4wincmd w | wincmd J | tabnew | edit $LOCAL | vertical diffsplit $BASE | tabnew | edit $REMOTE | vertical diffsplit $BASE | 2tabprevious" \
> +                               "$LOCAL" "$BASE" "$REMOTE" "$MERGED"
> +               else
> +                       "$merge_tool_path" -f -d -c 'wincmd l' \
> +                               "$LOCAL" "$MERGED" "$REMOTE"
> +               fi
> +               ;;
>         esac
>  }

It's pretty rad how we're able to get that much vim goodness out of this snippet of configuration.

There seems to be an issue here, though. The $LOCAL values are passed to the "edit $LOCAL", "edit $REMOTE" and "vertical diffsplit $BASE" commands as-is. It seems like this would break when the filenames contain spaces. Is that correct?

If so, does vimscript have a way to quote those arguments? Does surrounding the variable with escaped double-quotes ("... | edit \"$LOCAL\" | ...") work? (... for everything except files with embedded double-quotes in their name, which might be an acceptable limitation).

Show 7 quoted lines
>
> @@ -63,7 +73,7 @@ exit_code_trustable () {
>
>  list_tool_variants () {
>         for prefix in '' g n; do
> -               for suffix in '' 1 2 3; do
> +               for suffix in '' 1 2 3 4; do

Pre-existing, but we typically try to avoid multiple statements on a single line. It seems worth fixing this up in a preparatory patch since we're touching these lines.

for prefix in '' g n
do
    for suffix in '' 1 2 3 4
    do
        ...
    done
done
Show 15 quoted lines
>                         echo "${prefix}vimdiff${suffix}"
>                 done
>         done
> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index 8cc64729ad..755b4c0a4a 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -836,6 +836,7 @@ test_expect_success 'mergetool --tool-help shows recognized tools' '
>         git mergetool --tool-help >mergetools &&
>         grep vimdiff mergetools &&
>         grep vimdiff3 mergetools &&
> +       grep vimdiff4 mergetools &&
>         grep gvimdiff2 mergetools &&
>         grep araxis mergetools &&
>         grep xxdiff mergetools &&

Looks good otherwise, thanks for the RFC patch. I'd recommend getting the docs and quoting stuff sorted out as the next step towards getting this merged.

Thanks!

-- David

Junio C Hamano· Oct 25, 2021, 18:18 UTC · re: David Aguilar · lore

Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant

David Aguilar <davvid@gmail.com> writes:
> I'm personally not opposed to the vimdiff4 variants (we already have 3
> others) but what I think might be missing is a bit of documentation
> that documents the builtin tools and their variants.

Hmph, are we encouraging everybody to add yet another variant? I wonder if we can stop at adding a single "vimdiffX" variant that takes the layout information (like the one this vimdiff4 passes to the underlying tool via the command line option) in a configuration variable and stop adding more variants, or is vim's specification of the layout we use here via the command line not flexible enough to serve all future needs? I also wonder if all the existing vimdiff variants can be done in terms of such a vimdiffX implementation.

Show 9 quoted lines
> Right now git-mergetool.txt includes config/mergetool.txt for
> documenting its config variables. It might be worth having a common
> "mergetools.txt" where the builtin tools and variants can be
> documented and then we can include that file from both
> git-mergetool.txt and git-difftool.txt.
>
> That would be a good place to write up the differences between the
> variants, and the diagram you included in the commit message would be
> helpful there as well.
Yup, in any case, I do like the suggestion to document the variants.
Thanks, both.
Fernando Ramos· Oct 25, 2021, 20:13 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant

Thanks David and Juno for your feedback.
I completely agree that adding another vimdiffX variant is ... not elegant.

So I've been thinking a bit more about how this whole "vim layout" mechanism can be made more generic and this is what I came up with:

  1. Let's add a new configuration variable to the "vimdiff" merge tool called
     "layout":
         [mergetool "vimdiff"]
         layout = ...
  2. If this new variable is *not* present, vim will behave in the same way it
     does today (ie. a top row with the local, base and remote buffers, and a
     bottom row with the merged buffer).
  3. In all other cases, the contents of the "layout" variable will be
     intepreted following these rules:
       - ";" is used to separate "tab descriptors"
       - "," is used to separate "row descriptors"
       - "|" is used to separate "column descriptors"
       - "+" is used to load buffers that won't be displayed by default
     This will be better understood with some examples that emulate the behavior
     of the current "vimdiff", "vimdiff1", "vimdiff2" and "vimdiff3" variants as
     well as the proposed "vimdiff4" one:
       vimdiff  --> layout = "LOCAL | BASE | REMOTE, MERGED"
           ------------------------------------------
           |             |           |              |
           |   LOCAL     |   BASE    |   REMOTE     |
           |             |           |              |
           ------------------------------------------
           |                                        |
           |                MERGED                  |
           |                                        |
           ------------------------------------------
       vimdiff1 --> layout = "LOCAL* | REMOTE"
           ------------------------------------------
           |                   |                    |
           |                   |                    |
           |                   |                    |
           |     LOCAL         |    REMOTE          |
           |                   |                    |
           |                   |                    |
           |                   |                    |
           ------------------------------------------
           NOTE: In this case (where there is no "MERGED"
           buffer specified in the "layout" string), a "*"
           is needed to indicate which file will be the one
           containing the final version of the file after
           resolving conflicts.
       vimdiff2 --> layout = "LOCAL | MERGED | REMOTE"
           ------------------------------------------
           |             |           |              |
           |             |           |              |
           |             |           |              |
           |   LOCAL     |   BASE    |   REMOTE     |
           |             |           |              |
           |             |           |              |
           |             |           |              |
           |             |           |              |
           ------------------------------------------
       vimdiff3 --> layout = "LOCAL + REMOTE + BASE + MERGED"
           ------------------------------------------
           |                                        |
           |                                        |
           |                                        |
           |               MERGED                   |
           |                                        |
           |                                        |
           |                                        |
           ------------------------------------------
           NOTE: LOCAL, REMOTE and BASE are loaded as hidden
           buffers and you need to recall them explicitely.
       vimdiff4 --> layout = "BASE | LOCAL | REMOTE, MERGED; BASE | LOCAL; BASE | REMOTE"
 
           ------------------------------------------
           | <TAB #1> |  TAB #2  |  TAB #3  |       |
           ------------------------------------------
           |             |           |              |
           |   LOCAL     |   BASE    |   REMOTE     |
           |             |           |              |
           ------------------------------------------
           |                                        |
           |                MERGED                  |
           |                                        |
           ------------------------------------------
           ------------------------------------------
           |  TAB #1  | <TAB #2> |  TAB #3  |       |
           ------------------------------------------
           |                   |                    |
           |                   |                    |
           |                   |                    |
           |     BASE          |    LOCAL           |
           |                   |                    |
           |                   |                    |
           |                   |                    |
           ------------------------------------------
           ------------------------------------------
           |  TAB #1  |  TAB #2  | <TAB #3> |       |
           ------------------------------------------
           |                   |                    |
           |                   |                    |
           |                   |                    |
           |     BASE          |    REMOTE          |
           |                   |                    |
           |                   |                    |
           |                   |                    |
           ------------------------------------------

The nice thing about this approach is that, as we have seen, it is generic enough to rule all current variants obsolete.

So, please let me know what you think about this:
  * Do you like this approach? Or am I trying to crack a nut with a sledgehammer
    by making the whole thing too complex?
  * In case you like it, should we keep the old "vimdiff1", "vimdiff2" and
    "vimdiff3" variants for backwards compatibility?
    If the answer is "yes", I'll just alias them to the new "layout" mechanism
    so that the amount of extra code needed for supporting them is minimal.

If you tell me you like this proposal, I'll go ahead and implement a patch for all of this, taking into consideration David's suggestions for avoiding problems with file with spaces in their names and also adding new documentation for all of this.

    NOTE: The only non-trivial thing about implementing this is how to parse the
    "layout" variable syntax *in bash* to convert it into a sequence of vim
    commands that achieves the expected outcome... but seems like a funny
    weekend project :)

If you think it is not worth the effort, let me know if it is OK to just add "vimdiff4" + documentation instead for now (or something else).

Thanks.
   
Junio C Hamano· Oct 27, 2021, 16:46 UTC · re: Fernando Ramos · lore

Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant

Fernando Ramos <greenfoo@u92.eu> writes:
> The nice thing about this approach is that, as we have seen, it is generic
> enough to rule all current variants obsolete.

I am not a vimdiff user (or mergetools in general---I should use it as appropriately from time to time myself), but it would be great if such a general "layout rule language" can be used to replicate the existing variants.

Show 9 quoted lines
> So, please let me know what you think about this:
>
>   * Do you like this approach? Or am I trying to crack a nut with a sledgehammer
>     by making the whole thing too complex?
>
>   * In case you like it, should we keep the old "vimdiff1", "vimdiff2" and
>     "vimdiff3" variants for backwards compatibility?
>     If the answer is "yes", I'll just alias them to the new "layout" mechanism
>     so that the amount of extra code needed for supporting them is minimal.

It is great that the new "description based layout" can replicate the existing variants, and it is natural migration path to redo the existing variants with the new mechanism once the dust settles as a separate step. The end-users should be able to rely on their existing configuration to keep working the same way as they are used to.

Thanks.

← back to recent threads