* [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant
@ 2021-10-19 21:20 Fernando Ramos
2021-10-19 22:12 ` Fernando Ramos
2021-10-24 22:54 ` David Aguilar
0 siblings, 2 replies; 6+ messages in thread
From: Fernando Ramos @ 2021-10-19 21:20 UTC (permalink / raw)
To: git; +Cc: gitster, seth, levraiphilippeblain, rogi
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant
2021-10-19 21:20 [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant Fernando Ramos
@ 2021-10-19 22:12 ` Fernando Ramos
2021-10-24 22:54 ` David Aguilar
1 sibling, 0 replies; 6+ messages in thread
From: Fernando Ramos @ 2021-10-19 22:12 UTC (permalink / raw)
To: git; +Cc: gitster, seth, levraiphilippeblain, rogi
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 :)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant
2021-10-19 21:20 [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant Fernando Ramos
2021-10-19 22:12 ` Fernando Ramos
@ 2021-10-24 22:54 ` David Aguilar
2021-10-25 18:18 ` Junio C Hamano
1 sibling, 1 reply; 6+ messages in thread
From: David Aguilar @ 2021-10-24 22:54 UTC (permalink / raw)
To: Fernando Ramos
Cc: Git Mailing List, Junio C Hamano, Seth House, levraiphilippeblain, rogi
On Tue, Oct 19, 2021 at 2:22 PM Fernando Ramos <greenfoo@u92.eu> wrote:
>
> 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.
>
> 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).
>
> @@ -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
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant
2021-10-24 22:54 ` David Aguilar
@ 2021-10-25 18:18 ` Junio C Hamano
2021-10-25 20:13 ` Fernando Ramos
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2021-10-25 18:18 UTC (permalink / raw)
To: David Aguilar
Cc: Fernando Ramos, Git Mailing List, Seth House, levraiphilippeblain, rogi
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.
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant
2021-10-25 18:18 ` Junio C Hamano
@ 2021-10-25 20:13 ` Fernando Ramos
2021-10-27 16:46 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Fernando Ramos @ 2021-10-25 20:13 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Aguilar, Git Mailing List, Seth House, levraiphilippeblain, rogi
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant
2021-10-25 20:13 ` Fernando Ramos
@ 2021-10-27 16:46 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2021-10-27 16:46 UTC (permalink / raw)
To: Fernando Ramos
Cc: David Aguilar, Git Mailing List, Seth House, levraiphilippeblain, rogi
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.
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-27 16:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 21:20 [RFC PATCH] mergetools/vimdiff: add vimdiff4 merge tool variant Fernando Ramos
2021-10-19 22:12 ` Fernando Ramos
2021-10-24 22:54 ` David Aguilar
2021-10-25 18:18 ` Junio C Hamano
2021-10-25 20:13 ` Fernando Ramos
2021-10-27 16:46 ` Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.