I had been invited as dmd committer a month ago, but haven't been able to consume pull requests.

The top three of pulls at 2012/08/20:
  37 pulls : my front-end changes
  17 pulls : yebblies's front-end changes
  13 pulls : dawgfoto's backend and front-end changes
-> 67/103 (65% of all pulls)
and, as far as I seen half of them are trivial bug fixes and LGTM.

But they are yet not merged by the following reasons.
#1. Some of them are mine, so I cannot merge without others reviewing.
#2. Some of them are already outdated, so I cannot merge them without
small fix-up.
    But, even if I add some fix-ups to them, I can not merge them by
the reason #1 .

I think the most problem is too few reviewers for dmd, but it's difficult to solve in short term.

To solve the deadlocks, I'd like to suggest development process improvements.

For #1:
If the pull request passes the auto tester, I can merge without others
reviewing.
This is based on "reviewing after committing", but it seems to me that
would not be a problem,
because current dmd release cycle has "beta-phase" to fix regressions.

For #2:
I would add fixups in my local, and post pull requests which contains
it, then treats them as #1.

This is for only _trivial_ bug fixes, and not for big ones and enhancements.

How about?
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

I strongly disagree with your implication that the auto-tester counts as any sort of a review.  It can only detect incorrectness, not declare correctness.  There's little evidence to suggest that anyone is going to look at the code post-commit and I'm not comfortable with hoping that someone will.  Hoping that someone's testing happens to catch them during a very tiny beta period (a different problem unto itself) is just fantasy.

Sorry, but the fix to not enough people with front end experience doing code reviews is not to not do code reviews.

My 2 cents,
Brad

On Mon, 20 Aug 2012, kenji hara wrote:

> I had been invited as dmd committer a month ago, but haven't been able to consume pull requests.
> 
> The top three of pulls at 2012/08/20:
>   37 pulls : my front-end changes
>   17 pulls : yebblies's front-end changes
>   13 pulls : dawgfoto's backend and front-end changes
> -> 67/103 (65% of all pulls)
> and, as far as I seen half of them are trivial bug fixes and LGTM.
> 
> But they are yet not merged by the following reasons.
> #1. Some of them are mine, so I cannot merge without others reviewing.
> #2. Some of them are already outdated, so I cannot merge them without
> small fix-up.
>     But, even if I add some fix-ups to them, I can not merge them by
> the reason #1 .
> 
> I think the most problem is too few reviewers for dmd, but it's difficult to solve in short term.
> 
> To solve the deadlocks, I'd like to suggest development process improvements.
> 
> For #1:
> If the pull request passes the auto tester, I can merge without others
> reviewing.
> This is based on "reviewing after committing", but it seems to me that
> would not be a problem,
> because current dmd release cycle has "beta-phase" to fix regressions.
> 
> For #2:
> I would add fixups in my local, and post pull requests which contains
> it, then treats them as #1.
> 
> This is for only _trivial_ bug fixes, and not for big ones and enhancements.
> 
> How about?
> _______________________________________________
> dmd-internals mailing list
> dmd-internals@puremagic.com
> http://lists.puremagic.com/mailman/listinfo/dmd-internals
> 
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On Mon, Aug 20, 2012 at 8:09 PM, Brad Roberts <braddr@puremagic.com> wrote:
> Sorry, but the fix to not enough people with front end experience doing code reviews is not to not do code reviews.

Agreed, but I'd like to add that this is a general problem with DMD development (maintainability?) - even Walter regularly commits code which doesn't even pass the auto tester.

David
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On 8/20/12 2:31 PM, David Nadlinger wrote:
> On Mon, Aug 20, 2012 at 8:09 PM, Brad Roberts<braddr@puremagic.com>  wrote:
>> Sorry, but the fix to not enough people with front end experience doing
>> code reviews is not to not do code reviews.
>
> Agreed, but I'd like to add that this is a general problem with DMD
> development (maintainability?) - even Walter regularly commits code
> which doesn't even pass the auto tester.

This issue is a symptom of a simple underlying problem: we don't have enough contributors. Kenji tried to find a solution within the current constraints, whereas Brad responded from an "enough reviewers" frame of reference.

I think we need to add more active committers to dmd.


Andrei
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On Mon, 20 Aug 2012, Andrei Alexandrescu wrote:

> On 8/20/12 2:31 PM, David Nadlinger wrote:
> > On Mon, Aug 20, 2012 at 8:09 PM, Brad Roberts<braddr@puremagic.com>  wrote:
> > > Sorry, but the fix to not enough people with front end experience doing code reviews is not to not do code reviews.
> > 
> > Agreed, but I'd like to add that this is a general problem with DMD development (maintainability?) - even Walter regularly commits code which doesn't even pass the auto tester.
> 
> This issue is a symptom of a simple underlying problem: we don't have enough contributors. Kenji tried to find a solution within the current constraints, whereas Brad responded from an "enough reviewers" frame of reference.
> 
> I think we need to add more active committers to dmd.
> 
> 
> Andrei

I wasn't suggesting anything other than 'less code revew' isn't a solution that's in our best interest.

If you want my opinion, Walter shouldn't be writing code unless the review queue is empty.  It's harsh, but it'd net us far better throughput of fixes and keep the contributors contributing.  We loose contributors and constrict their contribution rate by not allowing work to flow into the tree.

With increased throughput and increased contributors, they'd be in a better position to also review either others code, making the need for Walter to be the reviewer lower.

Lastly, as part of the code review, Walter needs to stop running the tests.  That's what the pull auto tester is for.  Look at the code, comment where not ready, merge where ready.

Later,
Brad
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On Aug 20, 2012, at 4:01 PM, Brad Roberts <braddr@puremagic.com> wrote:

> On Mon, 20 Aug 2012, Andrei Alexandrescu wrote:
> 
>> On 8/20/12 2:31 PM, David Nadlinger wrote:
>>> On Mon, Aug 20, 2012 at 8:09 PM, Brad Roberts<braddr@puremagic.com>  wrote:
>>>> Sorry, but the fix to not enough people with front end experience doing code reviews is not to not do code reviews.
>>> 
>>> Agreed, but I'd like to add that this is a general problem with DMD development (maintainability?) - even Walter regularly commits code which doesn't even pass the auto tester.
>> 
>> This issue is a symptom of a simple underlying problem: we don't have enough contributors. Kenji tried to find a solution within the current constraints, whereas Brad responded from an "enough reviewers" frame of reference.
>> 
>> I think we need to add more active committers to dmd.
>> 
>> 
>> Andrei
> 
> I wasn't suggesting anything other than 'less code revew' isn't a solution that's in our best interest.
> 
> If you want my opinion, Walter shouldn't be writing code unless the review queue is empty.  It's harsh, but it'd net us far better throughput of fixes and keep the contributors contributing.  We loose contributors and constrict their contribution rate by not allowing work to flow into the tree.
> 
> With increased throughput and increased contributors, they'd be in a better position to also review either others code, making the need for Walter to be the reviewer lower.
> 
> Lastly, as part of the code review, Walter needs to stop running the tests.  That's what the pull auto tester is for.  Look at the code, comment where not ready, merge where ready.
> 
> Later,
> Brad

I'm not particularly familiar with Linux kernel development, but my understanding is that smaller commits are aggregated by select helpers, and then the larger changsets are merged by the BDFL.  Maybe something similar would work for DMD?
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On 20 Aug 2012, at 22:40, Jason House wrote:
> I'm not particularly familiar with Linux kernel development, but my understanding is that smaller commits are aggregated by select helpers, and then the larger changsets are merged by the BDFL.  Maybe something similar would work for DMD?

The problem is to attract enough contributors so that at some point a sufficiently large number of them become "core contributors" who are familiar enough with an area of the codebase to review patches.

Of course, this is unlikely to happen as long as everyone is discouraged after the first few pull request by a queue of 100 open pull request, and so we are back at the start again. ;)

David
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On Mon, 20 Aug 2012, Jason House wrote:

> > I wasn't suggesting anything other than 'less code revew' isn't a solution that's in our best interest.
> > 
> > If you want my opinion, Walter shouldn't be writing code unless the review queue is empty.  It's harsh, but it'd net us far better throughput of fixes and keep the contributors contributing.  We loose contributors and constrict their contribution rate by not allowing work to flow into the tree.
> > 
> > With increased throughput and increased contributors, they'd be in a better position to also review either others code, making the need for Walter to be the reviewer lower.
> > 
> > Lastly, as part of the code review, Walter needs to stop running the tests.  That's what the pull auto tester is for.  Look at the code, comment where not ready, merge where ready.
> > 
> > Later,
> > Brad
> 
> I'm not particularly familiar with Linux kernel development, but my understanding is that smaller commits are aggregated by select helpers, and then the larger changsets are merged by the BDFL.  Maybe something similar would work for DMD?

The key difference is that the helpers in the linux world are _deep_ experts in the parts they aggregate.  Currently the only person that qualifies w/in the D community is Don for the CTFE parts of dmd.


_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On 20 August 2012 23:09, David Nadlinger <code@klickverbot.at> wrote:
> On 20 Aug 2012, at 22:40, Jason House wrote:
>>
>> I'm not particularly familiar with Linux kernel development, but my understanding is that smaller commits are aggregated by select helpers, and then the larger changsets are merged by the BDFL.  Maybe something similar would work for DMD?
>
>
> The problem is to attract enough contributors so that at some point a sufficiently large number of them become "core contributors" who are familiar enough with an area of the codebase to review patches.
>
> Of course, this is unlikely to happen as long as everyone is discouraged after the first few pull request by a queue of 100 open pull request, and so we are back at the start again. ;)
>
> David

On the matter of the 100 open pull requests -- I don't have any idea how we can deal with this. More people would help with the issue mentioned in the original post (which is basically that Kenji is a prolific author!), but wouldn't reduce the number of open pull requests in general. It seems to me to be an inevitable consequence of GitHub, where we're using a single list for both 'work in progress' and 'ready for review'.

Fundamentally, I have no idea of how a sensible development
methodology can be constructed using GitHub pull requests.
When your only way to contribute code is via a pull request, and when
the only way to closing a pull request are *merge it OR *reject it, it
seems inevitable that the number of pull requests will increase
indefinitely. If it is mostly OK but needs a bit more work, it stays
open. If it is deferred to the next cycle, it stays open. If it needs
discussion, it stays open. If it is OK but has a merge conflict with
another pull request, it stays open. An alternative would be to close
it whenever any kind of issue is found, but then we lose the 'work in
progress' information and reduce the number of people looking at the
code.

The more I use git, the more I like it; the more I use github, the
more I hate it. It seems to promote an organization which doesn't make
any sense. As far as I can tell, you can't even get the single most
important piece of information: which pull requests have I made (for
all projects), and how many are still open? Which branches can I
safely remove, now? You can however trivially change a pull request,
even after it's been reviewed, by accidentally pushing to the same
branch! To me, github is a giant WTF.
Now everyone raves about how good github is, so I must be missing
something. Presumable, we are using it horribly wrongly. In any case,
I am certain that we need to change something, not just add extra
manpower.

I went through the old pull requests (those  of number 650 or less),
especially the yebblies ones, and merged everything I could. Almost
all of the remaining ones have some problem. But you can't tell this,
by just looking at the open pull requests. And since you cannot mark
the pull requests in any searchable way, you cannot see which have
been triaged and which have not. AFAIK you can't even see which have
been commented on most recently.
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On Tuesday, August 21, 2012 09:43:52 Don Clugston wrote:
>  As far as I can tell, you can't even get the single most
> important piece of information: which pull requests have I made (for
> all projects), and how many are still open?

You can easily get at the list of pull requests that you currently have open across all projects on github by clicking on "Pull Requests" on the main page. You can also get the list of _all_ of the pull requests that have been closed (be it because they were merged or just closed) by clicking on the "Closed" button on that pull requests page.

- Jonathan M Davis
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals