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

Launch Diff Tool does not work for resolving merge conflicts under Xfce #524

Closed
anderben opened this Issue Nov 23, 2015 · 6 comments

Comments

Projects
None yet
2 participants
@anderben

anderben commented Nov 23, 2015

I'm using linux mint 17.2 with the Xfce desktop and git-cola 2.4. I've configured kdiff3 as my diff tool and merge tool.

In a 'merge with conflicts' scenario, selecting 'Launch Diff Tool' on a staged file with no-conflicts will successfully launch kdiff3. However, selecting 'Launch Diff Tool' on an unmerged file with conflicts fails to launch kdiff3. The error in the git-cola terminal is:

xfce4-terminal: Unknown option "mergetool"

#354 raised the issue that git-cola has an undocumented dependency on xterm, which commit 6b55a71 was supposed to fix but it looks like xfce4-terminal is unable to parse the command used to launch kdiff3 in 3-way merge mode when xterm is not available.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Nov 24, 2015

Member

Does it work if you configure yours to use xterm? We need to be able to launch a terminal and a sub-command within it. Not all terminals support this.

If you know the secret sauce for xfce4-terminal then we could look into special-casing it.

Member

davvid commented Nov 24, 2015

Does it work if you configure yours to use xterm? We need to be able to launch a terminal and a sub-command within it. Not all terminals support this.

If you know the secret sauce for xfce4-terminal then we could look into special-casing it.

@anderben

This comment has been minimized.

Show comment
Hide comment
@anderben

anderben Nov 24, 2015

I was able to get it to work with xterm once I installed xterm and explicitly set cola.terminal to use it.
As for the secret sauce, I had a play around with the git command-line and I'm guessing the issue may be that the command to execute in the xfce4-terminal is not enclosed in quotes i.e.

xfce4-terminal -e git mergetool --no-prompt --tool=kdiff3 myConflictedFile

fails with the error reported above, however:

xfce4-terminal -e "git mergetool --no-prompt --tool=kdiff3 myConflictedFile"

does work correctly and launches kdiff3 as expected.

anderben commented Nov 24, 2015

I was able to get it to work with xterm once I installed xterm and explicitly set cola.terminal to use it.
As for the secret sauce, I had a play around with the git command-line and I'm guessing the issue may be that the command to execute in the xfce4-terminal is not enclosed in quotes i.e.

xfce4-terminal -e git mergetool --no-prompt --tool=kdiff3 myConflictedFile

fails with the error reported above, however:

xfce4-terminal -e "git mergetool --no-prompt --tool=kdiff3 myConflictedFile"

does work correctly and launches kdiff3 as expected.

@davvid davvid closed this in 343dd36 Nov 24, 2015

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Nov 24, 2015

Member

This same issue also made it so that gnome-terminal is usable.

konsole and xterm, from an outsider's perspective, have the most sane handling for the -e flag. I would almost suggest filing a sug against xfce4-terminal and gnome-terminal to make their -e flags consistent. It's also nice to not require the user to pack the arguments into a single string, as it minimizes shell quoting issues.

We now special-case these so at least it works. Thanks for the help.

Member

davvid commented Nov 24, 2015

This same issue also made it so that gnome-terminal is usable.

konsole and xterm, from an outsider's perspective, have the most sane handling for the -e flag. I would almost suggest filing a sug against xfce4-terminal and gnome-terminal to make their -e flags consistent. It's also nice to not require the user to pack the arguments into a single string, as it minimizes shell quoting issues.

We now special-case these so at least it works. Thanks for the help.

@anderben

This comment has been minimized.

Show comment
Hide comment
@anderben

anderben Nov 25, 2015

No worries. I forgot to mention any existing quotes within the command string will need to be escaped.

anderben commented Nov 25, 2015

No worries. I forgot to mention any existing quotes within the command string will need to be escaped.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Nov 26, 2015

Member

Would you mind testing the latest "master" branch and see if it's better behaved? It should do the right thing.

Member

davvid commented Nov 26, 2015

Would you mind testing the latest "master" branch and see if it's better behaved? It should do the right thing.

@anderben

This comment has been minimized.

Show comment
Hide comment
@anderben

anderben Nov 29, 2015

I gave master a quick spin and it looks like it works ok now. I didn't have conflicted file with embedded quotes in the name handy so I wasn't able to check if that scenario was handled correctly, but that's probably a pretty rare use case.

anderben commented Nov 29, 2015

I gave master a quick spin and it looks like it works ok now. I didn't have conflicted file with embedded quotes in the name handy so I wasn't able to check if that scenario was handled correctly, but that's probably a pretty rare use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment