On Wed, Nov 20, 2019 at 09:13:04AM -0800, Elijah Newren wrote: > > 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... Thank you very much for taking the time and effort to write it up. It summarized some of my main gripes with PR-based collaboration on GitHub with such clarity that I would never been able to achive. (The recent "Pain points in Git's patch flow" thread reminded me that I saved this message and even marked it as important ages ago... but haven't gotten around to read it until now. https://public-inbox.org/git/YHaIBvl6Mf7ztJB3@xxxxxxxxxx/T/ ) > 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
