Pain points in PRs [was: Re: RFC: Moving git-gui development to GitHub]

["SZEDER Gábor" <szeder.dev@gmail.com>]

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@gmail.com> 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@google.com/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