You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Alexander Murmann <am...@pivotal.io> on 2018/09/12 18:46:15 UTC
[discuss] Should we evaluate at commit messages as part of PR review?
Hi everyone,
We have a wiki page
<https://cwiki.apache.org/confluence/display/GEODE/Commit+Message+Format>
that discusses why good commit messages matter and links to a even better
article on the topic. In addition to what's described in those documents,
better commit messages also would make it easier to have good PR messages.
Good commit and PR messages also provide more context to the reviewer who
in turn now can do a better job at reviewing the pull request.
Looking at our git log gives me the impression that we aren't always living
up to that standard. In fact we frequently aren't even close.
I propose taking clear and well formatted commit messages into account as
part of our PR review process. Lacking commit messages can be just as bad
as bad naming in our code.
Thoughts?
Re: [discuss] Should we evaluate at commit messages as part of PR
review?
Posted by Udo Kohlmeyer <ud...@apache.org>.
-1 on removing the JIRA ticket number from the summary line.
I believe that it is a vital piece of the comment, and putting it
upfront enforces that all work is committed with a corresponding JIRA.
I'm on the fence when it comes to the whole "let's put a lot more
information into the git comment." I feel that the information provided
should be enough to have an understanding what was done in the commit..
But given that we have a "single commit" policy, it might end up having
to provide A LOT of detail into the commit message.
So how much info is too much and how much is not enough.. This becomes a
question of perspective and judgement...
--Udo
On 9/14/18 11:55, Bradford Boyle wrote:
> How would people feel about removing the requirement to include the
> "GEODE-XXXX: " prefix in the summary line? That accounts for about 25% of
> the 52 character limit. We could move it to the first non-summary line of
> the commit message.
>
> --Bradford
>
Re: [discuss] Should we evaluate at commit messages as part of PR review?
Posted by Alexander Murmann <am...@pivotal.io>.
While the wiki links to the great article, I wonder if it would make sense
to explicitly call out the parts that we most care about in the wiki
itself, especially if we end up agreeing that not meeting some criteria
might result in a rejected PR. To me those would be violating the format
and not including the "why?" when it's not obvious. I honestly care less
about details on the "what?" in most cases, since the commit itself
obviously includes all the changes.
On Fri, Sep 14, 2018 at 3:08 PM, Kirk Lund <kl...@apache.org> wrote:
> Correction! The wiki page does have a link to
> *https://chris.beams.io/posts/git-commit/
> <https://chris.beams.io/posts/git-commit/> *at the bottom of the page:
>
> How to write a Git commit message <https://chris.beams.io/posts/
> git-commit/>
>
> On Fri, Sep 14, 2018 at 3:05 PM, Kirk Lund <kl...@apache.org> wrote:
>
> > *The commit message should follow imperative style.* The wiki page seems
> > to be missing this even though we agreed to it several times on the
> > dev-list over the last 3 years. I'll add this to the wiki page.
> >
> > You can then say *"If I apply this commit, then it will..."* for any of
> > the git commits. For an explanation, see *https://chris.beams.io/posts/
> git-commit/
> > <https://chris.beams.io/posts/git-commit/>*
> >
> > Example:
> >
> > *Use this:*
> >
> > *GEODE-xxxx: Fix failing CompositePropertySourceTest*
> >
> > Instead of:
> > GEODE-xxxx: Fixing failing CompositePropertySourceTest
> > GEODE-xxxx: Fixed failing CompositePropertySourceTest
> >
> > And *definitely* instead of:
> > GEODE-xxxx: failing CompositePropertySourceTest
> > GEODE-xxxx: polishing stuff
> > GEODE-xxxx: CompositePropertySourceTest is failing intermittently
> >
> > Just to be clear, these last 5 are examples of how you should NOT word
> the
> > commit message.
> >
> > On Fri, Sep 14, 2018 at 12:11 PM, Alexander Murmann <amurmann@pivotal.io
> >
> > wrote:
> >
> >> I do find it very helpful to have the ticket number at the beginning of
> >> the
> >> title. It makes it really easy to scan the output of `git log --oneline`
> >> or
> >> GitX to see what tickets happened recently or since a certain tag.
> >>
> >> On Fri, Sep 14, 2018 at 11:55 AM, Bradford Boyle <bb...@pivotal.io>
> >> wrote:
> >>
> >> > How would people feel about removing the requirement to include the
> >> > "GEODE-XXXX: " prefix in the summary line? That accounts for about 25%
> >> of
> >> > the 52 character limit. We could move it to the first non-summary line
> >> of
> >> > the commit message.
> >> >
> >> > --Bradford
> >> >
> >>
> >
> >
>
Re: [discuss] Should we evaluate at commit messages as part of PR review?
Posted by Kirk Lund <kl...@apache.org>.
Correction! The wiki page does have a link to
*https://chris.beams.io/posts/git-commit/
<https://chris.beams.io/posts/git-commit/> *at the bottom of the page:
How to write a Git commit message <https://chris.beams.io/posts/git-commit/>
On Fri, Sep 14, 2018 at 3:05 PM, Kirk Lund <kl...@apache.org> wrote:
> *The commit message should follow imperative style.* The wiki page seems
> to be missing this even though we agreed to it several times on the
> dev-list over the last 3 years. I'll add this to the wiki page.
>
> You can then say *"If I apply this commit, then it will..."* for any of
> the git commits. For an explanation, see *https://chris.beams.io/posts/git-commit/
> <https://chris.beams.io/posts/git-commit/>*
>
> Example:
>
> *Use this:*
>
> *GEODE-xxxx: Fix failing CompositePropertySourceTest*
>
> Instead of:
> GEODE-xxxx: Fixing failing CompositePropertySourceTest
> GEODE-xxxx: Fixed failing CompositePropertySourceTest
>
> And *definitely* instead of:
> GEODE-xxxx: failing CompositePropertySourceTest
> GEODE-xxxx: polishing stuff
> GEODE-xxxx: CompositePropertySourceTest is failing intermittently
>
> Just to be clear, these last 5 are examples of how you should NOT word the
> commit message.
>
> On Fri, Sep 14, 2018 at 12:11 PM, Alexander Murmann <am...@pivotal.io>
> wrote:
>
>> I do find it very helpful to have the ticket number at the beginning of
>> the
>> title. It makes it really easy to scan the output of `git log --oneline`
>> or
>> GitX to see what tickets happened recently or since a certain tag.
>>
>> On Fri, Sep 14, 2018 at 11:55 AM, Bradford Boyle <bb...@pivotal.io>
>> wrote:
>>
>> > How would people feel about removing the requirement to include the
>> > "GEODE-XXXX: " prefix in the summary line? That accounts for about 25%
>> of
>> > the 52 character limit. We could move it to the first non-summary line
>> of
>> > the commit message.
>> >
>> > --Bradford
>> >
>>
>
>
Re: [discuss] Should we evaluate at commit messages as part of PR review?
Posted by Kirk Lund <kl...@apache.org>.
*The commit message should follow imperative style.* The wiki page seems to
be missing this even though we agreed to it several times on the dev-list
over the last 3 years. I'll add this to the wiki page.
You can then say *"If I apply this commit, then it will..."* for any of the
git commits. For an explanation, see *https://chris.beams.io/posts/git-commit/
<https://chris.beams.io/posts/git-commit/>*
Example:
*Use this:*
*GEODE-xxxx: Fix failing CompositePropertySourceTest*
Instead of:
GEODE-xxxx: Fixing failing CompositePropertySourceTest
GEODE-xxxx: Fixed failing CompositePropertySourceTest
And *definitely* instead of:
GEODE-xxxx: failing CompositePropertySourceTest
GEODE-xxxx: polishing stuff
GEODE-xxxx: CompositePropertySourceTest is failing intermittently
Just to be clear, these last 5 are examples of how you should NOT word the
commit message.
On Fri, Sep 14, 2018 at 12:11 PM, Alexander Murmann <am...@pivotal.io>
wrote:
> I do find it very helpful to have the ticket number at the beginning of the
> title. It makes it really easy to scan the output of `git log --oneline` or
> GitX to see what tickets happened recently or since a certain tag.
>
> On Fri, Sep 14, 2018 at 11:55 AM, Bradford Boyle <bb...@pivotal.io>
> wrote:
>
> > How would people feel about removing the requirement to include the
> > "GEODE-XXXX: " prefix in the summary line? That accounts for about 25% of
> > the 52 character limit. We could move it to the first non-summary line of
> > the commit message.
> >
> > --Bradford
> >
>
Re: [discuss] Should we evaluate at commit messages as part of PR review?
Posted by Alexander Murmann <am...@pivotal.io>.
I do find it very helpful to have the ticket number at the beginning of the
title. It makes it really easy to scan the output of `git log --oneline` or
GitX to see what tickets happened recently or since a certain tag.
On Fri, Sep 14, 2018 at 11:55 AM, Bradford Boyle <bb...@pivotal.io> wrote:
> How would people feel about removing the requirement to include the
> "GEODE-XXXX: " prefix in the summary line? That accounts for about 25% of
> the 52 character limit. We could move it to the first non-summary line of
> the commit message.
>
> --Bradford
>
Re: [discuss] Should we evaluate at commit messages as part of PR review?
Posted by Bradford Boyle <bb...@pivotal.io>.
How would people feel about removing the requirement to include the
"GEODE-XXXX: " prefix in the summary line? That accounts for about 25% of
the 52 character limit. We could move it to the first non-summary line of
the commit message.
--Bradford
Re: [discuss] Should we evaluate at commit messages as part of PR review?
Posted by Jianxia Chen <jc...@apache.org>.
There is a good article on this topic that Anthony recommended a while ago:
http://chris.beams.io/posts/git-commit/
Jianxia
On Wed, Sep 12, 2018 at 4:30 PM Michael Stolz <ms...@pivotal.io> wrote:
> +1 to descriptive commit messages
>
> Done well they will save time on every post commit access to that commit.
>
> --
> Mike Stolz
> Principal Engineer - Gemfire Product Manager
> Mobile: 631-835-4771
>
> On Sep 12, 2018 4:15 PM, "Alexander Murmann" <am...@pivotal.io> wrote:
>
> > While our wiki page primarily calls out the 50/74 rule which is
> important,
> > my bigger concern is in that I'd love to see commit messages that explain
> > why a change was made. Sometimes that information is in the reference
> JIRA
> > ticket, but not always. Especially with bug fixes we tend to not explain
> > what actually caused the bug and how the code changes address it. It's
> also
> > nice to minimize moving back and forth between editor and browsing JIRA
> and
> > even better not to have to read 20+ messages long comment threads about a
> > bug in JIRA to find out what the problem was. No hard rule can make sure
> we
> > are doing that right, only human reviewers and care can. I don't think
> it's
> > actually any extra work, since we are already reading the PR description
> > and commit messages when we are doing a PR review. How else can you
> > understand the PR? In fact better commit messages should save time here
> and
> > not steal it.
> >
> > I totally agree that we don't always need a long text. "Fix typo in XYZ
> > output" is totally fine. Both the why and what are obvious.
> >
> > On Wed, Sep 12, 2018 at 4:07 PM, Helena Bales <hb...@pivotal.io> wrote:
> >
> > > I'm a fan of having useful commit messages. I would prefer this to be
> > > another checkbox in our list of 6 things to do for Pull Requests as
> > opposed
> > > to something that is strictly enforced. There are cases where a simple
> > > one-liner commit message is enough. I personally use commit messages
> > often,
> > > even when looking back at my own work. Additionally, I think it is a
> > useful
> > > exercise as it forces the dev to think back on the original problem and
> > > think about how the solution addresses that.
> > >
> > > tl;dr -- +1 for commit messages
> > >
> > > ~Helena
> > >
> > > On Wed, Sep 12, 2018 at 11:50 AM, Pulkit Chandra <pc...@pivotal.io>
> > > wrote:
> > >
> > > > Have we thought about git hooks as a way to enforce policy
> > > > https://git-scm.com/book/en/v2/Customizing-Git-An-Example-
> > > > Git-Enforced-Policy
> > > > ?
> > > >
> > > > *Pulkit Chandra*
> > > >
> > > >
> > > > On Wed, Sep 12, 2018 at 2:46 PM Alexander Murmann <
> amurmann@pivotal.io
> > >
> > > > wrote:
> > > >
> > > > > Hi everyone,
> > > > >
> > > > > We have a wiki page
> > > > > <https://cwiki.apache.org/confluence/display/GEODE/
> > > Commit+Message+Format
> > > > >
> > > > > that discusses why good commit messages matter and links to a even
> > > better
> > > > > article on the topic. In addition to what's described in those
> > > documents,
> > > > > better commit messages also would make it easier to have good PR
> > > > messages.
> > > > > Good commit and PR messages also provide more context to the
> reviewer
> > > who
> > > > > in turn now can do a better job at reviewing the pull request.
> > > > >
> > > > > Looking at our git log gives me the impression that we aren't
> always
> > > > living
> > > > > up to that standard. In fact we frequently aren't even close.
> > > > >
> > > > > I propose taking clear and well formatted commit messages into
> > account
> > > as
> > > > > part of our PR review process. Lacking commit messages can be just
> as
> > > bad
> > > > > as bad naming in our code.
> > > > >
> > > > > Thoughts?
> > > > >
> > > >
> > >
> >
>
Re: [discuss] Should we evaluate at commit messages as part of PR review?
Posted by Michael Stolz <ms...@pivotal.io>.
+1 to descriptive commit messages
Done well they will save time on every post commit access to that commit.
--
Mike Stolz
Principal Engineer - Gemfire Product Manager
Mobile: 631-835-4771
On Sep 12, 2018 4:15 PM, "Alexander Murmann" <am...@pivotal.io> wrote:
> While our wiki page primarily calls out the 50/74 rule which is important,
> my bigger concern is in that I'd love to see commit messages that explain
> why a change was made. Sometimes that information is in the reference JIRA
> ticket, but not always. Especially with bug fixes we tend to not explain
> what actually caused the bug and how the code changes address it. It's also
> nice to minimize moving back and forth between editor and browsing JIRA and
> even better not to have to read 20+ messages long comment threads about a
> bug in JIRA to find out what the problem was. No hard rule can make sure we
> are doing that right, only human reviewers and care can. I don't think it's
> actually any extra work, since we are already reading the PR description
> and commit messages when we are doing a PR review. How else can you
> understand the PR? In fact better commit messages should save time here and
> not steal it.
>
> I totally agree that we don't always need a long text. "Fix typo in XYZ
> output" is totally fine. Both the why and what are obvious.
>
> On Wed, Sep 12, 2018 at 4:07 PM, Helena Bales <hb...@pivotal.io> wrote:
>
> > I'm a fan of having useful commit messages. I would prefer this to be
> > another checkbox in our list of 6 things to do for Pull Requests as
> opposed
> > to something that is strictly enforced. There are cases where a simple
> > one-liner commit message is enough. I personally use commit messages
> often,
> > even when looking back at my own work. Additionally, I think it is a
> useful
> > exercise as it forces the dev to think back on the original problem and
> > think about how the solution addresses that.
> >
> > tl;dr -- +1 for commit messages
> >
> > ~Helena
> >
> > On Wed, Sep 12, 2018 at 11:50 AM, Pulkit Chandra <pc...@pivotal.io>
> > wrote:
> >
> > > Have we thought about git hooks as a way to enforce policy
> > > https://git-scm.com/book/en/v2/Customizing-Git-An-Example-
> > > Git-Enforced-Policy
> > > ?
> > >
> > > *Pulkit Chandra*
> > >
> > >
> > > On Wed, Sep 12, 2018 at 2:46 PM Alexander Murmann <amurmann@pivotal.io
> >
> > > wrote:
> > >
> > > > Hi everyone,
> > > >
> > > > We have a wiki page
> > > > <https://cwiki.apache.org/confluence/display/GEODE/
> > Commit+Message+Format
> > > >
> > > > that discusses why good commit messages matter and links to a even
> > better
> > > > article on the topic. In addition to what's described in those
> > documents,
> > > > better commit messages also would make it easier to have good PR
> > > messages.
> > > > Good commit and PR messages also provide more context to the reviewer
> > who
> > > > in turn now can do a better job at reviewing the pull request.
> > > >
> > > > Looking at our git log gives me the impression that we aren't always
> > > living
> > > > up to that standard. In fact we frequently aren't even close.
> > > >
> > > > I propose taking clear and well formatted commit messages into
> account
> > as
> > > > part of our PR review process. Lacking commit messages can be just as
> > bad
> > > > as bad naming in our code.
> > > >
> > > > Thoughts?
> > > >
> > >
> >
>
Re: [discuss] Should we evaluate at commit messages as part of PR review?
Posted by Alexander Murmann <am...@pivotal.io>.
While our wiki page primarily calls out the 50/74 rule which is important,
my bigger concern is in that I'd love to see commit messages that explain
why a change was made. Sometimes that information is in the reference JIRA
ticket, but not always. Especially with bug fixes we tend to not explain
what actually caused the bug and how the code changes address it. It's also
nice to minimize moving back and forth between editor and browsing JIRA and
even better not to have to read 20+ messages long comment threads about a
bug in JIRA to find out what the problem was. No hard rule can make sure we
are doing that right, only human reviewers and care can. I don't think it's
actually any extra work, since we are already reading the PR description
and commit messages when we are doing a PR review. How else can you
understand the PR? In fact better commit messages should save time here and
not steal it.
I totally agree that we don't always need a long text. "Fix typo in XYZ
output" is totally fine. Both the why and what are obvious.
On Wed, Sep 12, 2018 at 4:07 PM, Helena Bales <hb...@pivotal.io> wrote:
> I'm a fan of having useful commit messages. I would prefer this to be
> another checkbox in our list of 6 things to do for Pull Requests as opposed
> to something that is strictly enforced. There are cases where a simple
> one-liner commit message is enough. I personally use commit messages often,
> even when looking back at my own work. Additionally, I think it is a useful
> exercise as it forces the dev to think back on the original problem and
> think about how the solution addresses that.
>
> tl;dr -- +1 for commit messages
>
> ~Helena
>
> On Wed, Sep 12, 2018 at 11:50 AM, Pulkit Chandra <pc...@pivotal.io>
> wrote:
>
> > Have we thought about git hooks as a way to enforce policy
> > https://git-scm.com/book/en/v2/Customizing-Git-An-Example-
> > Git-Enforced-Policy
> > ?
> >
> > *Pulkit Chandra*
> >
> >
> > On Wed, Sep 12, 2018 at 2:46 PM Alexander Murmann <am...@pivotal.io>
> > wrote:
> >
> > > Hi everyone,
> > >
> > > We have a wiki page
> > > <https://cwiki.apache.org/confluence/display/GEODE/
> Commit+Message+Format
> > >
> > > that discusses why good commit messages matter and links to a even
> better
> > > article on the topic. In addition to what's described in those
> documents,
> > > better commit messages also would make it easier to have good PR
> > messages.
> > > Good commit and PR messages also provide more context to the reviewer
> who
> > > in turn now can do a better job at reviewing the pull request.
> > >
> > > Looking at our git log gives me the impression that we aren't always
> > living
> > > up to that standard. In fact we frequently aren't even close.
> > >
> > > I propose taking clear and well formatted commit messages into account
> as
> > > part of our PR review process. Lacking commit messages can be just as
> bad
> > > as bad naming in our code.
> > >
> > > Thoughts?
> > >
> >
>
Re: [discuss] Should we evaluate at commit messages as part of PR review?
Posted by Helena Bales <hb...@pivotal.io>.
I'm a fan of having useful commit messages. I would prefer this to be
another checkbox in our list of 6 things to do for Pull Requests as opposed
to something that is strictly enforced. There are cases where a simple
one-liner commit message is enough. I personally use commit messages often,
even when looking back at my own work. Additionally, I think it is a useful
exercise as it forces the dev to think back on the original problem and
think about how the solution addresses that.
tl;dr -- +1 for commit messages
~Helena
On Wed, Sep 12, 2018 at 11:50 AM, Pulkit Chandra <pc...@pivotal.io>
wrote:
> Have we thought about git hooks as a way to enforce policy
> https://git-scm.com/book/en/v2/Customizing-Git-An-Example-
> Git-Enforced-Policy
> ?
>
> *Pulkit Chandra*
>
>
> On Wed, Sep 12, 2018 at 2:46 PM Alexander Murmann <am...@pivotal.io>
> wrote:
>
> > Hi everyone,
> >
> > We have a wiki page
> > <https://cwiki.apache.org/confluence/display/GEODE/Commit+Message+Format
> >
> > that discusses why good commit messages matter and links to a even better
> > article on the topic. In addition to what's described in those documents,
> > better commit messages also would make it easier to have good PR
> messages.
> > Good commit and PR messages also provide more context to the reviewer who
> > in turn now can do a better job at reviewing the pull request.
> >
> > Looking at our git log gives me the impression that we aren't always
> living
> > up to that standard. In fact we frequently aren't even close.
> >
> > I propose taking clear and well formatted commit messages into account as
> > part of our PR review process. Lacking commit messages can be just as bad
> > as bad naming in our code.
> >
> > Thoughts?
> >
>
Re: [discuss] Should we evaluate at commit messages as part of PR review?
Posted by Pulkit Chandra <pc...@pivotal.io>.
Have we thought about git hooks as a way to enforce policy
https://git-scm.com/book/en/v2/Customizing-Git-An-Example-Git-Enforced-Policy
?
*Pulkit Chandra*
On Wed, Sep 12, 2018 at 2:46 PM Alexander Murmann <am...@pivotal.io>
wrote:
> Hi everyone,
>
> We have a wiki page
> <https://cwiki.apache.org/confluence/display/GEODE/Commit+Message+Format>
> that discusses why good commit messages matter and links to a even better
> article on the topic. In addition to what's described in those documents,
> better commit messages also would make it easier to have good PR messages.
> Good commit and PR messages also provide more context to the reviewer who
> in turn now can do a better job at reviewing the pull request.
>
> Looking at our git log gives me the impression that we aren't always living
> up to that standard. In fact we frequently aren't even close.
>
> I propose taking clear and well formatted commit messages into account as
> part of our PR review process. Lacking commit messages can be just as bad
> as bad naming in our code.
>
> Thoughts?
>