New issue
Advanced search Search tips

Issue 667281 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----



Sign in to add a comment

CQ label config in Gerrit should contain "copyAllScoresIfNoChange = false"

Project Member Reported by tandrii@chromium.org, Nov 21 2016

Issue description

Labels: Proj-Gerrit-Migration
Labels: Milestone-Dogfood Pri-2
Owner: aga...@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by aga...@chromium.org, Nov 22 2016

I don't follow. In the case where everything except the timestamp is the same, why would we want to cancel the commit? The tryjobs were just as valid, and we're going to make a much bigger change (rebasing) when we hit Submit anyway, so why remove the label?
The CQ attempt is canceled as soon as CQ notices that a newer patchset is inserted. Copying the label means trigger CQ immediately on new patchset.

Also, AFAIU almost nobody will upload a new CL with just timestamp as diff *intentionally*. Most likely something went wrong in git cl or its usage and user will re-upload properly as soon user detects the problem.

Comment 5 by aga...@chromium.org, Nov 22 2016

Ok, now I'm really confused.
1) Why is the CQ attempted canceled? There's no relative diff, so the CQ run is still valid, so it should just keep running. This seems like a feature request for CQ.
2) In the mean time, what's wrong with copying the label and immediately re-CQing? Is it just a capacity concern?

I still don't see any downsides to copying labels when they still logically apply. Saying that the CQ label shouldn't be copied when there's literally no diff seems like pre-optimizing, and folks will complain.

(Similarly, folks want the CQ label to remain in place when they edit the commit message and nothing else. Not having it stay in place in this case would be silly.)
Cc: machenb...@chromium.org
1) canceled for simplicity. Indeed a valid FR already filed by me  http://crbug.com/634944  and even has a small proposal targeted to solve precisely what you wrote at the end of your comment above.

2) wasted capacity given my argument above.
Maybe also power user confusion, +machenbach@ who reported this in the first place.

Once  issue 634944  is implemented, this should be won't fix. But I don't see 634944 coming to us before prod Gerrit's prod freeze coming in a few weeks.

Comment 7 by aga...@chromium.org, Nov 22 2016

It seems like machenbach's confusion came from the fact that he observed the CQ+2 label be sticky across a NoChange patchset, and assumed that it would also be sticky across non-trivial new patchsets. Since that isn't the case -- the label definitely disappears if any code changes -- I don't think I want to change anything here.

As you said, the number of people who *accidentally* upload a new-but-identical patchset will be vanishingly small, so I'm not worried about capacity.
I noted the difference to Rietveld and file the bug just out of interest. I'm totally fine personally with the new behavior.

I uploaded an empty patchset accidentally because I mistakenly thought I could change the cl description this way. I also got smarter there.

Comment 9 by aga...@chromium.org, Nov 29 2016

Status: WontFix (was: Assigned)
Awesome. In that case, I'm going to declare this WontFix since I believe that the current behavior is more desired. We can revisit this later if other people get confused.

Sign in to add a comment