So? now that we've moved to github, what is the preferred method for committing stuff to Phobos?  Should we continue pushing directly into the main repository, or should we do it "the git way" and send pull requests?

I have a suggestion for a workflow:

1. Commit and push changes to own Phobos fork.
2. Send pull request to main Phobos repo.
3. Someone, which can be anyone in the Phobos team, reviews the code and
merges it.

This gives a certain amount of quality control, in that at least one other person sees the code before it is admitted, while not putting the responsibility of reviewing all incoming code on one person.

How does that sound?

-Lars

On Thursday 03 February 2011 03:30:16 Lars Tandle Kyllingstad wrote:
> So? now that we've moved to github, what is the preferred method for committing stuff to Phobos?  Should we continue pushing directly into the main repository, or should we do it "the git way" and send pull requests?
> 
> I have a suggestion for a workflow:
> 
> 1. Commit and push changes to own Phobos fork.
> 2. Send pull request to main Phobos repo.
> 3. Someone, which can be anyone in the Phobos team, reviews the code and
> merges it.
> 
> This gives a certain amount of quality control, in that at least one other person sees the code before it is admitted, while not putting the responsibility of reviewing all incoming code on one person.
> 
> How does that sound?

Well, I'm not sure that _every_ commit needs to be reviewed by another person (e.g. if you're just doing documentation changes or they're changes that you're sure of). And if you think that a commit doesn't really need anyone else to look at it, then pushing should be fine. But I do think that it makes good sense to use pull requests for any commits where you want someone else to look it over before it goes into the main branch. And, of course, patches from non-Phobos developers would come in through the pull mechanism rather than pushing.

- Jonathan M Davis

On 2/3/11 6:21 AM, Jonathan M Davis wrote:
> On Thursday 03 February 2011 03:30:16 Lars Tandle Kyllingstad wrote:
>> So? now that we've moved to github, what is the preferred method for committing stuff to Phobos?  Should we continue pushing directly into the main repository, or should we do it "the git way" and send pull requests?
>>
>> I have a suggestion for a workflow:
>>
>> 1. Commit and push changes to own Phobos fork.
>> 2. Send pull request to main Phobos repo.
>> 3. Someone, which can be anyone in the Phobos team, reviews the code and
>> merges it.
>>
>> This gives a certain amount of quality control, in that at least one other person sees the code before it is admitted, while not putting the responsibility of reviewing all incoming code on one person.
>>
>> How does that sound?
>
> Well, I'm not sure that _every_ commit needs to be reviewed by another person (e.g. if you're just doing documentation changes or they're changes that you're sure of).

I think every commit should be seen by at least two persons. It has happened that an apparently trivial fix has caused trouble.

Andrei

On Thu, 2011-02-03 at 10:17 -0600, Andrei Alexandrescu wrote:
> On 2/3/11 6:21 AM, Jonathan M Davis wrote:
> > On Thursday 03 February 2011 03:30:16 Lars Tandle Kyllingstad wrote:
> >> So? now that we've moved to github, what is the preferred method for committing stuff to Phobos?  Should we continue pushing directly into the main repository, or should we do it "the git way" and send pull requests?
> >>
> >> I have a suggestion for a workflow:
> >>
> >> 1. Commit and push changes to own Phobos fork.
> >> 2. Send pull request to main Phobos repo.
> >> 3. Someone, which can be anyone in the Phobos team, reviews the code and
> >> merges it.
> >>
> >> This gives a certain amount of quality control, in that at least one other person sees the code before it is admitted, while not putting the responsibility of reviewing all incoming code on one person.
> >>
> >> How does that sound?
> >
> > Well, I'm not sure that _every_ commit needs to be reviewed by another person (e.g. if you're just doing documentation changes or they're changes that you're sure of).
> 
> I think every commit should be seen by at least two persons. It has happened that an apparently trivial fix has caused trouble.

...and some of those times, the trouble surfaced *after* other people had made further commits, making it difficult to figure out just which commit caused it.

I agree that all code should be reviewed, even if it's just a couple of people saying "looks good" in the pull request discussion.  The one making the request can even take care of merging the code, once enough people have given their "ok".

-Lars

Lars, you seem to know your way around the process. Do you think you could put together a short document and a collection of scripts that people can use? The workflows involved are:

1. I am done making a change and want to submit it for review.

2. Reviewers gave feedback and I need to change my stuff and submit it back for review. Preferably there would be an obvious connection between the first and subsequent submissions.

3. Reviewers are okay with my changes, I need to upload them.

Ideally we'd have a means of enforcement, i.e. no code makes it that didn't get "OK" at least from one reviewer. Also, even if one reviewer gave "OK" another one could veto.

We use a similar process at Facebook and it is extremely helpful.


Andrei

On Thursday 03 February 2011 08:17:17 Andrei Alexandrescu wrote:
> On 2/3/11 6:21 AM, Jonathan M Davis wrote:
> > On Thursday 03 February 2011 03:30:16 Lars Tandle Kyllingstad wrote:
> >> So? now that we've moved to github, what is the preferred method for committing stuff to Phobos?  Should we continue pushing directly into the main repository, or should we do it "the git way" and send pull requests?
> >> 
> >> I have a suggestion for a workflow:
> >> 
> >> 1. Commit and push changes to own Phobos fork.
> >> 2. Send pull request to main Phobos repo.
> >> 3. Someone, which can be anyone in the Phobos team, reviews the code and
> >> merges it.
> >> 
> >> This gives a certain amount of quality control, in that at least one other person sees the code before it is admitted, while not putting the responsibility of reviewing all incoming code on one person.
> >> 
> >> How does that sound?
> > 
> > Well, I'm not sure that _every_ commit needs to be reviewed by another person (e.g. if you're just doing documentation changes or they're changes that you're sure of).
> 
> I think every commit should be seen by at least two persons. It has happened that an apparently trivial fix has caused trouble.

Well, if that's what we want to do, git certainly makes that workflow work/flow better than it would with svn.

- Jonathan M Davis

On Thu, 2011-02-03 at 10:54 -0600, Andrei Alexandrescu wrote:
> Lars, you seem to know your way around the process. Do you think you could put together a short document and a collection of scripts that people can use? The workflows involved are:
> 
> 1. I am done making a change and want to submit it for review.
> 
> 2. Reviewers gave feedback and I need to change my stuff and submit it back for review. Preferably there would be an obvious connection between the first and subsequent submissions.
> 
> 3. Reviewers are okay with my changes, I need to upload them.

Sure, I can write a "contributor's guide". :)  I don't know how much we can do with scripts, though.  A lot of the process is just point'n-click on GitHub.


> Ideally we'd have a means of enforcement, i.e. no code makes it that didn't get "OK" at least from one reviewer. Also, even if one reviewer gave "OK" another one could veto.
> 
> We use a similar process at Facebook and it is extremely helpful.

I'll check whether GitHub supports anything like that.

-Lars

On Thu, 2011-02-03 at 18:25 +0100, Lars Tandle Kyllingstad wrote: [ . . . ]
> I'll check whether GitHub supports anything like that.

Use Gerrit perhaps?

-- 
Russel. ============================================================================= Dr Russel Winder      t: +44 20 7585 2200   voip: sip:russel.winder at ekiga.net 41 Buckmaster Road    m: +44 7770 465 077   xmpp: russel at russel.org.uk London SW11 1EN, UK   w: www.russel.org.uk  skype: russel_winder
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.puremagic.com/pipermail/phobos/attachments/20110203/51600a60/attachment.pgp>

On 2/3/11 11:25 AM, Lars Tandle Kyllingstad wrote:
> On Thu, 2011-02-03 at 10:54 -0600, Andrei Alexandrescu wrote:
>> Lars, you seem to know your way around the process. Do you think you could put together a short document and a collection of scripts that people can use? The workflows involved are:
>>
>> 1. I am done making a change and want to submit it for review.
>>
>> 2. Reviewers gave feedback and I need to change my stuff and submit it back for review. Preferably there would be an obvious connection between the first and subsequent submissions.
>>
>> 3. Reviewers are okay with my changes, I need to upload them.
>
> Sure, I can write a "contributor's guide". :)  I don't know how much we can do with scripts, though.  A lot of the process is just point'n-click on GitHub.

I'd like some scripts such as:

1. "git submit" -> submits for first review
2. "git resubmit" -> resubmits for subsequent reviews (with an argument
identifying the commit)
3. "git approved" -> submission approved, push the commit

Git allows defining such custom commands in ~/.gitconfig. If a point-and-click procedure needs to be done instead of the above, let's just document that. Essentially if we boil this down to a one-page cheat sheet that anyone can use, that would be perfect.

>> Ideally we'd have a means of enforcement, i.e. no code makes it that didn't get "OK" at least from one reviewer. Also, even if one reviewer gave "OK" another one could veto.
>>
>> We use a similar process at Facebook and it is extremely helpful.
>
> I'll check whether GitHub supports anything like that.

Thanks very much!


Andrei

On 3 February 2011 17:45, Lars Tandle Kyllingstad <lars at kyllingen.net> wrote:
> On Thu, 2011-02-03 at 10:17 -0600, Andrei Alexandrescu wrote:
>> On 2/3/11 6:21 AM, Jonathan M Davis wrote:
>> > On Thursday 03 February 2011 03:30:16 Lars Tandle Kyllingstad wrote:
>> >> So? now that we've moved to github, what is the preferred method for committing stuff to Phobos? ?Should we continue pushing directly into the main repository, or should we do it "the git way" and send pull requests?
>> >>
>> >> I have a suggestion for a workflow:
>> >>
>> >> 1. Commit and push changes to own Phobos fork.
>> >> 2. Send pull request to main Phobos repo.
>> >> 3. Someone, which can be anyone in the Phobos team, reviews the code and
>> >> merges it.
>> >>
>> >> This gives a certain amount of quality control, in that at least one other person sees the code before it is admitted, while not putting the responsibility of reviewing all incoming code on one person.
>> >>
>> >> How does that sound?
>> >
>> > Well, I'm not sure that _every_ commit needs to be reviewed by another person (e.g. if you're just doing documentation changes or they're changes that you're sure of).
>>
>> I think every commit should be seen by at least two persons. It has happened that an apparently trivial fix has caused trouble.
>
> ...and some of those times, the trouble surfaced *after* other people had made further commits, making it difficult to figure out just which commit caused it.
>
> I agree that all code should be reviewed, even if it's just a couple of people saying "looks good" in the pull request discussion. ?The one making the request can even take care of merging the code, once enough people have given their "ok".

At least, we should have two people (the coder and the reviewer) with
the responsibility to revert the change immediately if it causes the
auto-tester to break.
I'm a bit sick of seeing failures on the autotester that take days to
be resolved.

My view: I actually don't think it's a big issue to have a bad commit (because it's so easy to revert them), but it's a huge issue to have a bad commit that isn't addressed immediately.