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

From: David Aguilar <davvid@gmail.com>
Date: Oct 24, 2021, 22:54 UTC
Message-ID: <CAJDDKr5frTgh4_x5yvskJfppew3ntvpgBe9MnUB9CfGQaw1TLQ@mail.gmail.com>
In-Reply-To: <20211019212020.25385-1-greenfoo@u92.eu>

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