Prevent rebasing onto both commits and their ancestrs

[ilyagr]

Previously, something like rebase -r @ -d a -d a-- created a commit that has one parent be an ancestor of another parent. Now, such ancestors are automatically removed from the list.

The implementation simply moves the check that I introduced in https://github.com/martinvonz/jj/commit/67738a87542dbaa1ab0ce0460ebcbddf4d53c7fe from rebase_revision to the more low-level rebase_commit.

There are two potential issues to resolve still:

• The user should probably be informed when this occurs, similarly to https://github.com/martinvonz/jj/commit/b21a123bc8947e51f84d498e78c952d8511a25bc (which deals with a special case of this situation). I'm not sure how best to do that (should there be some system for functions to generate user-facing warnings?).

• I'm not sure if the use of unwrap in rebase_commit is kosher, but I was not sure how feasible it is to make the return type of rebase_commit be a Result.

---

This also includes a separate commit consolidating a test I wrote previously with other tests. I'm not sure whether this makes the resulting test difficult to follow, let me know.

[martinvonz]

Previously, something like rebase -r @ -d a -d a-- created a commit that has one parent be an ancestor of another parent. Now, such ancestors are automatically removed from the list.

It's not uncommon to create merges where one parent is an ancestor of the other. GitHub does that with its default merge method (not the rebase or squash methods). So that probably means that we want to allow the user to create such commits, but I'm not sure if we want to let them use jj rebase for it. It seems pretty harmless to allow it to me.

The implementation simply moves the check that I introduced in 67738a8 from rebase_revision to the more low-level rebase_commit.

There are two potential issues to resolve still:

My biggest concern is that it now happens at such a low level that code that explicitly wants to create such commits is harder to write. However, that's a pre-existing problem because we already remove ancestors in low-level code when we auto-rebase descendants. For example, if you do jj new -m merge @ @- and then jj describe -m 'new description' @--, the merge will go away. That seems like a bug. I'm not sure what the right fix is. For example, let's say we run jj describe on B here:

E
|\
| D
|/
C
|
B
|
A

Then it seems like we should preserve the shape and let the rewritten E have two parents. What if the user instead ran jj abandon B? Does that change the answer? How about jj abandon C? How about jj rebase -s C -d A? I don't know what I think.

• The user should probably be informed when this occurs, similarly to b21a123 (which deals with a special case of this situation). I'm not sure how best to do that (should there be some system for functions to generate user-facing warnings?).

It should probably even be an error if the user ran jj rebase -d a -d a-- and it should probably be silent if it happened e.g. when rebasing D while running jj abandon C in this case:

D
|\
B C
|/
A

• I'm not sure if the use of unwrap in rebase_commit is kosher, but I was not sure how feasible it is to make the return type of rebase_commit be a Result.

That should be easy to do. You can probably just change the type and add a few ? operators to call sites.

This also includes a separate commit consolidating a test I wrote previously with other tests. I'm not sure whether this makes the resulting test difficult to follow, let me know.

I haven't looked at the test yet.

[ilyagr]

Your thoughts sound reasonable. I can think of two different approaches here.

Approach 1: Full support for confusing parentage

Ignoring the implementation details, I think it would be self-consistent for jj to fully support a parent of a commit being an ancestor of another parent (let's call it "confusing parentage"). This would mean that all the commands you mentioned in your "Then it seems like we should preserve the shape..." paragraph should preserve the shape, since that's just preserving existing confusing parentage.

We could still have high-level commands -- e.g. jj abandon or jj rebase -r when applied to a parent of a merge commit as in https://github.com/martinvonz/jj/commit/67738a87542dbaa1ab0ce0460ebcbddf4d53c7fe -- default to not creating new instances of confusing parentage, like they do now. However, if we are fully supporting confusing parentage, such commands should probably have an option to create confusing parentage if that's what the user wants.

Approach 2: Minimal support for confusing parentage

The other approach would be to declare that jj supports confusing parentage only as far as absolutely necessary for git compatibility, and any command touching such a commit is allowed to, and probably will, undo the confusing parentage by removing parents that are ancestors of other parents. We could make some exceptions, e.g. for the jj describe example you mentioned. Then, I think jj can simply not provide a way of creating confusing parentage.

---

The "full support" approach is probably ultimately better, at least in terms of git compatibility. If confusing parentage is common enough, perhaps choosing this approach is unavoidable. However, it's a lot more work, since as far as I can tell, jj's implementation is currently much closer to the "minimal support" (second) approach. I'd have to look and think more at how this can be done; perhaps you already have a design in mind.

---

One part of what you said confused me. You say:

So that probably means that we want to allow the user to create such commits, but I'm not sure if we want to let them use jj rebase for it. It seems pretty harmless to allow it to me.

and also

It should probably even be an error if the user ran jj rebase -d a -d a--

Did you change your mind in the middle of the post, or is there some distinction I'm missing?

P.S. I will probably not give this more than very superficial attention for a couple of days at least.

[martinvonz]

I think approach 1 is what we want. It's just a bit tricky to decide what's "new" and what's "old" confusing parentage. Maybe we could do that by marking rebased commits as "preserve as parent". Abandoned commits would be "do not preserve as parent". The we apply some appropriate rules to propagate that per-commit flag as we rebase the descendants. Let's take the example I had above:

E
|\
| D
|/
C
|
B
|
A

For jj abandon B, we would first rebase C onto A. Since its old parent (B) was not marked as "preserve", we remove redundant parents (except that we can always skip that check for single-parent commits like C). We mark C as "preserve". Then we rebase D and mark it is "preserve". Finally, we rebase E and since both parents were marked "preserve", we rebase it without removing redundant parents.

For jj abandon C, we would first rebase D onto B and mark D as "preserve". Then we rebase E and notice that C was not marked "preserve", so we remove redundant parents afterwards.

I haven't thought through many other cases, but hopefully something like that could work.

If you decide to implement that, I think it would belong in DescendantRebaser.

Did you change your mind in the middle of the post, or is there some distinction I'm missing?

I think my comment saying it would be harmless to allow confusing parentage was my initial feeling. When I said jj rebase -d a -d a-- should be an error, I think that was only if we decided that we should not allow confusing parentage. So if we do allow it, we should not error out in that case. If don't allow it, then it should be an error instead of a warning.

[ilyagr]

I'll close this for now. I'm not sure if/when I want to think about this again, and I'm not even sure there's a problem to be fixed anymore.