Please edit the commit message when merge a PR
When a PR is consistent on several commits, the final commit message composed by GitHub contains messages of all these commits: "fix typo", "address yyy comments", "revert zzz". If the initial commit message contained errors (e.g. absent issue number), it is easy to edit the title and text of a PR, but the initial commit message lefts unchanged. GitHub allows to edit the commit message of squashed commit, and please don't ignore this possibility. Otherwise the commit message in the repository will be ugly if not worse.
On Mon, Jul 10, 2017 at 4:14 PM, Serhiy Storchaka storchaka@gmail.com wrote:
When a PR is consistent on several commits, the final commit message composed by GitHub contains messages of all these commits: "fix typo", "address yyy comments", "revert zzz". If the initial commit message contained errors (e.g. absent issue number), it is easy to edit the title and text of a PR, but the initial commit message lefts unchanged. GitHub allows to edit the commit message of squashed commit, and please don't ignore this possibility. Otherwise the commit message in the repository will be ugly if not worse.
+1! (and thank you for writing this email, Serhiy)
I can't think of a way to automatically prevent a PR from merging if body of the squashed commit contains "fix typo" commits. I think this is a pretty annoying problem and perhaps we should ask contributors to squash multiple commits themselves even if we continue to use the "squash and merge" option.
I would prefer to ask the author to squash and/or rebase his/her commits rather than having to edit the commit message myself. I prefer that the commit message is part of the review, and not only done by the one who clicks on the Merge button.
It would prefer mistakes in the commit message.
GitHub PR UI is not ideal to review commit messages :-/
Victor
2017-07-10 15:14 GMT+02:00 Serhiy Storchaka storchaka@gmail.com:
When a PR is consistent on several commits, the final commit message composed by GitHub contains messages of all these commits: "fix typo", "address yyy comments", "revert zzz". If the initial commit message contained errors (e.g. absent issue number), it is easy to edit the title and text of a PR, but the initial commit message lefts unchanged. GitHub allows to edit the commit message of squashed commit, and please don't ignore this possibility. Otherwise the commit message in the repository will be ugly if not worse.
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
On Jul 10, 2017, at 04:57 PM, Victor Stinner wrote:
I would prefer to ask the author to squash and/or rebase his/her commits rather than having to edit the commit message myself. I prefer that the commit message is part of the review, and not only done by the one who clicks on the Merge button.
It would prefer mistakes in the commit message.
GitHub PR UI is not ideal to review commit messages :-/
GitLab is roughly the same, and I always edit commit messages when squash merging. While ideally the author could do this, I think sensible commit messages are more important, so +1 for having the committer edit them at their discretion.
-Barry
Often the committer has more context to write a proper commit message, and asking the contributor to do the squash is just wasting time (plus in general we *don't* want contributors to squash, since that loses the context for the review). So I'm with Sergey. This is how we do it in the mypy-related projects.
On Mon, Jul 10, 2017 at 8:09 AM, Barry Warsaw barry@python.org wrote:
On Jul 10, 2017, at 04:57 PM, Victor Stinner wrote:
I would prefer to ask the author to squash and/or rebase his/her commits rather than having to edit the commit message myself. I prefer that the commit message is part of the review, and not only done by the one who clicks on the Merge button.
It would prefer mistakes in the commit message.
GitHub PR UI is not ideal to review commit messages :-/
GitLab is roughly the same, and I always edit commit messages when squash merging. While ideally the author could do this, I think sensible commit messages are more important, so +1 for having the committer edit them at their discretion.
-Barry
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
-- --Guido van Rossum (python.org/~guido)
On 7/10/2017 10:49 AM, Berker Peksağ wrote:
On Mon, Jul 10, 2017 at 4:14 PM, Serhiy Storchaka storchaka@gmail.com wrote:
When a PR is consistent on several commits, the final commit message composed by GitHub contains messages of all these commits: "fix typo", "address yyy comments", "revert zzz". If the initial commit message contained errors (e.g. absent issue number), it is easy to edit the title and text of a PR, but the initial commit message lefts unchanged. GitHub allows to edit the commit message of squashed commit, and please don't ignore this possibility. Otherwise the commit message in the repository will be ugly if not worse.
+1! (and thank you for writing this email, Serhiy)
Ditto.
I can't think of a way to automatically prevent a PR from merging if body of the squashed commit contains "fix typo" commits. I think this is a pretty annoying problem and perhaps we should ask contributors to squash multiple commits themselves even if we continue to use the "squash and merge" option.
When people did that, we asked them not to. When reviewing, I want to be able to see both commits in response to comments and the total diff (by clicking files at the top). If people push a squash merge after a PR is approved both by CI and core reviewer, they might make a mistake. In any case, another round of CI is triggered.
In any case, I find editing easy. I often delete all and copy from the new blurb. When I write a patch myself, I try to write at least some commit messages that should remain in the final result. So I might copy some commit messages into the blurb.
2017-07-10 17:35 GMT+02:00 Guido van Rossum guido@python.org:
Often the committer has more context to write a proper commit message, and asking the contributor to do the squash is just wasting time (plus in general we *don't* want contributors to squash, since that loses the context for the review). So I'm with Sergey. This is how we do it in the mypy-related projects.
Oh, I noticed that comments were hidden after a rebase or squash. But I never understood the link between squash/rebase and hidden comments.
Ok, it makes sense to prefer merge and multiple commits to workaround UI limitations of GitHub PRs :-)
Victor
On Mon, 10 Jul 2017 at 07:57 Berker Peksağ berker.peksag@gmail.com wrote:
On Mon, Jul 10, 2017 at 4:14 PM, Serhiy Storchaka storchaka@gmail.com wrote:
When a PR is consistent on several commits, the final commit message composed by GitHub contains messages of all these commits: "fix typo", "address yyy comments", "revert zzz". If the initial commit message contained errors (e.g. absent issue number), it is easy to edit the title and text of a PR, but the initial commit message lefts unchanged. GitHub allows to edit the commit message of squashed commit, and please don't ignore this possibility. Otherwise the commit message in the repository will be ugly if not worse.
+1! (and thank you for writing this email, Serhiy)
I can't think of a way to automatically prevent a PR from merging if body of the squashed commit contains "fix typo" commits. I think this is a pretty annoying problem and perhaps we should ask contributors to squash multiple commits themselves even if we continue to use the "squash and merge" option.
There's isn't a way to block a merge at that stage. But one thing I've been thinking about is adding a check to Bedevere post-merge that sees if the commit message was left unchanged (not quite sure if I can come up with a reliable heuristic, though). In instances where the committers forgot, Bedevere would simply leave a message saying something like, "Hey, thanks for taking the time to merge a PR, but please don't forget to clean up the commit message before merging." Basically a friendly reminder to not forget next time (I'm also thinking of doing something similar for the formatting of the PR title after merging).
11.07.17 19:39, Brett Cannon пише:
There's isn't a way to block a merge at that stage. But one thing I've been thinking about is adding a check to Bedevere post-merge that sees if the commit message was left unchanged (not quite sure if I can come up with a reliable heuristic, though). In instances where the committers forgot, Bedevere would simply leave a message saying something like, "Hey, thanks for taking the time to merge a PR, but please don't forget to clean up the commit message before merging." Basically a friendly reminder to not forget next time (I'm also thinking of doing something similar for the formatting of the PR title after merging).
+1 for adding a post-merge check and a friendly reminder. I still see merged commits containing messages from all intermediate changes. I'm afraid that it would be rough from my side to remind about this every time, but the bot is impartial.
participants (7)
-
Barry Warsaw -
Berker Peksağ -
Brett Cannon -
Guido van Rossum -
Serhiy Storchaka -
Terry Reedy -
Victor Stinner