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

[Junio C Hamano <gitster@pobox.com>]

Fernando Ramos <greenfoo@u92.eu> writes:

> When running 'git mergetool -t vimdiff', a new configuration option
> ('mergetool.vimdiff.layout') can now be used to select how the user
> wants the different windows, tabs and buffers to be displayed.
>
> If the option is not provided, the layout will be the same one that was
> being used before this commit (ie. two rows with LOCAL, BASE and COMMIT
> in the top one and MERGED in the bottom one).
>
> The 'vimdiff' variants ('vimdiff{1,2,3}') still work but, because they
> represented nothing else than different layouts, are now internally
> implemented as a subcase of 'vimdiff' with the corresponding
> pre-configured 'layout'.
>
> Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
> ---
>  mergetools/vimdiff | 548 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 521 insertions(+), 27 deletions(-)
>
> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
> index 96f6209a04..5bf77a5388 100644
> --- a/mergetools/vimdiff
> +++ b/mergetools/vimdiff
> @@ -1,48 +1,440 @@
> +# This script can be run in two different contexts:
> +#
> +#   - From git, when the user invokes the "vimdiff" merge tool. In this context
> +#     this script expects the following environment variables (among others) to
> +#     be defined (which is something "git" takes care of):
> +#
> +#       - $BASE
> +#       - $LOCAL
> +#       - $REMOTE
> +#       - $MERGED
> +#
> +#     In this mode, all this script does is to run the next command:
> +#
> +#         vim -f -c ... $LOCAL $BASE $REMOTE $MERGED
> +#
> +#     ...where the "..." string depends on the value of the
> +#     "mergetool.vimdiff.layout" configuration variable and is used to open vim
> +#     with a certain layout of buffers, windows and tabs.
> +#
> +#   - From the unit tests framework ("t" folder) by sourcing this script and
> +#     then manually calling "run_unit_tests", which will run a battery of unit
> +#     tests to make sure nothing breaks.
> +#     In this context this script does not expect any particular environment
> +#     variable to be set.
> +
> +
> +################################################################################
> +## Internal functions (not meant to be used outside this script)
> +################################################################################
> +
> +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?

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

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

>  merge_cmd () {
> +	layout=$(git config mergetool.$merge_tool.layout)
> +
>  	case "$1" in
>  	*vimdiff)
> -		if $base_present
> +		if test -z "$layout"
>  		then
> -			"$merge_tool_path" -f -d -c '4wincmd w | wincmd J' \
> -				"$LOCAL" "$BASE" "$REMOTE" "$MERGED"
> -		else
> -			"$merge_tool_path" -f -d -c 'wincmd l' \
> -				"$LOCAL" "$MERGED" "$REMOTE"
> +			# Default layout when none is specified
> +			layout="(LOCAL,BASE,REMOTE)/MERGED"
>  		fi
>  		;;
> ...
> +	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?