Skip to content

Use git difftool for merge.tool identifiers #900

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

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

Conversation

moguls753
Copy link

@moguls753 moguls753 commented Apr 17, 2025

As mentioned in rails/rails#54912, Thor’s merge_tool used to invoke the raw Git merge.tool value
directly, which fails for built‑in identifiers like vimdiff3.

This change updates merge_tool to:

  1. Use ENV["THOR_MERGE"] if set, or

  2. Fall back to running git difftool --no-index

git difftool will respect the diff.tool configuration (and pick a suitable default if none is set).

Fixes rails/rails#54912


return tool if system("command -v #{Shellwords.escape(tool)} > /dev/null 2>&1")

"git mergetool --no-prompt --tool=#{tool}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I don't think this works. git mergetool will only work when there are conflicting files when attempting to merge (in the context of git). In thor's case there is no "conflict", so git mergetool will just bailout.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're right. Maybe it would be better to use git difftool --no-index instead, since git mergetool is looking for conflicts in git's index. The manpage of git difftool says:

git difftool falls back to git mergetool config variables when the difftool equivalents have not been defined.

If nothing is configured you'll get:

This message is displayed because 'diff.tool' is not configured.
See 'git difftool --tool-help' or 'git help config' for more details.
'git difftool' will now attempt to use one of the following tools:
opendiff kdiff3 tkdiff xxdiff meld kompare gvimdiff diffuse diffmerge ecmerge p4merge araxis bc codecompare smerge nvimdiff vimdiff emerge
Viewing (1/1): '~/app/config/application.rb20250418-22187-1vvz3o.rb'
Launch 'vimdiff' [Y/n]?

This seems like the best approach; adding --no-prompt will suppress the configuration message. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may be able to confirm but this doesn't work either. git difftool will create 2 readonly buffers. So users will be presented with the diff but won't be able to make any changes. Also I don't think --no-index is a thing on the difftool command.

I had a quick look to find a solution and this is way more tricky. Basically the problem is that running a git command is not suited for what we are trying to do.
Our best bet if we wanted to stick with git and respect a user's configuration would be to create our own shell executable and use the git mergetool-lib https://git-scm.com/docs/git-mergetool--lib

But tbh this seems like a lot of work and it's probably best to ask a maintainer of this repo whether they'd be ok with such change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback! You're right git difftool opens 2 readonly buffers, but this is just a flag, you can in fact save your changes, either by using :wq! or by unsetting the flag via :set noreadonly. I think it just calls vim -R internally. From :help readonly:

'readonly' 'ro' boolean (default off)
local to buffer |local-noglobal|
If on, writes fail unless you use a '!'. Protects you from
accidentally overwriting a file. Default on when Vim is started
in read-only mode ("vim -R") or when the executable is called "view".

Regarding --no-index: git difftool is just a frontend for git diff, it does support the --no-index flag as we need it in this context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! Great, then that make sense to me 👍 .

@moguls753 moguls753 changed the title Use git mergetool for merge.tool identifiers Use git difftool for merge.tool identifiers Apr 22, 2025
@moguls753 moguls753 closed this Apr 22, 2025
@moguls753 moguls753 reopened this Apr 22, 2025
@moguls753 moguls753 requested a review from Edouard-chin April 22, 2025 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rails assumes that git's merge.tool is a command on the system
2 participants