559: Posting a new diff against a discarded changeset should create a new review request
- WontFix
- Review Board
| amor****@gmai***** (Google Code) (Is this you? Claim this profile.) | |
| Sept. 29, 2008 |
What's the URL of the page containing the problem? http://reviewboard/r/new/ What steps will reproduce the problem? I create a new review request (enter repository, change number, and provide a diff generated by 'p4 diff <CLN>'). I then look at the diffs and realize that I'm not ready to publish the review, so I discard it. I then generate new diffs, and go through the same process to create a new review request. However, this new review request treats my new diffs as revision 2 of the diffs. This is pretty annoying, as I hadn't published the diffs yet so there was no need to keep track of the diff revisions. What operating system are you using? What browser? Ubuntu 7.10 with Firefox 2.0.0.14
Is this actually a new review request or a new diff on the same review request?
-
+ NeedInfo
It is a new review request -- with new diffs -- for the same changeset number.
Okay, you should be using the same review request, not trying to create new ones. That's not really the code path we're expecting people to use. That said, if this is triggering a problem, we should look into it, but I think we've fixed it in SVN.
-
- NeedInfo + New
It is not clear at all when I 'Discard' a review request that state is being saved for it.
I've run into the same issue. I think if I create a diff, it should not append it to discarded reviews for the same changelist even if they exist.
-
+ Component-Reviews -
+ Posting a new diff against a discarded changeset should create a new review request
After discussing this with Christian, I'm going to say that we're not going to change this behavior. If you change the diff on an existing changeset, even if previous review requests are discarded, you may want that history available. If you're making a completely unrelated change, it's appropriate to create a new changeset for it.
-
- New + WontFix
Please note that, as per comment #2, the title of the bug should be "Posting a new diff against a changeset for which a review request was previously discarded should create a new review request" (I don't think I have perms to change the title). It is bad UI to have a 'Discard' button which saves state.
I may be changing "Discard" down the road but that's just a label. It's not the same as deleting (admins actually have a special "Delete" button they can use). We do not destroy history, ever, and so Discard doesn't actually delete anything.
I think it's not uncommon for a developer to want to really 'discard' diffs that they had uploaded, so that reviewers would not be presented with multiple revisions of a changeset when only the last one is the one that the developer wished to publish.
It's the developer's responsibility to view the diff before making it public. There's risks associated with actually deleting a diff. Any reviews, comments, anything associated with the diff will also go away, and then any replies based on those, etc. In the future when we have extensions, it could really break things if the code's expecting those diffs to be there. Same with actually deleting review requests. We have a very strict policy of not deleting history because things break when this happens. Developers can easily preview diffs and then replace them before publishing the draft, but once it's published, it's part of history.