Option to revert without sending to CQ in PolyGerrit |
||||||||
Issue descriptionIt would be nice to be able to create a new CL that reverts a landed CL without immediate landing the revert. My use case is I reverted a CL and now I want to revert the revert and then iterate on that to fix the issue in the CL that was originally revert.
,
Mar 16 2017
Maybe there is already another workflow for this use case that I should be following.
,
Mar 16 2017
What are you using to revert the change? If you use the [Revert] button in the Gerrit UI the result should be a new, un-merged change as you describe.
,
Mar 16 2017
It was a Rietveld feature that allowed users to not bypass the CQ during a revert. This should be implemented in the Chromium plugins. Moving this to crbug.
,
Mar 16 2017
,
Mar 16 2017
,
Mar 16 2017
Rietveld used to automatically tack on those keywords during a revert. I am not sure we need a separate checkbox to not bypass CQ in PG because you can edit the complete revert message before submitting.
,
Mar 16 2017
In Rietveld, clicking the Revert button would 1) Prompt for a reason 2) Ask if you wanted to automatically CQ the resulting CL 3) Create a new CL with the inverse diff 4) Modify the commit message based on step (1) and add various footers 5) Click the CQ checkbox if the answer to (2) was yes In Gerrit, clicking the Revert button today should 1) Creates a new CL containing the output of "git revert <hash>" 2) Modify the commit message to include various footers 3) Set CR+1 and CQ+2 The gerrit version is much simpler, but is missing certain features such as a) prompting for a reason before creating the revert b) not auto-CQing if the user doesn't want to c) not auto-CQing if the change is more than 24 hours old These features should be added to the commitqueue plugin somehow.
,
Mar 16 2017
a) prompting for a reason before creating the revert It has a <INSERT REASONING HERE> but does not verify that it is changed. The plugin could prompt if a reason is not entered. b) not auto-CQing if the user doesn't want to This is supported. You just have to manually remove the lines from the 'Revert Commit Message' in the popup. The plugin could move that CQ keywords higher up above the original description section to make it more noticeable. c) not auto-CQing if the change is more than 24 hours old This is supported. I added it in https://bugs.chromium.org/p/chromium/issues/detail?id=677274
,
Mar 16 2017
$0.02: a) <INSERT REASONING HERE> has indeed been left in place a few times by V8 devs. b) I didn't find it obvious which line to remove. I personally change TBR= to R=, and also remove NOPRESUBMIUT=True, then for projects with presubmit builder I'm quite certain CQ won't land immediately. Have I missed smth?
,
Mar 16 2017
a) Yeah, there needs to be a single-purpose, empty text box that they fill in. b) Removing lines from the popup is bad UX. Unchecking a check box is better. c) That doesn't prevent it from setting CQ+2. It still starts the CQ, it just runs through the whole CQ instead of bypassing it. Anyway, point is, the commitqueue plugin needs to work to hit feature parity with Rietveld.
,
Mar 16 2017
b) I think your c) should have been in your b). Meaning that there is no way to prevent from setting CQ+2 by modifying the description. This should definitely be a checkbox. I'll change the bug description. c) This was the same behavior in Rietveld for > 24 hours ago. Rietveld did auto-CQ if change was old. It just did not add those keywords.
,
Mar 16 2017
And I can take this.
,
Mar 16 2017
Ah, I see. I didn't dive into the Rietveld code, just looked at what I observed. FWIW, I think there should be a checkbox for "should the CQ start immediately", and that checkbox should default to checked for recent changes and default to unchecked for older changes.
,
Mar 28 2017
This is the feature request where I had added this ability to Rietveld: https://bugs.chromium.org/p/chromium/issues/detail?id=354457
,
Mar 28 2017
Sent out for review cl/151345460
,
Apr 3 2017
Change is live. There is now a checkbox to choose if the revert change should be sent to the CQ. The checkbox will not be checked by default if the change is less than 24 hours old. Tested by: * Reverting a change older than 1 day (https://skia-review.googlesource.com/c/11121/). * Reverting a change not older than 1 day (https://skia-review.googlesource.com/c/11124). Marking as fixed. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bsalo...@google.com
, Mar 16 2017