On Sun, Nov 27, 2016 at 05:45:38PM -0800, David Aguilar wrote:
> 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:
> 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