code review workflow

[Dan Kortschak]

After suggestions by Vladimír regarding approaches to code review and
git history, I have been trying out a code review workflow that works
more like the rietveld/gerrit approach in its impact on the project
history. It turns out this approach is already documented, e.g. [1].

How do people feel about this approach and, regardless of the approach
taken, formalising the code review/contribution process in a document?

Dan

[1]https://github.com/cockroachdb/cockroach/blob/master/CONTRIBUTING.md#code-review-workflow

[Ethan Burns]

I like the strategy outlined in the linked doc. It's the approach that I try to use on my github projects if I'm lucky enough to have a friend to review them.


Ethan

[Sebastien Binet]

hi,

On Sun, Jan 18, 2015 at 11:00 PM, Dan Kortschak
<dan.ko...@adelaide.edu.au> wrote:
> After suggestions by Vladimír regarding approaches to code review and
> git history, I have been trying out a code review workflow that works
> more like the rietveld/gerrit approach in its impact on the project
> history. It turns out this approach is already documented, e.g. [1].
>
> How do people feel about this approach and, regardless of the approach
> taken, formalising the code review/contribution process in a document?

I am fine with it.
I would even go one step further and use a git-merge with fast-forward
to have a more linear history.
requiring to squash the commits of the feature branch before the merge
and then merging with --no-ff presents us with a tree with "little
bubbles" of commits with much noise:
* | | | | 32e5070 Merge pull request #78 from sbinet/io-support
|\ \ \ \ \
| |_|/ / /
|/| | | |
| * | | | 16f372e (origin/io-support) mat64: implement Binary(Un)Marshaler
* | | | | e777627 Merge pull request #87 from gonum/errors
|\ \ \ \ \
| * | | | | f95ec98 Fixup error docs and drop Must
|/ / / / /
* | | | | 373197b Merge pull request #85 from gonum/de-zero
|\ \ \ \ \
| * | | | | b520df3 Remove receiver zeroing in Pow test
|/ / / / /
* | | | | 41e0763 Merge pull request #86 from gonum/usezeroed

(with a merge commit and a squashed-commit, you get 50% of
overhead/noise in the commit history)

I am not sure this can be achieved with vanilla github... (that's
probably why go went with gerrit+git-review)

-s

[Brendan Tracey]

I’m in favor of going closer to the go team approach. Is the "git rebase -i HEAD~n” step necessary, or can one just use “git rebase -i origin/master”?

> --
> You received this message because you are subscribed to the Google Groups "gonum-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to gonum-dev+...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Ethan Burns]

I am OK with merge bubbles. If you are careful about rebasing before merging then they don't create a non-linear, crisscrossing history.  They also serve to document who clicked merge. And, I occasionally find that the "empty" merge commits serve as a convenient place for Fixes #NNN comments. Consider when a branch containing multiple commits serve to fix the issue, not any single commit itself; the merge commit is a perfect place to document the fix.

Ethan

[Ethan Burns]

To be clear, I only recommend multi-commit PRs and merge bubbles if committers use git rebase -i to cleanup their branches. If branches aren't clean, isolated, logical steps then it just creates a mess. In that case, I prefer Sebastian's recommendation: just squash everything down to one commit and fast-forward.

[Dan Kortschak]

This is the kind of thing I was considering. Initially the empty commits troubled me, but they are the links to the code review discussion and show who was the commiter.

Aside, Is there a way to specify github does fast forward merges on pull request merges or would that require merging on a local machine and then pushing to github? I haven't found this.

[Vladimír Chalupecký]

I agree with Ethan's comment, they seem like a good guideline. It is also the approach that I am silently using in gonum/optimize, although I tend to avoid the merge commits. I also highly recommend setting the default push strategy to simple, it is not mentioned in the above link.

It is possible to get to the code review discussion for commits even from squashed and rebased branches through the GitHub web interface: after clicking on a particular commit, the link #XX to the PR discussion is just beneath the title.

It seems to me that fast forward merges require merging on a local machine.

And finally, yes, it would be nice to have the process formalised in a document.

Vladimir

[Sebastien Binet]

On Mon, Jan 19, 2015 at 7:42 PM, Ethan Burns <burns...@gmail.com> wrote:
> I am OK with merge bubbles. If you are careful about rebasing before merging
> then they don't create a non-linear, crisscrossing history. They also serve
> to document who clicked merge. And, I occasionally find that the "empty"
> merge commits serve as a convenient place for Fixes #NNN comments. Consider
> when a branch containing multiple commits serve to fix the issue, not any
> single commit itself; the merge commit is a perfect place to document the
> fix.

to be clear: I am not against the merge bubbles, but I feel like they
are a bit superfluous when you squash the commits of a dev-branch to
one single commit. :)

in anycase, I think it'd be wise to always rebase before merging a PR
(and yes, or rather no, I don't know whether that's possible using
github's web interface. I surmise it isn't.)

-s

[Dan Kortschak]

On 20/01/2015, at 7:09 PM, "Sebastien Binet" <seb....@gmail.com> wrote:

> to be clear: I am not against the merge bubbles, but I feel like they
> are a bit superfluous when you squash the commits of a dev-branch to
> one single commit. :)

With the caveat that without them we don't get to keep a link to the code review discussion. The gerrit approach is possible; amend the commit message with a link to the code review, but this seems onerous unless someone want to write a git extension for it.

> (and yes, or rather no, I don't know whether that's possible using
> github's web interface. I surmise it isn't.)

I spent some time looking around and I don't think it is possible.

[Vladimír Chalupecký]

> to be clear: I am not against the merge bubbles, but I feel like they
> are a bit superfluous when you squash the commits of a dev-branch to
> one single commit. :)

With the caveat that without them we don't get to keep a link to the code review discussion.

 

I think that github keeps that link for us, see my above post (which does not mean that it wouldn't be nice to have it accessible also from the command line in git log). I think that merge bubbles are ok if the commit is non-trivial, but in that case it should perhaps be a series of smaller commits, just like Ethan wrote.

[Dan Kortschak]

To clarify, I mean a link from a commit in the git history to the code review discussion. I can't see how github would insert that into the history without doing what it does (the alternative that I suggested would break peoples' repos if it were done automatically). I have found links from history to code review discussion (not just laundry lists of code review discussion which will always be kept in the issues list) are very useful when dealing with the standard library and would like to keep that here. This can be done by amending the commit to refer to the PR issue, but it will be forgotten/omitted sometimes without a doubt.

[Vladimír Chalupecký]

To clarify, I mean a link from a commit in the git history to the code review discussion. I can't see how github would insert that into the history without doing what it does (the alternative that I suggested would break peoples' repos if it were done automatically). I have found links from history to code review discussion (not just laundry lists of code review discussion which will always be kept in the issues list) are very useful when dealing with the standard library and would like to keep that here. This can be done by amending the commit to refer to the PR issue, but it will be forgotten/omitted sometimes without a doubt.

I understand and agree that it can be nice and useful to have such a link in git history. I am just trying to say a tiny comment that even if there is no such a link in 'git log', then it is no big deal. Github does not insert it into the history, but the code review discussion is accessible anyway through a link at the commit's web page (https://github.com/gonum/<package>/commit/<hash>). So apart from the obvious, not so useful route issue/PR -> commit, there is also the useful route commit->PR (even without a link in the git history). 

[Dan Kortschak]

On Tue, 2015-01-20 at 17:17 -0800, Vladimír Chalupecký wrote:
> Github does not insert it into the history, but the code review
> discussion is accessible anyway through a link at the commit's web
> page (https://github.com/gonum/<package>/commit/<hash>). So apart from
> the obvious, not so useful route issue/PR -> commit, there is also the
> useful route commit->PR (even without a link in the git history).

I didn't realise that was there. That's good enough for me.

[Jonathan Lawlor]

I'm fine following any procedure that you agree on, but when there is agreement, would one of you mind adding a document to one of the repos which explains the exact steps?