I agree with Nicholas's definition of what is allowed to TBR: things that are not code changes. To expand on his list, I think this also includes svn prop changes and string translation dumps (xtb file updates).
Historically, we have checked in code TBR on the merge branch during the initial phases of getting it to compile and link. When we were using gvn, this still meant you had to send an email. Whoever the TBR was sent to is still responsible for reviewing the change and making sure it's correct. Any feedback on the change still needs to be addressed in a follow up change. The idea being that getting merge branch to compile/link shouldn't block on the review since there are a lot of small things to do. I don't think the intent was ever that the merge would be reviewed only when landed. I think we were a bit sloppy with this in the most recent merge. This is possibly because it is tempting to just svn commit with TBR in the commit description. This means no email gets sent and the patch is never actually reviewed. Part of the difficulty is that Reitveld can't handle large changes, so it requires manually emailing the viewvc link after the commit. I think when the next merge happens, we just need to be more careful of this and try to make sure there's an email associated with every commit. If we want to enforce it, we could make an svn commit hook. The svn hook would make sure there's either a Reitveld URL or a TBR in the commit log. If there's a TBR, it would send an email to the viewvc link. tony On Fri, 10 Oct 2008, Nicolas Sylvain wrote: > On Fri, Oct 10, 2008 at 11:37 AM, Ojan Vafai <[EMAIL PROTECTED]> wrote: > > > I hope not to start a flame-war here, but I'd like to see written down > > somewhere our team policy on committing code. There are some issues that > > seem underspecified to me. These are of course just my feelings on these > > issues. I hope that coming out of this discussion we can agree on a formal > > policy. > > > > 1. Use the trybots: It's at the point where I think that no one should > > *ever* commit code without at least looking at the results of the > > trybots, > > unless it is an emergency fix for a closed tree (or of course if the > > trybots > > are down). I imagine there won't be much disagreement here and we can > > just > > add this to the appropriate documentation on the Sites page. > > 2. Don't > > TBR: I see inconsistency with the team culture around what is acceptable > > to TBR. My experience with the rest of Google is that the *only* acceptable > > changes to TBR are ones that fix closed trees. I would feel a lot more > > comfortable if we had a hard rule like that, but I understand others feel > > differently. In either case, can we generate a hard list of the things that > > are acceptable toTBR? > > 3. Watch the waterfall: Noone should ever commit code unless they can > > stick around for the next hour to make sure they didn't cause > > regressions, > > or unless they can ask someone else to monitor the tree for them and act > > appropriately. Not doing one of those two things means that when your > > checkin inadvertently breaks the build it falls on the shoulders of > > either > > the sheriff or whoever happens to be online if it's after hours. > > > > > I agree with all this. To answer your question about "what is acceptable to > TBR". Everything that has code should not be TBR. On the other hand I would > personally not mind if someone changes the DEPS file, the test_fixable.txt > list or the VERSION file with a TBR checkin. > > What do we do for branches? My understanding was that code reviews for > branches were not mandatory, since the code has to be reviewed anyway when > it's merge back to trunk. Is it true? How do you proceed for the webkit > merge branch? > > Nicolas > > > > 1. > > > > This all piggy-backs on Marc-Antione's email yesterday about keeping the > > tree green. It is possible to keep the tree considerably more green than we > > currently do and I think the above would be an enormous step in that > > direction. Keeping the tree green makes our team globally more efficient and > > keeps our sheriffs from hating their jobs. :) > > > > Thoughts? > > > > Ojan > > > > > > > > > > > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Chromium-dev" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/chromium-dev?hl=en -~----------~----~----~----~------~----~------~--~---
