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