| Message ID | pull.1773.v3.git.1725607643479.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] git gui: add directly calling merge tool from gitconfig | expand |
Am 06.09.24 um 09:27 schrieb ToBoMi via GitGitGadget: > From: Tobias Boesch <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. The configuration is "mergetool.$tool.cmd"! Personally, I would avoid the words "gitconfig" and "config" (here and in the rest of the commit message), neither of which are English. "Configuration" would be OK, IMO. > > gitconfig example: > [merge] > tool = vscode > [mergetool "vscode"] > path = the/path/to/Code.exe > cmd = \"Code.exe\" --wait --merge \"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$MERGED\" > > 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. In addtition it also shows a hint to add > a config entry to use the tool as an unsupported tool with degraded > support. > > If a wrong mergetool.cmd is configured by accident, it gets 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. > > "Beyond Compare 3" and "Visual Studio Code" were tested as manually > configured merge tools. > > Signed-off-by: Tobias Boesch <tobias.boesch@miele.com> > --- > git-gui/lib/mergetool.tcl | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl > index e688b016ef6..ccbc1a46554 100644 > --- a/git-gui/lib/mergetool.tcl > +++ b/git-gui/lib/mergetool.tcl > @@ -272,8 +272,24 @@ 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 {}} { > + if {([string first {[} $tool_cmd] != -1) || ([string first {]} $tool_cmd] != -1)} { > + error_popup [mc "Unable to process square brackets in mergetool.cmd configuration option.\ > + Please remove the square brackets."] > + return Condition and error text are OK. But see below. > + } else { > + foreach command_part $tool_cmd { > + lappend cmdline [subst -nobackslashes -nocommands $command_part] > + } Good. I have seen a few examples in the Tcl manual with lappend in the loop body, and it seems to be customary to set the list variable to an empty value before the loop, i.e. set cmdline {} > + } > + } else { > + error_popup [mc "Unsupported merge tool '%s'.\n > + Currently unsupported tools can be added and used as unsupported tools with degraded support\ > + by adding the command of the tool to the \"mergetool.cmd\" option in the config. > + See the configuration documentation for more details." $tool] This error message needs a bit more work (some of this also applies to the message above): - A tool is only unsupported as long as there is no usable configuration. Once mergetool.$tool.cmd is set to something we can handle, calling the tool "unsupported" isn't appropriate, I would think. How about Unsupported merge tool '%s'. To use this tool, configure "mergetool.%s.cmd" as shown in the git-config manual page. - The configuration variable that we use is not mergetool.cmd, but mergetool.$tool.cmd. - Continuation lines must not be indented. Indented text appears indented in the error message. - Watch out whether an explicit \n is given, whether the line-break is escaped or not; all of this has meaning. - Looking at other multi-line error messages in git-gui.sh, the convention is mc["First paragraph goes here. Second paragraph. All of it is on one line in the source code. Third paragraph. No \n appears anywhere."] > + return > + } > } > } As a matter of personal taste, I prefer to structure code with error exits like so (but it is totally acceptable if you disagree): if {check for error 1} { error msg1 return } if {check for error 2} { error msg2 return } regular case goes here without indentation Note that there are no else-branches. This reduces the indentation levels. -- Hannes
> -----Ursprüngliche Nachricht----- > Von: Johannes Sixt <j6t@kdbg.org> > Gesendet: Sonntag, 8. September 2024 14:21 > An: Boesch, Tobias <tobias.boesch@miele.com> > Cc: git@vger.kernel.org; ToBoMi via GitGitGadget <gitgitgadget@gmail.com> > Betreff: Re: [PATCH v3] git gui: add directly calling merge tool from gitconfig > > Am 06.09.24 um 09:27 schrieb ToBoMi via GitGitGadget: > > From: Tobias Boesch <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. > > The configuration is "mergetool.$tool.cmd"! > Right - changed it to "mergetool.<mergetool name>.cmd", since this is a descriptive text and not a script. I personally prefer a more human friendly writing. > Personally, I would avoid the words "gitconfig" and "config" (here and in the > rest of the commit message), neither of which are English. > "Configuration" would be OK, IMO. > Will be changed. > > > > gitconfig example: > > [merge] > > tool = vscode > > [mergetool "vscode"] > > path = the/path/to/Code.exe > > cmd = \"Code.exe\" --wait --merge \"$LOCAL\" \"$REMOTE\" > \"$BASE\" \"$MERGED\" > > > > 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. In addtition it also shows a > > hint to add a config entry to use the tool as an unsupported tool with > > degraded support. > > > > If a wrong mergetool.cmd is configured by accident, it gets 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. > > > > "Beyond Compare 3" and "Visual Studio Code" were tested as manually > > configured merge tools. > > > > Signed-off-by: Tobias Boesch <tobias.boesch@miele.com> > > --- > > > git-gui/lib/mergetool.tcl | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl > > index e688b016ef6..ccbc1a46554 100644 > > --- a/git-gui/lib/mergetool.tcl > > +++ b/git-gui/lib/mergetool.tcl > > @@ -272,8 +272,24 @@ 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 {}} { > > + if {([string first {[} $tool_cmd] != -1) || ([string first {]} > $tool_cmd] != -1)} { > > + error_popup [mc "Unable to process square > brackets in mergetool.cmd configuration option.\ > > + Please remove > the square brackets."] > > + return > > Condition and error text are OK. But see below. > > > + } else { > > + foreach command_part $tool_cmd { > > + lappend cmdline [subst -nobackslashes > -nocommands $command_part] > > + } > > Good. > > I have seen a few examples in the Tcl manual with lappend in the loop body, > and it seems to be customary to set the list variable to an empty value before > the loop, i.e. > > set cmdline {} > Done in next patch. > > + } > > + } else { > > + error_popup [mc "Unsupported merge tool '%s'.\n > > + Currently unsupported > tools can be added and used as unsupported tools with degraded support\ > > + by adding the > command of the tool to the \"mergetool.cmd\" option in the config. > > + See the configuration > documentation for more details." $tool] > > This error message needs a bit more work (some of this also applies to the > message above): > > - A tool is only unsupported as long as there is no usable configuration. Once > mergetool.$tool.cmd is set to something we can handle, calling the tool > "unsupported" isn't appropriate, I would think. Junio C Hamano suggested to mark the tools unsupported in his review. I'll change it back and remove the unsupported hint. > How about > > Unsupported merge tool '%s'. > > To use this tool, configure "mergetool.%s.cmd" as shown in the git-config > manual page. > Will be changed. > - The configuration variable that we use is not mergetool.cmd, but > mergetool.$tool.cmd. > > - Continuation lines must not be indented. Indented text appears indented in > the error message. > > - Watch out whether an explicit \n is given, whether the line-break is escaped > or not; all of this has meaning. > > - Looking at other multi-line error messages in git-gui.sh, the convention is > > mc["First paragraph goes here. > > Second paragraph. All of it is on one line in the source code. > > Third paragraph. No \n appears anywhere."] > I didn't want to have unindented text ion the source code, but I'll change it. > > + return > > + } > > } > > } > > As a matter of personal taste, I prefer to structure code with error exits like so > (but it is totally acceptable if you disagree): > > if {check for error 1} { > error msg1 > return > } > if {check for error 2} { > error msg2 > return > } > regular case > goes here > without indentation > > Note that there are no else-branches. This reduces the indentation levels. > I tried to set this up - it failed because the square brackets in the if condition suddenly get "counted" as command executions or in other words; the curly braces are ignored. I was unable to get around that. > -- Hannes ------------------------------------------------------------------------------------------------- imperial-Werke oHG, Sitz Bünde, Registergericht Bad Oeynhausen - HRA 4825
diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl index e688b016ef6..ccbc1a46554 100644 --- a/git-gui/lib/mergetool.tcl +++ b/git-gui/lib/mergetool.tcl @@ -272,8 +272,24 @@ 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 {}} { + if {([string first {[} $tool_cmd] != -1) || ([string first {]} $tool_cmd] != -1)} { + error_popup [mc "Unable to process square brackets in mergetool.cmd configuration option.\ + Please remove the square brackets."] + return + } else { + foreach command_part $tool_cmd { + lappend cmdline [subst -nobackslashes -nocommands $command_part] + } + } + } else { + error_popup [mc "Unsupported merge tool '%s'.\n + Currently unsupported tools can be added and used as unsupported tools with degraded support\ + by adding the command of the tool to the \"mergetool.cmd\" option in the config. + See the configuration documentation for more details." $tool] + return + } } }