Re: [PATCH 1/2] mergetool: honor mergetool.$tool.trustExitCode for built-in tools

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 29, 2016, 18:57 UTC
Message-ID: <xmqq37iaglm1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161129093807.24637-1-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 46 quoted lines

> Built-in merge tools contain a hard-coded assumption about
> whether or not a tool's exit code can be trusted to determine
> the success or failure of a merge.  Tools whose exit codes are
> not trusted contain calls to check_unchanged() in their
> merge_cmd() functions.
>
> A problem with this is that the trustExitCode configuration is
> not honored for built-in tools.
>
> Teach built-in tools to honor the trustExitCode configuration.
> Extend run_merge_cmd() so that it is responsible for calling
> check_unchanged() when a tool's exit code cannot be trusted.
> Remove check_unchanged() calls from scriptlets since they are no
> longer responsible for calling it.
>
> When no configuration is present, exit_code_trustable() is
> checked to see whether the exit code should be trusted.
> The default implementation returns false.
>
> Tools whose exit codes can be trusted override
> exit_code_trustable() to true.
>
> Reported-by: Dun Peal <dunpealer@gmail.com>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  git-mergetool--lib.sh    | 56 ++++++++++++++++++++++++++++++++++++++----------
>  mergetools/araxis        |  2 --
>  mergetools/bc            |  2 --
>  mergetools/codecompare   |  2 --
>  mergetools/deltawalker   |  6 +++++-
>  mergetools/diffmerge     |  4 ++++
>  mergetools/diffuse       |  2 --
>  mergetools/ecmerge       |  2 --
>  mergetools/emerge        |  4 ++++
>  mergetools/examdiff      |  2 --
>  mergetools/kdiff3        |  4 ++++
>  mergetools/kompare       |  4 ++++
>  mergetools/meld          |  3 +--
>  mergetools/opendiff      |  2 --
>  mergetools/p4merge       |  2 --
>  mergetools/tkdiff        |  4 ++++
>  mergetools/tortoisemerge |  2 --
>  mergetools/vimdiff       |  2 --
>  mergetools/winmerge      |  2 --
>  mergetools/xxdiff        |  2 --
>  20 files changed, 71 insertions(+), 38 deletions(-)

OK, so the idea is that the backends that used to touch $BACKUP and used to call check_unchanged stop doing these two things, while other backends override exit_code_trustable(). That makes the change to the backends very straightforward to understand.

Show 13 quoted lines

>  # Run a either a configured or built-in merge tool
>  run_merge_cmd () {
> -	merge_cmd "$1"
> +	mergetool_trust_exit_code=$(trust_exit_code "$1")
> +	if test "$mergetool_trust_exit_code" = "true"
> +	then
> +		merge_cmd "$1"
> +	else
> +		touch "$BACKUP"
> +		merge_cmd "$1"
> +		check_unchanged
> +	fi
>  }

Looks sensible.  Thanks.  Will queue.