On 5/3/11 12:38 AM, Don Clugston wrote:
> On 2 May 2011 23:18, Jonathan M Davis<jmdavisProg at gmx.com>  wrote:
>> Still, on the whole, I think that the pull requests have had a positive influence on code quality, and I think that they're a good thing.
>
> I have to say, I *totally* disagree with that. We've just three
> completely erroneous pull requests merges in.
> The autotester is currently failing because of it.
> (1) your changes to std.datetime, which shouldn't have been pulled in
> until the CTFE bug was fixed;
> (2) two pull requests moving std.math intrinsics to a new druntime
> file core.math.
> This had been discussed as being as being a bad idea, and yet it got
> pulled in anyway, and it doesn't even compile Phobos, on ANY platform.
>
> I would say, the evidence shows:
> (1)  the pull request system DEFINITELY slows development down dramatically.
> (2) the pull request system DOES NOT improve quality.
>
> Obviously, code reviews are good.
> The pull requests seem to be working well for DMD. But for Phobos,
> basically, they suck.

I think that would be too harsh an assessment. Let's not be hasty to generalize.

First, the author of a pull request is to be trusted that has tested the code on at least one platform - no ifs and buts. That's what allows the person approving a pull request to click "automatic merge" after reviewing for potential platform issues. In case of platform-sensitive code, the pull request author must request testing on other platforms. (I do realize that such a decision has fuzzy boundaries.)

Second, in the (hopefully) rare cases when the trunk does break, it's not difficult to turn the clock back to a working version. That makes the slowdown, when it does occur, nowhere near dramatic.

The pull request system is here to stay. It allows wide collaboration, makes for a palpable sense of achievement for contributors, and fosters an atmosphere of collaboration and excellence. What we need to do is improve our tools, process, and git mastery.


Andrei

On 3 May 2011 18:04, Andrei Alexandrescu <andrei at erdani.com> wrote:
> On 5/3/11 12:38 AM, Don Clugston wrote:
>>
>> On 2 May 2011 23:18, Jonathan M Davis<jmdavisProg at gmx.com> ?wrote:
>>>
>>> Still, on the whole, I think that the pull requests have had a positive influence on code quality, and I think that they're a good thing.
>>
>> I have to say, I *totally* disagree with that. We've just three
>> completely erroneous pull requests merges in.
>> The autotester is currently failing because of it.
>> (1) your changes to std.datetime, which shouldn't have been pulled in
>> until the CTFE bug was fixed;
>> (2) two pull requests moving std.math intrinsics to a new druntime
>> file core.math.
>> This had been discussed as being as being a bad idea, and yet it got
>> pulled in anyway, and it doesn't even compile Phobos, on ANY platform.
>>
>> I would say, the evidence shows:
>> (1) ?the pull request system DEFINITELY slows development down
>> dramatically.
>> (2) the pull request system DOES NOT improve quality.
>>
>> Obviously, code reviews are good.
>> The pull requests seem to be working well for DMD. But for Phobos,
>> basically, they suck.
>
> I think that would be too harsh an assessment. Let's not be hasty to generalize.

A bit harsh, but still, three pull requests broke the build last week.
We've only ever had 34 Phobos pull requests.
Failure rate is therefore at least 10%, but I think there's been other bad ones.

> First, the author of a pull request is to be trusted that has tested the code on at least one platform - no ifs and buts.

Yes, definitely, but...

> That's what allows the
> person approving a pull request to click "automatic merge" after reviewing
> for potential platform issues.

Unfortunately, this isn't true!  If some time has elapsed between
initiation of the pull request, and the merge, there is no guarantee
that the code STILL works. It really needs to be tested again, if it
is nontrivial.
The 'stale merge' risk is particularly important because we have dmd,
druntime, and Phobos all in separate repositories.
It isn't a purely theoretical problem, it's the cause of last week's breakages.

> In case of platform-sensitive code, the pull
> request author must request testing on other platforms. (I do realize that
> such a decision has fuzzy boundaries.)

> Second, in the (hopefully) rare cases when the trunk does break, it's not difficult to turn the clock back to a working version. That makes the slowdown, when it does occur, nowhere near dramatic.

Yes, but that's a general property of a version control system.
Generally speaking, breaking the trunk is not so bad, provided it gets
fixed quickly.
The person who does the merge should be responsible for reverting it,
if it breaks. Which thus far has not worked well; we've had trunk
broken
for extended periods of time (including now!)

> The pull request system is here to stay. It allows wide collaboration, makes for a palpable sense of achievement for contributors, and fosters an atmosphere of collaboration and excellence. What we need to do is improve our tools, process, and git mastery.

Pull requests offer a clear benefit for anything which involves a
change or addition to an API.
Also, they obviously make sense for anyone who doesn't have write
access to Phobos.

The primary problem I see with pull requests is that they introduce a gap between testing and merging.

But I just think there's a whole class of commits where pull requests
will NEVER add value. I don't see *any* value in pull requests for
improvements to comments, fixes of internal bugs inside functions,
etc.
In such cases, pull requests are just busy work. Creating pull
requests for such things dilutes the value of the pull request system,
IMHO.
I agree with David. Would be worth thinking about the process a bit more.

On 5/4/11 2:38 AM, Don Clugston wrote:
> On 3 May 2011 18:04, Andrei Alexandrescu<andrei at erdani.com>  wrote:
>> Second, in the (hopefully) rare cases when the trunk does break, it's not difficult to turn the clock back to a working version. That makes the slowdown, when it does occur, nowhere near dramatic.
>
> Yes, but that's a general property of a version control system.
> Generally speaking, breaking the trunk is not so bad, provided it gets
> fixed quickly.
> The person who does the merge should be responsible for reverting it,
> if it breaks. Which thus far has not worked well; we've had trunk
> broken
> for extended periods of time (including now!)

One property that git has that probably many other VCSs don't is that you can work on an older tree and then easily merge it with the trunk.

> But I just think there's a whole class of commits where pull requests
> will NEVER add value. I don't see *any* value in pull requests for
> improvements to comments, fixes of internal bugs inside functions,
> etc.
> In such cases, pull requests are just busy work. Creating pull
> requests for such things dilutes the value of the pull request system,
> IMHO.
> I agree with David. Would be worth thinking about the process a bit more.

It's difficult to define the "trivial" category. Again, at work we did have the experience that something was considered trivial but caused breakages when deployed. Anyhow, probably we're not at the size we can afford to review each change.


Andrei

>________________________________
>From: Andrei Alexandrescu <andrei at erdani.com>

>It's difficult to define the "trivial" category. Again, at work we did have the experience that something was considered trivial but caused breakages when deployed. Anyhow, probably we're not at the size we can afford to review each change.


The Phobos community could be huge, and I still wouldn't have time to review lots of changes.? I've only reviewed one, and that was because it was a pull request changing my code (RedBlackTree).? Not saying that quantity of developers doesn't help, but certainly there are other factors.

I'd rather have the pull requests reserved for things that are complex or new, and leave small bug fixes to straight commits.? Requiring a review for everything is overkill, and actually counterproductive.? Why can't we have simple guidelines that can be overridden when needed?? The only hard rule is, don't break the build.? If it breaks the build, it needs to be reverted.

-Steve

Works for me.

Andrei

On 5/4/11 8:06 AM, Steve Schveighoffer wrote:
>
>
>
>> ________________________________
>> From: Andrei Alexandrescu<andrei at erdani.com>
>
>> It's difficult to define the "trivial" category. Again, at work we did have the experience that something was considered trivial but caused breakages when deployed. Anyhow, probably we're not at the size we can afford to review each change.
>
>
> The Phobos community could be huge, and I still wouldn't have time to review lots of changes.  I've only reviewed one, and that was because it was a pull request changing my code (RedBlackTree).  Not saying that quantity of developers doesn't help, but certainly there are other factors.
>
> I'd rather have the pull requests reserved for things that are complex or new, and leave small bug fixes to straight commits.  Requiring a review for everything is overkill, and actually counterproductive.  Why can't we have simple guidelines that can be overridden when needed?  The only hard rule is, don't break the build.  If it breaks the build, it needs to be reverted.
>
> -Steve
>
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos

Le 2011-05-04 ? 11:06, Steve Schveighoffer a ?crit :

>> From: Andrei Alexandrescu <andrei at erdani.com>
> 
>> It's difficult to define the "trivial" category. Again, at work we did have the experience that something was considered trivial but caused breakages when deployed. Anyhow, probably we're not at the size we can afford to review each change.
> 
> The Phobos community could be huge, and I still wouldn't have time to review lots of changes.  I've only reviewed one, and that was because it was a pull request changing my code (RedBlackTree).  Not saying that quantity of developers doesn't help, but certainly there are other factors.
> 
> I'd rather have the pull requests reserved for things that are complex or new, and leave small bug fixes to straight commits.  Requiring a review for everything is overkill, and actually counterproductive. Why can't we have simple guidelines that can be overridden when needed?  The only hard rule is, don't break the build.  If it breaks the build, it needs to be reverted.

I'd like to make a suggestion.

Perhaps it'd make sense that many small things could be aggregated in a single pull request and reviewed together. This way the reviewer wouldn't have to merge/build/unittest each commit separately, it'd be done in bunches. All you need is to maintain a branch for those small things, and issue a pull request for that branch every time you feel it contains enough for a review.

-- 
Michel Fortin
michel.fortin at michelf.com
http://michelf.com/