NEW 43948
Bugzilla "review" action should not paste full diff as comment by default 
https://bugs.webkit.org/show_bug.cgi?id=43948
Summary Bugzilla "review" action should not paste full diff as comment by default
Eric Seidel (no email)
Reported 2010-08-12 22:23:35 PDT
Bugzilla "review" action should not paste full diff as comment by default This makes it very easy for users to kill bugs: https://bugs.webkit.org/show_bug.cgi?id=43595#c24 We have working line-by-line diffs, we should make that the default behavior, and replace the "clear main comment box" button with a "paste full diff into comment box" button for anyone who wants this old behavior.
Attachments
Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2010-08-13 01:33:27 PDT
Darin might have an opinion here. Last time I touched this I broke his workflow, and I'd rather not do that again. :)
Alexey Proskuryakov
Comment 2 2010-08-13 07:05:14 PDT
I prefer to not use the line by line diffs, and edit the full diff instead. Not that I find my current workflow efficient, but removing the full diff would make it even worse.
Eric Seidel (no email)
Comment 3 2010-08-13 08:10:52 PDT
There needs to be some sort of safety net though. It's *way* too easy to spam a bug with the "edit the diff" behavior. Making it non-default is one way to give it a safety net. But not the only way. Mark's mistake is not the only bug which has died due to someone making a huge comment from our "review patch" tool. Even people who understand the tool don't edit down the diff nearly enough and make gigantic comments.
Ojan Vafai
Comment 4 2010-08-13 09:55:33 PDT
I imagine we could just detect this case and deal with it. For example, if the number of diff lines before the first review comment or after the last one are >20, we could truncate them to 20. This would be a simple tweak to the JS on the review page.
Adam Barth
Comment 5 2010-08-13 10:13:35 PDT
Maybe we should clear the big comment box the first time someone saves a line-by-line comment if the big box still has the original contents.
Eric Seidel (no email)
Comment 6 2010-08-17 09:10:27 PDT
https://bugs.webkit.org/show_bug.cgi?id=43055 was also just destroyed by this poor default. We really need to remove this noose from our review system IMO.
Darin Adler
Comment 7 2010-08-17 12:11:06 PDT
I edit the full diff too. Before we added the button to add individual diffs, this error was rare, and the script I wrote to remove the full diff if you don’t edit it, made it almost never happen. The change here was adding that new workflow. Currently, that gets in my way when I want to search for things within the patch. I click to select a word and copy it to search, and a text field gets inserted into the patch. This is inconvenient. I don’t think the new feature for adding comments with individual lines to the patch review has a good UI yet. It confuses people and results in patches with full diffs. Please don’t fix this by removing the old workflow!
Eric Seidel (no email)
Comment 8 2010-08-19 07:19:21 PDT
Another victim: https://bugs.webkit.org/show_bug.cgi?id=44174#c6
Darin Adler
Comment 9 2010-08-19 11:24:08 PDT
Someone should take my JavaScript to remove the diff if it's still there and make it handle more cases. Also, I would find it acceptable if pasting in the diff was triggered by a button instead of default. Generally speaking I think we are making that review page harder to understand and use with each change right now.
Note You need to log in before you can comment on or make changes to this bug.