git/github commit hooks

[Sam Just <sj...@salesforce.com>]

Last thursday, we a had a short discussion about possibly changing the
merge process to allow unsquashed commits and the use of the github
merge button.  One sticking point is that we'd like an automatic way
to enforce some commit message metadata requirements and formatting.

Git lets you define some hooks for validating commits and commit
messages locally, see
https://git-scm.com/book/gr/v2/Customizing-Git-Git-Hooks.
Specifically, you can define a commit-msg hook which gets to validate
the file containing the commit message before allowing the commit.  I
think https://developer.github.com/webhooks/ can be leveraged to do
the same checks on a github PR prior to allowing the PR to be merged,
but haven't had time yet to figure out precisely how.
-Sam

Re: git/github commit hooks

[Enrico Olivelli <eo...@gmail.com>]

2017-10-11 18:58 GMT+02:00 Ivan Kelly <iv...@apache.org>:

> Responses inline.
>
> >> Last thursday, we a had a short discussion about possibly changing the
> >> merge process to allow unsquashed commits
> >>
> >
> > I don't think is the best way.
> > The history of a pull request may have several minor commits and IMO it
> is
> > better to hide such conversation in the official git history of the
> project
> > If we think that the resulting commit is too big maybe we should split
> the
> > patch into smaller parts.
> Sam isn't suggesting that we keep all the little updates in the commit
> log. Rather, he is suggesting that PRs get split into self contained.
>
> Large diffs are just bad.
>
> [1] gives a good summary of why, but I'll add that in a globally
> distributed open source project, the problem of large diffs being hard
> to review is magnified tenfold. We don't all live in the same part of
> the world, so the turn around between posting the PR, getting
> feedback, fixing feedback and getting more feedback is long. For the
> most part we don't work for the same organizations, so code reviews
> are often bumped for other tasks that have higher priority in our
> jobs. Again, this lengthens turn around time. And the longer turn
> around time is, the harder it is to retain any context on what you
> were reviewing.
>
> Swapping back into a context takes time and effort, leads to "review
> fatigue" and ultimately to reviewers just skimming the patch and
> giving it a LGTM, rather than doing a proper review. This is a real
> problem. I recall seeing a research paper from google(I think?), where
> they correlated diff size to bug generation rates (I can't find the
> reference now, but will keep looking).
>
> We need to reduce the amount of context that needs to be swapped back
> in when coming back to a review, and the largest part of this is
> reducing the size of patches.
>
> [2] has a good primer on how to keep PRs and patches small. If anyone
> wants help breaking up a patch before code review, I would be more
> than happy to lend a hand.
>
> [1] https://medium.com/@kurtisnusbaum/large-diffs-are-
> hurting-your-ability-to-ship-e0b2b41e8acf
> [2] https://graysonkoonce.com/stacked-pull-requests-keeping-
> github-diffs-small/



I really think that most of the problem is related to having huge diffs as
Ivan is claiming

I am really sorry that we broke GitHub with patch for BP-15, with toom any
commits and comments.
Maybe we used the PR dashboard is a bad way.
We did a discussion and we continued the conversation on the PR even if we
changed opinion several times.
In the original document for PB15 there is a small subset of the changes
that will be in with that patch.
That patch introduces a new framework for mocking BookKeeper client
artifacts for instance :-)
It could be done in another patch.


I will experiment with BP-14 for stacked PRs !

Enrico



>
>
> > | and the use of the github merge button.
> >
> > I don't like the GitHub button, it is easy to use it even with the
> > smartphone and it is very dangerous ! I would like to disable it at all.
> > I am actually running very well with the bk-merge-script as it does all
> the
> > validation we want.
> I'm neither yay nor nay on the button, but if we do require the merge
> script, we should disable the button completely.
>


+1 I would like to do this very soon


>
> > The checks from jenkins/travis/coveralls.io...could be flaky and it
> would
> > be very annoying to have to wait for a full green light for a pull
> request
> > to be merged
> This is a problem in itself, and needs to be addressed.
>

Yup, we are working on flaky tests and we are on the way


> -Ivan
>

Re: git/github commit hooks

[Ivan Kelly <iv...@apache.org>]

Responses inline.

>> Last thursday, we a had a short discussion about possibly changing the
>> merge process to allow unsquashed commits
>>
>
> I don't think is the best way.
> The history of a pull request may have several minor commits and IMO it is
> better to hide such conversation in the official git history of the project
> If we think that the resulting commit is too big maybe we should split the
> patch into smaller parts.
Sam isn't suggesting that we keep all the little updates in the commit
log. Rather, he is suggesting that PRs get split into self contained.

Large diffs are just bad.

[1] gives a good summary of why, but I'll add that in a globally
distributed open source project, the problem of large diffs being hard
to review is magnified tenfold. We don't all live in the same part of
the world, so the turn around between posting the PR, getting
feedback, fixing feedback and getting more feedback is long. For the
most part we don't work for the same organizations, so code reviews
are often bumped for other tasks that have higher priority in our
jobs. Again, this lengthens turn around time. And the longer turn
around time is, the harder it is to retain any context on what you
were reviewing.

Swapping back into a context takes time and effort, leads to "review
fatigue" and ultimately to reviewers just skimming the patch and
giving it a LGTM, rather than doing a proper review. This is a real
problem. I recall seeing a research paper from google(I think?), where
they correlated diff size to bug generation rates (I can't find the
reference now, but will keep looking).

We need to reduce the amount of context that needs to be swapped back
in when coming back to a review, and the largest part of this is
reducing the size of patches.

[2] has a good primer on how to keep PRs and patches small. If anyone
wants help breaking up a patch before code review, I would be more
than happy to lend a hand.

[1] https://medium.com/@kurtisnusbaum/large-diffs-are-hurting-your-ability-to-ship-e0b2b41e8acf
[2] https://graysonkoonce.com/stacked-pull-requests-keeping-github-diffs-small/

> | and the use of the github merge button.
>
> I don't like the GitHub button, it is easy to use it even with the
> smartphone and it is very dangerous ! I would like to disable it at all.
> I am actually running very well with the bk-merge-script as it does all the
> validation we want.
I'm neither yay nor nay on the button, but if we do require the merge
script, we should disable the button completely.

> The checks from jenkins/travis/coveralls.io...could be flaky and it would
> be very annoying to have to wait for a full green light for a pull request
> to be merged
This is a problem in itself, and needs to be addressed.

-Ivan

Re: git/github commit hooks

[Enrico Olivelli <eo...@gmail.com>]

Sam,
I agree we have to make some clean up and add some more rule.
My comments inline

Thank you
Enrico

2017-10-10 0:01 GMT+02:00 Sam Just <sj...@salesforce.com>:

> Last thursday, we a had a short discussion about possibly changing the
> merge process to allow unsquashed commits
>

I don't think is the best way.
The history of a pull request may have several minor commits and IMO it is
better to hide such conversation in the official git history of the project
If we think that the resulting commit is too big maybe we should split the
patch into smaller parts.

| and the use of the github merge button.

I don't like the GitHub button, it is easy to use it even with the
smartphone and it is very dangerous ! I would like to disable it at all.
I am actually running very well with the bk-merge-script as it does all the
validation we want.
The checks from jenkins/travis/coveralls.io...could be flaky and it would
be very annoying to have to wait for a full green light for a pull request
to be merged



> One sticking point is that we'd like an automatic way
> to enforce some commit message metadata requirements and formatting.
>

Using the script actually enforces this and make every step automatic and
error prone.
We can enhance it


>
> Git lets you define some hooks for validating commits and commit
> messages locally, see
> https://git-scm.com/book/gr/v2/Customizing-Git-Git-Hooks.
> Specifically, you can define a commit-msg hook which gets to validate
> the file containing the commit message before allowing the commit.  I
> think https://developer.github.com/webhooks/ can be leveraged to do
> the same checks on a github PR prior to allowing the PR to be merged,
> but haven't had time yet to figure out precisely how.
>



> -Sam
>