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

Am 28.08.24 um 10:31 schrieb ToBoMi via GitGitGadget:

From: deboeto <redacted>

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

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

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