Ask for confirmation before pushing to remote - #9969
Conversation
|
I generally don't think we need more flags which hook into the UI and as necaqua previously put it "having jj undo makes having a such options unnecessary". |
1254654 to
ea02ec6
Compare
I generally agree with this sentiment, and it's exactly for that reason I'm submitting this PR. You don't need any safe guards on most jj commands, because the ability to quickly & easily undo them is the safe guard; but |
Pushing changes to a remote is one of the few operations that cannot be undone using `jj undo`, so extra attention is needed to make sure we use it correctly. Currently, the only way to see what `jj git push` will do ahead of time using the `--dry-run` flag, though using it with every push adds a lot of friction for the user. This change adds a config option `git.confirm-before-push` to prompt the user before changes are pushed to the remote. Implemented in #9969
ea02ec6 to
7c9d963
Compare
And since the purpose of this command is to synchronize the local state with the external one, this is something which works as intended. If Git weren't Git and we had a native remote such discussions would be unnecessary since we could have a general mechanism for replaying state updates on the server side. |
|
I kind of like the idea, since there is a big fuzzy area in my understanding of how |
Pushing changes to a remote is one of the few operations that cannot be undone using `jj undo`, so extra attention is needed to make sure we use it correctly. Currently, the only way to see what `jj git push` will do ahead of time using the `--dry-run` flag, though using it with every push adds a lot of friction for the user. This change adds a config option `git.confirm-before-push` to prompt the user before changes are pushed to the remote. Implemented in #9969
7c9d963 to
e60cc49
Compare
PhilipMetzger
left a comment
There was a problem hiding this comment.
I still wouldn't do this but this has some major implications for which we should respect our users and don't add a breaking change in such a way.
We first should make this opt-in and then warn on the old behavior before just making it interactive for everyone (and only after the warning is removed this could be the behavior).
| /// Automatically answer all prompts with "yes" and run non-interactively | ||
| #[arg(long, short)] | ||
| yes: bool, |
There was a problem hiding this comment.
nit: I don't think we should do it this way, if we're doing it at all
There was a problem hiding this comment.
I do believe a flag like this should exist for the purpose of shell scripts, AI agents, or other places where interaction must be avoided without modifying the user configs. This particular flag is inspired by apt install, which has -y/--yes/--assume-yes. Do you have a different design in mind?
There was a problem hiding this comment.
Do you have a different design in mind?
If this feature is opt-in only such a flag is unnecessary, as I said if we're doing it all it should be something which we slowly roll out.
| let needs_confirm = !args.dry_run | ||
| && !args.yes | ||
| && match tx.settings().get("git.confirm-before-push")? { | ||
| PushConfirmChoice::Always => true, | ||
| PushConfirmChoice::Never => false, | ||
| PushConfirmChoice::Auto => ref_updates.bookmarks.len() + ref_updates.tags.len() > 1, | ||
| }; |
There was a problem hiding this comment.
nit: To make it a clean migration for our users, this initially should be opt-in and a warning should appear for the "old/current" behavior.
There was a problem hiding this comment.
I mean you implemented a warning but please look at past migrations how we usually do that.
There was a problem hiding this comment.
I don't think we need any warnings or hints. It's unlikely that the confirmation will be enabled by default. jj git push is the command to push changes, so why should it be interrupted by a confirmation prompt? The user can run --dry-run if needed.
There was a problem hiding this comment.
@yuja This confirmation prompt is to catch mistakes, and getting a jj git push wrong is particularly annoying because it instantly updates an external remote, which you can't easily jj undo. The --dry-run flag is great when you're unsure what jj git push is going to do and you want to check manually, but you don't always know ahead of time when you're about to make a mistake. Always running the same command twice, once with and without the flag, is possible but it adds a lot more friction than pressing "y" on a prompt.
There was a problem hiding this comment.
@PhilipMetzger I'm sorry, I'm not sure what you mean. I based the current implementation on the hint you get when you don't have a merge editor configured. What past migration do you have in mind?
There was a problem hiding this comment.
You can add an alias to run jj git push --dry-run ...; <prompt>; jj git push ....
https://docs.jj-vcs.dev/latest/config/#aliases
Configurable push revset will also help if you occasionally get surprising results with the default revset.
ShiroKSH
left a comment
There was a problem hiding this comment.
Two changes requested: make the confirmation match the signed push, and correct the documented setting name.
Pushing changes to a remote is one of the few operations that cannot be undone using `jj undo`, so extra attention is needed to make sure we use it correctly. Currently, the only way to see what `jj git push` will do ahead of time using the `--dry-run` flag, though using it with every push adds a lot of friction for the user. This change adds a prompt to `jj git push` which, after listing the effects this push will have on the remote, asks the user to confirm before updating the remote. This interaction is controlled by the setting `git.confirm-before-push` (always/never/auto), and can be skipped using the `-y`/`--yes` flag. Implemented in #9969
e60cc49 to
ade9d68
Compare
Pushing changes to a remote is one of the few operations that cannot be undone using `jj undo`, so extra attention is needed to make sure we use it correctly. Currently, the only way to see what `jj git push` will do ahead of time using the `--dry-run` flag, though using it with every push adds a lot of friction for the user. This change adds a prompt to `jj git push` which, after listing the effects this push will have on the remote, asks the user to confirm before updating the remote. This interaction is controlled by the setting `git.confirm-before-push` (always/never/auto), and can be skipped using the `-y`/`--yes` flag. Implemented in #9969
ade9d68 to
358ec6f
Compare
jj git pushis one of the few commands that cannot be undone easily usingjj undo, so special care is needed to make sure we get it right every time. Currently, the only way to inspect whatjj git pushis going to change about the remote ahead of time is using--dry-run, though doing this on every push adds a lot of friction to the development process.This PR adds an option to prompt the user when running
jj git push:This prompt is configured using the config
git.confirm-before-push, which has three possible values:always,never, andauto, the last of which only prompts the user when multiple bookmarks or tags are being pushed at once. The default option isauto; interactions can be skipped using the-y/--yesflag.(No LLMs were used to create this PR)
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)how it works, how it's organized), including any code drafted by an LLM.
an eye towards deleting anything that is irrelevant, clarifying anything
that is confusing, and adding details that are relevant. This includes,
for example, commit descriptions, PR descriptions, and code comments.