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

external diff-editors: allow 3-pane diff, set up meld-3. #2003

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

ilyagr
Copy link
Collaborator

@ilyagr ilyagr commented Aug 8, 2023

Implementation for 3-pane diff as discussed in #1905 (reply in thread).

To test it out, patch this PR and try

cargo run -- --config-toml 'ui.diff-editor="meld-3"' diffedit -r @-

If you'd like to try programs other than meld, just use a merge tool config that has $output in its edit-args.

I am also thinking about trying out a conflict resolution interface based on this.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (src/config-schema.json)
  • I have added tests to cover my changes

@ilyagr
Copy link
Collaborator Author

ilyagr commented Aug 8, 2023

Re the HELP WANTED commit: @yuja suggested I simply have a separate function to check if a variable is in the args. I'll probably do that at some point.

The biggest issue slowing down this PR is now the lack of tests.

@ilyagr ilyagr force-pushed the 3pane branch 6 times, most recently from a3064cd to bec6547 Compare August 15, 2023 02:56
@Dr-Emann
Copy link
Collaborator

Dr-Emann commented Aug 15, 2023

Wild idea, we could do something like

enum InterpolationChunk<'a> {
    Literal(&'a str),
    Variable {
        name: &'a str,  // e.g. "output"
        original: &'a str, // e.g. "$output", maybe someday "${output}"
    }
}

fn interpolated_variables(s: &'_ str) -> impl Iterator<Item=InterpolationChunk<'_>> {}

(of course with better names. VariableStrChunk? TemplateChunk?)

Which would allow doing the replacements and querying if a variable is present efficiently.

@ilyagr ilyagr force-pushed the 3pane branch 11 times, most recently from b5e89c3 to 8a0a78b Compare August 21, 2023 04:30
@ilyagr ilyagr changed the title (Draft) Allow 3-pane diff external diff-editors: allow 3-pane diff, set up meld-3. Aug 21, 2023
@ilyagr
Copy link
Collaborator Author

ilyagr commented Aug 21, 2023

@Dr-Emann That's a nice idea. If we start doing something more complicated with arguments, we should try it. For now, I'll just follow Yuya's simple suggestion.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Aug 21, 2023

This PR is a bit more polished now. I did not completely adapt JJ-INSTRUCTIONS text to the 3-pane diff, but I hope that the preambles I added will eliminate confusion.

I may add tests for JJ-INSTRUCTIONS to this in the future or in a separate PR.

I am not sure whether to remove the (currently unused) possibility to start out $output with the left side of the diff. I am considering using that for conflict resolution, if I ever get to that.

@ilyagr ilyagr marked this pull request as ready for review August 21, 2023 04:35
@ilyagr ilyagr force-pushed the 3pane branch 4 times, most recently from 64dd590 to 94c4c07 Compare August 21, 2023 06:08
Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

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

Thanks! I'm curious to hear feedback about this once people start using it. I think it sounds like a big improvement.

cli/src/merge_tools/external.rs Outdated Show resolved Hide resolved
cli/src/merge_tools/external.rs Outdated Show resolved Hide resolved
@ilyagr
Copy link
Collaborator Author

ilyagr commented Aug 21, 2023

Yeah, I've been using this and I like it quite a bit!

I'm not super-happy about it from the technical perspective: I can't get this to work nicely with anything other than meld (vimdiff will likely work, but it takes some work), and this should really be a difftool's job. A script calling meld would have worked, but not on Windows. Overall, I think this is the best we can do right now.

@ilyagr ilyagr enabled auto-merge (rebase) August 22, 2023 01:48
Copy link
Collaborator

@yuja yuja left a comment

Choose a reason for hiding this comment

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

Just fyi, kdiff3's behavior would be close to meld-3 since the diff panes are readonly.

cli/src/merge_tools/external.rs Outdated Show resolved Hide resolved
@ilyagr
Copy link
Collaborator Author

ilyagr commented Aug 22, 2023

Thanks, Yuya!

Just fyi, kdiff3's behavior would be close to meld-3 since the diff panes are readonly.

It is similar, even without any need for the trickery with meld. However, it's frustratingly disfunctional. Let me know if you know how to get it to work.

The issues are:

  1. The output window starts with a merge conflict for every diff.
  2. There seems to be a command designed to fix it: "Merge=> Select B everywhere". However, it doesn't work for me. Kdiff3 starts telling me weird things I don't understand about "losing unsaved merge". I think that command saves the file in addition to selecting B everywhere? In any case, I couldn't get it to work. Let me know if you can.

I was thinking of filing a feature request to execute "Select B everywhere" after starting a "diff merge" (in KDiff3 parlance), but issue number 2 made me throw my hands up in frustration.

@yuja
Copy link
Collaborator

yuja commented Aug 22, 2023

it's frustratingly disfunctional.

Yep. I don't recommend kdiff3 over meld for diff editing. I just mean it's similar to meld ... -o $right.

  1. The output window starts with a merge conflict for every diff.

  2. There seems to be a command designed to fix it: "Merge=> Select B everywhere". However, it doesn't work for me. Kdiff3 starts telling me weird things I don't understand about "losing unsaved merge". I think that command saves the file in addition to selecting B everywhere? In any case, I couldn't get it to work. Let me know if you can.

I think you could do that in "Directory merge" pane (select A or B, then "Run Operation for Current Item), but it's not intuitive. I don't know a better option.

@ilyagr ilyagr merged commit 038867f into jj-vcs:main Aug 22, 2023
@ilyagr ilyagr deleted the 3pane branch August 22, 2023 03:19
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.

4 participants