Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

git gui: add directly calling merge tool from gitconfig #1773

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ToBoMi
Copy link

@ToBoMi ToBoMi commented Aug 15, 2024

cc: Johannes Sixt [email protected]

Changes since v1:

  • Used existing option mergetool.cmd in gitconfig to trigger the direct call of the merge tool configured in the config instead adding a new option mergeToolFromConfig
  • Removed assignment of merge tool path to a variable and reused the already existing one: merget_tool_path
  • Changed formatting of the commit message
  • Added more context and an examples to the commit message

Changes since v2:

  • Changed commit ident
  • Added hint to add a mergetool as an unsupprted tool
  • Minor typos
  • Highlighted proper nouns in commit message
  • Only using mergetool.cmd now - not using mergetool.path anymore
  • Removed gitconfig term in user message
  • Changed lines length of commit message
  • tcl commands in mergetool.cmd are now detected and not executed anymore
  • mergetool.cmd string parts are now substituted as list, not as a whole string
  • Made a more clear user hint on how to configure an unsupported mergetool

Changes since v3:

  • Corrected wrong configuration option in commit message
  • Replaced "config" and "gitconfig" with "configuration" in commit message
  • Initialised cmdline list before appending values in loop
  • Removed hint that unsupported tools have degraded support
  • Changed popup message formatting in the popup and in the source code

Changes since v4:

  • Removed trailing whitespace

cc: Johannes Sixt [email protected]

Copy link

gitgitgadget bot commented Aug 15, 2024

Welcome to GitGitGadget

Hi @ToBoMi, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that either:

  • Your Pull Request has a good description, if it consists of multiple commits, as it will be used as cover letter.
  • Your Pull Request description is empty, if it consists of a single commit, as the commit message should be descriptive enough by itself.

You can CC potential reviewers by adding a footer to the PR description with the following syntax:

CC: Revi Ewer <[email protected]>, Ill Takalook <[email protected]>

NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description,
because it will result in a malformed CC list on the mailing list. See
example.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this. Note that any reviewers CC'd via the list in the PR description will not actually be sent emails.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

Copy link

gitgitgadget bot commented Aug 15, 2024

There are issues in commit 7119d6d:
git Gui: Add directly calling merge tool from gitconfig
Commit not signed off

@ToBoMi ToBoMi force-pushed the add_merge_tool_from_config_file branch 2 times, most recently from 995601d to ad83e81 Compare August 15, 2024 13:46
@ToBoMi ToBoMi changed the title git Gui: Add directly calling merge tool from gitconfig git gui: add directly calling merge tool from gitconfig Aug 15, 2024
@ToBoMi
Copy link
Author

ToBoMi commented Aug 16, 2024

I am new to this and would like to be /allowed. I cannot link users from other pull requests. @dscho can you please allow me?

@dscho
Copy link
Member

dscho commented Aug 16, 2024

/allow

Copy link

gitgitgadget bot commented Aug 16, 2024

User ToBoMi is now allowed to use GitGitGadget.

WARNING: ToBoMi has no public email address set on GitHub;
GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use.

Copy link

gitgitgadget bot commented Aug 16, 2024

Error: Could not determine full name of ToBoMi

@ToBoMi
Copy link
Author

ToBoMi commented Aug 16, 2024

/preview

Copy link

gitgitgadget bot commented Aug 16, 2024

Preview email sent as [email protected]

@ToBoMi
Copy link
Author

ToBoMi commented Aug 16, 2024

/preview

Copy link

gitgitgadget bot commented Aug 16, 2024

Preview email sent as [email protected]

@ToBoMi ToBoMi force-pushed the add_merge_tool_from_config_file branch from ad83e81 to 59e8f45 Compare August 19, 2024 08:42
@ToBoMi
Copy link
Author

ToBoMi commented Aug 19, 2024

/submit

Copy link

gitgitgadget bot commented Aug 19, 2024

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1773/ToBoMi/add_merge_tool_from_config_file-v1

To fetch this version to local tag pr-1773/ToBoMi/add_merge_tool_from_config_file-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1773/ToBoMi/add_merge_tool_from_config_file-v1

Copy link

gitgitgadget bot commented Aug 24, 2024

On the Git mailing list, Johannes Sixt wrote (reply to this):

Am 19.08.24 um 13:29 schrieb ToBoMi via GitGitGadget:
> From: deboeto <[email protected]>
> 
> * 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 <[email protected]>

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

> 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

Copy link

gitgitgadget bot commented Aug 24, 2024

User Johannes Sixt <[email protected]> has been added to the cc: list.

@ToBoMi ToBoMi force-pushed the add_merge_tool_from_config_file branch from 59e8f45 to e77d6de Compare August 27, 2024 14:01
@ToBoMi
Copy link
Author

ToBoMi commented Aug 28, 2024

/preview

Copy link

gitgitgadget bot commented Aug 28, 2024

Preview email sent as [email protected]

@ToBoMi
Copy link
Author

ToBoMi commented Aug 28, 2024

/submit

Copy link

gitgitgadget bot commented Aug 28, 2024

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1773/ToBoMi/add_merge_tool_from_config_file-v2

To fetch this version to local tag pr-1773/ToBoMi/add_merge_tool_from_config_file-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1773/ToBoMi/add_merge_tool_from_config_file-v2

Copy link

gitgitgadget bot commented Aug 28, 2024

On the Git mailing list, Junio C Hamano wrote (reply to this):

"ToBoMi via GitGitGadget" <[email protected]> writes:

> From: deboeto <[email protected]>

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.

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.

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.

Copy link

gitgitgadget bot commented Aug 31, 2024

On the Git mailing list, Johannes Sixt wrote (reply to this):

Am 28.08.24 um 10:31 schrieb ToBoMi via GitGitGadget:
> From: deboeto <[email protected]>
> 
> 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.
> 
> 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\"

I found it annoying that I had to configure .path in addition to .cmd.
Typically, you would put the correct path into the .cmd configuration.
In fact, `git mergetool` works without .path and fails when .cmd does
not contain the correct path.

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

Good.

While testing I configured meld incorrectly once and got no feedback
whatsoever, but I would not attribute this to this patch.

There is no such thing called "gitconfig". Just strike "in gitconfig".

> If a wrong mergetool.cmd is configured by accident
> it is beeing handled by git gui already. In this
> 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.

Good.

> 
> Beyond compare 3 and Visual Studio code were
> tested as manually configured merge tools.
> 
> Signed-off-by: Tobias Boesch <[email protected]>

You updated this line, but not the From: line. Would you mind
configuring your user.name and then `git commit --amend --reset-author`?

>  git-gui/lib/mergetool.tcl | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl
> index e688b016ef6..4c4e8f47bb0 100644
> --- a/git-gui/lib/mergetool.tcl
> +++ b/git-gui/lib/mergetool.tcl
> @@ -272,8 +272,14 @@ proc merge_resolve_tool2 {} {
>  		}
>  	}
>  	default {
> -		error_popup [mc "Unsupported merge tool '%s'" $tool]
> -		return
> +		set tool_cmd [get_config mergetool.$tool.cmd]
> +		if {$tool_cmd ne {}} {
> +			set tool_cmd_file_vars_resolved [subst -nobackslashes -nocommands $tool_cmd]

I just learnt that a string value containing double-quotes is broken
into a list in the expected way (keeps quoted parts together as a single
element). However, this form of substitution replaces variable values
with arbitrary text without taking into account that the original string
is actually a list. Should we not break the string into a list first,
and apply the substitution on the list elements?

If there is a straight-forward way to do this (say, an obvious two-liner
at most), we should do it. Otherwise, I can live with this solution for
now because it requires file names with double-quotes to break the
expected list nature.

There is another thing, though, that I would not want to take as
lightly: The -nocommands modifier of `subst` does not live up to its
promises, and it is even the documented behavior: command substitutions
in array indexes are still executed. Consider this configuration:

[merge]
        tool = evil
[mergetool "evil"]
        cmd = meld \"$REMOTE([exit])\"

Guess what happens when I run the merge tool? It exits Git GUI!

I suggest to reject any configuration that contains an opening bracket
'[' or anything else that introduces a command execution.

> +			set cmdline [lreplace $tool_cmd_file_vars_resolved 0 0 $merge_tool_path]
> +		} else {
> +			error_popup [mc "Unsupported merge tool '%s'. Is the tool command and path configured properly in gitconfig?" $tool]

Can we not have a more helpful text? How about

			error_popup [mc "Unsupported merge tool '%s'.

See the git-config manual page how to configure mergetool.%s.cmd
suitably." $tool $tool]

> +			return
> +		}
>  	}
>  	}
>  

-- Hannes

Copy link

gitgitgadget bot commented Aug 31, 2024

User Johannes Sixt <[email protected]> has been added to the cc: list.

@ToBoMi ToBoMi force-pushed the add_merge_tool_from_config_file branch 2 times, most recently from 6636903 to c8c0107 Compare September 6, 2024 06:59
@ToBoMi
Copy link
Author

ToBoMi commented Sep 6, 2024

/submit

Copy link

gitgitgadget bot commented Sep 6, 2024

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1773/ToBoMi/add_merge_tool_from_config_file-v3

To fetch this version to local tag pr-1773/ToBoMi/add_merge_tool_from_config_file-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1773/ToBoMi/add_merge_tool_from_config_file-v3

Copy link

gitgitgadget bot commented Sep 6, 2024

On the Git mailing list, Johannes Sixt wrote (reply to this):

Am 06.09.24 um 08:32 schrieb [email protected]:
>> Von: Johannes Sixt <[email protected]>
>> While testing I configured meld incorrectly once and got no feedback
>> whatsoever, but I would not attribute this to this patch.
>>
> 
> That's odd. I tested this again by setting merge.tool to "meld" and
> configured mergetool.cmd to "some wrong path". When starting the
> mergetool I got a popup saying that meld was not found in path.
But if the configuration is

   cmd = "meld far too many arguments provided"

and 'meld' *is* in the path, then there is no feedback because meld can
be started successfully, but reports an error only on stdout or stderr,
which is ignored by Git GUI. And the exit code seems to be ignored, too.

But this can be treated in a follow-up patch if necessary.

-- Hannes

Copy link

gitgitgadget bot commented Sep 8, 2024

On the Git mailing list, Johannes Sixt wrote (reply to this):

Am 06.09.24 um 09:27 schrieb ToBoMi via GitGitGadget:
> From: Tobias Boesch <[email protected]>
> 
> 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.
> 
> 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.

The configuration is "mergetool.$tool.cmd"!

Personally, I would avoid the words "gitconfig" and "config" (here and
in the rest of the commit message), neither of which are English.
"Configuration" would be OK, IMO.

> 
> 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. In addtition it also shows a hint to add
> a config entry to use the tool as an unsupported tool with degraded
> support.
> 
> If a wrong mergetool.cmd is configured by accident, it gets handled
> by git gui already. In this 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.
> 
> Signed-off-by: Tobias Boesch <[email protected]>
> ---

>  git-gui/lib/mergetool.tcl | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl
> index e688b016ef6..ccbc1a46554 100644
> --- a/git-gui/lib/mergetool.tcl
> +++ b/git-gui/lib/mergetool.tcl
> @@ -272,8 +272,24 @@ proc merge_resolve_tool2 {} {
>  		}
>  	}
>  	default {
> -		error_popup [mc "Unsupported merge tool '%s'" $tool]
> -		return
> +		set tool_cmd [get_config mergetool.$tool.cmd]
> +		if {$tool_cmd ne {}} {
> +			if {([string first {[} $tool_cmd] != -1) || ([string first {]} $tool_cmd] != -1)} {
> +				error_popup [mc "Unable to process square brackets in mergetool.cmd configuration option.\
> +								Please remove the square brackets."]
> +				return

Condition and error text are OK. But see below.

> +			} else {
> +				foreach command_part $tool_cmd {
> +					lappend cmdline [subst -nobackslashes -nocommands $command_part]
> +				}

Good.

I have seen a few examples in the Tcl manual with lappend in the loop
body, and it seems to be customary to set the list variable to an empty
value before the loop, i.e.

				set cmdline {}

> +			}
> +		} else {
> +			error_popup [mc "Unsupported merge tool '%s'.\n
> +							Currently unsupported tools can be added and used as unsupported tools with degraded support\
> +							by adding the command of the tool to the \"mergetool.cmd\" option in the config.
> +							See the configuration documentation for more details." $tool]

This error message needs a bit more work (some of this also applies to
the message above):

- A tool is only unsupported as long as there is no usable
configuration. Once mergetool.$tool.cmd is set to something we can
handle, calling the tool "unsupported" isn't appropriate, I would think.
How about

Unsupported merge tool '%s'.

To use this tool, configure "mergetool.%s.cmd" as shown in the
git-config manual page.

- The configuration variable that we use is not mergetool.cmd, but
mergetool.$tool.cmd.

- Continuation lines must not be indented. Indented text appears
indented in the error message.

- Watch out whether an explicit \n is given, whether the line-break is
escaped or not; all of this has meaning.

- Looking at other multi-line error messages in git-gui.sh, the
convention is

	mc["First paragraph goes here.

Second paragraph. All of it is on one line in the source code.

Third paragraph. No \n appears anywhere."]

> +			return
> +		}
>  	}
>  	}

As a matter of personal taste, I prefer to structure code with error
exits like so (but it is totally acceptable if you disagree):

   if {check for error 1} {
       error msg1
       return
   }
   if {check for error 2} {
       error msg2
       return
   }
   regular case
   goes here
   without indentation

Note that there are no else-branches. This reduces the indentation levels.

-- Hannes

@ToBoMi ToBoMi force-pushed the add_merge_tool_from_config_file branch from c8c0107 to b5db899 Compare September 11, 2024 14:19
@ToBoMi
Copy link
Author

ToBoMi commented Sep 11, 2024

/submit

Copy link

gitgitgadget bot commented Sep 11, 2024

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1773/ToBoMi/add_merge_tool_from_config_file-v4

To fetch this version to local tag pr-1773/ToBoMi/add_merge_tool_from_config_file-v4:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1773/ToBoMi/add_merge_tool_from_config_file-v4

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 configuration.
New merge tools can be set up there without a source code change.

Those options are used only by pure git in contrast to git gui. git calls
the configured merge tools directly from the configuration while git Gui
doesn't.

With this change git gui can call merge tools configured in the
configuration directly without a change in git gui source code.
It needs a configured "merge.tool" and a configured
"mergetool.<mergetool name>.cmd" configuration entry as shown in the
git-config manual page.

Configuration 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. In addtition it also shows a hint to add
a configuration entry to use the tool as an unsupported tool with degraded
support.

If a wrong "mergetool.cmd" is configured by accident, it gets handled
by git gui already. In this 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.

Signed-off-by: Tobias Boesch <[email protected]>
@ToBoMi ToBoMi force-pushed the add_merge_tool_from_config_file branch from b5db899 to c37db60 Compare September 12, 2024 10:14
@ToBoMi
Copy link
Author

ToBoMi commented Sep 12, 2024

/submit

Copy link

gitgitgadget bot commented Sep 12, 2024

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1773/ToBoMi/add_merge_tool_from_config_file-v5

To fetch this version to local tag pr-1773/ToBoMi/add_merge_tool_from_config_file-v5:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1773/ToBoMi/add_merge_tool_from_config_file-v5

Copy link

gitgitgadget bot commented Sep 14, 2024

On the Git mailing list, Johannes Sixt wrote (reply to this):

Am 12.09.24 um 12:17 schrieb ToBoMi via GitGitGadget:
> Configuration example:
> [merge]
> 	tool = vscode
> [mergetool "vscode"]
> 	path = the/path/to/Code.exe
> 	cmd = \"Code.exe\" --wait --merge \"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$MERGED\"

This example is not up-to-date anymore, is it?

Also, below are two cases where "mergetool.cmd" is mentioned
incorrectly.

> 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. In addtition it also shows a hint to add
> a configuration entry to use the tool as an unsupported tool with degraded
> support.
> 
> If a wrong "mergetool.cmd" is configured by accident, it gets handled
> by git gui already. In this 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.

> --- a/git-gui/lib/mergetool.tcl
> +++ b/git-gui/lib/mergetool.tcl
> @@ -272,8 +272,26 @@ proc merge_resolve_tool2 {} {
>  		}
>  	}
>  	default {
> -		error_popup [mc "Unsupported merge tool '%s'" $tool]
> -		return
> +		set tool_cmd [get_config mergetool.$tool.cmd]
> +		if {$tool_cmd ne {}} {
> +			if {([string first {[} $tool_cmd] != -1) || ([string first {]} $tool_cmd] != -1)} {
> +				error_popup [mc "Unable to process square brackets in mergetool.$tool.cmd configuration option.

This $tool in the format string breaks [mc]. It must be %s and an
argument. I'll fix this up while queuing.

> +
> +Please remove the square brackets."]
> +				return
> +			} else {
> +				set cmdline {}
> +				foreach command_part $tool_cmd {
> +					lappend cmdline [subst -nobackslashes -nocommands $command_part]
> +				}
> +			}
> +		} else {
> +			error_popup [mc "Unsupported merge tool '%s'.
> +
> +To use this tool, configure \"mergetool.%s.cmd\" as shown in the git-config\> +manual page." $tool $tool]

I am surprised that the backslash does not paste the two lines together
without a space. "git-config" and "manual" do appear as separate words
in the error message. Nevertheless, since I do not know how this pans
out in the translation files, I will remove the line continuation and
write all on one line.

> +			return
> +		}
>  	}
>  	}

Thank you for your contribution! Below is the range-diff between this
submission and the queued version.

-- Hannes

1:  03e92d6 ! 1:  8ff65c7 git gui: add directly calling merge tool from configuration
    @@ Commit message
         [merge]
                 tool = vscode
         [mergetool "vscode"]
    -            path = the/path/to/Code.exe
    -            cmd = \"Code.exe\" --wait --merge \"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$MERGED\"
    +            cmd = \"the/path/to/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. In addtition it also shows a hint to add
    -    a configuration entry to use the tool as an unsupported tool with degraded
    -    support.
    +    Without the "mergetool.<mergetool name>.cmd" entry and an unsupported
    +    "merge.tool" entry, git gui behaves mainly as before this change and
    +    informs the user about an unsupported merge tool. In addtition, it also
    +    shows a hint to add a configuration entry to use the tool as an
    +    unsupported tool with degraded support.
     
    -    If a wrong "mergetool.cmd" is configured by accident, it gets handled
    -    by git gui already. In this 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.
    +    If a wrong "mergetool.<mergetool name>.cmd" is configured by accident,
    +    it gets handled by git gui already. In this 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.
     
         Signed-off-by: Tobias Boesch <[email protected]>
    +    Signed-off-by: Johannes Sixt <[email protected]>
     
      ## lib/mergetool.tcl ##
     @@ lib/mergetool.tcl: proc merge_resolve_tool2 {} {
    @@ lib/mergetool.tcl: proc merge_resolve_tool2 {} {
     +		set tool_cmd [get_config mergetool.$tool.cmd]
     +		if {$tool_cmd ne {}} {
     +			if {([string first {[} $tool_cmd] != -1) || ([string first {]} $tool_cmd] != -1)} {
    -+				error_popup [mc "Unable to process square brackets in mergetool.$tool.cmd configuration option.
    ++				error_popup [mc "Unable to process square brackets in \"mergetool.%s.cmd\" configuration option.
     +
    -+Please remove the square brackets."]
    ++Please remove the square brackets." $tool]
     +				return
     +			} else {
     +				set cmdline {}
    @@ lib/mergetool.tcl: proc merge_resolve_tool2 {} {
     +		} else {
     +			error_popup [mc "Unsupported merge tool '%s'.
     +
    -+To use this tool, configure \"mergetool.%s.cmd\" as shown in the git-config\
    -+manual page." $tool $tool]
    ++To use this tool, configure \"mergetool.%s.cmd\" as shown in the git-config manual page." $tool $tool]
     +			return
     +		}
      	}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants