cli: git push: show hint when there are bookmarks pending deletion #6332
Conversation
| @@ -1961,6 +1961,14 @@ fn test_git_push_deleted(subprocess: bool) { | |||
| work_dir | |||
| .run_jj(["bookmark", "delete", "bookmark1"]) | |||
| .success(); | |||
| let output = work_dir.run_jj(["git", "push"]); | |||
There was a problem hiding this comment.
Should I add a comment here explaining what exactly this is testing for, like elsewhere in this file?
winterqt
force-pushed
the
push-mtqlyqzvnxyp
branch
from
cd8e8e2 to
d62688f
Compare
winterqt
force-pushed
the
push-mtqlyqzvnxyp
branch
from
d62688f to
d853395
Compare
cli/src/commands/git/push.rs
Outdated
| writeln!( | ||
| ui.hint_default(), | ||
| "You can push {deleted_bookmarks} deleted bookmark{s} with `--deleted`", | ||
| s = if deleted_bookmarks > 1 { "s" } else { "" } |
There was a problem hiding this comment.
Should I add a trailing comma here?
There was a problem hiding this comment.
rustfmt doesn't, so on second thought, I won't; it's inconsistent with how all of the other macros are formatted throughout the codebase. (Ideally it'd be able to do it same as function calls, but I understand why it can't/doesn't.)
There was a problem hiding this comment.
Oh, I misread the comment, sorry. I meant period should be added to hint/warning messages. (Our style is a bit random about that, though.)
cli/src/commands/git/push.rs
Outdated
| @@ -393,6 +393,27 @@ pub fn cmd_git_push( | |||
| ), | |||
| remote = remote.as_symbol() | |||
| ); | |||
|
|
|||
| if bookmark_updates.is_empty() { | |||
There was a problem hiding this comment.
Can you include in the commit message why you want this hint?
If your intuition was that jj git push (with no arguments) would push deleted bookmarks, it doesn't make sense to mention deleted bookmarks only if there are no other updates.
There was a problem hiding this comment.
What happened was that I created a local bookmark that pointed to @-, but then wanted to delete it. So I did that, then jj git pushed, assuming it would push since it was one of @'s ancestors, but then forgot that I needed to pass --delete.
I had that case in mind, which is why I did that. You're right that it would probably be better to do it unconditionally.
There was a problem hiding this comment.
How's that for the commit message, does it explain why I think this is useful? I can add more detail, it's not the easiest to articulate :)
There was a problem hiding this comment.
What happened was that I created a local bookmark that pointed to
@-, but then wanted to delete it. So I did that, thenjj git pushed,
In that case, maybe we can look for remote bookmarks to be deleted within the default push revset range (plus the remote bookmarks themselves.) It won't help if the deleted local bookmarks had been rewritten/rebased, but can greatly reduce the amout of false positives.
There was a problem hiding this comment.
In that case, maybe we can look for remote bookmarks to be deleted within the default push revset range (plus the remote bookmarks themselves.)
Won't we have to look at a previous operation for that, since the default push revset doesn't match in this case?
There was a problem hiding this comment.
greatly reduce the amout of false positives
How would this lead to false positives? I'm unaware.
There was a problem hiding this comment.
Won't we have to look at a previous operation for that,
I meant something like ancestors(remote_bookmarks(..)..@, 2) & <remote bookmarks to be deleted>.
How would this lead to false positives? I'm unaware.
There's no clue to assume that the user intended to push deleted bookmarks, right? It seems just wrong to list random deleted bookmarks.
There was a problem hiding this comment.
I meant something like
ancestors(remote_bookmarks(..)..@, 2) & <remote bookmarks to be deleted>.
Hmm... yeah, maybe that might work well. Perhaps the message should be something like Hint: You can delete X nearby remote bookmark(s) (and Y additional ones) with jj git push --deleted or something.
There's no clue to assume that the user intended to push deleted bookmarks, right? It seems just wrong to list random deleted bookmarks.
Fair. Well, they are tracked, so I'd imagine there's a good chance they want to? (And if they don't, then they'll be deleted as soon as they pass --deleted.
There was a problem hiding this comment.
Though I am now questioning the usefulness of this 😅 I'll defer to you on that end.
There was a problem hiding this comment.
Since the default isn't jj git push --all, I don't think it's great idea to list "all" deleted bookmarks.
Though I am now questioning the usefulness of this 😅 I'll defer to you on that end.
I personally don't think this hint will help. It's not easy to guess which deleted bookmarks would belong to the current push target (if they weren't deleted), and it's kidna obvious that jj git push doesn't push deleted bookmarks unless specified.
winterqt
force-pushed
the
push-mtqlyqzvnxyp
branch
from
d853395 to
c3da5a1
Compare
winterqt
changed the title
This comes from a personal experience where the following sequence of events happened: 1. `jj git push -c @-` 2. Oops, that was the wrong one, so I'll `jj bookmark delete` and `jj git push` 3. Hm... why didn't that do anything? Oh, right, `--deleted` As described in the diff, this hint only fires when `jj git push` is used with no arguments, as any user that passes `--all`, `--tracked`, or `--revisions` (targeting a deleted bookmark) will already get an error instructing them to use `--deleted`.
winterqt
force-pushed
the
push-mtqlyqzvnxyp
branch
from
c3da5a1 to
127d3ad
Compare
This comes from a personal experience where the following sequence of events happened:
jj git push -c @-jj bookmark deleteandjj git push--deletedAs described in the diff, this hint only fires when
jj git pushis used with no arguments, as any user that passes--all,--tracked, or--revisions(targeting a deleted bookmark) will already get an error instructing them to use--deleted.Checklist
If applicable:
CHANGELOG.md