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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2013, 04:55 UTC
Message-ID: <7v7glfetus.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1362856860-15205-3-git-send-email-kevin@bracey.fi>

Kevin Bracey <kevin@bracey.fi> writes:

Show 21 quoted lines

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