Am 28.08.24 um 10:31 schrieb ToBoMi via GitGitGadget:
> From: deboeto <tobias.boesch@xxxxxxxxx>
>
> git gui can open a merge tool when conflicts are
> detected (Right click in the diff of the file with
> conflicts).
> The merge tools that are allowed to
> use are hard coded into git gui.
>
> If one wants to add a new merge tool it has to be
> added to git gui through a source code change.
> This is not convenient in comparison to how it
> works in git (without gui).
>
> git itself has configuration options for a merge tools
> path and command in the git config.
> New merge tools can be set up there without a
> source code change.
>
> Those options are used only by pure git in
> contrast to git gui. git calls the configured
> merge tools directly from the config while git
> Gui doesn't.
>
> With this change git gui can call merge tools
> configured in the gitconfig directly without a
> change in git gui source code.
> It needs a configured merge.tool and a configured
> mergetool.cmd config entry.
OK.
> gitconfig example:
> [merge]
> tool = vscode
> [mergetool "vscode"]
> path = the/path/to/Code.exe
> cmd = \"Code.exe\" --wait --merge \"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$MERGED\"
I found it annoying that I had to configure .path in addition to .cmd.
Typically, you would put the correct path into the .cmd configuration.
In fact, `git mergetool` works without .path and fails when .cmd does
not contain the correct path.
> Without the mergetool.cmd configuration and an
> unsupported merge.tool entry, git gui behaves
> mainly as before this change and informs the user
> about an unsupported merge tool, but now also
> shows a hint to add a config entry for the tool
> in gitconfig.
Good.
While testing I configured meld incorrectly once and got no feedback
whatsoever, but I would not attribute this to this patch.
There is no such thing called "gitconfig". Just strike "in gitconfig".
> If a wrong mergetool.cmd is configured by accident
> it is beeing handled by git gui already. In this
> case git gui informs the user that the merge tool
> couldn't be opened. This behavior is preserved by
> this change and should not change.
Good.
>
> Beyond compare 3 and Visual Studio code were
> tested as manually configured merge tools.
>
> Signed-off-by: Tobias Boesch <tobias.boesch@xxxxxxxxx>
You updated this line, but not the From: line. Would you mind
configuring your user.name and then `git commit --amend --reset-author`?
> git-gui/lib/mergetool.tcl | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl
> index e688b016ef6..4c4e8f47bb0 100644
> --- a/git-gui/lib/mergetool.tcl
> +++ b/git-gui/lib/mergetool.tcl
> @@ -272,8 +272,14 @@ proc merge_resolve_tool2 {} {
> }
> }
> default {
> - error_popup [mc "Unsupported merge tool '%s'" $tool]
> - return
> + set tool_cmd [get_config mergetool.$tool.cmd]
> + if {$tool_cmd ne {}} {
> + set tool_cmd_file_vars_resolved [subst -nobackslashes -nocommands $tool_cmd]
I just learnt that a string value containing double-quotes is broken
into a list in the expected way (keeps quoted parts together as a single
element). However, this form of substitution replaces variable values
with arbitrary text without taking into account that the original string
is actually a list. Should we not break the string into a list first,
and apply the substitution on the list elements?
If there is a straight-forward way to do this (say, an obvious two-liner
at most), we should do it. Otherwise, I can live with this solution for
now because it requires file names with double-quotes to break the
expected list nature.
There is another thing, though, that I would not want to take as
lightly: The -nocommands modifier of `subst` does not live up to its
promises, and it is even the documented behavior: command substitutions
in array indexes are still executed. Consider this configuration:
[merge]
tool = evil
[mergetool "evil"]
cmd = meld \"$REMOTE([exit])\"
Guess what happens when I run the merge tool? It exits Git GUI!
I suggest to reject any configuration that contains an opening bracket
'[' or anything else that introduces a command execution.
> + set cmdline [lreplace $tool_cmd_file_vars_resolved 0 0 $merge_tool_path]
> + } else {
> + error_popup [mc "Unsupported merge tool '%s'. Is the tool command and path configured properly in gitconfig?" $tool]
Can we not have a more helpful text? How about
error_popup [mc "Unsupported merge tool '%s'.
See the git-config manual page how to configure mergetool.%s.cmd
suitably." $tool $tool]
> + return
> + }
> }
> }
>
-- Hannes
