Posting a new diff against a discarded changeset should create a new review request

[GoogleCodeExporter]

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

Original issue reported on code.google.com by amord...@gmail.com on 29 Jul 2008 at 9:03

[GoogleCodeExporter]

Is this actually a new review request or a new diff on the same review request?

Original comment by chip...@gmail.com on 30 Jul 2008 at 12:51

• Changed state: NeedInfo

[GoogleCodeExporter]

It is a new review request -- with new diffs -- for the same changeset number.

Original comment by amord...@gmail.com on 30 Jul 2008 at 1:32

[GoogleCodeExporter]

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.

Original comment by chip...@gmail.com on 30 Jul 2008 at 2:33

• Changed state: New

[GoogleCodeExporter]

It is not clear at all when I 'Discard' a review request that state is being 
saved
for it.

Original comment by amord...@gmail.com on 30 Jul 2008 at 2:38

[GoogleCodeExporter]

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.

Original comment by nithinsu...@gmail.com on 18 Aug 2008 at 12:48

[GoogleCodeExporter]

Original comment by trowb...@gmail.com on 21 Sep 2008 at 1:39

• Changed title: Posting a new diff against a discarded changeset should create a new review request
• Added labels: Component-Reviews

[GoogleCodeExporter]

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.

Original comment by trowb...@gmail.com on 29 Sep 2008 at 8:14

• Changed state: WontFix

[GoogleCodeExporter]

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.

Original comment by amord...@gmail.com on 29 Sep 2008 at 8:58

[GoogleCodeExporter]

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.

Original comment by chip...@gmail.com on 29 Sep 2008 at 9:13

[GoogleCodeExporter]

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.

Original comment by amord...@gmail.com on 30 Sep 2008 at 2:26

[GoogleCodeExporter]

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.

Original comment by chip...@gmail.com on 30 Sep 2008 at 3:02