
About pull request reviews - lelf
https://help.github.com/articles/about-pull-request-reviews/
======
web007
I can create a PR, get it reviewed / approved, then change 100% of the
underlying code and still merge it.

Having required reviews is a step in the right direction, but not quite good
enough to work as a proper code review / signoff for strict compliance
requirements.

~~~
TheAceOfHearts
Could you expand on your use-case? I'm unfamiliar with this kind of scenario
and I'd like to learn more about your constraints and their reason.

Maybe I'm being completely naïve here, but it would seem like anyone with very
strict compliance requirements wouldn't be using GitHub in the first place. Is
this line of reasoning wrong?

At least for my small (5-ish people) team, it's been great. It makes it much
easier to communicate the current status. But since we're such a small team,
we largely just trust everyone to be careful. We generally don't commit
directly to master and all PRs are reviewed. But if there's a good reason to
commit directly or immediately merge a PR without review, we're fine with
that. We'll usually just notify the team on Slack and they'll find out in the
morning. For example: we get a bug report and there's a quick fix, but nobody
is around to review it. The developer is encouraged to merge, confirm it's
working on staging, and release it.

Being able to release multiple times per day might not seem like a big deal,
but it means you start releasing smaller changes with increased frequency, so
even if something goes wrong, you can easily revert and you know exactly what
to investigate.

~~~
web007
For ISO / PCI / SOX / HIPAA / contractual compliance in any "real" company
there is a requirement of a review on all code shipped to production. "Real"
because it's a huge pain in the ass, and you only do it if you have to work
with another such company or if you're constrained by regulation to do so.

It doesn't matter the number of commiters, size of changes or frequency of
release. You're bound to a process for reasons outside your control.

That's the strict version that I'd like them to implement, and what I thought
we got out of enabling required reviews before reading in more detail. Even
for one of their use-cases as described their flow won't work: allowing review
of stylistic change to match project requirements. As it stands you can
approve a change, then someone can screw with tabs-vs-spaces or reformat and
merge on their own. Even rebasing to squash or reorganize doesn't require a
subsequent approval. As suggested elsewhere, you'd have to automate the check
(good idea anyway) and force it as a required test vs using approvals anyway.

Two things would improve the process. One would be implementing the strict
mode, where merges can only happen after an approved review and any new commit
(or even rebase) would trigger a reset. The second option would be to force
two-man rule, where the committer can't merge their own PR. That fixes the
compliance piece, and that's probably technically easier but logistically more
complicated; person A can open a PR, then person B can approve AND make
changes on the branch and still submit. That second flow is probably fine for
OSS stuff, but is already covered by public repos and only allowing owners to
merge. For a team it's kind of half-and-half, it depends on your requirements
if it will pass compliance muster.

------
dcousens
Honestly, just being able to "tick" each commit would be great. On
[https://github.com/bitcoin/bitcoin/](https://github.com/bitcoin/bitcoin/) it
is common practice to include the commit hash in your approval comment so you
can easily review where you were up to if a rebase occurs.

~~~
rcaught
A rebase would change all commit hashes, all the way to the base branch. Your
commit hash in the comment would not be relevant.

~~~
couchand
Git UIs should expose the tree hash as well to mitigate that issue.

~~~
cyphar
That wouldn't help. If you needed to rebase (there was a conflict) then the
tree hash will necessarily be different. In fact, unless you are rebasing on a
commit and it's reversion then you'll probably get a different tree hash.

~~~
couchand
Indeed. What I'm looking for isn't the tree hash it's the hash of the patch
itself. I don't think that's explicit anywhere.

~~~
cyphar
Patches in git are generated on-the-fly. This is actually one of the critical
things that made git so much better than conventional version control systems
at the time -- it doesn't need to track diffs (which means it doesn't have to
handle the infinite fractal of problems that spiral out of rebasing diffs). It
can just compute them when it needs them.

Think of git like a snapshotting filesystem that has a VC interface added on
top.

~~~
couchand
Where does that leave rebasing, then? That's explicitly an operation on diffs:
take these diffs and apply them to that.

~~~
cyphar
Rebasing is implemented by generating diffs, yes. But git doesn't actually
store these diffs -- they're all generated on the fly from tree snapshots. If
your VC stores diffs rather than the actual contents of files then you can't
be sure that you'll get out exactly the same data as you put into it. There
are also a bunch of other problems that happen as a result.

------
Pirate-of-SV
An option of having a variable number of required approvals would be nice.

Or something to enforce: "One person creates pull request, another reviews, a
third merges."

------
tln
I hope they keep iterating on the UI.

* can't initiate a review from any tab * can't quickly give an official approval * have to decide whether comments are in a review or not

I'd like to comment on the commit and commit messages directly (eg, "Please
rebase out the WIP comments here")

There's lots of friction when looking anywhere ELSE in the code during a code
review. I'd like to be able to make a comment on eg, a line of code that
needed to be changed in a file not in the review.

------
bpicolo
Github PRs are getting much better, and that's good!

One thing missing for me now is an interdiff feature. E.g. show me the diff
minus <some initial commit that's a big vendor commit>.

@<username> is a pretty common pattern for mentioning people for reviews right
now. Any chance we get a text shortcut for adding reviewers? Not strictly
necessary but a nice-to-have

------
grrowl
It's a great feature, although the UX for packaging several comments into one
or multiple reviews, each with their own status/requirement feels a little
over the top. Even later, it's comments interspersed through the conversation,
instead of being able to view and take action on a single review as a unit.

------
nojvek
Github needs to get some ideas from codeflow. It would be great to be able to
see iterations of review and the differences between iterations.

Being able to see reviewer status. Signed off, awaiting changes, reviewing,
waiting for review.

------
tveita
You can request a review from someone, but I can't find any way to list pull
requests that are awaiting a review from you.

They don't even show up with an involves: filter

~~~
jtietema
so true, it lacks an inbox feature like Bitbucket Server (Atlassian Stash)
has. On top of that:

\- keeping track of changes over the life time of a PR is impossible, you have
review the whole thing over and over again \- toggle notes (comments) on a per
file basis is silly and unpractical on PR's with lots of files and comments.
\- there is no way to see which PR from a list you already approved.

And the list goes on and on. I have been using Github Enterprise at my new job
since recently and it is by far the poorest of tools I have ever used to
review code.

