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: branch: add "move" command that can update branches by revset or name #3895

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

yuja
Copy link
Contributor

@yuja yuja commented Jun 16, 2024

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

Copy link
Member

@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.

LGTM, but maybe @arxanas and @emesterhazy have opinions

@ilyagr
Copy link
Contributor

ilyagr commented Jun 17, 2024

This is a clever command! I'm slightly worried people might confuse it with jj branch set, but hopefully the notice when jj branch move moves multiple branches and jj undo will help with that.

Re the commit message:

jj branch move --from 'heads(::@- & branches())' --to @-

Was the --to meant to be @? Was & branches() meant to be inside parentheses? At first thought, it would seem that jj branch move --from @- --to @ what people would usually want. jj branch move --from 'heads(::@-)' seems possibly better if some parents of @ are ancestors of others; I'm not sure though. Perhaps I'm missing entirely what you are trying to do?

As I commented in #3884, I'm not immediately sure whether this helps with that issue.

@yuja
Copy link
Contributor Author

yuja commented Jun 17, 2024

jj branch move --from 'heads(::@- & branches())' --to @-

Was the --to meant to be @?

I'm assuming here non-editing workflow that the @ commit is usually empty.

Was & branches() meant to be inside parentheses? At first thought, it would seem that jj branch move --from @- --to @ what people would usually want. jj branch move --from 'heads(::@-)' seems possibly better if some parents of @ are ancestors of others; I'm not sure though. Perhaps I'm missing entirely what you are trying to do?

My typical workflow is to create many commits on anonymous branch, and move the branch pointer (e.g. main) if these commits get ready. So the immediate parent usually doesn't have a named branch.

@ilyagr
Copy link
Contributor

ilyagr commented Jun 17, 2024

Ah, I see. You are relying on the behavior of heads(revset) that removes any commit inside revset that's an ancestor of another, regardless of whether the intermediate commits are in the revset or not. It's a little confusing (well, I keep forgetting about it, at least), but quite powerful here.

@yuja yuja force-pushed the push-mntnosnwkyow branch from a64542e to 42e9ed7 Compare June 17, 2024 03:16
yuja added 2 commits June 18, 2024 10:58
This basically backs out 8706fad "cli: inline check for
non-fast-forwardable branch move." I'm going to add another subcommand that
moves existing branches.
… name

This basically supersedes the current "branch set" command. The plan is to turn
"branch set" into an "upsert" command, and deprecate "branch create". (jj-vcs#3584)
Maybe we can also add "branch set --new" flag to only allow creation of new
branches. One reason behind this proposed change is that "set" usually allows
both "creation" and "update". However, we also need a typo-safe version of
"set" to not create new branches by accident.

"jj branch move" is useful when advancing ancestor branches. Let's say you've
added a couple of commits on top of an existing PR branch, you can advance the
branch by "jj branch move --from 'heads(::@- & branches())' --to @-". If this
pattern is super common, maybe we can add --advance flag for short.

One drawback of this change is that "git branch --move" is equivalent to
"jj branch rename". I personally don't find this is confusing, but it's true
that "move" sometimes means "rename".
@yuja yuja force-pushed the push-mntnosnwkyow branch from 42e9ed7 to 1a8ad44 Compare June 18, 2024 02:23
@ilyagr
Copy link
Contributor

ilyagr commented Jun 18, 2024

A quick summary of my thoughts at the moment (mainly so that I don't feel like I'm slowing this down):

  • I generally like this plan and jj branch move specifically, I have no specific concerns about this PR at the moment.

  • Given time, I'd review this more carefully, but if you're comfortable enough with it (Update: and not waiting on other people to provide input), feel free to merge it.

  • I'm only 90% on-board with the plan from the first paragraph of the commit description, but in ways that have little to do with this PR. Mainly, I don't see the benefit of renaming jj branch create to jj branch set --new. I'm not strongly opposed to it, but all else being equal, I mildly prefer jj branch create, for aesthetic reasons. If you or somebody feels strongly enough that all else is not equal, I'm OK with it; if someone convinces me -- even better.

    I know you felt that it makes some difference for cli: branch: on create/set/rename, assume deleted tracking local branch still exists #3884, but I haven't understood that argument so far.

  • I also feel like we should share the plan in the other thread, not everybody may have seen this discussion. I hope that everyone will like it; again, it seems to me like a good plan.

    (Update: This doesn't have to block this PR, since we aren't doing any breaking changes just yet.)

@yuja
Copy link
Contributor Author

yuja commented Jun 18, 2024

Thanks, I'll merge this as is.

  • I'm only 90% on-board with the plan from the first paragraph of the commit description, but in ways that have little to do with this PR. Mainly, I don't see the benefit of renaming jj branch create to jj branch set --new. I'm not strongly opposed to it, but all else being equal,

I don't have strong opinion about that either. My thinking is as follows:

  • If branch set does upsert (or has --create option), branch create is redundant. The existence of create seems rather confusing.
  • It's weird if "create" preserved existing tracking state. It's equally bad if set --create did, but it seems okay if the option is named as set --deny-move-existing-branch.
  • Tags are usually set only once, so tag set shouldn't allow to overwrite by default. Do we add tag set that fails by default in addition to tag create?

Anyways, this is the discussion belonging to #3584. I'll post comments or small incremental PRs there.

@yuja yuja merged commit b8e921e into jj-vcs:main Jun 18, 2024
17 checks passed
@yuja yuja deleted the push-mntnosnwkyow branch June 18, 2024 03:48
@ilyagr
Copy link
Contributor

ilyagr commented Jun 18, 2024

Thanks again for implementing this, it's a cool idea!

If branch set does upsert (or has --create option), branch create is redundant.

I agree that having both jj branch set --create and jj branch create would be weird; we should pick one.

It's weird if "create" preserved existing tracking state. It's equally bad if set --create did, but it seems okay if the option is named as set --deny-move-existing-branch.

I think I agree that set --deny-move-existing-branch doesn't feel quite as broken if it preserves the tracking state, so this makes sense if the implementation cost of implementing a better behavior is steep.

If the implementation cost is not too steep, I think the UI will still feel better if it warns or errors in the case when the new branch is (possibly unexpectedly) already tracking remotes. Then, it might as well be called set --create or just create.

Tags are usually set only once, so tag set shouldn't allow to overwrite by default. Do we add tag set that fails by default in addition to tag create?

We could just have tag create and tag move, and omit tag set for this reason.

@yuja
Copy link
Contributor Author

yuja commented Jun 18, 2024

If the implementation cost is not too steep, I think the UI will still feel better if it warns or errors in the case when the new branch is (possibly unexpectedly) already tracking remotes. Then, it might as well be called set --create or just create.

To me, "create" implies more guarantee and should fail whereas "set"/"rename" can warn, but yeah, it's just feeling. Implementation-wise, there aren't much difference between error and warning.

Tags are usually set only once, so tag set shouldn't allow to overwrite by default. Do we add tag set that fails by default in addition to tag create?

We could just have tag create and tag move, and omit tag set for this reason.

"tag move" sounds a bit odd to me, but again, it's just feeling. Maybe because moving tags means fixing mistake (so it's more like overwriting than moving)?

@ilyagr
Copy link
Contributor

ilyagr commented Jun 18, 2024

Depending on how much we dislike tag move, we could give the users a little lecture unless they give a --force or --i-hereby-swear-that-i-am-up-to-no-good flag. Or we could not provide tag move and require jj tag delete; jj tag create instead.

Alternatively, we could have a notion of "immutable tags" for tags of a certain form, in case people use tags for reasons other than marking releases.

I am not sure which is best, but I think there are a bunch of options.

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.

3 participants