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

> -----Ursprüngliche Nachricht-----
> Von: Johannes Sixt <j6t@kdbg.org>
> Gesendet: Samstag, 31. August 2024 15:52
> An: Boesch, Tobias <tobias.boesch@miele.com>
> Cc: ToBoMi via GitGitGadget <gitgitgadget@gmail.com>; git@vger.kernel.org
> 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@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.
>
> > 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@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`?
>
> >  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