Update WELCOME.md with CC list instructions#2161
Update WELCOME.md with CC list instructions#2161gitster wants to merge 1 commit intogitgitgadget:mainfrom
Conversation
Clarified instructions regarding CC list placement in PR descriptions. Too many people mistakenly put them in their commit message but GitGitGadget does not pay attention to them.
dscho
left a comment
There was a problem hiding this comment.
Wow! A contribution by the Git maintainer Himself!
While I understand, and agree with, the motivation, the implementation should probably involve a more precise logic. Here is my suggestion:
| NOTE: The CC list discussed here is to be added to the PR (Pull Request) description, *NOT* in your commit messages! | ||
|
|
There was a problem hiding this comment.
Hmm. As far as PR comments go, the welcome message is already quite long. So long, in fact, that its cognitive load virtually guarantees that any new contributor will miss some important bit of it.
Also, it strikes me as the wrong layer to just lump this warning into the welcome message. A welcome message should be, well, welcoming. That is not exactly accomplished by shouting at everyone indiscriminately that they should not include CC: lists in their commits when the vast majority of the audience already avoids that.
A more appropriate layer to do this is most likely phase 2 of the commit linter, which already warns about missing Signed-off-by trailers. It should be relatively simple to check for CC lists, too, something along these lines should do the trick:
private containsCCLines = (): void => {
const ccLines = this.lines.filter((line) => line.match(/^(\s*)Cc:\s+(\S*@\S*\.)/i));
if (ccLines.length > 0) {
this.block([
"Contains Cc: lines (which need to be moved to the PR description):",
...ccLines
].join("\n"));
}
};It would be quite easy to verify that it works by adding a unit test similar to the one testing a commit message that misses the Signed-off-by: trailer.
(see commit log message)