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

FR: Add a "--tool" flag to resolve #2575

Closed
matts1 opened this issue Nov 13, 2023 · 7 comments
Closed

FR: Add a "--tool" flag to resolve #2575

matts1 opened this issue Nov 13, 2023 · 7 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@matts1
Copy link
Collaborator

matts1 commented Nov 13, 2023

Is your feature request related to a problem? Please describe.
According to the docs,

  • The ui.merge-editor key specifies the tool used for three-way merge tools by jj resolve
  • You can run jj diff --tool=<tool> to specify which tool to use to diff

It makes sense to also add this flag for conflict resolution.

Describe the solution you'd like

Describe alternatives you've considered

Additional context
Add any other context or screenshots about the feature request here.

@matts1 matts1 changed the title FR: Use merge tools for "jj resolve" FR: Add a "--tool" flag to resolve Nov 13, 2023
@ilyagr ilyagr added the enhancement New feature or request label Nov 13, 2023
@ilyagr
Copy link
Collaborator

ilyagr commented Nov 13, 2023

Makes sense.

This would also make sense for the diff editor used by jj split, jj move -i, etc.

At the moment, a workaround is to run jj --config-toml='ui.merge-editor="TOOL"' resolve.

This can be made easier with a shell script, see #2575 (comment) below.

@martinvonz
Copy link
Member

martinvonz commented Nov 13, 2023

The way we do alias expansion means that you might not even need a shell script for it; you can define an alias like this:

[aliases]
meld = ['--config-toml', 'ui.merge-editor="meld"']

And then use it with jj meld resolve. Strange but true.

@ilyagr
Copy link
Collaborator

ilyagr commented Nov 14, 2023

You can go even fancier:

[aliases]
meld = ['--config-toml', 'ui.merge-editor="meld"',
        '--config-toml', 'ui.diff-editor="meld-3"', 
        '--config-toml', 'ui.diff.tool="meld"',
       ]

Alternatively, here's the script I was thinking of. I called it jj.tool:

#!/bin/sh
TOOL="$1"
shift
jj --config-toml "[ui]
diff-editor = \"$TOOL\"
merge-editor = \"$TOOL\"
diff.tool = \"$TOOL\"" "$@";

It might be more readable if it had several --config-toml-s, like the alias example.

@ilyagr ilyagr added the good first issue Good for newcomers label Nov 15, 2023
@essiene
Copy link
Collaborator

essiene commented Dec 11, 2023

I'd like to take a stab at this is no one else is working on it.

@martinvonz
Copy link
Member

I'd like to take a stab at this is no one else is working on it.

Thanks! I've assigned this and #1457 to you.

@essiene
Copy link
Collaborator

essiene commented Jan 7, 2024

Is the --tool flag limited to only diff related external tools?

I don't know enough yet, but I'm wondering if we want to be able to easily "opt-in" a command to take a --tool flag, which can be any external command.

I will propose an approach to see if it makes sense, but I want to know what seems like a good direction in general.

@martinvonz
Copy link
Member

I think the merge tools generally take multiple arguments so just pointing to a binary is not enough. See https://martinvonz.github.io/jj/v0.13.0/config/#setting-up-a-custom-merge-tool for how they're configured. I think we'll want --tool to take a name pointing into that config section (i.e. kdiff3, meld or kdiff3 in the example on that page).

yuja added a commit to yuja/jj that referenced this issue Mar 1, 2024
This gets rid of the last UserSettings dependency from edit_diff_external().
I'm going to remove it from edit_diff() too, and let callers pass a
preconfigured MergeTool struct instead.

These changes will make it easier to add --tool=<name> argument jj-vcs#2575.
yuja added a commit to yuja/jj that referenced this issue Mar 2, 2024
This gets rid of the last UserSettings dependency from edit_diff_external().
I'm going to remove it from edit_diff() too, and let callers pass a
preconfigured MergeTool struct instead.

These changes will make it easier to add --tool=<name> argument jj-vcs#2575.
yuja added a commit that referenced this issue Mar 2, 2024
This gets rid of the last UserSettings dependency from edit_diff_external().
I'm going to remove it from edit_diff() too, and let callers pass a
preconfigured MergeTool struct instead.

These changes will make it easier to add --tool=<name> argument #2575.
yuja added a commit to yuja/jj that referenced this issue Mar 3, 2024
I didn't add e2e tests to all commands, but the added tests should cover
diff_editor/diff_selector/merge_editor() calls.

Closes jj-vcs#2575
yuja added a commit to yuja/jj that referenced this issue Mar 3, 2024
I didn't add e2e tests to all commands, but the added tests should cover
diff_editor/diff_selector/merge_editor() calls.

Closes jj-vcs#2575
@yuja yuja closed this as completed in 7ce25f8 Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants