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

-----Ursprüngliche Nachricht-----
Von: Johannes Sixt [off-list ref]
Gesendet: Samstag, 24. August 2024 15:38
An: Boesch, Tobias [off-list ref]
Cc: git@vger.kernel.org; ToBoMi via GitGitGadget <redacted>
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:

quoted

From: deboeto <redacted>

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

quoted

* Disabled by default, since option is most likely
    never set
* bc3 and vscode tested

Signed-off-by: deboeto <redacted>

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.

quoted

---
    git gui: add directly calling merge tool from gitconfig

Published-As:
https://github.com/gitgitgadget/git/releases/tag/pr-

1773%2FToBoMi%2Fad

quoted

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

quoted

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

quoted

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.

quoted

+                   set cmdline_config [get_config mergetool.$tool.cmd]
+                   set cmdline_substituted [subst -nobackslashes -

nocommands $cmdline_config]

quoted

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

quoted

+           } else {
+                   error_popup [mc "Unsupported merge tool '%s'"

$tool]

quoted

+                   return
+           }
    }
    }


base-commit: b9849e4f7631d80f146d159bf7b60263b3205632

-- Hannes

-------------------------------------------------------------------------------------------------
imperial-Werke oHG, Sitz Bünde, Registergericht Bad Oeynhausen - HRA 4825