Add --insert-after and --insert-before to jj new

[samueltardieu]

I probably missed helper functions that would have made the code prettier. The discussion is in #1155.

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• I have added tests to cover my changes

[samueltardieu]

Caution: it panics when creating a cycle in the graph, for example when you try to base a revision on a commit and one of its descendants

[martinvonz]

@samueltardieu: This PR and PR #1167 are still in draft state, as you probably know. Does that mean that you're still working on both or are they ready for review? If they are, which one would you prefer to keep? I think I like this one better. I'm not sure I've gotten a good sense of what others prefer.

[ilyagr]

I've used this a bit, and it works well (though i haven't tried it with multiple revision arguments). However, I'm starting to lean towards having a new jj insert command instead. (Sorry if I'm dragging this on. If people are tired of splitting hairs, I don't insist on it). I have a few reasons:

• My fingers tend to want to type jj insert --before instead of jj new --insert or even jj new --before. (For my usage, I created the aliases I mentioned in the other comment).
• It's OK for it to only take a single revset unlike jj new. This avoids the issue of what happens if jj new --after is given multiple revisions and hopefully avoids loops.
• Later, we could extend it to support jj insert --after A -r B:D. At that point, we could say that the -r option conflicts with the --update/--edit/--stay options described below.

One issue here is that it's less clear whether jj insert should edit the inserted commit. I'm thinking that it should have an -m option and arguments --update/--edit/--stay for whether to jj update (aka jj new; the option should also have an alias) to the new revision or jj new. I'd make --update the default.

[samueltardieu]

@samueltardieu: This PR and PR #1167 are still in draft state, as you probably know. Does that mean that you're still working on both or are they ready for review? If they are, which one would you prefer to keep? I think I like this one better. I'm not sure I've gotten a good sense of what others prefer.

I'm still working on this one to deal with edge cases, mostly to avoid loops. When this is done I'll mark it as ready so that I can get a review, and moreover we can decide whether if this stays in new or if it gets a new command (as per @ilyagr comment just above this one) and if it should edit the new commit or not.

[ilyagr]

Thanks Samuel! I have a few minor and one case where I think this will do the wrong thing (see below).

I'm also wondering whether we should refactor the rebase_descendants function so that it's usable here and the code is more modular. That's certainly not a blocker for discussing this PR.

[martinvonz]

For the record - and I think I said this somewhere else already - being able to insert new commits before multiple other commits seems very powerful, but I'm still not sure how useful it is :) I'm also still unsure if the user will typically want to edit the newly created commit. I don't mind getting this merged despite those concerns. We can just move it later. I don't know at what point we need to start worrying more about not making breaking changes in the CLI...

[samueltardieu]

At least jj new -A and jj new -B with several target changes stays consistent with jj new -A and jj new -B with only one target change, as well as with jj new with one or more targets: we always obtain one empty commit, that we edit, whose parents or children are exactly what has been requested on the command line. I don't know if this will be useful, but it doesn't break anything.

This is why I would rather keep this in jj new at the moment. However, it would great to have --update/--edit/--stay options as proposed by @ilyagr. I would make --edit the default for jj new, and --update (or --checkout) the default for jj merge.

Since it is possible to make command aliases, one can alias jj insert to be the same as jj new --insert-before --checkout for example. I don't know if we should have many small commands with close behaviours or fewer commands as long as their behaviour is consistent (i.e. avoid the git checkout nightmare).

[martinvonz]

I don't know if this will be useful, but it doesn't break anything.

Agreed. It may not be very useful (or maybe it is?), but it's consistent, and the single-parent case behaves as one would expect and is useful.

To me, the decision between jj new --insert-before/--insert-after and jj insert --before/--after will mostly be decided based on whether people will typically want the --update, --edit, or --stay version. Let's hear what users tell us. By the way, --checkout may be better than --update, since that would be consistent with the command name (jj checkout).

[samueltardieu]

(pushed again to get a new Windows runner – the build was still ongoing after 2 hours, and not close to the end)

[martinvonz]

Looks good. Thanks!

[ilyagr]

Thank you! Let's try it out :)

I feel like we should try to refactor and make rebase and this code use the same helper functions, but this can be done later, and now we'll have nice tests to guide us.