Re: [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function
- From
Johannes Sixt <j6t@kdbg.org>
- Date
- Dec 29, 2020, 08:50 UTC
- Message-ID
- <3eb8c8d9-5b56-82ff-21d8-725a6c269f66@kdbg.org>
- In-Reply-To
- <20201228192919.1195211-5-seth@eseth.com>
Am 28.12.20 um 20:29 schrieb Seth House:
Show 9 quoted lines
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index e3c7d78d1d..929192d0f8 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -334,6 +334,8 @@ merge_file () {
> checkout_staged_file 2 "$MERGED" "$LOCAL"
> checkout_staged_file 3 "$MERGED" "$REMOTE"
>
> + initialize_merge_tool "$merge_tool"In my earlier review, I was not explicit about the lack of error handling of this invocation, because I hoped that you would notice it yourself. `initialize_merge_tool` does have a few failure modes via `setup_tool` that are not really unlikely; ignoring errors would be wrong, I think.
Before this change, the errors would be handled as part of the failing `run_merge_tool` call at the end of function `merge_file`. But now errors are ignored. Just appending `|| return` would not be appropriate at this point because a lot has already happened before the call that has to be rewound. Would it be possible to move the call above the `mergetool_tmpdir_init` call, so that nothing has to be rewound?
Show 5 quoted lines
> + > if test "$( > git config --get --bool "mergetool.$merge_tool.automerge" || > git config --get --bool "mergetool.automerge" || >
-- Hannes