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

conflcts.rs: label conflict markers with conflict #, side #, whether it's a side or a diff #3459

Merged
merged 3 commits into from
May 6, 2024

Conversation

ilyagr
Copy link
Collaborator

@ilyagr ilyagr commented Apr 5, 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
>>>>>>>

Before this PR, I found the difference between such conflicts to be quite hard to keep track of as a user.


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 used it 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.

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

lib/src/conflicts.rs Outdated Show resolved Hide resolved
@ilyagr ilyagr force-pushed the conflictsides branch 2 times, most recently from 0a16107 to 72c5e79 Compare April 5, 2024 23:58
Copy link
Collaborator

@thoughtpolice thoughtpolice left a comment

Choose a reason for hiding this comment

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

I'd really like to see this. Just a quick nit after a basic first pass.

lib/src/conflicts.rs Show resolved Hide resolved
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.

Makes sense. Thanks! Sorry it took me so long to get to this PR.

ilyagr added 3 commits May 5, 2024 18:27
…by a label

The format is 7 characters of the separator followed by a space and arbitrary
text, followed by a newline. Separator followed by a newline is also allowed.
E.g.:

<<<<<<< Random text
%%%%%%% Random text
 line 2
-line 3
+left
 line 4
+++++++ Random text
right
%%%%%%% Random text
 line 2
+forward
 line 3
 line 4
>>>>>>> Random text

This commit only allows reading such conflicts.

I considered allowing longer separators (`<<<<<<<<<<<<<< Random text`), but we
wouldn't currently write them, so let's be strict for now.

7 characters if they are followed by a space and arbitrary text
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
Copy link
Collaborator Author

ilyagr commented May 6, 2024

Thank you for the review!

1 similar comment
@ilyagr
Copy link
Collaborator Author

ilyagr commented May 6, 2024

Thank you for the review!

@ilyagr ilyagr enabled auto-merge (rebase) May 6, 2024 01:34
@ilyagr ilyagr merged commit 70b517c into martinvonz:main May 6, 2024
16 checks passed
@ilyagr ilyagr deleted the conflictsides branch May 6, 2024 01:42
ilyagr added a commit that referenced this pull request May 21, 2024
This follows up on #3459 and adds a
label to the closing delimeter of each conflict, e.g.  "Conflict 1 of 3
ends".

I didn't initially put any label at the ending delimeter since the
starting delimeter is already marked with "Conflict 1 of 3". However,
I'm now realizing that when I resolve conflicts, I usually go from top
to bottom. The first thing I do is delete the starting conflict
delimeter. It is when I get to the *end* of the conflict that I wonder
whether there are any more conflicts left in the file.
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