Re: [PATCH v2 2/3] mergetools/p4merge: create a base if none available

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

Kevin Bracey <kevin@bracey.fi> writes:

> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 795edd2..aa9a732 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -249,6 +249,19 @@ clear_local_git_env() {
>  	unset $(git rev-parse --local-env-vars)
>  }
>  
> +# Generate a virtual base file for a two-file merge. On entry the
> +# base file $1 should be a copy of $2. Uses git apply to remove
> +# lines from $1 that are not in $3, leaving only common lines.
> +create_virtual_base() {
> +	sz0=$(wc -c <"$1")
> +	@@DIFF@@ -u -La/"$1" -Lb/"$1" "$2" "$3" | git apply --no-add
> +	sz1=$(wc -c <"$1")
> +
> +	# If we do not have enough common material, it is not
> +	# worth trying two-file merge using common subsections.
> +	expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$1"
> +}
> +

This rewrite is wrong.  It should be

> +	sz0=$(wc -c <"$1")
> +	@@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$3" | git apply --no-add
> +	sz1=$(wc -c <"$1")

for it to make sense.  "diff $1 $3" is a change to go from $1 to $3;
with "-La/$1 -Lb/$1", we declare that the change is to be applied to
$1, and use --no-add to only use the removal from the diff when we
edit $1 using this mechanism.

The end effect is to in-place edit "$1" to remove what is not common
with "$3", and sz0/sz1 computation is done on "$1" for this reason.
Does it (i.e. "$1") shrink sufficiently when we remove the material
that is not common in it (i.e. "$1") and "$3"?

This part is a two-file operation between $1 and $3; there is
nothing you would want to pass $2 to influence what the above three
lines do.

It may happen that the caller has two copies of the same thing,
$orig and $src1, and uses one for $1 and the other for $2, so you
won't observe the damage from the incorrect rewriting of the above
logic, but it invites the next caller to incorrectly feed something
totally unrelated to $1 and $2.

Please fix it to a function that takes two temporary paths, not
three.