While reading the article I got a weird uncomfortable feeling about putting all this information in the PR. After sitting with that for a while I realized what it was.
This is very valuable information, putting it all in the PR means that it's lost when you want to take your project to a different git server. I'm not surprised that this is not a consideration coming from a microsoft blog post, but as a FOSS developer this is a terrible idea.
My suggestion would be to treat PRs as bureaucracy. Keep the discussion scoped to the process, e.g. "this is what you need to do for this change to be accepted". The information that the author suggests seems to belong, imo, to the ticket. That's "the business side". Then there's the code, and PRs are the bridge between the two.
I don't know, that's how I see it, but to be fair I've worked in all kinds of environments where they worked the way the author describes too. So at the end of the day, if it works for you... :D
it's lost when you want to take your project to a different git server.
it's lost way earlier - once it's merged, finding relevant MR/PR can be PITA and it's not in the top3 places I'd search for it (code, docs, commits)
especially if there was some repo-wide refactoring and it changed places (but it's not as big concern, as code comments also get detached from relevant lines sometimes)
Good point. In fact the best way to find a PR is to go to the ticket and find all the associated PRs (assuming you have that kind of automation; if you don't, you should). Again, PRs are a form you need to fill to get your change merged, they are bureaucracy (and I mean this in a positive way, the right amount of bureaucracy is good).
I think you are assuming that the 'PR' message is distinct from the commit message.
Why "commit" in singular? What happens with the description if you have more than one? The author makes no mention of commits in their article, I see no evidence to your point.
What happens with the description if you have more than one?
Each commit message describes a change. If you need to read more than one commit message to understand what a change is, then that's a problem.
The author makes no mention of commits in their article, I see no evidence to your point.
The distinction he's making is between the messages that live in the code and the messages that describe changes.
I don't see how you would read these as anything other than commit messages. What system are you using where the commit messages are not the messages that you read when reviewing a PR?
In all of my working career, it has been a struggle to convince people to write meaningful messages directly into the commits themselves rather than as the PR description. The companies I've worked at have PR description templates that people fill out, neglecting more granular information in the commit messages themselves.
They've all used GitHub. Even with the outages recently, my current company talked about moving to a different code forge and decided we can't justify the effort at the moment.
The author makes no mention of commits in their article, I see no evidence to your point.
The distinction he's making is between the messages that live in the code and the messages that describe changes.
I don't see how you would read these as anything other than commit messages.
Agreed that my interpretation also was "commit message vs code comment", weirdly precisely because commit messages were not mentioned, so "surely" for the author PR descriptions & commit messages are equivalent. E.g. AFAIR that's how things work in Gerrit: each commit gets its own review, so the "PR description" is just the commit message?
But I could see an interpretation where the author's preferred tool for researching a branch's history is the forge's PR list, instead of the git log, so they mostly never drop down to individual commit messages?
What system are you using where the commit messages are not the messages that you read when reviewing a PR?
Offhand:
In projects exchanging patches with git send-email, isn't it sometimes the case that maintainers apply series without merging it, just literally applying the series on trunk & pushing? In which case the "cover letter" ([PATCH 0/n]) never gets checked in?
In GitLab at least, the merge commit's message (if the project opts in to a merge commit) can be configured to contain a bunch of things, and by default this does not include the MR description, which the review UI emphasizes more than individual commit messages.
Now having said that, for the purposes of "looking over all of the PRs that went into the branch recently" while "chasing down a regression", I'd definitely be looking at git logs (vs mailing list threads or forge PR lists; might eventually fetch the discussion for a PR, but I wouldn't start there), hence my assumption that the author sees "PR description" as equivalent to "commit message".
Our repo is configured to use the PR description as the squash-merge commit message. So that's what people see from "git log" on main. Nobody really pays attention to the commits on the PR branch.
That's how we do it. It means main consists of 1 PR = 1 commit, and the commit message is the PR description + the concatenation of all the PRs squashed commit messages.
Often the PR description/title and commit message are the same thing (first line of commit message is PR title, rest are description). I've never worked at MS but the complete lack of discussion of commit messages leads me to believe they do it that way too. Otherwise this is a really weird post.
While reading the article I got a weird uncomfortable feeling about putting all this information in the PR. After sitting with that for a while I realized what it was.
This is very valuable information, putting it all in the PR means that it's lost when you want to take your project to a different git server. I'm not surprised that this is not a consideration coming from a microsoft blog post, but as a FOSS developer this is a terrible idea.
My suggestion would be to treat PRs as bureaucracy. Keep the discussion scoped to the process, e.g. "this is what you need to do for this change to be accepted". The information that the author suggests seems to belong, imo, to the ticket. That's "the business side". Then there's the code, and PRs are the bridge between the two.
I don't know, that's how I see it, but to be fair I've worked in all kinds of environments where they worked the way the author describes too. So at the end of the day, if it works for you... :D
I read the post waiting for him to get to the part where he actually mentions commit messages and was baffled when it never happened. What on earth.
I’m guessing they do squash commits, so the PR title and description become the title and description of the commit that lands on the main branch.
it's lost way earlier - once it's merged, finding relevant MR/PR can be PITA and it's not in the top3 places I'd search for it (code, docs, commits)
especially if there was some repo-wide refactoring and it changed places (but it's not as big concern, as code comments also get detached from relevant lines sometimes)
Good point. In fact the best way to find a PR is to go to the ticket and find all the associated PRs (assuming you have that kind of automation; if you don't, you should). Again, PRs are a form you need to fill to get your change merged, they are bureaucracy (and I mean this in a positive way, the right amount of bureaucracy is good).
I think you are assuming that the 'PR' message is distinct from the commit message. I don't think that was the expectation here.
Why "commit" in singular? What happens with the description if you have more than one? The author makes no mention of commits in their article, I see no evidence to your point.
Because that's the atomic unit.
Each commit message describes a change. If you need to read more than one commit message to understand what a change is, then that's a problem.
The distinction he's making is between the messages that live in the code and the messages that describe changes.
I don't see how you would read these as anything other than commit messages. What system are you using where the commit messages are not the messages that you read when reviewing a PR?
In all of my working career, it has been a struggle to convince people to write meaningful messages directly into the commits themselves rather than as the PR description. The companies I've worked at have PR description templates that people fill out, neglecting more granular information in the commit messages themselves.
What tooling do they use? This sounds like something that can happen only if the workflow is specifically tied to a single code forge (e.g. GitHub).
While Raymond Chen has been at Microsoft, they’ve gone through at least four code forge changes and three revision-control systems (possibly more).
They've all used GitHub. Even with the outages recently, my current company talked about moving to a different code forge and decided we can't justify the effort at the moment.
Agreed that my interpretation also was "commit message vs code comment", weirdly precisely because commit messages were not mentioned, so "surely" for the author PR descriptions & commit messages are equivalent. E.g. AFAIR that's how things work in Gerrit: each commit gets its own review, so the "PR description" is just the commit message?
But I could see an interpretation where the author's preferred tool for researching a branch's history is the forge's PR list, instead of the git log, so they mostly never drop down to individual commit messages?
Offhand:
In projects exchanging patches with
git send-email, isn't it sometimes the case that maintainers apply series without merging it, just literally applying the series on trunk & pushing? In which case the "cover letter" ([PATCH 0/n]) never gets checked in?In GitLab at least, the merge commit's message (if the project opts in to a merge commit) can be configured to contain a bunch of things, and by default this does not include the MR description, which the review UI emphasizes more than individual commit messages.
Now having said that, for the purposes of "looking over all of the PRs that went into the branch recently" while "chasing down a regression", I'd definitely be looking at git logs (vs mailing list threads or forge PR lists; might eventually fetch the discussion for a PR, but I wouldn't start there), hence my assumption that the author sees "PR description" as equivalent to "commit message".
Merge commit also has a message
Our repo is configured to use the PR description as the squash-merge commit message. So that's what people see from "git log" on main. Nobody really pays attention to the commits on the PR branch.
Is that an atypical setup?
I ban squash merge in general but either way you can get the PR info on the commit message
That's how we do it. It means
mainconsists of 1 PR = 1 commit, and the commit message is the PR description + the concatenation of all the PRs squashed commit messages.My merge commit messages are usually perfunctory.
I should do better.
Often the PR description/title and commit message are the same thing (first line of commit message is PR title, rest are description). I've never worked at MS but the complete lack of discussion of commit messages leads me to believe they do it that way too. Otherwise this is a really weird post.