> -----Ursprüngliche Nachricht-----
> Von: Johannes Sixt <j6t@xxxxxxxx>
> Gesendet: Samstag, 24. August 2024 15:38
> An: Boesch, Tobias <tobias.boesch@xxxxxxxxx>
> Cc: git@xxxxxxxxxxxxxxx; ToBoMi via GitGitGadget <gitgitgadget@xxxxxxxxx>
> Betreff: Re: [PATCH] git gui: add directly calling merge tool from gitconfig
>
Thanks for the review.
> Am 19.08.24 um 13:29 schrieb ToBoMi via GitGitGadget:
> > From: deboeto <tobias.boesch@xxxxxxxxx>
> >
> > * 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?
>
I think that would work. I'll change the patch to use the mergetool.cmd variable
as the trigger for using the configured merge tool.
If the mergetool configuration option is set to a supported tool
(merge.tool = vscode) it will cause git gui to use the supported tool (hard coded
into the source code - as before this change).
If the mergetool configuration option is set to an UNsupported tool and
mergetool.cmd is set for the chosen mergetool it will use the command from
that option.
Patch will be submitted soon.
> 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?
>
I tested this together with the change of the activating option mentioned before.
With that in mind I cannot create a case were the mergetool.cmd is used on a
supported tool, because mergetool.cmd is only used for UNsupported tools.
1. Assuming the merge.tool is set to a supported tool:
- In this case the supported tool is used, no matter if the mergetool.cmd
is set or not
2. Assuming the merge.tool is set to an UNsupported tool:
- Then the variable IS evaluated
- If it is set to an invalid command or a wrong mergetool.path is given,
Git gui will complain as before this change that the command is not
Found in PATH
> 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?
>
Currently it doesn't.
I don't know if it should, because I guess that git also has no other possibility
than to call this command for a merge unconditionally - even when the base file
name is empty.
I haven't had such a case that I can remember. How can it be triggered?
Doesn't all merges have a common ancestor as long as the histories are related?
> > * Disabled by default, since option is most likely
> > never set
> > * bc3 and vscode tested
> >
> > Signed-off-by: deboeto <tobias.boesch@xxxxxxxxx>
>
> 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.
Corrected - please review again.
>
> > ---
> > git gui: add directly calling merge tool from gitconfig
> >
> > Published-As:
> > https://github.com/gitgitgadget/git/releases/tag/pr-
> 1773%2FToBoMi%2Fad
> > d_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.
>
> > 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.
>
> > + } else {
> > + error_popup [mc "Unsupported merge tool '%s'"
> $tool]
> > + return
> > + }
> > }
> > }
> >
> >
> > base-commit: b9849e4f7631d80f146d159bf7b60263b3205632
>
> -- Hannes
-------------------------------------------------------------------------------------------------
imperial-Werke oHG, Sitz Bünde, Registergericht Bad Oeynhausen - HRA 4825
