| Message ID | pull.1773.v2.git.1724833917245.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] git gui: add directly calling merge tool from gitconfig | expand |
"ToBoMi via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: deboeto <tobias.boesch@miele.com> Use the same ident (human readable name plus e-mail address) you have on your Signed-off-by: line below for this "From: " in-body header. > 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. Even if you configure an unknown tool, it would not get any benefit from what git-{diff,merge}tool--lib.sh gives the known diff/merge backends, would it? Instead of a more thorough support for known tools done in setup_tool(), an unknown tool would be handled by setup_user_tool() in git-mergetool-lib.sh which gives somewhat degraded support. So "can be set up without" may be true, but giving an impression that a tool that is set up like so would work just like a known tool is misleading. By the way, we do ask contributors to avoid overly long lines, 50-col limt is a bit overly short and makes the resulting text harder to read than necessary. > 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\" > > 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. > > If a wrong mergetool.cmd is configured by accident > it is beeing handled by git gui already. In this "is beeing" -> "is being", but "it gets handled by Git GUI already" should be sufficient. > 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. Quote proper nouns for readability? E.g. "Beyond Compare 3" and "Visual Studio Code" were ... Thanks.
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
Thanks for he review. > -----Ursprüngliche Nachricht----- > Von: Junio C Hamano <gitster@pobox.com> > Gesendet: Mittwoch, 28. August 2024 19:08 > An: ToBoMi via GitGitGadget <gitgitgadget@gmail.com> > Cc: git@vger.kernel.org; Boesch, Tobias <tobias.boesch@miele.com> > Betreff: Re: [PATCH v2] git gui: add directly calling merge tool from gitconfig > > "ToBoMi via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: deboeto <tobias.boesch@miele.com> > > Use the same ident (human readable name plus e-mail address) you have on > your Signed-off-by: line below for this "From: " in-body header. > > > 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. > > Even if you configure an unknown tool, it would not get any benefit from > what git-{diff,merge}tool--lib.sh gives the known diff/merge backends, would > it? Instead of a more thorough support for known tools done in setup_tool(), > an unknown tool would be handled by > setup_user_tool() in git-mergetool-lib.sh which gives somewhat degraded > support. > That might be. I don't know about this handling. Would it be a problem to not have this handling for unsupported tools? Since the concept of supported tools is not removed by this patch, tools can still be added as supported, to get this thorough handling. I personally prefer to have an unsupported tool, that I can configure and use right now and add official support for it later, instead of having some well-supported tools which exclude the tool I want to use right now and no option to add it quickly. That is why I didn't add support for the tool I want to use right now. I wanted it to be more universal, so that every tool I can configure will work immediately. > So "can be set up without" may be true, but giving an impression that a tool > that is set up like so would work just like a known tool is misleading. > I don't want this patch to give that impression. How can this be avoided from your point of view? The degraded functionality for unsupported tools could be mentioned in the message for an unsupported tool introduced enhanced in this patch. It could tell the user that the current tool is not supported, but that it can be setup with degraded support in the config. An updated message will be part of the next patch. > By the way, we do ask contributors to avoid overly long lines, 50-col limt is a > bit overly short and makes the resulting text harder to read than necessary. > > > 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\" > > > > 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. > > > > If a wrong mergetool.cmd is configured by accident it is beeing > > handled by git gui already. In this > > "is beeing" -> "is being", but "it gets handled by Git GUI already" > should be sufficient. > > > 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. > > Quote proper nouns for readability? E.g. > > "Beyond Compare 3" and "Visual Studio Code" were ... > > Thanks. I'll correct the minor suggestions in the next patch version. ------------------------------------------------------------------------------------------------- imperial-Werke oHG, Sitz Bünde, Registergericht Bad Oeynhausen - HRA 4825
> -----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. > 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@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? > 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
Am 06.09.24 um 08:32 schrieb tobias.boesch@miele.com: >> Von: Johannes Sixt <j6t@kdbg.org> >> 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. But if the configuration is cmd = "meld far too many arguments provided" and 'meld' *is* in the path, then there is no feedback because meld can be started successfully, but reports an error only on stdout or stderr, which is ignored by Git GUI. And the exit code seems to be ignored, too. But this can be treated in a follow-up patch if necessary. -- Hannes
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] + 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] + return + } } }