On Mon, Feb 3, 2014 at 8:07 AM, Andrew Edwards <edwards.ac@gmail.com> wrote:

I took issue with it because by adding this extra step, we are inducing
> additional delay in the entire process. As you said, one review should suffice. All that's left to do after that is flip a switch indicating that it needs to be in the release branch. This can be accomplish in one of two ways: 1) set the Asignee to AndrewEdwards following the merge; 2) Set the Milestone to 2.065 anytime during the creation, review or merger of the request. Come to think of it, a combination of the two would probably provide the best solution. The author assigns the milestone when creating the pull. Upon seeing this milestone the reviewer assigns the pull to me after the merge. A two step process that requires one additional step from two different people.
>
>
This seems realistic.  If any pulls don't merge cleanly into the release branch, or haven't been assigned to you and you think they should be, a comment on the pull asking for a new pull or clarification should get it done.  After all, the pull author will know how to do this best.

On 1/29/14, 12:07 AM, Andrew Edwards wrote:
> On 1/28/14, 1:04 PM, Brad Roberts wrote:
>> IMHO, a much more workable solution is to use pull requests just like
>> for any other branch.  If someone is requesting a merge to a release
>> branch, then they should assemble the pull request and submit it.  If
>> you are deciding a fix should be merged to the release branch, put
>> together the pull request just like anyone else would.  That gains
>> several advantages:
>>
>>   1) gives a good chance to review exactly what changes are going to
>> be made
>>   2) gives the auto-tester a chance to validate then changes
>>   3) gives a chance for additional eyeballs to be watching for mistakes
>>
>> The only con is that it's more steps, but without those steps, the
>> gains aren't possible.  For any regular developer, putting together a
>> pull request is something they can do in their sleep, so the cost is
>> pretty small.
>>
> I'm not necessarily against this but I have a few questions.
>
> 1) A change is placed in git-hub and reviewed prior to being merged into
> master. Without such a review it will not be accepted. Why now should we
> hold another review session prior to picking something to include into
> the branch? Isn't it better to require a minimum number of reviewers
> (say three for good measures) to approve a change before to committing
> to master? That way auto designation of such changes can be made at the
> time of review with a majority vote from the reviewers.

Not at all.  As a release branch progresses, it naturally grows further out of sync with the master branch.  That by itself lends to some justification for a re-review, even if a very quick on.  Additionally, it's a sanity check to allow for the opportunity to make sure what's being merged is what's intended to be merged.  Pushing directly into _any_ branch rather than going through a merge pull raises the risk of accidents happening.  Such accidents have happened more than a few times throughout our history.  Even more have happened and caused no damage in wrongly constructed pull requests, indicating the value of stepping through a pull request and avoiding damaging the master branch.  It has much less to do with correctness of the change, but rather with validation of the merge and it's continued correctness within the differing code base.

> 2) We just agreed upon a naming scheme that you insisted had to meet a
> certain convention in order to guarantee validation by the auto-tester.

Not relevant.

> If the auto tester already test this branch, and it does, why now would
> I need another way of monitoring what changes made to the branch?

See above.

> 3) How often have you seen a request for comment on a particular pull go
> unanswered? Just this past week, several request were made by Daniel
> Murphy than no on responded to. In the end, he made the decision on his
> own. Here are a couple that still remain unanswered:
>
> https://github.com/D-Programming-Language/dmd/pull/3120#issuecomment-33344986
> https://github.com/D-Programming-Language/dmd/pull/3118#issuecomment-33309502
> https://github.com/D-Programming-Language/phobos/pull/1864#issuecomment-32484778

Don't conflate the multitude of open pull requests to master with the tiny number of pull requests that are likely to ever be opened against the release branches.  The usage patterns, while similar, aren't the same.  Additionally, our processes are evolving.  One instance of it not going well has only minor bearing on continuing to seek one that balances correctness, safety, and ease of practice.

> By doing this we would be unnecessarily inducing another delay in the
> process: which is counterproductive.

I don't believe that's accurate.  What I suggest is a substitution of a request for you to merge that comes in via a bug notation, an email, or a pull request note in the master branch pull with a pull request.  The latter being significantly less likely to get lost in the noise than most of the former.  Lastly, by constructing and submitting a pull request, chances are even higher that by the time you see it, it's already both been validated by the requester as matching his intent and by the auto-tester as passing tests.  That just leaves a little button pressing to perform the merge.


Anyway, those are my 2 cents and they're certainly not the only way of handling things, just the way I would.

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

On 2/3/14, 12:33 AM, Brad Roberts wrote:
> On 1/29/14, 12:07 AM, Andrew Edwards wrote:
>> 1) A change is placed in git-hub and reviewed prior to being merged into
>> master. Without such a review it will not be accepted. Why now should we
>> hold another review session prior to picking something to include into
>> the branch? Isn't it better to require a minimum number of reviewers
>> (say three for good measures) to approve a change before to committing
>> to master? That way auto designation of such changes can be made at the
>> time of review with a majority vote from the reviewers.
>
> Not at all.  As a release branch progresses, it naturally grows further out of sync with the master branch.  That by itself lends to some justification for a re-review, even if a very quick on. Additionally, it's a sanity check to allow for the opportunity to make sure what's being merged is what's intended to be merged. Pushing directly into _any_ branch rather than going through a merge pull raises the risk of accidents happening.  Such accidents have happened more than a few times throughout our history.  Even more have happened and caused no damage in wrongly constructed pull requests, indicating the value of stepping through a pull request and avoiding damaging the master branch.  It has much less to do with correctness of the change, but rather with validation of the merge and it's continued correctness within the differing code base.
>
>> 2) We just agreed upon a naming scheme that you insisted had to meet a
>> certain convention in order to guarantee validation by the auto-tester.
>
> Not relevant.
>
I don't see why not. Take a walk with me 4 weeks after the final release of 2.065. At that time the new release cycle begins for 2.066. Would I need a pull request for everything in master that is about to make it into 2.066 without the added scrutiny of this safer process? Why is it not important to provide the same scrutiny to these changes as you would to changes that follow for the branch specifically?

>> By doing this we would be unnecessarily inducing another delay in the
>> process: which is counterproductive.
>
> I don't believe that's accurate.  What I suggest is a substitution of a request for you to merge that comes in via a bug notation, an email, or a pull request note in the master branch pull with a pull request.  The latter being significantly less likely to get lost in the noise than most of the former.  Lastly, by constructing and submitting a pull request, chances are even higher that by the time you see it, it's already both been validated by the requester as matching his intent and by the auto-tester as passing tests.  That just leaves a little button pressing to perform the merge.
>
So if I understand this right, you are suggesting that the author of a pull create two separate pull requests. One against master, which is tested by the auto tester, must be reviewed by one or more developer(s), approved, and merged into master once approval is granted. Once it passes the first phase, the author creates another pull request for the same changed to be merged into the release branch, which again must be auto tested, reviewed, and approved for merger to the branch. Finally, after the branch is released, it must be merged back into master and retested by the auto-tester. Am I understanding that correctly? If I am, isn't it simper to create the pull against the branch, merge it after approval and merge the branch back to master after release? That is the correct way and and it ultimately achieves everything your are seeking.

It would work like this:

    Sunday 04:00AM to Saturday 10:00PM - pull requests generated against branch or master are merged upon approval
    Saturday 10:00PM to Sunday 04:00AM - new tag is generated against branch and pushed upstream. Release binaries are built from new tag. Branch (beta or otherwise) is merged to master
    Sunday 04:00 - immediately after merging branch to master, resume merging approved changes to branch or master as appropriate

Six hour window for building to address any issues that might arise. Window may shrink as the process becomes more fluid. This keeps branch and master from diverging and puts the changes in their appropriate location to begin with. It also completely eliminates the guessing game of what needs to be in the branch. Safe, lean, and efficient.
_______________________________________________
dmd-beta mailing list
dmd-beta@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-beta