CQ label config in Gerrit should contain "copyAllScoresIfNoChange = false" |
||||
Issue description
,
Nov 21 2016
,
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?
,
Nov 22 2016
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.
,
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.)
,
Nov 22 2016
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.
,
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.
,
Nov 23 2016
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.
,
Nov 29 2016
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 |
||||
Comment 1 by tandrii@chromium.org
, Nov 21 2016