On Wed, Nov 20, 2019 at 4:20 AM Birger Skogeng Pedersen <birger.sp@xxxxxxxxx> wrote: > > Hei Elijah, > > On Wed, Oct 30, 2019 at 7:21 AM Elijah Newren <newren@xxxxxxxxx> wrote: > > Projects which switch to GitHub tend to have overall commit quality go > > down IMO, because the system (a) makes it nearly impossible to review > > commit messages, so people eventually degrade to writing really bad > > ones, > What do you mean here, exactly? In what way is it "nearly impossible" > to review commit messages in GH? My lengthy rant wasn't good enough for you? ;-) Well, I'll try even harder to bore everyone to death, then and extend my rant a bit... Reviewing is the process of providing feedback on proposed changes. Code review tools and mechanisms typically provide ways to (a) see proposed changes in isolation and (b) comment on individual lines and preserve context (with the goal of later merging a group of commits that implement something useful). git-format-patch and git-send-email combined with usage of email clients that know how to quote previous emails and let you respond inline are a natural way of achieving both (a) and (b). GUI tools can, of course, also achieve something similar by showing proposed changes and allowing commenting on individual lines in context. GitHub fails pretty hard on both counts, particularly for commit messages. It guides people to an overall diff rather than to the diffs inside individual commits and completely omits all commit messages when you do so. It does provide a way to access individual commits and their diffs, though it makes it somewhat unnatural. (It at least isn't as awful as it used to be in years past, when any comments on individual commits were completely lost and separated from the PR.) And even if you do "go against the grain" to comment on individual commits, there is no provided mechanism for commenting on the commit message itself. Instead it has to be given as a general comment on the overall set of changes, which then loses the context of what you are commenting on unless you re-include and quote it yourself. That usually doesn't happen, so when you comment on four commit messages in a review, you have four separate global comments and someone attempting to respond to them doesn't get to see the commit messages next to them, resulting in confusion. Even if you do re-include and quote the commit message bits you are commenting on, the resulting comment isn't in any way tied to the commit in question in the UI. So people who use GitHub for code review just don't bother. They write non-isolated commits and far from rarely use awful commit messages. Then they merge this abomination of history, or possibly even worse, they squash merge it all to make it impossible for any future readers to be able to dissect. Yeah, yeah, small features so that the review is smaller and easier. That is important, yes, but it still conflates two things and thus ruins reviews. Each PR should implement something useful. Commits should be designed both for current and future reviewers to see a clear path towards how that useful thing was implemented. Sometimes one commit is enough, but conflating the two necessarily either means sometimes creating one-commit PRs that don't actually implement anything useful, or a cognitive overload for code reviewers. GitHub simultaneously encourages bad behavior (bad commit messages since they are designed to not be reviewable, non-isolated commits, fixup commits that are never properly squashed, etc.) and penalizes good behavior (folks who try to clean up their sequence of commits are met with problems ranging from GitHub screwing up the topological ordering of a linear commit history, to poor ability to see incremental changes whenever rebasing happens, to reckless squash merging of all their careful work into a single commit as something close to an act of war against any future readers who want to dig into why certain changes were made). Yes, GitHub has gotten much better at code reviews; it's merely abysmally awful these days as opposed to a complete joke as it was in years past. But it's still so bad that I have seen people try to solve this by having a sequence of PRs per (small) feature they want to implement, even though GitHub provides no way to denote dependencies or ordering between PRs. You may think I've gone off on a bunch of tangents, but fundamentally I believe that almost all of these other problems predominantly arise as secondary and tertiary effects of not understanding that commit messages should be a first class citizen of code review. Sure, you can claim all you want that it is entirely possible to review commit messages within the GitHub UI and it's just extremely inconvenient, yadda, yadda, but the truth of the matter is that people everywhere struggle to even do code reviews at all, and when they do they all too often turn into rubberstamp exercises or don't delve deeply enough. In that context, I believe my "nearly impossible" wording is entirely warranted. Using a tool that simultaneously encourages bad behavior and penalizes good behavior will not so surprisingly yield bad behavior. GitHub PRs are such a tool, IMO. (To be fair, I'll note that GitHub has awesome code browsing, really easy setup and administration of new repositories and organizations, simple and reasonable and thus pretty nice code search, etc., etc. I'm not saying GitHub is a bad tool, I actually think most of it is a very excellent tool; I am just claiming that the PR section of it is very bad.) Elijah
