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.

Sign up for GitHub

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

Jump to bottom

cli: push new non-tracking bookmarks by default, rename --allow-new flag #5173

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

cli: push new non-tracking bookmarks by default, rename --allow-new flag #5173

wants to merge 1 commit into from
+213 −169

Conversation

yuja
Copy link
Contributor

@yuja yuja commented Dec 22, 2024

#5094

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

@yuja
cli: push new non-tracking bookmarks by default, rename --allow-new flag
e5ab0d0 
This partially reverts 296961c "cli: git push: do not push new bookmarks by
default." Apparently, the restriction introduced by that patch was too strict
for some use cases. The new rule is that any non-tracking (or local-only)
bookmarks will be pushed to new remote by default, but --allow-new-tracking is
required if a bookmark is already tracking other (real) remotes.

Closes jj-vcs#5094
@istudyatuni
Copy link

Will it be possible to not push some bookmarks without needing to specify -b [bookmark]?

@yuja
Copy link
Contributor Author

yuja commented Dec 25, 2024

Will it be possible to not push some bookmarks without needing to specify -b [bookmark]?

New non-tracking bookmarks within the default push revsets? no.

If you like the current --allow-new behavior, please speak up in #5094. (I personally don't feel --allow-new was a mistake, but appears that people don't like it.)

martinvonz
martinvonz approved these changes Dec 27, 2024
View reviewed 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.

Thanks!

@yuja
Copy link
Contributor Author

yuja commented Dec 28, 2024

I'll leave this open for a while because of #5094 (comment). I don't want to introduce breaking change again.

@martinvonz
Copy link
Member

I'll leave this open for a while because of #5094 (comment). I don't want to introduce breaking change again.

Makes sense.

I think we're too close to the release to merge this now. The release is planned for Wednesday, but I'm not sure I'll get around to it since I'm on vacation.

@yuja yuja mentioned this pull request Jan 8, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinvonz martinvonz martinvonz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuja @istudyatuni @martinvonz