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

cli rebase: add tests for weird ancestry, fix an assumption in a comment #2644

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

ilyagr
Copy link
Contributor

@ilyagr ilyagr commented Nov 27, 2023

In Git, a commit's direct parent is allowed to also be an indirect ancestor at the same time. jj currently tries to prevent this situation, but does allow it. The correctness of rebase -r A -d descendant_of_A currently depends on this jj-specific behavior; we should change that.

Cc #2600


Aside: I have plans to refactor jj rebase -r so that it becomes straightforward to conclude that it does not block fixing #2600, hopefully in the near future. It's slowed down slightly by my wanting to refactor jj new and DescendantRebaser at the same time and have them all use the same code.

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

@ilyagr ilyagr marked this pull request as ready for review November 27, 2023 20:28
@ilyagr ilyagr changed the title cli rebase: add tests for weird ancestry, fix an assumption in a comment cli rebase -r: add tests for weird ancestry, fix an assumption in a comment Nov 27, 2023
@ilyagr ilyagr force-pushed the rebasetest branch 2 times, most recently from b692fdc to 21eb893 Compare November 28, 2023 00:48
@ilyagr
Copy link
Contributor Author

ilyagr commented Nov 28, 2023

To elaborate on the "Aside" in the PR description, #2646 would be the first step of that plan. All the tests introduced here pass with that PR as well.

@ilyagr ilyagr force-pushed the rebasetest branch 2 times, most recently from e604ec6 to 8e8e70d Compare November 28, 2023 01:11
cli/tests/test_rebase_command.rs Show resolved Hide resolved
@ilyagr ilyagr force-pushed the rebasetest branch 2 times, most recently from af15dfa to 153f27a Compare November 29, 2023 00:36
@ilyagr ilyagr changed the title cli rebase -r: add tests for weird ancestry, fix an assumption in a comment cli rebase: add tests for weird ancestry, fix an assumption in a comment Nov 29, 2023
@ilyagr ilyagr force-pushed the rebasetest branch 4 times, most recently from d51eb11 to 0b3efd3 Compare November 29, 2023 01:23
Note that one of the new tests panics; this is a newly discovered bug.

In Git, a commit's direct parent is allowed to also be an indirect ancestor
at the same time. `jj` currently tries to prevent this situation, but does
allow it. The correctness of `rebase -r A -d descendant_of_A` currently depends
on this jj-specific behavior; we should change that.

Cc jj-vcs#2600
@ilyagr ilyagr enabled auto-merge (rebase) November 29, 2023 05:26
@ilyagr ilyagr merged commit bb72def into jj-vcs:main Nov 29, 2023
15 checks passed
@ilyagr ilyagr deleted the rebasetest branch November 29, 2023 05:50
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.

2 participants