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

conflicts: materialize simplified conflicts for each file #3719

Merged
merged 8 commits into from
Jun 14, 2024

Conversation

bnjmnt4n
Copy link
Collaborator

@bnjmnt4n bnjmnt4n commented May 20, 2024

Closes #2795.

Checklist

If applicable:

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

@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-mrtpwxolwznv branch 2 times, most recently from b642d0a to 4756533 Compare May 20, 2024 08:51
@bnjmnt4n bnjmnt4n marked this pull request as ready for review May 20, 2024 09:01
@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-mrtpwxolwznv branch from 4756533 to 58951f2 Compare May 20, 2024 15:51
cli/src/cli_util.rs Show resolved Hide resolved
lib/src/merge.rs Outdated Show resolved Hide resolved
@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-mrtpwxolwznv branch from 58951f2 to d66c17b Compare May 21, 2024 06:58
@martinvonz
Copy link
Owner

Could you also add some tests making sure that a simplified conflict can be edited? I haven't checked carefully, so I'm sorry if there already are tests for that.

@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-mrtpwxolwznv branch 3 times, most recently from 8d7d42d to bb1bd58 Compare May 21, 2024 13:43
@bnjmnt4n
Copy link
Collaborator Author

Updated. While modifying some changes in this PR, I was able to use the built version to run jj resolve, which simplified some conflicts from 3-sided to 2-sided. 😎

lib/src/local_working_copy.rs Outdated Show resolved Hide resolved
lib/src/local_working_copy.rs Outdated Show resolved Hide resolved
cli/tests/test_resolve_command.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ilyagr ilyagr left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

This PR doesn't do quite what I thought it would; I was thinking of a much more limited simplification, possibly without needing any sort of deduplicate. I wonder whether I missed something important or whether this should be simplified to do less.

lib/tests/test_local_working_copy.rs Outdated Show resolved Hide resolved
lib/src/merge.rs Outdated Show resolved Hide resolved
lib/src/merge.rs Show resolved Hide resolved
lib/src/merge.rs Outdated Show resolved Hide resolved
@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-mrtpwxolwznv branch 2 times, most recently from 7b975fd to 4496a58 Compare May 22, 2024 10:31
@bnjmnt4n bnjmnt4n changed the title Simplify conflicts for each file when materializing conflicts: materialize simplified and deduplicated conflicts for each file May 22, 2024
@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-mrtpwxolwznv branch 2 times, most recently from 3f0e03f to 4b59505 Compare May 22, 2024 11:27
@bnjmnt4n
Copy link
Collaborator Author

Actually, does anybody have an example of a set of tree merges which creates a conflict which can be further simplified on the file level? I'm not sure if I'm doing something wrong but I can't seem to figure out a good example.

@joyously
Copy link

Does "conflict simplified on the file level" include all aspects: contents, name, existence, executable bit? Or is this just about contents?

@ilyagr
Copy link
Collaborator

ilyagr commented May 24, 2024

Actually, does anybody have an example of a set of tree merges which creates a conflict which can be further simplified on the file level? I'm not sure if I'm doing something wrong but I can't seem to figure out a good example.

Here's the example I had:

ilyagr@conflictbugs

If it doesn't work for you as is, look for one of the parent commits (currently ilyagr@2a3abbe and its parent, named " Make tree-level-conflicts the default in tests" and "make new trees the default in tests
"). You may need one or both of them. (Actually, I'll make a PR out of them in a moment; I'm not sure it's "ready to go" but it might be worth discussing)

The examples you already have might also start working with those commits.

@ilyagr
Copy link
Collaborator

ilyagr commented May 24, 2024

Let me know if patching #3746 helps. I don't remember whether it was necessary for demonstrating #3223, #2895, or both.

@bnjmnt4n
Copy link
Collaborator Author

Thanks @ilyagr for the pointer, I didn't realize the code in the tests were using the legacy MergedTrees. Maybe there's a difference between the legacy and new code paths which meant my tests weren't working as expected. I'll take a look and see if there's any differences in behavior.

@bnjmnt4n
Copy link
Collaborator Author

Does "conflict simplified on the file level" include all aspects: contents, name, existence, executable bit? Or is this just about contents?

This PR will only simplify the contents of conflicted files, which have the same path. (This includes deletions.)

@ilyagr
Copy link
Collaborator

ilyagr commented May 24, 2024

Actually, does anybody have an example of a set of tree merges which creates a conflict which can be further simplified on the file level?

A more concrete example that would work for integration tests and be more or less representative of the most important case: have a merge of two commits that both modify file1 and do not touch file2. The merge will have a conflict in file1. Have another merge of two commits that both modify file2 and do not touch file1. Then, merge the two merges.

@edmundnoble
Copy link

Using this branch, I've noticed that in a merge commit, some changes from parent commits disappeared entirely. Perhaps this is the issue @ilyagr is seeing? The commit is available at https://github.com/kadena-io/chainweb-node/tree/push-lzwyzupppxrz.

@bnjmnt4n
Copy link
Collaborator Author

Actually, does anybody have an example of a set of tree merges which creates a conflict which can be further simplified on the file level?

How about a set of jj commands which cause this to happen directly to commits instead of the internal MergedTrees?

@martinvonz
Copy link
Owner

Actually, does anybody have an example of a set of tree merges which creates a conflict which can be further simplified on the file level?

How about a set of jj commands which cause this to happen directly to commits instead of the internal MergedTrees?

If I understand correctly what you're asking for, this should work:

jj git init
echo a > file1
echo a > file2
jj new @-
echo a > file1
echo b > file2
jj new @-
echo b > file1
echo c > file2
jj new 'all:visible_heads()'

You should now have a 3-sided conflict file2 and in the root tree. Because we don't do per-file simplification yet, the conflict in file1 would also be 3-sided, but it could be simplified to a 2-sided conflict.

Is that what you were looking for?

@ilyagr
Copy link
Collaborator

ilyagr commented May 29, 2024

Martin's suggestion should work fine. My suggestion from #3719 (comment) is a lot more verbose, but I find it in some ways easier to reason about:

jj new -m 'base'
echo base > fileA
echo base > fileB

jj new; jj branch create A1
echo X > fileA
jj new @-; jj branch create A2
echo Y > fileA

jj new @-; jj branch create B1
echo Z > fileB
jj new @-; jj branch create B2
echo W > fileB

# jj new A1 A2 B1 B2 would work at this point already,
# but I find the following easier to think about:

jj new A1 A2; jj branch create conflictA
jj new B1 B2; jj branch create conflictB
# conflictA has a conflict in fileA, no change to fileB
# conflictB has a conflict in fileB, no change to fileA

jj new conflictA conflictB
# The conflicts in fileA and fileB should have 2 sides
# each, but currently will have more sides.
cat fileA
cat fileB

lib/src/conflicts.rs Outdated Show resolved Hide resolved
lib/tests/test_local_working_copy.rs Outdated Show resolved Hide resolved
lib/src/conflicts.rs Outdated Show resolved Hide resolved
@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-mrtpwxolwznv branch from cf10d28 to 90988bb Compare June 9, 2024 04:59
CHANGELOG.md Outdated Show resolved Hide resolved
cli/tests/test_resolve_command.rs Outdated Show resolved Hide resolved
cli/tests/test_resolve_command.rs Outdated Show resolved Hide resolved
@bnjmnt4n bnjmnt4n marked this pull request as draft June 10, 2024 10:45
@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-mrtpwxolwznv branch 5 times, most recently from 7c24c82 to 22a7683 Compare June 10, 2024 19:31
@bnjmnt4n bnjmnt4n marked this pull request as ready for review June 10, 2024 19:34
lib/src/merge.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ilyagr ilyagr left a comment

Choose a reason for hiding this comment

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

Thank you very much! The new update_from_simplified worked out very nicely.

I had a few last comments for tests, all minor. Please also wait for Martin to approve.

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

Thanks again for this! Looking forward to this getting released.

lib/src/merge.rs Outdated Show resolved Hide resolved
lib/src/merge.rs Outdated Show resolved Hide resolved
lib/src/merge.rs Show resolved Hide resolved
lib/src/conflicts.rs Outdated Show resolved Hide resolved
@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-mrtpwxolwznv branch 2 times, most recently from 95e810e to b8c8500 Compare June 14, 2024 17:30
@bnjmnt4n bnjmnt4n enabled auto-merge (rebase) June 14, 2024 21:46
@bnjmnt4n bnjmnt4n merged commit 7c9f28a into main Jun 14, 2024
17 checks passed
@bnjmnt4n bnjmnt4n deleted the bnjmnt4n/push-mrtpwxolwznv branch June 14, 2024 22:05
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.

Simplify conflicts per file when materializing them
5 participants