If someone has authored a PR with multiple commits, and then a committer adds one more commit e.g. to tweak the NEWS entry, when the patch is squashed and merged, will GitHub still choose the original author as the primary author? Or would it then be necessary to add “Co-authored-by:” (with the author’s name) to the commit message? (They are really the “Author” rather than “Co-author.”) I want to be sure that the author gets full credit.
Also: is it still necessary to add “Patch by” to the commit message in this case? Come to think of it, would it still be necessary to add “Patch by” and “Co-authored-by” to the commit message even if the committer didn’t add an additional commit?
I’m not 100% certain because I almost always prefer leaving comments and letting the author address the problem rather than pushing to the PR myself, but I believe it chooses the PR author as the primary author of the commit, and then the committer who pushed an additional change is added to the “Co-authored-by”. The same thing happens if the PR author directly applies a suggestion using the GitHub UI. So, I don’t think the “Patch by” or “Co-authored-by” is necessary to attribute the PR author as the author of the commit.
If the committer did not push a commit to the PR and only merged it, I’m certain that GitHub chooses the PR author as the sole author of the commit (assuming the author did not directly commit any suggestions in the GitHub UI or make direct changes to the “Co-authored-by” for any commits to the PR). For a recent example, see GH-20153 and its respective commit. So, the “Patch by” or “Co-authored-by” is not necessary in that case, as far as I’m aware.
On a related note regarding “Co-authored-by”, I’ve personally been unaware of how to best utilize it in the PRs of others. For my own, I typically try to use it for any review comments that result in a direct change. Is it generally acceptable to make manual “Co-authored-by” entries in the final merged commit message in someone else’s PR when merging it if the author forgot to attribute others that significantly contributed?
AFAIK, the “Patch by” at the end of the Misc/NEWS entry is also there for the purposes of attribution in the changelog and potentially in the whatsnew if the change is later added there. But, it’s not strictly necessary for record-keeping purposes, since as mentioned, GitHub links the PR to the author via the commit history.
As a result, I’ve typically considered that to be more of an opt-in step. I tend to bring it to the attention of contributors with something like this: “For attribution purposes, you can optionally add a ‘Patch by .’ at the end of the Misc/NEWS entry.”
I think the “Patch by” thing was a letfover from Mercurial era where only the committer can commit and that was the only way to record that the patch was contributed by someone else other than the committer.
I think it is not necessary to add “Patch By” in the commit message, I think it’s ok in the Misc/News files.
If someone has authored a PR with multiple commits, and then a committer adds one more commit e.g. to tweak the NEWS entry, when the patch is squashed and merged, will GitHub still choose the original author as the primary author?
Yes, the original author will be credited as the commit author in this case.
GitHub does automatically add the committer as the co-author though. So if you don’t want to be credited, then you can remove the co-authored-by:you line.
On a related note regarding “Co-authored-by”, I’ve personally been unaware of how to best utilize it in the PRs of others.
Some situations are:
applying other people’s mercurial patch to GitHub. This happens less recently, but had been more often when we had just moved to GitHub
PR author adapted or used part of other people’s patch and created their own PR on GitHub
backporting other people’s PR
Regarding whether or not to use the co-authored-by: when you suggested changes to the PR (using GitHub’s Suggest Change web UI), personally I think since you did re-write that line of code and the PR author accepted it and used it, then it’s worth keeping the co-authored-by:you line.
I don’t know. That doesn’t seem right to me. That would basically mean that anytime a reviewer made a suggestion on a PR that the author agreed to, they’d need to be added as a co-author. That seems like a significant broadening of the definition. Like, if you think about someone who writes a book, the book’s editor might do a ton of work on making suggestions, but they’re not listed as a co-author of the book.
If the suggestion is small, I would actually suggest the reverse and remove the Co-author attribution. (That’s what I did when I made some small changes to someone else’s PR before committing.) Just because GitHub’s UI does something, I don’t think we need to adopt that.
So, I think that it makes sense to keep it for more impactful changes to the PR, but for minor edits/suggestions (such as typo fixes, Misc/NEWS, etc), it might be better to remove the “Co-authored-by” prior to merging.
That also brings forth the following question: should the “Co-authored-by” be manually added if a reviewer suggests a highly substantial specific change the PR, but the author writes it themselves (exactly as suggested) instead of using the GitHub UI to commit the suggestion? In general, I think this would be reasonable, but it would likely have to be decided on a case-by-case basis.
Reviewer info is part of GitHub metadata and it can be seen in the PR (and also can be queried via API) I prefer that we don’t add “Reviewed-by” to commit message.
In our setup, reviewers generally have so much more power and reputation than contributors (except when reviewing work by other committers) that it would have to be a very substantial suggestion indeed before I would suggest letting a reviewer explicitly take part of the credit (beyond the fact that they reviewed), and even then only if the contributor agrees.
The exception would be when two people are effectively collaborating on a commit – there’s no way that I know of to fairly share the credit between two people, so it would make sense for one of the collaborators to act as the contributor and the other as the reviewer. (Though in this case I would suggest that a second reviewer would be needed, since neither of the co-authors can be considered unbiased.)
That seems very reasonable to me. In particular, I think an important part I was missing was the “only if the contributor agrees”. Since they are the original author of the PR, it completely makes sense to ask for their explicit consent prior to adding any co-authors to the commit, in addition to the suggestion(s) of the potential co-authors being of fundamental importance to the changes made. Thanks for the feedback.