
More code review tools - Oompa
https://github.com/blog/2123-more-code-review-tools
======
js2
It would be nice if GitHub supported a Gerrit-inspired code-review process,
where instead of having to choose between:

1) piling new commits onto the existing branch/PR, or

2) force-pushing and completely losing the old commits on the server

You could instead push into a "magic" ref-spec and the server would retain the
original commits, but also the rewritten commits, such that a PR can have
multiple revisions. This is somewhat hard to explain unless you're familiar
with Gerrit and its "patch set" workflow.

Why? Because my preference is to rewrite history in order to address code
review comments, such that what is finally merged leaves a clean history. But
it is also valuable during the code review process to be able to look across
the revisions to make sure everything has been properly addressed.

The only way to do both, today, is to create a new branch and PR for each
"revision". i.e. new-feature, new-feature-rev1, new-feature-rev2. Then close
the original PR and reference it from the new PR. A bit tedious.

~~~
joshpeek
Hey, I'm one of the devs for this new PR stuff. This flow is something we're
hoping to improve as well. Force push support is pretty second class right
now, any previous commits and discussion basically gets lost. It sucks.

So we already use a internal refspec for tracking the latest PR HEAD. You can
actually manually fetch this under `refs/pull/123/head`. However, this is only
the latest HEAD, not any previous histories.

My idea was to expand this to include tracking all historical HEAD refs. We
came up with a clever/hacky idea to string together a chain of dummy commits
pointing at all the historical HEADs. This fake commit chain would only be
used internally, but it would ensure git's gc reachability checks don't sweep
up any old PR HEADs. Its an alternative to creating a new ref for each
previous HEAD or maintaining text reflogs for an entire repo network. Both
have some performance issues for larger repos.

Its something we're still working on, but it still helps to vocalize your
support for wanting improved force push support. :D

~~~
gmartres
> Its something we're still working on, but it still helps to vocalize your
> support for wanting improved force push support. :D

YES! I wish for that feature everyday. Gerrit does it exactly right.

Related to this, I really, really hope that you will consider displaying
commits in a PR in the correct order, suppose that I have three commits on top
of master:

    
    
      a->b->c->master

If I git rebase -i master and reorder my commits so that they look like this:

    
    
      c->a->b->master

Then the commit list in the PR will still be displayed in chronological order
instead of DAG order, this is very confusing! The only workaround I've found
is to amend every commit in my PR so that the timestamp order match the DAG
order, this isn't very fun to do.

By the way, the post explicitly says:

> Some teams choose to use a commit-by-commit workflow where each commit is
> treated as the unit of change and is isolated for review.

I'm glad that you're acknowledging that. But then why do comments on commits
still get hidden when they correspond to an outdated diff? I would love to be
able to comment on individual commits, but if half my comments are hidden
because they correspond to lines that don't match the current diff, then I'm
not going to do it for fear of my comments being missed. You should not hide
important information that I'm trying to communicate! Here's a better way to
do it: Add a "Done" button like Rietveld/Gerrit for every comment (this is a
very useful feature on its own) and hide comments _if and only if_ the "Done"
button has been clicked.

Anyway, I'm glad that people are working on this stuff and that they're
listening to the community, thank you!

~~~
cdcarter
Seriously -- disappearing comments is very sad.

~~~
scoot
Disappearing comments mean they've been adressed. And they don't actually
disappear, they're just minimised.

~~~
Elv13
No, if you comment on the commits themselves rather than the PR 3rd tabs, they
disappeared (at least before this change). The only place they were left is
your emails, an hardcoded URL or your feed. This was (is?) bad...

------
willchen
Does anybody else feel like GitHub has released more features in the last
month than the last 6 months? I'm not sure if it's just a coincidence with all
the attention they've gotten on HN, but these improvements are much
appreciated!

~~~
smaili
Definitely not a coincidence. Although it certainly begs the question of why
this wasn't done earlier given the sheer amount of capital they have.

~~~
grinich
They're focused on turning that capital into recurring revenue! i.e. building
out the GitHub Enterprise business

This is pretty common among companies that make the consumer->enterprise
transition. Also see Dropbox, Slack, etc.

~~~
jasonlotito
> building out the GitHub Enterprise business

None of what they just released impacts the GH Enterprise side of things.
Maybe it will long down the line, but suffice it to say, none of this sells or
keeps customers of GH Enterprise. The reality is, we've questioned our use of
GH, and while we are still paying for GHE, it's more or less because it's just
not expensive enough at this point to justify switching to something else.
We'd have to update some tooling, and migrating would require time.

Basically, GHE is a glorified code repository viewer, and even then, it does a
substandard job of that. Seriously, the ability to browse and search painful.

So yeah, if they are focused on GHE, these features don't suggest that.

~~~
sdesol
> the ability to browse and search painful

Hey just an FYI, we (my company, not GitHub) are working on improving the
browsing/searching situation for GitHub and GitHub Enterprise. You can see a
list of the improvements at:

[http://gitsense.github.io/github+gitsense.html](http://gitsense.github.io/github+gitsense.html)

We've only really started advertising in the last 3 months, but what has been
quite clear is, people really overestimate what GitHub and their API's are
capable of. This is not a criticism against GitHub, but more of a high praise
for how well they have cultivated the GitHub brand.

People really think GitHub not implementing commits search, or not being able
to search forked repositories (which is pretty bad from an Enterprise point of
view), is because it's not sexy, which is why they haven't implemented it yet.
And when they see what we are capable of doing with GitHub, they just assume
we are using GitHub's API, when in reality, the only time we use GitHub's API
is to get somebody's avatar.

We obviously can't complain though, as it provides us with a business
opportunity.

------
numbsafari
I'd love it if there was a way for me to queue my comments before submitting.
I often keep my comments in a separate textedit/nv window and then go back to
put them in since I want to keep track of questions that arise as I'm reading,
but I don't want to pepper someone with comments that would be resolved 200
lines later in the diff.

~~~
vikiomega9
Are 200 line diff common? I feel like having focused patches are useful for
everyone. Including git bisect and CI/CD.

~~~
oinksoft
The line count of a diff has nothing to do with how focused the patch is. Some
PRs are simply larger than others due to the language in use or the nature of
the code being changed.

Presumably a codebase in an array-oriented language would have miniscule PRs,
which is irrelevant (discussion of PL merits aside).

------
danpalmer
This is a great step in the right direction. I use GitHub for code review
every day, and it has historically been very poorly designed for thorough
reviews. These changes look great, I just hope that we get some sort of
checking-off of review points, and accept/reject functionality.

~~~
erichurkman
I'd love a UI way to enable `?w=1` whitespace mode, especially for
html/json/yaml files. I have a feeling a lot of reviewers don't know about it
and spend an inordinate amount of time squinting to see what changes when it
was a whitespace-only change.

~~~
tyingq
This! ?w=1 has existed for years, with no UI way to access it.

Compare two views of the same commit (found this by searching for any commit
on github with the word "indent" in it)

[https://github.com/douglascrockford/JSON-
js/commit/1e3869cb3...](https://github.com/douglascrockford/JSON-
js/commit/1e3869cb398ddf58d3d52efd735067093dc5bf3e) (133 additions and 127
deletions)

[https://github.com/douglascrockford/JSON-
js/commit/1e3869cb3...](https://github.com/douglascrockford/JSON-
js/commit/1e3869cb398ddf58d3d52efd735067093dc5bf3e?w=1) (30 additions and 24
deletions)

------
willchen
I've noticed some open source projects (particularly Angular 2) follow this
convention where they never actually "merge" the PR, but rather rebase it into
their main branch and have a message in the commit to close the PR.

The advantages seem to be that you get a cleaner git history, and you can keep
all the "in-between" / WIP commits that tell the story. Is this done through
an automated tooling or is someone manually rebasing it into master? It seems
to be a really useful practice so I'm curious how people do it. It would be
great if GitHub could offer this natively, as I think many power Git users
appreciate the benefits of rebasing over merging.

------
guelo
Sometimes when I submit a PR and I get a lot of feedback I get lost making
sure I've addressed every comment. My #1 feature request would be being able
to mark comment threads as resolved. The outdated comment feature sometimes
works for this use case but mostly it doesn't.

~~~
infogulch
I didn't see it mentioned but it looks like you can now 'react' to comments, a
la facebook. So you could choose one reaction, say, "Horray!", and use it as a
marker that you resolved it.

Now I wonder if the commenter is notified on every reaction. The only thing
that might make this more difficult.

~~~
positr0n
That would also be missing a filter to show all unresolved comments, which is
a huge use case. (Or maybe you can do that, if you can it is not very quick
and intuitive).

------
alexwebb2
Did they kill commit-level comments?

I can no longer make comments on a given commit - the entry box at the bottom
is gone. Looks like you have to scroll all the way back up to the top, switch
to the "Conversation" tab, and then make a PR-level comment instead of a
commit-level comment.

I hope I'm missing something here, because as it stands now it's a big step
backwards.

~~~
nightpool
They removed full-commit comments on PRs, as opposed to line-level comments on
either commits or diffs, or full-commit comments on non-PR commits, but I
don't think anyone used those—I haven't even seen anyone use something that
wasn't a line comment for a long, long time.

~~~
eridius
I've used commit-level comments before, but it is admittedly not very common.

~~~
majewsky
It is very useful in work cultures where people don't do PRs all the time
(particularly out of laziness or to do hotfixes), but you still want to
comment on the change.

~~~
alexwebb2
We do PRs for all changes, but strongly prefer to have comments about a
particular commit _within_ a PR be attached to the commit, and not just to the
PR.

With this change, if we have comments about a given commit in a PR, we will
now have to manually add a link to that commit in the comment so the developer
knows which commit the comment is referring to.

It wouldn't be as much of an issue if PR-level code review was easier. As it
stands now, the multi-commit view they just added (which is a step in the
right direction) is still useless to us because it includes merges commits as
well, which is _not that developer 's code_.

So PR-level code review is still a non-starter, and commit-level code review
just got clunkier.

I get that they can't support every possible workflow, but they could have at
least given us a warning they were going to pull support for a feature we use
(they do have those metrics) or at very least _acknowledge it somewhere_.

~~~
eridius
As a workaround you could just attach the comment to the very first or last
line in the commit diff. Of course, this won't work for commits that consist
entirely of non-diffable binary files, but it should work ok in most cases.

------
jakub_g
I'll plug my work as always when talking about the topic:

I wrote a small script for Chrome/Firefox that I found useful for reviewing
big PRs on GitHub. It gives you possibility to expand/collapse files, mark
files ok/fail in order come back to them later.

It works with mouse and keyboard (though I've noticed there are some issues
with tab order after GitHub upgraded their code, and small UI glitches, I'll
try to have a look at them soon)

It's a hack on top of GitHub so it needs maintenance every couple of months,
but overally it does its job well IMO.

I don't have much time to hack on it anymore, but community contribs are very
welcome. I wrote some potential ideas for improvements as GH issues in the
repo. AMA if you're interested.

[1] [https://github.com/jakub-g/gh-code-review-
assistant](https://github.com/jakub-g/gh-code-review-assistant)

------
eridius
These are some nice changes, though there's still plenty of things I want
GitHub to make better about code reviewing.

One of these changes, and one that annoys me every single day, is when I get
an email about a comment, the email doesn't include any context (e.g. it
should include the previous comments on that line and probably the hunk as
well). And when I click "View on GitHub" to see it in context, if the commit
is now on an outdated diff, I get taken to a page that _doesn 't show the
comment at all_. It takes me to the Files view, but the Files view doesn't
show outdated comments. If the comment is on an outdated diff then it really
should take me to the Conversation view with the comment in question expanded.

------
pizza
If next to every file, Github also listed its filesize, I would know exactly
what file to begin looking at when I encountered a new repo (to a good
approximation -- in any case, I would be able to intuit better decisions with
a combination of filenames, the information in hypothetical README.md's, and
filesizes than just READMEs and filenames alone..)

Maybe this doesn't scale or something? It's something I've felt necessary for
a while though. Maybe their user testing has concluded otherwise?

------
netcraft
I wish it were possible to put comments directly on a line of code in any
commit / repo outside of a PR. Sometimes someone wants me to review their code
that they aren't submitting anywhere.

~~~
timr
We've been doing exactly that at Omniref:
[https://www.omniref.com](https://www.omniref.com)

------
kdazzle
My biggest pet-peeve with GH code review is that line-comments are
automatically folded whenever that line of code is changed. So if the changes
to that line didn't relate to your CR or if they didn't actually fix anything,
then your comment will pretty much be lost to time.

Plus, it would be nice to see the discussion around a particular line without
having to go through the entire PR and unfolding each conversation to see if
it's the right one.

~~~
piotrkaminski
Then give Reviewable ([https://reviewable.io](https://reviewable.io)) a try.
:) Comments remain open until acknowledged / resolved and are automatically
displayed at the nearest applicable line in every diff.

Disclosure: I built the tool.

------
forgotpwtomain
I can't say I'm a fan. If I select a single commit, instead of giving me a
list with a single commit check-marked, it gives me only that single commit
and an option to 'show all commits'. So to change the commit you are viewing
requires clicking 'show all commits' (waiting for them to load) and then
selecting the other commit you want to view (which should just be a check
box).

Also it totally baffles me that these actions all require server-side
requests. It's really a lot easier to go to the old commits tab, and to ctrl
click an open tab for each commit then to use this new feature.

At one point Github offered the best and cleanest UI of all the alternatives -
but I doubt this will be the case for long at this rate.

------
hwangmoretime
Code review is something all of us do, but all of us do differently. Anyone
know of any nice frameworks, articles, or blog posts for code review? I'm
particularly interested in the case where knowledge transfer is a high
priority in the code review.

My current side project integrates feedback theory [1], to provide scaffolds
and other cues to help and remind reviewers to give high quality feedback.
Thus, my interest.

1:
[https://scholar.google.com/scholar?q=%22The+Effects+of+Feedb...](https://scholar.google.com/scholar?q=%22The+Effects+of+Feedback+Interventions+on+Performance%22&btnG=&hl=en&as_sdt=0%2C39&as_vis=1)

~~~
swanson
You might find this talk valuable -- it's my personal favorite:
[https://www.youtube.com/watch?v=PJjmw9TRB7s](https://www.youtube.com/watch?v=PJjmw9TRB7s)

------
Negative1
Nice additions. Can't help but feel at some point you'll be able to edit and
compile code directly on Github (with some sort of compiler/CL backend). Has
Github ever discussed integrating Atom directly into the site?

~~~
alxndr
There is a text editor; when viewing a Markdown file for example there's a
pencil icon in the top-right of the (rendered) file display.

------
xkarga00
Dear Github, can you please bring the search bar back (w/o the need to log
in)?

~~~
majewsky
+1 - I was so confused about this yesterday. For not just a moment, I thought
they had eliminated global search entirely.

------
stormbrew
Only related to review in a somewhat indirect way, but I really wish the
commit view on github (as well as other git tools) had an equivalent of
--left-only command line option to git-log. It's incredibly useful for viewing
a high level of the history of a repo that uses merge bubbles, and lack of
tooling around doing just that seems like that main reason people do things
like squash their branches before merging to master (which is where I think it
connects back to review).

------
artursapek
Awesome to see GitHub stepping to the plate with these updates. IMO the
biggest thing missing in the PR's diff view is a git blame column beside the
changes, so if someone is writing good commits I can glance through the entire
diff and see which changes were introduced together and why.

Just a feature request, and I know how annoying those can be on the receiving
end. Great updates either way.

------
known
Are there any
[https://en.wikipedia.org/wiki/Software_design_pattern](https://en.wikipedia.org/wiki/Software_design_pattern)
review tools?

------
pearlsteinj
I've been pleasantly surprised at how fast Github has been moving since that
critical open letter came out. For a giant company like Github they've been
releasing developer tools very quickly.

~~~
vdnkh
Makes you question what exactly they were doing before the letter came out.

~~~
softawre
Taking naps in big piles of cash? Heh.. Enterprise-only stuff?

~~~
asadlionpk
Yes. They were/are heavily focused on Enterprise features.

------
debacle
I'd like block-level comments and maybe even an in-commit commit function (the
ability to commit to a PR branch while in the PR). That's all I really care
about.

------
Jemaclus
I think the biggest thing I want is pagination for extremely large commits.
Any ideas if that's gonna happen?

~~~
lukevers
Me too. The feeling when you accidentally click on a very large diff and your
browser freezes for a few seconds is horrible.

------
fiatjaf
No matter how much they do, people will still complain.

