Open matts1 opened 2 months ago
Have you left out the straight-forward workflow? This is one where you simply make more commits, never changing previous ones.
What happens if you squash into an immutable commit, including into the root commit? Will you then be editing that immutable commit? I'm pretty sure we don't want snapshotting to rewrite such commits, especially not the root commit. Does that mean that snapshotting instead aborts and forces you to run some new command to recover?
Have you left out the straight-forward workflow? This is one where you simply make more commits, never changing previous ones.
You're correct that that's probably even simpler. I didn't even think about it though, because It doesn't work for any users of gerrit, piper, or git users committing to the jj repo directly though, since all those have you review code on a commit-level, not a PR level, and thus expect that you amend commits based on reviewer feedback rather than just adding new commits. So to me, while it might be a more simple abstraction, it's one that doesn't work (for me), since I inherently need those commits to be mutable.
You're welcome to file a bug to better support that workflow, but I've never used it and thus I'm not familiar with any gaps in that workflow.
What happens if you squash into an immutable commit, including into the root commit? Will you then be editing that immutable commit? I'm pretty sure we don't want snapshotting to rewrite such commits, especially not the root commit. Does that mean that snapshotting instead aborts and forces you to run some new command to recover?
Firstly, from what I can tell, the root commit appears to be truly immutable, not just a hint, unlike other commits. I was unable to squash into the root commit even with --ignore-immutable
, so that appears to be a non-issue.
jj --ignore-immutable squash --to @-
Error: The root commit 000000000000 is immutable
In order to squash into a non-root immutable commit, you'd need to write jj --ignore-immutable --to <immutable commit>
, which requires an explicit "I know what I'm doing, let me do this". I think that given that, no special casing is required, if you were to write jj --ignore-immutable --no-new-on-empty-commit squash
, and the parent commit was an immutable commit, it's fine to start editing that immutable commit. I see --ignore-immutable
as removing guardrails, and thus it's fine to not get the guardrails of "you can't edit an immutable commit". Also, in this particular situation, the user is saying "squash into this immutable commit", and so they've already decided that the immutable commit isn't so immutable after all.
I was unable to squash into the root commit even with
--ignore-immutable
, so that appears to be a non-issue.
What about jj new 'root()'; jj abandon
then? Where does that leave you? Ah, I suppose you're saying that the operation that attempts to leave you on the root commit is disallowed, so the abandon
in that case will error out. That sounds reasonable.
What about
jj new 'root()'; jj abandon
then? Where does that leave you? Ah, I suppose you're saying that the operation that attempts to leave you on the root commit is disallowed, so theabandon
in that case will error out. That sounds reasonable.
I hadn't considered that case, I've done additional exploration now to try and find all possible edge cases.
I took a look at jj help
and came up with a list of commands that are awkward:
parents(@)
, and deal with the root()
problem)--to
commit if --from
is abandoned)--edit
flag, it already deals with the root()
problem)--edit
flag)The corner cases I can see are:
jj abandon
on a merge commit
@
@
is preserved.jj squash
on a merge commit
jj squash --to <parent>
in the context of a merge commit when working with the edit workflow in jj
is basically an amend to a parent commit. It seems to me like we should be preserving the state of @
when we do this
2) jj squash --to <unrelated commit>
doesn't do jj new <value of --to>
, it takes you to jj new 'parents(@)'
--to
commitjj abandon
the child of root
jj git init --colocate .; touch foo; jj describe -m "foo"; jj abandon
would be nice if it succeeded.Why edit users should be first-class citizens I personally believe that combining
jj
and the edit workflow provides probably the simplest abstraction of a version control system that I've ever seen.
As a heavy user of the edit
workflow, I heavily agree with this statement and agree that jj squash
for the workflow should be improved.
The creation of new empty commits when the current commit is emptied is magic, and magic is bad as it's hard to explain, hard to understand, and often requires special-casing. I believe (and feel free to correct me if I'm wrong) that the only reason it exists in
jj
is that it maps well to the concept of@-
being the "current commit".
But I think the creation of the empty commit is a good thing wich is not magic. In my thinking it's a state machine to prevent you from falling into a bad state. As you note later, jj
never treats a users commit special which is a really good thing.
Since the proposal is to always stay in a edit
ing mode, when should a new commit get created for you? Since edit
ing a non-leaf commit always will create conflicts for people, which is a major pitfall of the workflow.
But I think the creation of the empty commit is a good thing wich is not magic. In my thinking it's a state machine to prevent you from falling into a bad state. As you note later,
jj
never treats a users commit special which is a really good thing.
I half agree with you here. I believe that it is magic, in the sense that it's a special-case that only occurs under very specific circumstances, treats the @
commit specially, and it's not really consistent with what the command says it does. That being said, I do believe that it's a good choice, as it prevents squash users from falling into a bad state (which is most users).
Since the proposal is to always stay in a
edit
ing mode, when should a new commit get created for you? Sinceedit
ing a non-leaf commit always will create conflicts for people, which is a major pitfall of the workflow.
I'm not proposing to change the default behaviour, so I don't believe that's an issue. This would only happen if you add the flag --no-new-on-empty-commit
or whatever you want to call it. If you opt into what's essentially the edit workflow flag, you would never have a new commit created for you when you don't explicitly ask jj
for it, except in the three specific circumstances I've described above.
While I prefer to automatically get a commit when squashing into a immutable commit, I think this should be a major improvement reminding me a bit of the time when there were no immutable commits.
Is squashing into an immutable commit that common? I noticed that both you and @martinvonz brought it up, but I've literally never done it, since an immutable commit should be... immutable?
I wouldn't mind if we added squashing an immutable commit to the special-cases where we created an empty commit. It isn't, strictly speaking, a special-case (in the sense of "things are presented to the user in a consistent and sensible manner without it"), but if people think strongly about it, which they appear to, we can definitely add it as a "this seems like a useful guard rail" thing.
Is squashing into an immutable commit that common? I noticed that both you and @martinvonz brought it up, but I've literally never done it, since an immutable commit should be... immutable?
Well, the immutable commits didn't exist when I started using jj
, so it was always something I needed to be aware of. But I'm pretty sure users will run into it, so we need to choose to the non foot-gunny thing for it.
I wouldn't mind if we added squashing an immutable commit to the special-cases where we created an empty commit. It isn't, strictly speaking, a special-case (in the sense of "things are presented to the user in a consistent and sensible manner without it"), but if people think strongly about it, which they appear to, we can definitely add it as a "this seems like a useful guard rail" thing.
Yes, the mechanic was the most useful guard rail introduced besides getting the immutable_heads()
revset.
Is squashing into an immutable commit that common? I noticed that both you and @martinvonz brought it up, but I've literally never done it, since an immutable commit should be... immutable?
Sorry that I forgot to reply to this. It's not that I think it's a common thing. But by simply being possible, it's a case we have to decide how to handle.
Since that PR was approved, it's now unclear to me what our policy is:
prev = ["prev", "--edit"]
. Would we be open to allow aliases such as rebase = ["rebase", "--skip-empty"]
Was there some discussion elsewhere
It seems like it was this comment:
I always thought of having a
ui.movement.always_edit
flag, which responds to passingjj next/prev --edit
.
4283 appears to break the whole "each command should work the same on every developer's machine regardless of config" principle that I was under the impression that we followed. Was there some discussion elsewhere that I'm not aware of? While I liked the change, I would have never expected it to be approved, and I'm wondering what the consequences are for our policy in general.
I don't think we changed the policy on #1509, it's just that next
/prev
are almost lightweight wrappers over new
/edit
. See Yuya's comment https://github.com/martinvonz/jj/issues/3947#issuecomment-2294511783.
It also is opt-in instead of opt-out, so there's no behavior change except when you're aware of it.
Since that PR was approved, it's now unclear to me what our policy is:
- Would we be open to a config flag to enable / disable the behaviour discussed above
- This is roughly equivalent to
prev = ["prev", "--edit"]
. Would we be open to allow aliases such asrebase = ["rebase", "--skip-empty"]
Either it needs further discussion or won't be allowed per #1509. But Martin or Yuya should chime in.
Is your feature request related to a problem? Please describe.
The creation of new empty commits when the current commit is emptied is magic, and magic is bad as it's hard to explain, hard to understand, and often requires special-casing. I believe (and feel free to correct me if I'm wrong) that the only reason it exists in
jj
is that it maps well to the concept of@-
being the "current commit". Is the magic worth it? For a squash workflow user, yes, absolutely, so I'm not advocating for the removal of the feature.For an edit workflow user though, this means that
jj squash
actually is equivalent tojj squash --from @- --to @--; jj commit -m ""
. This is completely nonsensical, and fights what any edit user wants to do.Why edit users should be first-class citizens I personally believe that combining
jj
and the edit workflow provides probably the simplest abstraction of a version control system that I've ever seen. To me, when I was first taughtjj
, I used the squash workflow, and tried to understand it as "fig (mercurial), but it runshg evolve
after every operation".However, by the time I'd used it for a few days, I found that trying to think of it in terms of mercurial was just holding me back, and I'd be better off thinking about it in terms of what it is -
jj
. The key thing to that understanding was realizing that I needed to think of a repository as a tree of commits, and that I needed to think of each commit as both a tree and a patch, not just one or the other. When I thought about it this way, everything became much simpler, and the edit workflow came naturally to me (I didn't read documentation on the edit workflow until after starting to use it).The edit workflow is simpler than the squash workflow, although it's probably wierder for people who try to think of things in terms of another version control system.
@
isn't special. Nor is@-
. They're commits like any other.jj edit foo
, make my changes, thenjj upload foo
. No amendIt's my personal opinion that most people use the squash workflow because the squash workflow is an attempt to map something that users are familiar with onto a system they're unfamiliar with.
For example, suppose you take someone unfamiliar with version control systems, and tell them "squash moves the content of one commit into another. If the source commit was left empty, it abandons it". You then demonstrate:
jj squash --from foo --to bar
foo
has disappeared, and the changes now appear in bar - completely unsurprisingjj squash
(I explain that by default, it goes--from @ --to @-
)Now, I won't argue that the edit workflow should be the primary workflow. I understand that it's currently less popular, and even though I believe that the edit workflow is simpler for users unfamiliar to VCSs, I believe that they will likely learn from someone familiar with the squash workflow, so even if it was better, it would be unlikely to end up the primary workflow. I'd also appreciate it if people didn't respond to this with "the squash workflow is easier for new people", as I don't believe such a discussion would be constructive, as reasonable minds can differ. All I'm saying is that the edit workflow is a valid workflow and should be treated as such.
Describe the solution you'd like
Option 1 I propose adding a global option
jj --no-new-on-empty-commit
. This would allow edit workflow users to aliasjj
to/path/to/jj --no-new-on-empty-commit
. I don't like this option, but I don't think option 2 would be accepted, and option 3 is a terrible user experience.Describe alternatives you've considered
Option 2 I'd honestly prefer a setting in
config.toml
to enable or disable this, but I suspect such a request wouldn't be approved, as jj has a policy of ensuring that if you writejj squash
and I writejj squash
, given the same state, we should always be performing the same options. I'm not a huge fan of that policy, but it does exist, and I do agree with the reason behind it, even if I disagree with the cost-benefit analysis (since I'd preferjj --no-config-toml
).Option 3 We could add that option to each individual command, but because of the policy mentioned above, it's not possible to alias
jj squash
tojj squash --option
, so I would have to create some kind of aliasjj sq
tojj squash --no-new-on-empty-commit
, and that's just extremely awkward to work with (and essentially recreating git porcelain).Additional context Add any other context or screenshots about the feature request here.