Re: [PATCH v2] mergetool(vimdiff): allow paths to contain spaces again

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 14, 2022, 16:27 UTC
Message-ID: <xmqqy1wv7j7u.fsf@gitster.g>
In-Reply-To: <pull.1287.v2.git.1657809063728.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 18 quoted lines

>  	if $base_present
>  	then
> -		eval "$merge_tool_path" \
> -			-f "$FINAL_CMD" "$LOCAL" "$BASE" "$REMOTE" "$MERGED"
> +		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
> @@ -424,8 +424,8 @@ merge_cmd () {
>  		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"
> +		eval '"$merge_tool_path"' \
> +			-f "$FINAL_CMD" '"$LOCAL"' '"$REMOTE"' '"$MERGED"'
>  	fi

If there were another syntactic fix we need in the future, we may by mistake fix one but not the other, and the test we add in this patch checks only one side but not the other. In a follow-up we may want to unify the two eval invocations to make the testing of this part more robust.

But as a minimum and obvious fix, stopping at the above is appropriate for this patch.

> + ...
> +	diff -u expect actual || at_least_one_ko=true

I wonder if we still should care about platforms that need to set GIT_TEST_CMP_USE_COPIED_CONTEXT while running our tests. If we use "diff -u" hardcoded like this here, we are declaring that they are now unwelcome to run our tests.

It might be just the matter of using "cmp -s" here (or run "diff" without any option). Do we care about emitting the difference in the output? I doubt it.

>  	if test "$at_least_one_ko" = "true"
>  	then
>  		return 255

Thanks.