Re: Properly merging pull requests

[Lawrence Velázquez]

> On Nov 3, 2016, at 2:46 PM, Lawrence Velázquez  wrote:
> 
> 
>> On Nov 3, 2016, at 2:16 PM, Rainer Müller  wrote:
>> 
>> On 2016-11-03 18:57, Lawrence Velázquez wrote:
 On Nov 3, 2016, at 1:46 PM, Rainer Müller  wrote:
 
 I think the proper way to do it on GitHub would be as follows:
 When the pull request author checked the box for "Allow modifications by
 maintainers" [1], you can force-push your changes to the pull request
 branch, replacing the original commits. Then you can merge the pull
 request from the GitHub web interface.
>>> 
>>> GitHub will also automatically close the PR as "merged" if you push the
>>> PR branch's changes to master from a local client. It's quite nice.
>> 
>> If I understand this correctly, the exact commits have to be on the pull
>> request branch for GitHub to recognize you want to close the PR. So I
>> would have to push them first to the pull request branch and then to master.
> 
> Yeah, I think that's right. That means that rebasing from the command
> line is not feasible for most PRs because the contributors' branch is
> unlikely to be based on the absolute latest master. So I think the
> process you mentioned (push to collaborator's branch, rebase from the
> web) might become our standard operating procedure.

Actually I think I was mistaken about this. Rebasing onto master + fast-forward 
merging + pushing from the command line works fine, as long as you force-push 
your local changes to the PR branch first. It doesn't seem to matter where the 
PR branch was branched from; GitHub knows to isolate the changes that aren't in 
the base branch.

vq
Sent from my iPhone
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: Properly merging pull requests

[Ivan Larionov]

My 2 cents:

* cherry-pick will always change commit hash and github won't see it as the
same commit as from original PR
* "Closed #10" syntax was designed to close Issues from commit messages. I
think if you do "Closes #10" for PR it will close a PR in the way it will
look like it was declined
* I think asking contributor to make required changes and then just merging
is a proper way to manage PRs. If you want to help contributor add comments
to bad lines or provide correct code snippets

On Thu, Nov 3, 2016 at 11:46 AM, Lawrence Velázquez 
wrote:

>
> > On Nov 3, 2016, at 2:16 PM, Rainer Müller  wrote:
> >
> > On 2016-11-03 18:57, Lawrence Velázquez wrote:
> >>> On Nov 3, 2016, at 1:46 PM, Rainer Müller  wrote:
> >>>
> >>> I think the proper way to do it on GitHub would be as follows:
> >>> When the pull request author checked the box for "Allow modifications
> by
> >>> maintainers" [1], you can force-push your changes to the pull request
> >>> branch, replacing the original commits. Then you can merge the pull
> >>> request from the GitHub web interface.
> >>
> >> GitHub will also automatically close the PR as "merged" if you push the
> >> PR branch's changes to master from a local client. It's quite nice.
> >
> > If I understand this correctly, the exact commits have to be on the pull
> > request branch for GitHub to recognize you want to close the PR. So I
> > would have to push them first to the pull request branch and then to
> master.
>
> Yeah, I think that's right. That means that rebasing from the command
> line is not feasible for most PRs because the contributors' branch is
> unlikely to be based on the absolute latest master. So I think the
> process you mentioned (push to collaborator's branch, rebase from the
> web) might become our standard operating procedure.
>
> vq
> ___
> macports-dev mailing list
> macports-dev@lists.macosforge.org
> https://lists.macosforge.org/mailman/listinfo/macports-dev
>



-- 
With best regards, Ivan Larionov.
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: Properly merging pull requests

[Lawrence Velázquez]

> On Nov 3, 2016, at 2:16 PM, Rainer Müller  wrote:
> 
> On 2016-11-03 18:57, Lawrence Velázquez wrote:
>>> On Nov 3, 2016, at 1:46 PM, Rainer Müller  wrote:
>>> 
>>> I think the proper way to do it on GitHub would be as follows:
>>> When the pull request author checked the box for "Allow modifications by
>>> maintainers" [1], you can force-push your changes to the pull request
>>> branch, replacing the original commits. Then you can merge the pull
>>> request from the GitHub web interface.
>> 
>> GitHub will also automatically close the PR as "merged" if you push the
>> PR branch's changes to master from a local client. It's quite nice.
> 
> If I understand this correctly, the exact commits have to be on the pull
> request branch for GitHub to recognize you want to close the PR. So I
> would have to push them first to the pull request branch and then to master.

Yeah, I think that's right. That means that rebasing from the command
line is not feasible for most PRs because the contributors' branch is
unlikely to be based on the absolute latest master. So I think the
process you mentioned (push to collaborator's branch, rebase from the
web) might become our standard operating procedure.

vq
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: Properly merging pull requests

[Rainer Müller]

On 2016-11-03 18:57, Lawrence Velázquez wrote:
>> On Nov 3, 2016, at 1:46 PM, Rainer Müller  wrote:
>>
>> I think the proper way to do it on GitHub would be as follows:
>> When the pull request author checked the box for "Allow modifications by
>> maintainers" [1], you can force-push your changes to the pull request
>> branch, replacing the original commits. Then you can merge the pull
>> request from the GitHub web interface.
> 
> GitHub will also automatically close the PR as "merged" if you push the
> PR branch's changes to master from a local client. It's quite nice.

If I understand this correctly, the exact commits have to be on the pull
request branch for GitHub to recognize you want to close the PR. So I
would have to push them first to the pull request branch and then to master.

Rainer
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: Properly merging pull requests

[Lawrence Velázquez]

> On Nov 3, 2016, at 1:46 PM, Rainer Müller  wrote:
> 
> I think the proper way to do it on GitHub would be as follows:
> When the pull request author checked the box for "Allow modifications by
> maintainers" [1], you can force-push your changes to the pull request
> branch, replacing the original commits. Then you can merge the pull
> request from the GitHub web interface.

GitHub will also automatically close the PR as "merged" if you push the
PR branch's changes to master from a local client. It's quite nice.

vq
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: Properly merging pull requests

[Lawrence Velázquez]

> On Nov 3, 2016, at 11:59 AM, Mojca Miklavec  wrote:
> 
> We have recently experienced problems with pull requests showing up as
> rejected on the web interface rather than merged (while the changes
> were in fact accepted, possibly with some modifications).
> 
> I admit my sins. In one case I did a minor modification (squashed
> commits, removed the "Id" line, edited the commit message), while
> basically everything else stayed the same as the author submitted and
> everyone would have preferred if GitHub displayed "Merged" rather than
> the red rejected sign.

I think (don't quote me on this) that GitHub considers a PR "merged"
when the base branch is updated and has incorporated the changes from
the head branch. If a committer modifies their local branch before
merging, the base branch does not look like it has merged in the head
branch because the new content is in fact different.

(This makes sense, in a way. After all, if you had to change a PR branch
before merging it, did you really accept the PR?)

> We could have asked the author to keep fixing the branch forever, but
> at some point it's simply faster and easier if an experienced
> MacPorter just finishes the obvious rather than turning the users away
> due to some stupid mistakes. The users can fix the mistakes in the
> next PR.

To reduce excessive communication, it does seems like a good idea to ask
contributors to allow us to push changes to their PR branches. Those
modified branches should then merge as desired.

https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

> In another case I (stupidly?) decided to add "Closes: #N" at the end
> of the commit message (I thought it would be useful to have a link to
> the original PR which is something that the committer cannot do anyway
> without knowing the number of PR in advance).

Why was that stupid? Did it cause a problem?

(Note that GitHub shows PR links on closing commits. Look just under the
commit message, next to "master":
https://github.com/macports/macports-infrastructure/commit/1e6b092)

> I probably also sinned by thinking of "playing safe" and using
> cherry-pick rather than "wrongly rebasing" and risking some undesired
> merge.

You cherry-picked instead of rebasing the PR branch onto master? That
shouldn't cause any problems, I think.

> Can someone provide some guidelines of DOs and DONTs when accepting
> (and modifying) pull requests? That should cover the cases that need:
> - squashing commits
> - minor edits of commit messages
> - other minor edits of the sources

My sense is that the only real rule is that you need to push
modifications before doing the merge. For most PRs, that would require
the contributor to enable the setting that I mentioned above.

vq
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: Properly merging pull requests

[Rainer Müller]

On 2016-11-03 16:59, Mojca Miklavec wrote:
> Hi,
> 
> We have recently experienced problems with pull requests showing up as
> rejected on the web interface rather than merged (while the changes
> were in fact accepted, possibly with some modifications).
> 
> I admit my sins. In one case I did a minor modification (squashed
> commits, removed the "Id" line, edited the commit message), while
> basically everything else stayed the same as the author submitted and
> everyone would have preferred if GitHub displayed "Merged" rather than
> the red rejected sign.
> 
> We could have asked the author to keep fixing the branch forever, but
> at some point it's simply faster and easier if an experienced
> MacPorter just finishes the obvious rather than turning the users away
> due to some stupid mistakes. The users can fix the mistakes in the
> next PR.
> 
> In another case I (stupidly?) decided to add "Closes: #N" at the end
> of the commit message (I thought it would be useful to have a link to
> the original PR which is something that the committer cannot do anyway
> without knowing the number of PR in advance). I probably also sinned
> by thinking of "playing safe" and using cherry-pick rather than
> "wrongly rebasing" and risking some undesired merge.

GitHub will already add an annotation right next to the branch name in
the commit view to show that this commit came from a pull request. As an
example, note "master (#2)" right below the commit message here:

https://github.com/macports/macports-ports/commit/ec150ad742e511cc38e5a0815c8b5805125d2aea

I think the proper way to do it on GitHub would be as follows:
When the pull request author checked the box for "Allow modifications by
maintainers" [1], you can force-push your changes to the pull request
branch, replacing the original commits. Then you can merge the pull
request from the GitHub web interface.

> Can someone provide some guidelines of DOs and DONTs when accepting
> (and modifying) pull requests? That should cover the cases that need:
> - squashing commits
> - minor edits of commit messages
> - other minor edits of the sources

If commits were made to address review comments, but these commits do
not follow our rules for commit messages, they need to be squashed or
edited. Otherwise, after the changes were brought to master with a
rebase, looking at these commit messages in the linear history would not
clearly show what they addressed.

Rainer

[1]
https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: Properly merging pull requests

[Lawrence Velázquez]

> On Nov 3, 2016, at 12:19 PM, Sterling Smith  wrote:
> 
> The user will not learn if you change it under his/her feet.  I think
> that you should make line by comments of changes that need to be made
> in the files tab of the pull request and ask the pull request
> submitter to make those changes.

While this is all fine in the abstract, in practice committers have
always felt free to improve contributors' patches before committing,
especially if said improvements are minor.

> Or you can make them yourself and push them to the branch of the pull
> request and ask the submitter if s/he is OK with them.

The contributor must first allow us to modify their PR branch:

https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

This seems very useful. Perhaps we should request that all contributors
do this when opening PRs.

vq
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: Properly merging pull requests

[Sterling Smith]

On Nov 3, 2016, at 8:59AM, Mojca Miklavec  wrote:

> Hi,
> 
> We have recently experienced problems with pull requests showing up as
> rejected on the web interface rather than merged (while the changes
> were in fact accepted, possibly with some modifications).
> 
> I admit my sins. In one case I did a minor modification (squashed
> commits, removed the "Id" line, edited the commit message), while
> basically everything else stayed the same as the author submitted and
> everyone would have preferred if GitHub displayed "Merged" rather than
> the red rejected sign.
> 
> We could have asked the author to keep fixing the branch forever, but
> at some point it's simply faster and easier if an experienced
> MacPorter just finishes the obvious rather than turning the users away
> due to some stupid mistakes. The users can fix the mistakes in the
> next PR.
The user will not learn if you change it under his/her feet.  I think that you 
should make line by comments of changes that need to be made in the files tab 
of the pull request and ask the pull request submitter to make those changes.  
Or you can make them yourself and push them to the branch of the pull request 
and ask the submitter if s/he is OK with them.  Either way, the submitter 
learns of the MacPorts Way, and will be less likely to make those mistakes in 
the future.  
> 
> In another case I (stupidly?) decided to add "Closes: #N" at the end
> of the commit message (I thought it would be useful to have a link to
> the original PR which is something that the committer cannot do anyway
> without knowing the number of PR in advance). I probably also sinned
> by thinking of "playing safe" and using cherry-pick rather than
> "wrongly rebasing" and risking some undesired merge.
> 
> Can someone provide some guidelines of DOs and DONTs when accepting
> (and modifying) pull requests? That should cover the cases that need:
> - squashing commits
> - minor edits of commit messages

When a committer is happy with the changes of the pull request, but not the 
commit messages, then they should request that the submitter make appropriate 
changes to the branch (with an interactive rebase), and force push those 
changes.  The branch will then be in a position to push the GitHub rebase 
button.
> 
> - other minor edits of the sources
See my comment above about pushing changes to the branch of the pull request 
and asking the submitter for their approval.
> 
> Thank you,
>Mojca

My two cents.

-Sterling


___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev