AW: [PATCH] git gui: add directly calling merge tool from gitconfig

> -----Ursprüngliche Nachricht-----
> Von: Johannes Sixt <j6t@kdbg.org>
> Gesendet: Samstag, 24. August 2024 15:38
> An: Boesch, Tobias <tobias.boesch@miele.com>
> Cc: git@vger.kernel.org; ToBoMi via GitGitGadget <gitgitgadget@gmail.com>
> Betreff: Re: [PATCH] git gui: add directly calling merge tool from gitconfig
>

> Am 19.08.24 um 13:29 schrieb ToBoMi via GitGitGadget:
> > 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?
>
> > * 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.
>
> > ---
> >     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.
>

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