-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
x/review/git-codereview: add change -d #9298
Comments
Never mind my dumb question, please. |
Adding this as optional flag (change -d) is not logical. Other flags of the "change" action slightly alter its behaviour, while "-d" would delete the branch/tag and abandon the change in gerrit. It might be a new action: "abandon". First it tries to abandon the change if it was not abandoned yet. If it was abandoned then it deletes the change branch and a tag. Possibly we could add flags to it to skip abandonment or branch deletion. |
Also @josharian @kevinburke according to owners. |
There are several different things this command could do (abandon gerrit changes, delete branch, delete tag), all destructive. And there are lots of ways for gerrit, branches, and tags to be out of sync. I'm nervous about all the ways that this could easily go wrong. |
I would not say it is destructive in all senses. It will delete tag and branch iff it was abandoned in first place (meaning that gerrit returns abandoned status for a CL). And abandoned change can be restored anytime from gerrit UI. So it cannot delete just a branch which was not mailed first nor some lone tag for successfully merged CL. |
Right. Although you'd also want to check that the branch consists only and entirely of abandoned changes, that were fully in sync with gerrit (i.e. matching commit hashes), and that the tag matches the head of the branch. Perhaps I'm unusual, but I fairly often mail only part of a branch, or work on a branch and sit on for a while (due to limited bandwidth), etc. I suppose I don't really object, if this is implemented very defensively, with good error messages in the case of things not being perfectly aligned. |
We should have a change -d command that you run at the same time you click Abandon in the web UI. Or maybe it even does the Abandon for you too. It's too hard to back everything out by hand.
@griesemer ran into this with the peano change. @adg
The text was updated successfully, but these errors were encountered: