Re: RFC: Moving git-gui development to GitHub

[Elijah Newren <newren@gmail.com>]

On Wed, Nov 20, 2019 at 4:20 AM Birger Skogeng Pedersen
<birger.sp@gmail.com> wrote:
>
> Hei Elijah,
>
> 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...


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