Re: [PATCH v2] git gui: add directly calling merge tool from gitconfig

From: Johannes Sixt <j6t@kdbg.org>
Date: Aug 31, 2024, 13:51 UTC
Message-ID: <61b9b041-97cf-47ac-84ef-1467aba873e3@kdbg.org>
In-Reply-To: <pull.1773.v2.git.1724833917245.gitgitgadget@gmail.com>

Am 28.08.24 um 10:31 schrieb ToBoMi via GitGitGadget:

Show 28 quoted lines

> From: deboeto <tobias.boesch@miele.com>
> 
> 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.

Show 6 quoted lines

> 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.

Show 6 quoted lines

> 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".

Show 5 quoted lines

> 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.

Show 5 quoted lines

> 
> Beyond compare 3 and Visual Studio code were
> tested as manually configured merge tools.
> 
> Signed-off-by: Tobias Boesch <tobias.boesch@miele.com>

You updated this line, but not the From: line. Would you mind configuring your user.name and then `git commit --amend --reset-author`?

Show 16 quoted lines

>  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]

Show 5 quoted lines

> +			return
> +		}
>  	}
>  	}
>

-- Hannes