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: make "set" do upsert as before #3932

Merged
merged 1 commit into from
Jun 23, 2024

Conversation

yuja
Copy link
Collaborator

@yuja yuja commented Jun 20, 2024

#3584

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

@arxanas
Copy link
Collaborator

arxanas commented Jun 20, 2024

Hmm, when I read this PR title (having forgotten about the previous discussion), I thought that --allow-new must be related to jj new somehow. Not sure if it's the best name given that we accidentally stole the meaning of "new" in a general context 🫤. I think --allow-create would be more accurate, but it's wordy.

@yuja
Copy link
Collaborator Author

yuja commented Jun 21, 2024

I thought that --allow-new must be related to jj new somehow. Not sure if it's the best name given that we accidentally stole the meaning of "new" in a general context 🫤. I think --allow-create would be more accurate, but it's wordy.

Hmm, I thought --allow-new is preferred, but I think --allow-create is also good. Some people including me were against --create because set --create doesn't always "create", but I don't see any argument against --allow-create (or --allow-new.)

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 21, 2024

I'm wondering whether we should just be more aggressive here and let set create branches immediately.

For example, for a release or two, we can print a warning when jj branch set creates a branch and suggest jj branch move if that's not what people wanted. There could be an option to disable the warning.


I also agree with #3932 (comment), --allow-create sounds better to me. new is becoming a loaded word in jj, and while we could use it here if we had to, we don't have to.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 21, 2024

Perhaps I'm not following what your plan is. I guess my concern is that if we merely create the option now and make it the default later, then it feels like the change would be just as breaking at that later point as it would be now if we made the change without the option. What is the option's purpose in your mind?

@yuja
Copy link
Collaborator Author

yuja commented Jun 21, 2024

My plan was:

  1. add --allow-new
  2. add warning if set moved existing branch (suggesting branch move instead
  3. make --allow-new the default

For example, for a release or two, we can print a warning when jj branch set creates a branch and suggest jj branch move if that's not what people wanted.

That sounds also good, and I like the idea.

I also agree with #3932 (comment), --allow-create sounds better to me.

Okay, there are two votes, so let's rename then.

Since "set <thing>" often adds a <thing> if not exists, it make some sense
that "branch set" does upsert. The current "branch set" use case is now covered
by "branch move", so it's okay to change the "set" behavior.

If new branch is created by "branch set", status message and hint will be
printed to help migration. The user should be able to undo creation if it was
a mistake.

Closes martinvonz#3584
@yuja yuja changed the title cli: branch: add "set --allow-new" flag cli: branch: make "set" do upsert as before Jun 21, 2024
@yuja
Copy link
Collaborator Author

yuja commented Jun 21, 2024

we can print a warning when jj branch set creates a branch and suggest jj branch move

Implemented this as it seemed simpler.

Copy link
Collaborator

@ilyagr ilyagr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

cli/src/commands/branch/set.rs Show resolved Hide resolved
@yuja yuja merged commit 3c80e34 into martinvonz:main Jun 23, 2024
17 checks passed
@yuja yuja deleted the push-uosxwykttnqn branch June 23, 2024 00:44
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