Thanks for he review.
> -----Ursprüngliche Nachricht-----
> Von: Junio C Hamano <gitster@xxxxxxxxx>
> Gesendet: Mittwoch, 28. August 2024 19:08
> An: ToBoMi via GitGitGadget <gitgitgadget@xxxxxxxxx>
> Cc: git@xxxxxxxxxxxxxxx; Boesch, Tobias <tobias.boesch@xxxxxxxxx>
> Betreff: Re: [PATCH v2] git gui: add directly calling merge tool from gitconfig
>
> "ToBoMi via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: deboeto <tobias.boesch@xxxxxxxxx>
>
> 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
