Re: trustExitCode doesn't apply to vimdiff mergetool

From: Jeff King <peff@peff.net>
Date: Nov 28, 2016, 02:01 UTC
Message-ID: <20161128020128.6jhdpg444xbshtzz@sigill.intra.peff.net>
In-Reply-To: <20161128014538.GA18691@gmail.com>

On Sun, Nov 27, 2016 at 05:45:38PM -0800, David Aguilar wrote:

Show 9 quoted lines

> I have a patch that makes it so that none of the tools do the
> check_unchanged logic themselves and instead rely on the
> library code to handle it for them.  This makes the
> implementation uniform across all tools, and allows tools to
> opt-in to trustExitCode=true.
> 
> This means that all of the builtin tools will default to
> trustExitCode=false, and they can opt-in by setting the
> configuration variable.

FWIW, that was the refactoring that came to mind when I looked at the code yesterday. This is the first time I've looked at the mergetool code, though, so you can take that with the appropriate grain of salt.

Your patch looks mostly good to me. One minor comment:

Show 32 quoted lines

>  	merge_cmd () {
> -		trust_exit_code=$(git config --bool \
> -			"mergetool.$1.trustExitCode" || echo false)
> -		if test "$trust_exit_code" = "false"
> -		then
> -			touch "$BACKUP"
> -			( eval $merge_tool_cmd )
> -			check_unchanged
> -		else
> -			( eval $merge_tool_cmd )
> -		fi
> +		( eval $merge_tool_cmd )
>  	}
>  }
>  
> @@ -225,7 +216,20 @@ run_diff_cmd () {
>  
>  # Run a either a configured or built-in merge tool
>  run_merge_cmd () {
> +	touch "$BACKUP"
> +
>  	merge_cmd "$1"
> +	status=$?
> +
> +	trust_exit_code=$(git config --bool \
> +		"mergetool.$1.trustExitCode" || echo false)
> +	if test "$trust_exit_code" = "false"
> +	then
> +		check_unchanged
> +		status=$?
> +	fi
> +

In the original, we only touch $BACKUP if we care about timestamps. I can't think of a reason it would matter to do the touch in the trustExitCode=true case, but you could also write it as:

  if test "$trust_exit_code" = "false"
  then
	touch "$BACKUP"
	merge_cmd "$1"
	check_unchanged
  else
	merge_cmd "$1"
  fi

  # now $? is from either merge_cmd or check_unchanged

Yours is arguably less subtle, though, which may be a good thing.

-Peff