> -----Ursprüngliche Nachricht-----
> Von: Johannes Sixt <j6t@xxxxxxxx>
> Gesendet: Samstag, 31. August 2024 15:52
> An: Boesch, Tobias <tobias.boesch@xxxxxxxxx>
> Cc: ToBoMi via GitGitGadget <gitgitgadget@xxxxxxxxx>; git@xxxxxxxxxxxxxxx
> Betreff: 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 <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.
>
I changed it to only use the mergetool.cmd and updated the configuration hint that mentions the configuration variables so that users know to only specify the cmd variable.
> > 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.
>
That's odd. I tested this again by setting merge.tool to "meld" and configured mergetool.cmd to "some wrong path". When starting the mergetool I got a popup saying that meld was not found in path.
> 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?
>
I can iterate directly on the input string as a list. Will be part of the next patch version.
> 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.
>
Good catch. I added code to prevent this in the next patch version.
When a command sequence is detected (basically square brackets in the command) the user get a hint to avoid square brackets in the mergetool.cmd.
> > + 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]
>
True. Updating the text.
> > + return
> > + }
> > }
> > }
> >
>
> -- Hannes
Minor suggestions will be fixed in the next patch.
-------------------------------------------------------------------------------------------------
imperial-Werke oHG, Sitz Bünde, Registergericht Bad Oeynhausen - HRA 4825
