Re: [PATCH v5 1/3] vimdiff: new implementation with layout support

[Fernando <greenfoo@u92.eu>]

> > +debug_print() { 
> > +	# Send message to stderr if global variable DEBUG is set to "true"
> > +
> > +	if test -n "$GIT_MERGETOOL_VIMDIFF_DEBUG"
> > +	then
> > +		>&2 echo "$@"
> > +	fi
> > +}
> 
> Do we want to keep this helper, and many calls to it sprinkled in
> this file, or are they leftover cruft?

I left it in case we ever need to debug this script in the future. But if you
think it's not worth it, I can delete it. We have three options:

  A) Leave it
  B) Completely remove it
  C) Remove the function and replace the places where it is being called by
     a commented out "echo" to stderr

Let me know what you prefer.


> Style.  "debug_print () {", i.e. SPACE on both sides of "()".

Sure. No problem. I'll fix all function declarations (they are all missing the
extra space before the opening parenthesis)


> > +substring() {
> > +	# Return a substring of $1 containing $3 characters starting at
> > +	# zero-based offset $2.
> > +	# 
> > +	# Examples:
> > +	#
> > +	#   substring "Hello world" 0 4  --> "Hell"
> > +	#   substring "Hello world" 3 4  --> "lo w"
> > +	#   substring "Hello world" 3 10 --> "lo world"
> > +
> > +	STRING=$1
> > +	START=$2
> > +	LEN=$3
> > +
> > +	echo "$STRING" | cut -c$(( START + 1 ))-$(( START + $LEN))
> > +}
> 
> The lack of space before the second closing "))" makes it look
> inconsistent. 

No problem. I'll fix it.


> We should be able to do this no external commands
> and just two variable substitutions and without relying on
> bash-isms, but the above should do.
> 

In v1 of this patch series, instead of this function, I was doing this other
thing:

    X=${Y:a:b}

...but that is a bash-ism, so I replaced it with what you see above ("echo" +
"cut")

I was not able to find another way of doing it using just standard POSIX shell
syntax [1] (notice that "cut" is included in IEEE Std 1003.1 [2] so it shouldn't
be an issue to rely on it)

In any case, if anyone knows how we could achieve the same without using
external commands, please let me know and I'll change it (in the meantime I'll
keep searching for alternatives too). If after a reasonable amount of time none
of us manages to find a solution I suggest to leave it as it is now.

[1] https://pubs.opengroup.org/onlinepubs/009604499/utilities/xcu_chap02.html
[2] https://pubs.opengroup.org/onlinepubs/009696699/utilities/cut.html


> > +	if $base_present
> > +	then
> > +		eval "$merge_tool_path" \
> > +			-f "$FINAL_CMD" "$LOCAL" "$BASE" "$REMOTE" "$MERGED"
> > +	else
> > +		# If there is no BASE (example: a merge conflict in a new file
> > +		# with the same name created in both braches which didn't exist
> > +		# before), close all BASE windows using vim's "quit" command
> > +
> > +		FINAL_CMD=$(echo "$FINAL_CMD" | \
> > +			sed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g')
> > +
> > +		eval "$merge_tool_path" \
> > +			-f "$FINAL_CMD" "$LOCAL" "$REMOTE" "$MERGED"
> > +	fi
> 
> 
> I wonder if there were an easy way to "compare" the $FINAL_CMD this
> new code feeds to $merge_tool_path and what was fed to it by the
> original code to show that we are not regressing what the end user
> sees.
> 
> The "run_unit_tests()" only compares the cmd generated for each
> given layout, but the original vimdiff$N didn't express them in
> terms of $layout this patch introduces, so unfortunately that is not
> it.
> 
> Ideas?

Before this patch series, this is what each variant fed into "$merge_tool_path":

  - vimdiff:
      -f -d -c '4wincmd w | wincmd J' $LOCAL $BASE $REMOTE $MERGED

  - vimdiff1:
      -f -d -c 'echon "..."' $LOCAL $REMOTE

  - vimdiff2:
      -f -d -c 'wincmd l' $LOCAL $MERGED $REMOTE

  - vimdiff3:
      -f -d -c 'hid | hid | hid' $LOCAL $REMOTE $BASE $MERGED

After this patch series, when one of these predefined variants is selected, a
fixed layout is chosen and translated into the final string fed into
"$merge_tool_path":

  - vimdiff --> (LOCAL,BASE,REMOTE)/MERGED
      -f -c "echo | split | vertical split | 1b | wincmd l | vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis" -c "tabfirst" $LOCAL $BASE $REMOTE $MERGED

  - vimdiff1 --> @LOCAL,REMOTE
      -f -c "echo | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis" -c "tabfirst" $LOCAL $BASE $REMOTE $MERGED
 
  - vimdiff2 --> LOCAL,MERGED,REMOTE
      -f -c "echo | vertical split | 1b | wincmd l | vertical split | 4b | wincmd l | 3b | tabdo windo diffthis" -c "tabfirst" $LOCAL $BASE $REMOTE $MERGED

  - vimdiff3 --> MERGED
      -f -c "echo | 4b | bufdo diffthis" -c "tabfirst" $LOCAL $BASE $REMOTE $MERGED

Thus, we need to do two things:

  1. Manually check (one time operation) that the *before* and *after* strings
     are equivalent (from the point of view of vim).

  2. Add a unit test that verifies that the layout associated to each variant
     actually produces the string listed above. That way we make sure the
     functionality does not break in the future.

Step (1) is what I have already done (but I encourage others to do the same...
the more eyes the better).

Step (2)... is already done! Notice how, on the "run_unit_tests()" function,
these layouts correspond to test cases numbers #07, #09, #01 and #10 :)

  NOTE: While re-reviewing this, I noticed the layout definition for "vimdiff1"
        was set to "@LOCAL,MERGED" instead of "@LOCAL,REMOTE", which is the
        correct value. I'll fix this in v6.

Should this be considered enough test for backwards compatibility?

  NOTE: Another option would be to *completely bypass* the layout mechanism when
        using one of the old variants and, in that case, feed exactly the same
        string we were feeding before... but I just remembered we decided
        against that in v2... so back to the question above :)