On Nov 25, 2015, at 11:32 PM, Ralph Goers <ralph.go...@dslextreme.com> wrote:
> 1. What makes you think all bugs are caught during code reviews (they aren’t)? I don’t, and I did not infer that. > 2. What makes you think that code reviews after the commit are any less > thorough than reviews required before the commit? Nothing. I did not say that. If the code review is done, it makes no difference when. I only said that CTR insures that the CODE REVIEW IS ACTUALLY DONE. > If you don’t trust your community to do code reviews after you commit then > there is a problem in your community. Forcing a code review to occur first > won’t fix that. > I don’t see it as not trusting the community. I think Flex is doing just fine right now. But Flex is a big code base and there’s areas where not a lot of people work on it. Especially, on a big code-base, there’s people working on different things. It’s totally reasonable for something to go under everyone’s radar — especially in an area of the code where there are not a lot of people working on it. Mandatory code reviews seems to me a method of making sure that it doesn’t get missed. If the code doesn’t get reviewed after x number of days, the person who's committing can send an email to the list asking for someone to look it over. What’s wrong with that? > In a CTR world you can choose to do every piece of work on a branch and ask > for a code review before you commit. That is your choice. But if you know > that the code you are committing is good because you have a thorough > knowledge of your product you shouldn’t be forced to have it reviewed before > you can commit it. I actually love the line in Kudu where it says that > automation insures quality. I am a big fan of that. In my experience having > lots of tests is the best way to insure stuff doesn’t get broken. > > So how did you catch the bugs in your code in Flex? Would you have preferred > that they stay on a branch for months so they could be reviewed before > committing? Despite how great people say git is I have still had lots of > problems resolving merge conflicts when the code isn’t merged back quickly. > If I understand what you are saying you would prefer that Flex use RTC > because you don’t trust your fellow committers to review your code. That is > a community problem that needs to be fixed. Forcing them to review the code > first isn’t the proper way to fix it. The bugs were found after the last release of Flex and reported by users. No. I’m not saying I want RTC. I don’t. I’m quite happy with CTR in my community. Small bugs in Flex even if not caught will not likely cause users millions of dollars. I’m okay if there might be more bugs in Flex and not requiring code review, because code review DOES make things more difficult. All I’m saying is that I understand the rationale as quality assurance for communities who consider the damage for regressive bugs to be very high. It seems like a certain amount of RTC can be a reasonable price to pay. > Ralph > >> On Nov 25, 2015, at 2:00 PM, Harbs <harbs.li...@gmail.com> wrote: >> >> If a review is required for non-code changes to the main branch, then I >> agree. >> >> I’m sure you agree that reviews on code make for less bugs. We all make >> mistakes and can overlook things. It seems kind of extreme to assume that >> this kind of required review is all about control. Since anyone who can >> commit can review, it’s kind of hard for me to swallow that. >> >> I assume your logic is that the reviews can come after the commit. Sure. But >> what if it doesn’t? >> >> Case in point: I made some pretty major changes to TLF in Flex which >> constituted a number of months of work. I’m willing to bet that not every >> commit I did was checked by others. I did a decent job, but there were a few >> regressive bugs in my code, and I accidentally reverted some code in my >> commit as well. In a workflow where my code would have to get one or more >> +1s before I committed it to the main branch, it’s likely that the reverted >> commit (at the least) would have been caught. >> >> I would actually welcome knowing someone looked over my code for a sanity >> check. >> >> Harbs >> >> On Nov 25, 2015, at 10:49 PM, Greg Stein <gst...@gmail.com> wrote: >> >>> That is pretty normal operation in both styles of workflow. My concern is >>> with trunk/master. Is a committer trusted enough to make changes directly? >>> >>> If all meaningful changes (ie. changing APIs and algorithms, not just >>> fixing typos w/o review) are not trusted, and require review/permission, >>> then I'm against that. >>> >>> It is good practice to put potentially disruptive code onto a branch while >>> it is developed, then merge it when complete. Trusting a committer to ask >>> for review before the merge is great. Requiring it, less so. >>> >>> But RTC on trunk/master is harmful. >>> >>> Cheers, >>> -g >>> >>> On Wed, Nov 25, 2015 at 2:44 PM, Harbs <harbs.li...@gmail.com> wrote: >>> >>>> What about commit to feature/bug brach, review and then commit to main >>>> branch? >>>> >>>> Is that CTR or RTC in your book? >>>> >>>> On Nov 25, 2015, at 10:42 PM, Greg Stein <gst...@gmail.com> wrote: >>>> >>>>> I object to Lucene's path, too. A committer's judgement is not trusted >>>>> enough to make a change without upload/review. They need permission first >>>>> (again: to use your term; it works great). >>>>> >>>>> On Wed, Nov 25, 2015 at 2:39 PM, Upayavira <u...@odoko.co.uk> wrote: >>>>> >>>>>> Some setups that people call RTC are actually CTR in your nomenclature, >>>>>> so we could be talking cross-purposes. That's all I'm trying to avoid. >>>>>> E.g. Lucene - everything happens in JIRA first (upload patch, wait for >>>>>> review), but once that has happened, you are free to commit away. So >>>>>> strictly, it is RTC, but not seemingly in the sense you are objecting >>>>>> to. >>>>>> >>>>>> Upayavira >>>>>> >>>>>> On Wed, Nov 25, 2015, at 08:35 PM, Greg Stein wrote: >>>>>>> I think this is a distraction. You said it best the other day: RTC >>>>>>> implies >>>>>>> the need for "permission" before making a change to the codebase. >>>>>>> Committers are not trusted to make a judgement on whether a change >>>> should >>>>>>> be made. >>>>>>> >>>>>>> CTR trusts committers to use their judgement. RTC distrusts committers, >>>>>>> and >>>>>>> makes them seek permission [though one of several mechanisms]. >>>>>>> >>>>>>> -g >>>>>>> >>>>>>> On Wed, Nov 25, 2015 at 10:47 AM, Upayavira <u...@odoko.co.uk> wrote: >>>>>>> >>>>>>>> Not replying to this mail specifically, but to the thread in >>>> general... >>>>>>>> >>>>>>>> People keep using the terms RTC and CTR as if we all mean the same >>>>>>>> thing. Please don't. If you must use these terms, please define what >>>>>> you >>>>>>>> mean by them. >>>>>>>> >>>>>>>> CTR is a less ambiguous term - I'd suggest we all assume that "commit" >>>>>>>> means a push to a version control system. >>>>>>>> >>>>>>>> However, RTC seems to mean many things - from "push to JIRA for review >>>>>>>> first, wait a bit, then commit to VCS" through "push to JIRA, and once >>>>>>>> you have sufficient +1 votes, you can commit" to "push to JIRA for a >>>>>>>> review, then another committer must commit it". >>>>>>>> >>>>>>>> If we're gonna debate RTC, can we please describe which of these we >>>> are >>>>>>>> talking about (or some other mechanism that I haven't described)? >>>>>>>> Otherwise, we will end up endlessly debating over the top of each >>>>>> other. >>>>>>>> >>>>>>>> Upayavira >>>>>>>> >>>>>>>> On Wed, Nov 25, 2015, at 09:28 AM, Harbs wrote: >>>>>>>>> AIUI, there’s two ways to go about RTC which is easier in Git: >>>>>>>>> 1) Working in feature/bug fix branches. Assuming RTC only applies to >>>>>> the >>>>>>>>> main branch, changes are done in separate branches where commits do >>>>>> not >>>>>>>>> require review. The feature/bug fix branch is then only merged back >>>>>> in >>>>>>>>> after it had a review. The reason this is easier is because >>>>>> branching and >>>>>>>>> merging is almost zero effort in Git. Many Git workflows don’t work >>>>>> on >>>>>>>>> the main branch anyway, so this is a particularly good fit for those >>>>>>>>> workflows. >>>>>>>>> 2) Pull requests. Using pull requests, all changes can be pulled in >>>>>> with >>>>>>>>> a single command. >>>>>>>>> >>>>>>>>> I’ve personally never participated in RTC (unless you count Github >>>>>>>>> projects and before I was a committer in Flex), so it could be I’m >>>>>>>>> missing something. >>>>>>>>> >>>>>>>>> Of course there’s nothing to ENFORCE that the commit is not done >>>>>> before a >>>>>>>>> review, but why would you want to do that? That’s where trust comes >>>>>> to >>>>>>>>> play… ;-) >>>>>>>>> >>>>>>>>> Harbs >>>>>>>>> >>>>>>>>> On Nov 25, 2015, at 4:08 AM, Konstantin Boudnik <c...@apache.org> >>>>>> wrote: >>>>>>>>> >>>>>>>>>> I don't think Git is particularly empowering RTC - there's nothing >>>>>> in >>>>>>>> it that >>>>>>>>>> requires someone to look over one's shoulder. >>>>>>>>> >>>>>>>> >>>>>>>> --------------------------------------------------------------------- >>>>>>>> To unsubscribe, e-mail: general-unsubscr...@incubator.apache.org >>>>>>>> For additional commands, e-mail: general-h...@incubator.apache.org >>>>>>>> >>>>>>>> >>>>>> >>>>>> --------------------------------------------------------------------- >>>>>> To unsubscribe, e-mail: general-unsubscr...@incubator.apache.org >>>>>> For additional commands, e-mail: general-h...@incubator.apache.org >>>>>> >>>>>> >>>> >>>> >>>> --------------------------------------------------------------------- >>>> To unsubscribe, e-mail: general-unsubscr...@incubator.apache.org >>>> For additional commands, e-mail: general-h...@incubator.apache.org >>>> >>>> >> > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: general-unsubscr...@incubator.apache.org > For additional commands, e-mail: general-h...@incubator.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: general-unsubscr...@incubator.apache.org For additional commands, e-mail: general-h...@incubator.apache.org
