[phobos] Push or pull?

[Don Clugston]

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.