[phobos] [D-Programming-Language/phobos] a15e68: Accidentally committed with a bunch of debugging c...

[Don Clugston]

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.