Re: [PATCH] git gui: add directly calling merge tool from gitconfig
- From
Johannes Sixt <j6t@kdbg.org>
- Date
- Aug 24, 2024, 13:38 UTC
- Message-ID
- <57d00f50-c652-4357-bf9b-02b93f99cfb5@kdbg.org>
- In-Reply-To
- <pull.1773.git.1724066944786.gitgitgadget@gmail.com>
Am 19.08.24 um 13:29 schrieb ToBoMi via GitGitGadget:
Show 13 quoted lines
> From: deboeto <tobias.boesch@miele.com> > > * git Gui can open a merge tool when conflicts are > detected. The merge tools that are allowed to > call have to be hard coded into git Gui > althgough there are configuration options for > merge tools git in the git config. Git calls > the configured merge tools directly from the > config while git Gui doesn't. > * git Gui can now call the tool configured in the > gitconfig directly. > * Can be enabled through setting > gui.mergeToolFromConfig
Can we do better than having a new configuration variable? Let's say you have configured merge.tool=vscode. This tool is not supported, but you have configured mergetool.vscode.cmd suitably. Can we not use the latter configuration variable unconditionally?
Likewise, say, you have configured merge.tool=bc3. This one *is* supported. What could go wrong if mergetool.bc3.cmd is used instead of the built-in command line? The behavior would change for users that configured mergetool.$tool.cmd for a supported tool. But would it change for the worse?
BTW, the code builds different command lines depending on whether a base file is available or not. How does mergetool.$tool.cmd handle the cases?
Show 5 quoted lines
> * Disabled by default, since option is most likely > never set > * bc3 and vscode tested > > Signed-off-by: deboeto <tobias.boesch@miele.com>
Some remarks on the commit message:
- The Signed-off-by line has legal consequences. Therefore, we require that authors use their genuine name, not an alias. Also, the From line must match the Signed-off-by line.
- Please have a look at the commit messages in the code base. The formatting presented here is very unusual. Please write in full sentences including punctuation, and use paragraphs where needed.
- Please state the problem that is being solved (in present tense). This should motivate the change, i.e., provide a convincing argument why the change is needed. Then state what the solution is in imperative mood, that is, an instruction to the code to change in such and such way. Use examples to clarify how the new feature can be used.
Show 24 quoted lines
> --- > git gui: add directly calling merge tool from gitconfig > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1773%2FToBoMi%2Fadd_merge_tool_from_config_file-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1773/ToBoMi/add_merge_tool_from_config_file-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1773 > > Documentation/config/gui.txt | 4 ++++ > git-gui/lib/mergetool.tcl | 11 +++++++++-- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/Documentation/config/gui.txt b/Documentation/config/gui.txt > index 171be774d24..e63d0b46e7c 100644 > --- a/Documentation/config/gui.txt > +++ b/Documentation/config/gui.txt > @@ -55,3 +55,7 @@ gui.blamehistoryctx:: > linkgit:gitk[1] for the selected commit, when the `Show History > Context` menu item is invoked from 'git gui blame'. If this > variable is set to zero, the whole history is shown. > + > +gui.mergeToolFromConfig:: > + If true, allow to call the merge tool configured in gitconfig > + in git gui directly. > \ No newline at end of file
Unfortunately, Documentation/config/gui.txt is not part of the Git GUI repository. Any changes to the documentation must be submitted as separate patch.
Please be careful not to introduce an incomplete last lines. Take note of "No newline at end of file". It should not be there.
Show 12 quoted lines
> diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl
> index e688b016ef6..fbd0889612a 100644
> --- a/git-gui/lib/mergetool.tcl
> +++ b/git-gui/lib/mergetool.tcl
> @@ -272,8 +272,15 @@ proc merge_resolve_tool2 {} {
> }
> }
> default {
> - error_popup [mc "Unsupported merge tool '%s'" $tool]
> - return
> + if {[is_config_true gui.mergetoolfromconfig]} {
> + set path [get_config mergetool.$tool.path]At this point, the value assigned to $path here is already available in $merge_tool_path.
> + set cmdline_config [get_config mergetool.$tool.cmd] > + set cmdline_substituted [subst -nobackslashes -nocommands $cmdline_config] > + set cmdline [lreplace $cmdline_substituted 0 0 $path]
I haven't yet taken the time to study what these lines do (I am far from fluent in Tcl) and have no opinion, yet.
Show 9 quoted lines
> + } else {
> + error_popup [mc "Unsupported merge tool '%s'" $tool]
> + return
> + }
> }
> }
>
>
> base-commit: b9849e4f7631d80f146d159bf7b60263b3205632-- Hannes