Re: [PATCH 0/2] mergetools: vimdiff3: fix regression

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Aug 7, 2022, 00:44 UTC
Message-ID: <CAMP44s39TW2nq+H7VHhFmXGQwFPs1-ZDm1XRcoFRzYJL+9RSvA@mail.gmail.com>
In-Reply-To: <Yu7byRmn4VtSpyWd@zacax395.localdomain>

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                                          NEW

That'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)

--- 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