--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
John mentioned an idea to mitigate the "my CL doesn't need trybots" mentality:If a developer commits a change without green tryjobs and subsequently breaks the tree (in a way that would have been prevented by tryjobs), they get a point.At the end of the month, we tabulate the points and the worst offenders get extra sheriff days.Thus those that break the tree in a preventable manner are tasked with cleaning up the tree.WDYT?
>
> TBR should never be used for
> -really simple changes that you think don't need to be reviewed
> -making a change that someone said should be done. they need to review it.
I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.
When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?
>There was a hunk touching some file unrelated to the change, and I've
> I'm not sure what occassionally included means?
>
occasionally included that file into the list of files in the CL when
doing gcl change.
>>Ok, thank you.
>> When the time came to re-land that CL again, it didn't change much - I
>> just removed that file from the patch.
>> Was it fine to TBR the people that had reviewed the CL already (I
>> actually did so)?
>
>
> I think this is another case of judgement, and I agree that if files by
> owners didn't change the second time around, no need to ask them to look at
> it again. Of course, someone needs to review the changes from the first
> commit to the second commit.
>
On Wed, Mar 27, 2013 at 1:09 AM, Alexander Potapenko <gli...@chromium.org> wrote:
I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.I'm not sure what occassionally included means?When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?I think this is another case of judgement, and I agree that if files by owners didn't change the second time around, no need to ask them to look at it again. Of course, someone needs to review the changes from the first commit to the second commit.
Is it acceptable to land a TBR for code that has been peer-programmed?
On Wed, Mar 27, 2013 at 11:27 AM, John Abd-El-Malek <jabde...@google.com> wrote:
On Wed, Mar 27, 2013 at 1:09 AM, Alexander Potapenko <gli...@chromium.org> wrote:
I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.I'm not sure what occassionally included means?When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?I think this is another case of judgement, and I agree that if files by owners didn't change the second time around, no need to ask them to look at it again. Of course, someone needs to review the changes from the first commit to the second commit.So I think this is parallel to the case where you get LGTMs, and run into a problem in the try bots, and fix that problem, and then go to land. My attitude to date has been that a re-review in that case is a judgement call--if the changes are big enough, I ask for a re-review, and if they aren't, I don't.So I'm not sure I agree with your "Of course"; care to comment on what I'm getting either right or wrong, in your opinion?
On Wed, Mar 27, 2013 at 8:48 AM, Randy Smith <rds...@google.com> wrote:
On Wed, Mar 27, 2013 at 11:27 AM, John Abd-El-Malek <jabde...@google.com> wrote:
On Wed, Mar 27, 2013 at 1:09 AM, Alexander Potapenko <gli...@chromium.org> wrote:
I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.I'm not sure what occassionally included means?When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?I think this is another case of judgement, and I agree that if files by owners didn't change the second time around, no need to ask them to look at it again. Of course, someone needs to review the changes from the first commit to the second commit.So I think this is parallel to the case where you get LGTMs, and run into a problem in the try bots, and fix that problem, and then go to land. My attitude to date has been that a re-review in that case is a judgement call--if the changes are big enough, I ask for a re-review, and if they aren't, I don't.So I'm not sure I agree with your "Of course"; care to comment on what I'm getting either right or wrong, in your opinion?How is that different from landing a change, unreviewed, of the diff that made the trybots pass?
This is why I try to have green try runs before sending for review. It's hard for me to predict what will fail on the try jobs after a reviewer sent their lgtm.
On Wed, Mar 27, 2013 at 8:48 AM, Randy Smith <rds...@google.com> wrote:
On Wed, Mar 27, 2013 at 11:27 AM, John Abd-El-Malek <jabde...@google.com> wrote:
On Wed, Mar 27, 2013 at 1:09 AM, Alexander Potapenko <gli...@chromium.org> wrote:
I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.I'm not sure what occassionally included means?When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?I think this is another case of judgement, and I agree that if files by owners didn't change the second time around, no need to ask them to look at it again. Of course, someone needs to review the changes from the first commit to the second commit.So I think this is parallel to the case where you get LGTMs, and run into a problem in the try bots, and fix that problem, and then go to land. My attitude to date has been that a re-review in that case is a judgement call--if the changes are big enough, I ask for a re-review, and if they aren't, I don't.So I'm not sure I agree with your "Of course"; care to comment on what I'm getting either right or wrong, in your opinion?How is that different from landing a change, unreviewed, of the diff that made the trybots pass?
On Wed, Mar 27, 2013 at 12:04 PM, John Abd-El-Malek <jabde...@google.com> wrote:
On Wed, Mar 27, 2013 at 8:48 AM, Randy Smith <rds...@google.com> wrote:
On Wed, Mar 27, 2013 at 11:27 AM, John Abd-El-Malek <jabde...@google.com> wrote:
On Wed, Mar 27, 2013 at 1:09 AM, Alexander Potapenko <gli...@chromium.org> wrote:
I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.I'm not sure what occassionally included means?When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?I think this is another case of judgement, and I agree that if files by owners didn't change the second time around, no need to ask them to look at it again. Of course, someone needs to review the changes from the first commit to the second commit.So I think this is parallel to the case where you get LGTMs, and run into a problem in the try bots, and fix that problem, and then go to land. My attitude to date has been that a re-review in that case is a judgement call--if the changes are big enough, I ask for a re-review, and if they aren't, I don't.So I'm not sure I agree with your "Of course"; care to comment on what I'm getting either right or wrong, in your opinion?How is that different from landing a change, unreviewed, of the diff that made the trybots pass?Reviews happen at several different levels simultaneously. LGTM simultaneously means: "Yeah, this makes sense to do", "basically the right architecture", "good, clean design", and "no nits that I can see" (and many other things I'm not calling out). IMO, several (not all) of those carry over for a tweak to get try bots to go green/get past the CQ.But at the core, my argument is the same as the one Matt brings up. If you require that you've gotten an LGTM for *precisely* the diff you're landing, you substantially raise the tax for merging if other CLs conflicting with you go in before yours, and for latency delay for try jobs (because you can never run them in parallel with the review). And the longer you work on a CL, the more potential merge conflicts, meaning that this can be an exponentially growing tax (I only wish I wasn't speaking from personal experience on CLs that I personally judged *did* require a re-review).When I put all this together, I feel like "getting a re-review on tweaks for an already approved CL" should be in the "use best judgement" category.
The only acceptable uses of TBR are:-fixing a tree breakage (i.e. because a configuration is broken which isn't run on the try server)-reverts/merges-TBRing owners of consumers of API changes which has been reviewed by owners of that API
--
In the past, I was told by owners of some high-level gypi files to TBR things like adding a new file to gypi, provided the lower-level reviewer approved the change as a whole...