New issue
Advanced search Search tips

Issue 702351 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----


Previous locations:
gerrit:5800


Sign in to add a comment

Option to revert without sending to CQ in PolyGerrit

Project Member Reported by bsalo...@google.com, Mar 16 2017

Issue description

It 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.
 

Comment 1 by bsalo...@google.com, Mar 16 2017

"... originally reverted."

Comment 2 by bsalo...@google.com, Mar 16 2017

Maybe there is already another workflow for this use case that I should be following.

Comment 3 by wyatta@google.com, Mar 16 2017

Status: AwaitingInformation (was: New)
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.

Comment 4 by rmis...@google.com, 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.

Comment 5 by rmis...@google.com, Mar 16 2017

Project: chromium
Moved issue gerrit:5800 to now be  issue chromium:702351 .

Comment 6 by rmis...@google.com, Mar 16 2017

Cc: aga...@chromium.org tandrii@chromium.org
Components: Infra>Codereview>Gerrit
Labels: Pri-2
Status: Available (was: AwaitingInformation)
Summary: Option to revert without immediately landing in PolyGerrit (was: Option to revert without immediately landing)

Comment 7 by rmis...@google.com, 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.

Comment 8 by aga...@chromium.org, Mar 16 2017

Cc: rmis...@chromium.org
Labels: Milestone-Launch Proj-Gerrit-Migration
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.

Comment 9 by rmis...@google.com, 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


$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?

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.

Comment 12 by rmis...@google.com, Mar 16 2017

Summary: Option to revert without sending to CQ in PolyGerrit (was: Option to revert without immediately landing in PolyGerrit)

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.

Comment 13 by rmis...@google.com, Mar 16 2017

Owner: rmis...@google.com
And I can take this.
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.

Comment 15 by rmis...@google.com, Mar 28 2017

Status: Started (was: Available)
This is the feature request where I had added this ability to Rietveld: https://bugs.chromium.org/p/chromium/issues/detail?id=354457

Comment 16 by rmis...@google.com, Mar 28 2017

Sent out for review cl/151345460
Status: Fixed (was: Started)
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