On 12 April 2017 at 00:21, R. David Murray <rdmurray@bitdance.com> wrote:
On Mon, 10 Apr 2017 21:53:44 -0700, Guido van Rossum <guido@python.org> wrote:
When the contributor makes multiple local commits without pushing to the PR, I recommend using --amend unless they have several commits that actually are logically distinct and relevant to the reviewer. (--amend is especially important when fixing lint bugs or typos).
Please don't use --amend across pushes to the PR
It's OK to just pull+rebase and then use git push -f, but honestly pull+merge is fine too
When responding to a review please NEVER use commit --amend, that's where reviewing becomes painful
In the devguide PR addressing this issue, I suggested we just say that one should never force push to the PR. I can see that a force push after a rebase *before* addressing review comments would be fine, but maybe it would be better to prefer merge since we're going to squash at the end anyway.
Yeah, Serhiy made this point when we were working on the instructions for committers editing a contributor's PR branch: https://docs.python.org/devguide/gitbootcamp.html#editing-a-pull-request-pri...
Since we do the squash & merge to get an atomic commit at the end, it doesn't make sense to do any force pushes along the way.
- It's up to the reviewer who merges to rewrite the commit message: the reviewer usually has a much better sense for what info in the commit message will still be interesting a month or a year from now than the contributor. (Often just copying the original comment from the top of the PR is adequate.)
Some of our core committers need to learn to do this :)
Technically coming up with a meaningful message when committing patches isn't a new requirement, but I admit it's a lot easier to forget to clean things up when there's a big green button right in front of you waiting to be pushed :)
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia