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

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

Fernando <greenfoo@u92.eu> writes:

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

If you anticipate that you'd need more debugging sessions as time
goes and people want to try to come up with different layouts, (A)
is preferrable, I would think.

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

Yes, I think I was the one who spotted the bash-ism in an earlier
round, if I am not mistaken.

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

Although "echo" is often built-in, "cut" and pipe mean a fork to
create an extra process, which some folks may want less of.

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

I already said that this is good enough for now, didn't I?  But see
below, and do not use it, it should work but it is uglier than a
simple single liner pipe.

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

Yes, and it is a good way to convey that the author did sufficient
due diligence to note in the proposed commit log message (1) these
two sets of actual vim commands, and (2) the fact that the author
made sure these "not literally identical" command line options
produce identical results for these existing modes.

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

Have a comment next to each of these four layout tests what variant
from pre-layout era it correspnds to, if you haven't done so.

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

Yes, with a bit to extend the proposed log message to help #1, and a
bit of comments next to the test cases to help #2.

Thanks.




#!/bin/sh
substring () {
	STRING=$1 START=$2 LEN=$3

	local strip= 
	while test 0 -lt "$START"
	do
		strip="?$strip"
		START=$(($START - 1))
	done
	if test -n "$strip"
	then
		STRING=${STRING#$strip}
	fi
	strip=
	COUNT=$(( ${#STRING} - $LEN ))
	while test 0 -lt "$COUNT"
	do
		strip="?$strip"
		COUNT=$(($COUNT - 1))
	done
	if test -n "$strip"
	then
		STRING=${STRING%$strip}
	fi
	echo "$STRING"
}

while read string start len
do
	substring $string $start $len
done