Jump to page: 1 2
Thread overview
[dmd-beta] Cherry-picking II
Jan 28, 2014
Andrew Edwards
Jan 28, 2014
Brad Roberts
Jan 28, 2014
Martin Nowak
Jan 29, 2014
Andrew Edwards
Feb 03, 2014
Brad Roberts
Feb 04, 2014
Andrew Edwards
Feb 02, 2014
Andrew Edwards
Feb 02, 2014
Jesse Phillips
Feb 02, 2014
Jesse Phillips
Feb 02, 2014
Andrew Edwards
Feb 03, 2014
Daniel Murphy
January 28, 2014
Recent experience with #3103 and #3151 suggests that there needs to be a better way of identifying what goes in the releases. Currently, I am honing in on regressions and ICEs because those are the stated objectives for this release. That being said, if I read one of these fixes and is says git master/head only, I simply mark it as ignore and move on. If another fix appearing down the line depends on the one I've ignored to be merged first, I do not know if it does not explicitly state.

Additionally, it causes a slight confusion when I encounter errors upon attempts to sync local branch with upstream branch, which I'm under the assumption that I'm the only one cherry picking to, because someone else is committing to that branch.

These two issues prompts me to suggest that instead of simply merge and forget or merge and cherry-pick yourselves that you simply assign the PR to me after the merge if it is intended to be included in the upcoming release cycle. With this one action, we can alleviate all confusion about what should be include in the release and prevent errors/conflicts when trying to commit to release branches upstream.

Your understanding and efforts are appreciated.

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


January 28, 2014
On 1/28/14 9:27 AM, Andrew Edwards wrote:
> Recent experience with #3103 and #3151 suggests that there needs to be a better way of identifying
> what goes in the releases. Currently, I am honing in on regressions and ICEs because those are the
> stated objectives for this release. That being said, if I read one of these fixes and is says git
> master/head only, I simply mark it as ignore and move on. If another fix appearing down the line
> depends on the one I've ignored to be merged first, I do not know if it does not explicitly state.
>
> Additionally, it causes a slight confusion when I encounter errors upon attempts to sync local
> branch with upstream branch, which I'm under the assumption that I'm the only one cherry picking to,
> because someone else is committing to that branch.
>
> These two issues prompts me to suggest that instead of simply merge and forget or merge and
> cherry-pick yourselves that you simply assign the PR to me after the merge if it is intended to be
> included in the upcoming release cycle. With this one action, we can alleviate all confusion about
> what should be include in the release and prevent errors/conflicts when trying to commit to release
> branches upstream.
>
> Your understanding and efforts are appreciated.
>
> Regards,
> Andrew

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.

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


January 28, 2014
On 01/28/2014 07: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. 
Yeah, let's test the merging as well.
_______________________________________________
dmd-beta mailing list
dmd-beta@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-beta


January 28, 2014
I think we still should try to form a culture of targeting PR into release branch instead of master in the long term, reserving cherry-pick approach for the rest. At least major regression fix contrbutors should try to do it to make Andrew life tiny bit easier.


On Tue, Jan 28, 2014 at 7:04 PM, Brad Roberts <braddr@puremagic.com> wrote:

> On 1/28/14 9:27 AM, Andrew Edwards wrote:
>
>> Recent experience with #3103 and #3151 suggests that there needs to be a
>> better way of identifying
>> what goes in the releases. Currently, I am honing in on regressions and
>> ICEs because those are the
>> stated objectives for this release. That being said, if I read one of
>> these fixes and is says git
>> master/head only, I simply mark it as ignore and move on. If another fix
>> appearing down the line
>> depends on the one I've ignored to be merged first, I do not know if it
>> does not explicitly state.
>>
>> Additionally, it causes a slight confusion when I encounter errors upon
>> attempts to sync local
>> branch with upstream branch, which I'm under the assumption that I'm the
>> only one cherry picking to,
>> because someone else is committing to that branch.
>>
>> These two issues prompts me to suggest that instead of simply merge and
>> forget or merge and
>> cherry-pick yourselves that you simply assign the PR to me after the
>> merge if it is intended to be
>> included in the upcoming release cycle. With this one action, we can
>> alleviate all confusion about
>> what should be include in the release and prevent errors/conflicts when
>> trying to commit to release
>> branches upstream.
>>
>> Your understanding and efforts are appreciated.
>>
>> Regards,
>> Andrew
>>
>
> 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.
>
>
> _______________________________________________
> dmd-beta mailing list
> dmd-beta@puremagic.com
> http://lists.puremagic.com/mailman/listinfo/dmd-beta
>


January 29, 2014
On 1/28/14, 1:04 PM, Brad Roberts wrote:
> On 1/28/14 9:27 AM, Andrew Edwards wrote:
>> Recent experience with #3103 and #3151 suggests that there needs to be a better way of identifying
>> what goes in the releases. Currently, I am honing in on regressions and ICEs because those are the
>> stated objectives for this release. That being said, if I read one of these fixes and is says git
>> master/head only, I simply mark it as ignore and move on. If another fix appearing down the line
>> depends on the one I've ignored to be merged first, I do not know if it does not explicitly state.
>>
>> Additionally, it causes a slight confusion when I encounter errors upon attempts to sync local
>> branch with upstream branch, which I'm under the assumption that I'm the only one cherry picking to,
>> because someone else is committing to that branch.
>>
>> These two issues prompts me to suggest that instead of simply merge and forget or merge and
>> cherry-pick yourselves that you simply assign the PR to me after the merge if it is intended to be
>> included in the upcoming release cycle. With this one action, we can alleviate all confusion about
>> what should be include in the release and prevent errors/conflicts when trying to commit to release
>> branches upstream.
>>
>> Your understanding and efforts are appreciated.
>>
>> Regards,
>> Andrew
>
> 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.

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. 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?

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

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

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


February 02, 2014
So this is where the discussion ends? No decision or further communication. All forward action suggest the status quo. We honestly need to do something here gents. Were it my decision to make, would have been handled on day one but since it's not, I need your participation.

Kenji, need you take a look at dmd/pull/#3140 and see what kind of problems will occur it is merged after #3103, #3151, #3174, and #3169 respectively.

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


February 02, 2014
Well, my opinion is that the quest to find a "simple" process is leading to reinventing the wheel with a fork and a spoon rather than a hammer and chisel. This new issue is not a new problem and would have been addressed with the appropriate process of creating pull requests against the release branch, or even merging to the release branch instead of master. But there seems to be some confusion about this (now that it has come up as a replacement for writing notes back and forth).

In your last email you took issue of adding a second review step, when the first doesn't get any attention. This is wrong, dead wrong. There should not be two reviews, there should only ever be one single pull request and that request should get the review.

Yes, this approach does require involvement of the patch contributors, but that is already true. The contributor is already tackling regressions for the new release, so they're already aware of where this fix needs to go. It just requires them to check out the release branch on the three repos before starting.

Even if the developers don't wish to push the request against release, they can still leave a comment on the Pull request (against master) stating this should go into release. At this point the pull request is NOT merged into master, instead it is merged into the release branch. This approach has the negative that the fix may not be complete or have merge conflicts due to dependency on previous changes (this would be where a comment should be left and request that the issues be addressed).

Once the release is complete, merge it back into master (this can actually be done at any time, but should always be done at the end of release).


On Sun, Feb 2, 2014 at 9:29 AM, Andrew Edwards <edwards.ac@gmail.com> wrote:

> So this is where the discussion ends? No decision or further communication. All forward action suggest the status quo. We honestly need to do something here gents. Were it my decision to make, would have been handled on day one but since it's not, I need your participation.
>
> Kenji, need you take a look at dmd/pull/#3140 and see what kind of problems will occur it is merged after #3103, #3151, #3174, and #3169 respectively.
>
> Thanks,
> Andrew
>
> _______________________________________________
> dmd-beta mailing list
> dmd-beta@puremagic.com
> http://lists.puremagic.com/mailman/listinfo/dmd-beta
>



-- 
Jesse Phillips


February 02, 2014
Well, what about getting back to sane git model but using inverse help from Release Manager? I mean making his job to make sure no fix commits go into master while release is in progress and reverting those who slip by an accident (with a reminder to redo pull request to proper base). This is much less of a burden than cherry-picking and may eventually teach everyone to make correct pulls without reminders :)


On Sun, Feb 2, 2014 at 8:04 PM, Jesse Phillips <jesse.k.phillips@gmail.com>wrote:

> Well, my opinion is that the quest to find a "simple" process is leading to reinventing the wheel with a fork and a spoon rather than a hammer and chisel. This new issue is not a new problem and would have been addressed with the appropriate process of creating pull requests against the release branch, or even merging to the release branch instead of master. But there seems to be some confusion about this (now that it has come up as a replacement for writing notes back and forth).
>
> In your last email you took issue of adding a second review step, when the first doesn't get any attention. This is wrong, dead wrong. There should not be two reviews, there should only ever be one single pull request and that request should get the review.
>
> Yes, this approach does require involvement of the patch contributors, but that is already true. The contributor is already tackling regressions for the new release, so they're already aware of where this fix needs to go. It just requires them to check out the release branch on the three repos before starting.
>
> Even if the developers don't wish to push the request against release, they can still leave a comment on the Pull request (against master) stating this should go into release. At this point the pull request is NOT merged into master, instead it is merged into the release branch. This approach has the negative that the fix may not be complete or have merge conflicts due to dependency on previous changes (this would be where a comment should be left and request that the issues be addressed).
>
> Once the release is complete, merge it back into master (this can actually be done at any time, but should always be done at the end of release).
>
>
> On Sun, Feb 2, 2014 at 9:29 AM, Andrew Edwards <edwards.ac@gmail.com>wrote:
>
>> So this is where the discussion ends? No decision or further communication. All forward action suggest the status quo. We honestly need to do something here gents. Were it my decision to make, would have been handled on day one but since it's not, I need your participation.
>>
>> Kenji, need you take a look at dmd/pull/#3140 and see what kind of problems will occur it is merged after #3103, #3151, #3174, and #3169 respectively.
>>
>> Thanks,
>> Andrew
>>
>> _______________________________________________
>> dmd-beta mailing list
>> dmd-beta@puremagic.com
>> http://lists.puremagic.com/mailman/listinfo/dmd-beta
>>
>
>
>
> --
> Jesse Phillips
>
> _______________________________________________
> dmd-beta mailing list
> dmd-beta@puremagic.com
> http://lists.puremagic.com/mailman/listinfo/dmd-beta
>


February 02, 2014
Considering Andrew is requesting help to identify which changes should be merged into release, I doubt this would be acceptable. The person creating the patch is the most likely candidate to know that his changes need to enter into release. Though if they are working on regressions, it is possible to identify the pull requests made for them.

If there was agreement to move in this direction, then me and others could probably help by yelling at those who make a pull against master and who pull the request into master.

Andrew has been doing a really good job. Working hard to get the builds to generate consistently, trying to identify what needs to go into release. But he's been put in a position where no one seems to want to make his time a little easier. The idea that a build manager can just take care of things and everyone else could just go about their business as usual always seemed very naive to me.

------
Now I'm going to rant on some of the things mention.

The freeze, fix, release, process is very easy to run, its the simplest process for making a release. However we've decided that isn't acceptable anymore, that means things must get complicated and that means things will change. If the goal is to make releasing a version as simple as possible, we're heading in the wrong direction. If the goal is to allow development to continue while pushing a release, and to support a release once it is out the door, then everyone must realize that this means there is some more work to organize that process and everyone will play a small role.

There was mention of not using branches and instead "forking." I'm sorry, git doesn't know the difference, they are the same damn thing in git's view, they are even managed practically the same. The only benefit from the article was a psychological one. The choice they made was to manage their branches separately; instead of patching one branch and merging/cherry-picking into another, they duplicated the effort in both branches, the branches were never to cross paths again (one dying out eventually). Essentially they just forced themselves into the limitations of SVN.


On Sun, Feb 2, 2014 at 11:34 AM, Михаил Страшун <public@dicebot.lv> wrote:

> Well, what about getting back to sane git model but using inverse help from Release Manager? I mean making his job to make sure no fix commits go into master while release is in progress and reverting those who slip by an accident (with a reminder to redo pull request to proper base). This is much less of a burden than cherry-picking and may eventually teach everyone to make correct pulls without reminders :)
>


February 02, 2014
On 2/2/14, 2:04 PM, Jesse Phillips wrote:
> Well, my opinion is that the quest to find a "simple" process is leading to reinventing the wheel with a fork and a spoon rather than a hammer and chisel. This new issue is not a new problem and would have been addressed with the appropriate process of creating pull requests against the release branch, or even merging to the release branch instead of master. But there seems to be some confusion about this (now that it has come up as a replacement for writing notes back and forth).
>
I agree with you that what we are doing now complicates matters more. It is not my choice that I made the decision to cherry-pick commits to the release branch, rather it is compromise that promotes forward progress. Fact is, only three people adhered to the request to create pull requests against the release branch and there are a lot more contributors than that. Honestly I believe this is the best approach but no matter how good one things a particular approach is, if no one is doing it, it serves no purpose.

> In your last email you took issue of adding a second review step, when the first doesn't get any attention. This is wrong, dead wrong. There should not be two reviews, there should only ever be one single pull request and that request should get the review.
>
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.

> Yes, this approach does require involvement of the patch contributors, but that is already true. The contributor is already tackling regressions for the new release, so they're already aware of where this fix needs to go. It just requires them to check out the release branch on the three repos before starting.
>
Though I'd like this to happen, I really doubt it is going to. Therefore, in order not to prolong the process, I've decided not to pursue this direction any longer.

> Even if the developers don't wish to push the request against release, they can still leave a comment on the Pull request (against master) stating this should go into release. At this point the pull request is NOT merged into master, instead it is merged into the release branch. This approach has the negative that the fix may not be complete or have merge conflicts due to dependency on previous changes (this would be where a comment should be left and request that the issues be addressed).
>
> Once the release is complete, merge it back into master (this can actually be done at any time, but should always be done at the end of release).
>
>
> On Sun, Feb 2, 2014 at 9:29 AM, Andrew Edwards <edwards.ac@gmail.com <mailto:edwards.ac@gmail.com>> wrote:
>
>     So this is where the discussion ends? No decision or further
>     communication. All forward action suggest the status quo. We
>     honestly need to do something here gents. Were it my decision to
>     make, would have been handled on day one but since it's not, I
>     need your participation.
>
>     Kenji, need you take a look at dmd/pull/#3140 and see what kind of
>     problems will occur it is merged after #3103, #3151, #3174, and
>     #3169 respectively.
>
>     Thanks,
>     Andrew
>
>     _______________________________________________
>     dmd-beta mailing list
>     dmd-beta@puremagic.com <mailto:dmd-beta@puremagic.com>
>     http://lists.puremagic.com/mailman/listinfo/dmd-beta
>
>
>
>
> -- 
> Jesse Phillips
>
>
> _______________________________________________
> dmd-beta mailing list
> dmd-beta@puremagic.com
> http://lists.puremagic.com/mailman/listinfo/dmd-beta



« First   ‹ Prev
1 2