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

Add another way to resolve merge conflicts with Meld #3109

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

Conversation

amiryal
Copy link
Collaborator

@amiryal amiryal commented Feb 20, 2024

I never tried to contribute my three_meld tool back to Git because it is ugly (with all the shell escaping). With JJ it is much simpler, the shell is not involved.

Checklist

If applicable:

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

Copy link

google-cla bot commented Feb 20, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Owner

@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 haven't tested it myself, but it sounds reasonable to me. @ilyagr might have more thoughts on this, so let's wait a day or so to give him a chance to comment.

# edited, it loses the original contents of `$base`. Here we offer an
# alternative that opens the original two sides compared to the base in two
# more tabs.
merge-args = ["--diff", "$left", "$output", "$right", "--diff", "$base", "$left", "--diff", "$base", "$right"]
Copy link
Collaborator

@ilyagr ilyagr Feb 21, 2024

Choose a reason for hiding this comment

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

Have you tried making the first of these a proper merge tab (with or without auto-merge)? I'm guessing it doesn't work?

If it worked, there wouldn't be a need to pre-populate the output pane.

Copy link
Collaborator

@ilyagr ilyagr Feb 21, 2024

Choose a reason for hiding this comment

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

I actually find this merge view with jj's conflict presentation in the middle confusing; the way jj currently shows merge conflict becomes misleading in the central tab.

image

Of the two other tabs, only one is actually useful in this case of rebase:

image

It really makes it seem like "impl Input..." is the base unless you think about it carefully.

From this, you can tell that this commit renames an identifier.

The other one is not very useful (for a rebase merge conflict, at least):

image

There's this weird fact that even though it's clear what the files are in the two extra tabs, each tab is confusing on its own.

I think I'm still OK with using this as an "experimental" merge view, unless we come up with something better, but I'm thinking about putting more warnings in the description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem I have with Meld’s 3-pane merging mode is that, when there is a conflict (an area marked with red background), it stops highlighting the character-level changes and leaves you an opaque wall of code in which to hunt for changes, as in a Find The Differences game:

image
meld left base right -o output --auto-merge

This is not what I signed up for when I launched a GUI app.

If you know how to read JJ’s conflict markers, then at least you get some line-level hints with merge-tool-edits-conflict-markers = true. This is the first tab of meld-3:

image
meld left output right

I presume I don’t need to explain the conflict markers here. Just remember that the part with +++++++ will be identical to one of the sides, the context ( ) and removed (-) lines in the %%%%%%% part come from the base, and the added (+) lines would be from the side that was not chosen for +++++++. Older versions of Meld would have made it more obvious, as they did not have the red background for conflicts, and identical parts used to be not highlighted.

Going back to my pain point, the lack of character-level highlighting in 3-pane mode, this is what the other two tabs are for. They let you see the changes each side had made, highlighted nicely. Normally, only the “local” (right) side will be interesting, because these are the changes we would like to apply. It is perfectly normal in my workflow to ignore the middle tab, though sometimes I do like to peek into what the “other” (left) side was doing.

Side note: Using the 3-pane mode without --auto-merge should be reserved for pathological cases where automatic merging does the wrong thing:

image
meld left base right -o output

I hope I covered all of your concerns. Now I will take it unto myself to distil it down to a few succinct words to include in the TOML comment and in the docs.

Copy link
Collaborator

@ilyagr ilyagr Feb 23, 2024

Choose a reason for hiding this comment

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

The problem I have with Meld’s 3-pane merging mode is that, when there is a conflict (an area marked with red background), it stops highlighting the character-level changes and leaves you an opaque wall of code in which to hunt for changes, as in a Find The Differences game:

I know this problem, and it makes sense to me that showing the two extra tabs helps with it.

However, it would make more sense to me if the first tab (3-pane one) would just show the normal Meld merge view, with --auto-merge:

image

It seems less confusing than the view your current setup shows as the first tab. On the other hand, I'm not sure whether Meld can show merge tabs and diff tabs at the same time.

If what I suggest were possible, the only difference between the meld and meld-3 merge tools would be the two extra tabs.

If it's not possible, we can go with what you have now and hope to improve it later.


A somewhat out-there option, especially if Meld cannot show merge tabs and diff tabs at the same time, would be to teach jj to produce different kinds of conflict markers. For example, I think that Git-style conflict markers would be less confusing in this UI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, I know I said I'm thinking about putting more warnings in the description, but your text LGTM at the moment. I think you improved it a bit since I last looked (👍 ), but it was quite clear before as well. We can always improve it later if needed. It'd take me effort to try to add warnings without making it less clear, and it's quite possible that I'd conclude that it's better as is if I tried.

@ilyagr
Copy link
Collaborator

ilyagr commented Feb 21, 2024

Interesting idea! I'll try this out.

CHANGELOG.md Show resolved Hide resolved
@amiryal
Copy link
Collaborator Author

amiryal commented Feb 27, 2024

I am letting this PR sit open for now, if you don’t mind. Recently I had an incident where jj diffedit actually was easier to reason about and work with than jj resolve for the same conflict.

My current thinking revolves around trying to organise the myriad different commands in JJ that might have been expressed as arguments to just a handful of more expressive commands. So many commands to move changes around, only differing in how you express the desired change to them and in how change_ids get reorganised as the outcome.

@ilyagr
Copy link
Collaborator

ilyagr commented Feb 28, 2024

Recently I had an incident where jj diffedit actually was easier to reason about and work with than jj resolve for the same conflict.

This does happen regularly to me as well for simpler conflicts, but I often appreciate jj resolve as well.

@ilyagr
Copy link
Collaborator

ilyagr commented Mar 22, 2024

I've been thinking a bit about possible ways to set up conflict resolution with a diff editing tool.

Here is the simplest one (call it A, I think I also prefer it at the moment):
Screenshot 2024-03-21 at 2 17 30 PM

After the resolution, it'd look like the following. Note that you can consult the left side to double-check your work:

Screenshot 2024-03-21 at 2 19 52 PM

A variation (call it B), in case the previous version is not clear enough for long conflicts:

Screenshot 2024-03-21 at 2 19 12 PM

And here's a three-pane possibility (call it C). (I later realized the Git-style merge conflict is in the wrong order in my fabricated example, the top and middle should be switched).

Screenshot 2024-03-21 at 2 22 26 PM

After resolving, it'd look like this:

Screenshot 2024-03-21 at 2 23 24 PM

@joyously
Copy link

I find it very confusing to mix conflict markers with Meld. The markers are showing two different versions in one place, so comparing that to one or the other is a bit strange. I think if Meld is involved, it should just be between the two clean versions, with no conflict markers.
And they should be prominently labeled, but I know that's harder to do in an external tool.

@ilyagr
Copy link
Collaborator

ilyagr commented Mar 22, 2024

I think if Meld is involved, it should just be between the two clean versions, with no conflict markers.

You can already get that using the normal meld tool for jj resolve. You can get a better version of it with 4-pane merge resolution tools (that is, tools that show the base of the conflict, the two sides, and the merge result in separate panes) like kdiff3, Beyond Compare, or even vimdiff. These are not going away. (It was the first feature I ever worked on with jj :) ).

On the other hand, while I think jj's favorite way of showing merge conflicts is more confusing at first, it seems to be very effective once you are used to it in many cases. Personally, these days I seem to prefer it to 4-pane merge tools often.

The fundamental reason is that, if I see the base of the conflict and the sides separately, I often have to diff the base and one of the sides manually (in my head), and then to apply that diff to the other side. jj's presentation of the conflict does the first of these steps automatically. (There are also some conflicts where jj's presentation does not help, especially when the base of the conflict has no useful information, but it does help in most cases).

Perhaps we should document and explain it better, I did have to discover all of this on my own and think about it before I got used to it.

In any case, I'd like there to also be a way to resolve conflicts with the focus on showing a side and a diff from it. One motivation for the above is that I miss being able to see my conflict resolution and the original conflict at the same time.

And they should be prominently labeled, but I know that's harder to do in an external tool.

I'm thinking of trying to improve this situation slightly in #1176 (which I won't solve completely). Unfortunately, creating meaningful labels all the time is hard, mostly because jj's conflicts can be manipulated in complicated ways, and it's hard to have consistent rules for what the label should be in every case.

ilyagr added a commit to ilyagr/jj that referenced this pull request May 6, 2024
For example, 

```
<<<<<<< Conflict 1 of 3
+++++++ Contents of side #1
left 3.1
left 3.2
left 3.3
%%%%%%% Changes from base to side #2
-line 3
+right 3.1
>>>>>>>
```

or

```
<<<<<<< Conflict 1 of 1
%%%%%%% Changes from base to side #1
-line 3
+right 3.1
+++++++ Contents of side #2
left 3.1
left 3.2
left 3.3
>>>>>>>
```

Currently, there is no way to disable these, this is TODO for a future
PR. Other TODOs for future PRs: make these labels configurable. After
that, we could support a `diff3/git`-like conflict format as well, in
principle.

Counting conflicts helps with knowing whether you fixed all the
conflicts while you are in the editor.

While labeling "side #1", etc, does not tell you the commit id or
description as requested in martinvonz#1176, I still think it's an improvement.
Most importantly, I hope this will make `jj`'s conflict format less
scary-looking for new users.

I've used this for a bit, and I like it. Without the labels, I would see
that the two conflicts have a different order of conflict markers, but I
wouldn't be able to remember what that means. For longer diffs, it can
be tricky for me to quickly tell that it's a diff as opposed to one of
the sides. This also creates some hope of being able to navigate a
conflict with more than 2 sides.

Another not-so-secret goal for this is explained in
martinvonz#3109 (comment). The
idea is a little weird, but I *think* it could be helpful, and I'd like
to experiment with it.
ilyagr added a commit that referenced this pull request May 6, 2024
For example, 

```
<<<<<<< Conflict 1 of 3
+++++++ Contents of side #1
left 3.1
left 3.2
left 3.3
%%%%%%% Changes from base to side #2
-line 3
+right 3.1
>>>>>>>
```

or

```
<<<<<<< Conflict 1 of 1
%%%%%%% Changes from base to side #1
-line 3
+right 3.1
+++++++ Contents of side #2
left 3.1
left 3.2
left 3.3
>>>>>>>
```

Currently, there is no way to disable these, this is TODO for a future
PR. Other TODOs for future PRs: make these labels configurable. After
that, we could support a `diff3/git`-like conflict format as well, in
principle.

Counting conflicts helps with knowing whether you fixed all the
conflicts while you are in the editor.

While labeling "side #1", etc, does not tell you the commit id or
description as requested in #1176, I still think it's an improvement.
Most importantly, I hope this will make `jj`'s conflict format less
scary-looking for new users.

I've used this for a bit, and I like it. Without the labels, I would see
that the two conflicts have a different order of conflict markers, but I
wouldn't be able to remember what that means. For longer diffs, it can
be tricky for me to quickly tell that it's a diff as opposed to one of
the sides. This also creates some hope of being able to navigate a
conflict with more than 2 sides.

Another not-so-secret goal for this is explained in
#3109 (comment). The
idea is a little weird, but I *think* it could be helpful, and I'd like
to experiment with it.
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