Re: [PATCH 0/2] mergetools: vimdiff3: fix regression
On Sat, Aug 6, 2022 at 4:23 PM Fernando Ramos <greenfoo@u92.eu> wrote:
Show 21 quoted lines
>
> On 22/08/06 02:17PM, Felipe Contreras wrote:
> >
> > I don't know why anyone would want to do that, but the code interprets
> > that as the user wanting '1b', which is completely ignored.
> >
> > If we are not going to care about these cases, we can just remove all this code:
> >
> > ...
> >
>
> Ah! I see now. You are completely right: it wouldn't make sense for anyone to
> specify "layout=LOCAL" (or REMOTE or BASE), but if he did *it wouldn't work*
> (only works with "layout=MERGED").
>
> That should be fixed. I'll update the patch with a new version to generate this
> command string:
>
> echo | silent 4b | set hidden | let tmp=bufnr('%') | silent bufdo diffthis | exe 'buffer '.tmp
> ^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^
> NEW NEWThat's not correct: `exe 'buffer '.tmp` would be executed in every buffer. To split the commands you need to do the bufdo in a separate execute command.
> Notes:
>
> - This is "easier" than moving "silent 4b" to the end, due to the way the
> code is structured.
Which is a clear hint that the code should be restructured.
> - I agree that this is absurdly complex for what we want to achieve with
> "vimdiff3" but let's put it this way: now everything can be achieved with
> the "layout" configuration option, even "useless" things such as setting it
> to "LOCAL".
Yes, but even that can be achieved in simpler ways (see the patch below).
Show 6 quoted lines
> > I understand the need if you want a complex layout, like
> > "MERGED+LOCAL,BASE,REMOTE", that's very nice, but if you just want
> > "MERGED", most of the code does nothing,
>
> With the fix above that shouldn't be a problem anymore: even if someone
> specifies "LOCAL" it will work, in an absurd way, but it will work :)
But the objective isn't to make "everything" work, the objective is to make everything the user might reasonably want to work. If some unreasonable use case can be supported with a minimal burden to maintenance, sure, support that too.
"LOCAL" is not that: it's an unreasonable use case that requires a bunch of extra code.
Either way, I think you are resisting too much a reshuffling of the code when it's very clear the single window mode would benefit from that since it doesn't need gen_cmd_aux() at all.
Here's a patch that shuffles the code around and makes it much easier to read and maintain (plus it keeps and fixes the support for unreasonable use cases like "LOCAL"):
(apologies in advance for gmail's possible wrapping)
Show changes to mergetools/vimdiff +20 −27
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -251,39 +251,34 @@ gen_cmd_aux () {
return
fi
+ # Shouldn't happen
+ echo "$CMD | echoerr 'BUG'"
+}
- # Step 4:
- #
- # If we reach this point, it means there are no separators and we just
- # need to print the command to display the specified buffer
-
- target=$(substring "$LAYOUT" "$start" "$(( end - start ))" | sed
's:[ @();|-]::g')
+get_buf () {
+ target=$(echo "$1" | sed 's:[ @();|-]::g')
+ buf="1"
if test "$target" = "LOCAL"
then
- CMD="$CMD | 1b"
+ buf="1"
elif test "$target" = "BASE"
then
- CMD="$CMD | 2b"
+ buf="2"
elif test "$target" = "REMOTE"
then
- CMD="$CMD | 3b"
+ buf="3"
elif test "$target" = "MERGED"
then
- CMD="$CMD | 4b"
-
- else
- CMD="$CMD | ERROR: >$target<"
+ buf="4"
fi
- echo "$CMD"
- return
+ echo "$buf"
}
-
gen_cmd () {
# This function returns (in global variable FINAL_CMD) the string that
# you can use when invoking "vim" (as shown next) to obtain a given
@@ -315,6 +310,14 @@ gen_cmd () {
LAYOUT=$1
+ # A single window is handled specially
+
+ if ! echo "$LAYOUT" | grep ",\|/" >/dev/null
+ then
+ buf=$(get_buf "$LAYOUT")
+ FINAL_CMD="-c \"set hidden | silent bufdo diffthis\" -c \"silent ${buf}b\""
+ return
+ fi
# Search for a "@" in one of the files identifiers ("LOCAL", "BASE",
# "REMOTE", "MERGED"). If not found, use "MERGE" as the default file
@@ -335,17 +338,7 @@ gen_cmd () {
CMD=$(gen_cmd_aux "$LAYOUT")
-
- # Adjust the just obtained script depending on whether more than one
- # windows are visible or not
-
- if echo "$LAYOUT" | grep ",\|/" >/dev/null
- then
- CMD="$CMD | tabdo windo diffthis"
- else
- CMD="$CMD | bufdo diffthis"
- fi
-
+ CMD="$CMD | tabdo windo diffthis"
# Add an extra "-c" option to move to the first tab (notice that we
# can't simply append the command to the previous "-c" string as
--
Felipe Contreras
Felipe Contreras