[Lustre-devel] Lustre code style Git commit hooks and
Andreas Dilger
inclusion into Lustre, I've created some Git commit hooks that can
be used to detect problems with the format and coding style of the
patch before it is submitted to Gerrit.
The b1_8, b2_1, and master (2.2) branches of lustre-release now have
scripts called build/commit-msg and build/prepare-commit-msg that
should be copied into the .git/hooks/ subdirectory of any Lustre
checkout. Currently the commit hooks are optional, but I would
recommend that everyone developing for Lustre would begin using them,
because I hope to make them mandatory in the future, once I'm sure
that they won't interfere with the development process.
The prepare-commit-msg hook will run the build/checkpatch.pl script
when "git commit" is run, and will append a comment with a summary
of any code style issues that it finds in the to-be-committed patch.
It is also possible to run checkpatch.pl manually before committing:
git diff [--cached] | build/checkpatch.pl -
While checkpatch.pl isn't going to catch all cases that don't follow
the Lustre style style guidelines (as documented in the wiki page:
http://wiki.whamcloud.com/display/PUB/Coding+Guidelines)
This can catch a lot of trivial mistakes that avoid spending the time
of inspectors and test systems, and the need for patch resubmission.
Errors from checkpatch.pl currently won't prevent committing the patch,
but you shouldn't ignore the warnings or errors lightly, since they
can cause the patch to be rejected later after it has consumed test
and inspection resources. The checkpatch.pl script also won't catch
all style problems, but it covers many of the basic problems. If you
feel the script is not agreeing with the Lustre "Coding Guidelines"
page, or you would like to improve it, please email me with details
and/or file a bug with a patch to fix the issue.
The commit-msg hook verifies the format of the commit message itself,
so that it contains a Jira ticket number, a "subsystem:" field, a
commit body, and a Signed-off-by: line. If no Gerrit "Change-Id:"
line is present it will automatically add this as well. The new
"subsystem:" field is similar to those used for kernel commit messages
all have, so that it is easier to decide what part of the code a patch
affects. Valid commit messages are of the form:
> LU-000 component: short description of change under 64 columns
>
> A more detailed explanation of the change being made. This can be
> as detailed as you'd like, possibly several paragraphs in length.
>
> Please provide a detailed explanation of what problem was solved,
> a good high-level description of how it was solved, and which
> parts of the code were affected (including function names as
> needed). Wrap lines at 72 columns or less.
>
> Signed-off-by: Random J Developer <ran...@developer.example.org>
> Change-Id: Ica9ed1612eab0c4673dee088f8b441d806c64932
If you haven't looked at the "Using Gerrit" and "Submitting Changes"
wiki pages recently, or want more detailed information about the
patch submission process, they contain a more detailed description:
http://wiki.whamcloud.com/display/PUB/Using+Gerrit
http://wiki.whamcloud.com/display/PUB/Submitting+Changes
I hope that these small additions will help make the Lustre code more consistent and easier to read, and make our inspection, testing, and landing process more efficient as we move into 2.2 development.
Cheers, Andreas
--
Andreas Dilger
Principal Engineer
Whamcloud, Inc.
_______________________________________________
Lustre-devel mailing list
Lustre...@lists.lustre.org
http://lists.lustre.org/mailman/listinfo/lustre-devel
Prakash Surya
feel enforcing these rules is better suited at the server level.
Currently, the proposed hooks will not allow any local commits to occur
if the message does not fit the format required. This presents
unnecessary overhead if the commit in question is intended to be
squashed or reworded later down the road.
It also prevents downstream commits, which are not intended to make it
upstream, from using any other commit message format. Thus, making it
impossible to use downstream bug numbers in the message which don't
take the form LU-nnn.
I think until those two use cases are accounted for, it will be tough
for me to use the proposed commit hooks. Perhaps, simply emitting a
warning on offending messages, leaving enforcement to the upstream
repository would suffice? Or bypass the local checks altogether if the
first line summary does not begin with LU-nnn? Or both?
--
Cheers,
Prakash
Andreas Dilger
> I like the idea of cleaning up the commit messages in Lustre, although I
> feel enforcing these rules is better suited at the server level.
There are already limited checks at the server level, and they may be tightened in the near future. The concerns about doing this included making sure that the verification scripts were working correctly first (i.e. as local hooks which can be bypassed if needed), to avoid too much disruption before developers got a chance to conform to the new requirements (i.e. some time after this initial announcement), and to catch the errors as early in the development process as possible (i.e. when the commit message is first created).
> Currently, the proposed hooks will not allow any local commits to occur
> if the message does not fit the format required. This presents
> unnecessary overhead if the commit in question is intended to be
> squashed or reworded later down the road.
True, but I've been using them for some time w/o undue effort for patches not intended for upstream use either. Note that there is a sample Signed-off-by: at the end of the commit comment that can be uncommented, and if you think it would be helpful a full template could be provided in the comments.
> It also prevents downstream commits, which are not intended to make it
> upstream, from using any other commit message format. Thus, making it
> impossible to use downstream bug numbers in the message which don't
> take the form LU-nnn.
>
> I think until those two use cases are accounted for, it will be tough
> for me to use the proposed commit hooks.
I thought about this previously, and tried (but did not commit) an environment variable that the commit-msg hook could could check to skip verification, something like:
COMCHECK=y git commit -a
> Perhaps, simply emitting a
> warning on offending messages, leaving enforcement to the upstream
> repository would suffice? Or bypass the local checks altogether if the
> first line summary does not begin with LU-nnn? Or both?
Since there are different Jira spaces (LU-nnn, ORI-nnn, MRP-nnn, etc.) this would be error prone.
Brian J. Murrell
> On 2011-11-03, at 4:28 PM, Prakash Surya <sur...@llnl.gov> wrote:
> > I like the idea of cleaning up the commit messages in Lustre, although I
> > feel enforcing these rules is better suited at the server level.
>
> There are already limited checks at the server level, and they may be tightened in the near future.
And the most the server (which is Gerrit that we are talking about her)
can do is to negatively review the patch as it does currently. There is
no mechanism to outright refuse to allow a push based on the content of
the patch, so client-side is still the best place to apply these checks
in an effort to prevent the patch from even being submitted if not
correct.
b.
James Simmons
> > On 2011-11-03, at 4:28 PM, Prakash Surya <sur...@llnl.gov> wrote:
> > > I like the idea of cleaning up the commit messages in Lustre, although I
> > > feel enforcing these rules is better suited at the server level.
> >
> > There are already limited checks at the server level, and they may be tightened in the near future.
>
> And the most the server (which is Gerrit that we are talking about her)
> can do is to negatively review the patch as it does currently. There is
> no mechanism to outright refuse to allow a push based on the content of
> the patch, so client-side is still the best place to apply these checks
> in an effort to prevent the patch from even being submitted if not
> correct.
Speaking of I just attempted to push a patch and noticed that no Change-Id
is being generated when I commit a new patch. This is with the default
hooks that I copied form build directory.
Andreas Dilger
>
>>>
> Speaking of I just attempted to push a patch and noticed that no Change-Id
> is being generated when I commit a new patch. This is with the default
> hooks that I copied form build directory.
Is this from the tip of the master branch?
An earlier version of the commit-msg hook used "git hash-object -t commit" and it caused an empty Change-Id string for older versions of Git. The "-t commit" option was removed from the latest version and solved the problem for the people that used it.
If updating to the latest version (or removing "-t commit") my fix this for you, please file a bug assigned to me and we can work it out.
The contents of the .git directory are not under revision control, so it isn't possible to automatically update the commit hooks. At some point I'd like to update the hooks via "make", but was holding off until I have more confidence that issues like this related to different development environments are fixed.
Thanks for the feedback.
Cheers, Andreas
Prakash Surya
> On 2011-11-09, at 9:27 AM, James Simmons <jsim...@infradead.org> wrote:
> >
> >>>
> > Speaking of I just attempted to push a patch and noticed that no Change-Id
> > is being generated when I commit a new patch. This is with the default
> > hooks that I copied form build directory.
>
> Is this from the tip of the master branch?
>
> An earlier version of the commit-msg hook used "git hash-object -t commit" and it caused an empty Change-Id string for older versions of Git. The "-t commit" option was removed from the latest version and solved the problem for the people that used it.
>
> If updating to the latest version (or removing "-t commit") my fix this for you, please file a bug assigned to me and we can work it out.
>
> The contents of the .git directory are not under revision control, so it isn't possible to automatically update the commit hooks. At some point I'd like to update the hooks via "make", but was holding off until I have more confidence that issues like this related to different development environments are fixed.
Alternatively, one could also use symlinks in .git to point to the files
in the lustre build directory. It's kind of a hack, but it keeps the
hooks up to date. And that way, the lustre build process doesn't have
to muck with the user's .git directory.
>
> Thanks for the feedback.
>
> Cheers, Andreas
> _______________________________________________
> Lustre-devel mailing list
> Lustre...@lists.lustre.org
> http://lists.lustre.org/mailman/listinfo/lustre-devel
--
Cheers,
Prakash
James Simmons
> > Speaking of I just attempted to push a patch and noticed that no Change-Id
> > is being generated when I commit a new patch. This is with the default
> > hooks that I copied form build directory.
>
> Is this from the tip of the master branch?
>
> An earlier version of the commit-msg hook used "git hash-object -t commit" and it caused an empty Change-Id string for older versions of Git. The "-t commit" option was removed from the latest version and solved the problem for the people that used it.
Yes your right. I updated the hooks and it works now. I also had to learn
you need a subsystem: field for commits now. Info about the subsystem
field is not the wiki.
Andreas Dilger
>> An earlier version of the commit-msg hook used "git hash-object -t commit" and it caused an empty Change-Id string for older versions of Git. The "-t commit" option was removed from the latest version and solved the problem for the people that used it.
>
> Yes your right. I updated the hooks and it works now. I also had to learn
> you need a subsystem: field for commits now. Info about the subsystem
> field is not the wiki.
Hmm, which wiki page are you referring to? The "Using Gerrit" page has had
the "component:" tag since june, long before I sent out the email,
and it was in the original email as well:
> The commit-msg hook verifies the format of the commit message itself,
> so that it contains a Jira ticket number, a "subsystem:" field, a
> commit body, and a Signed-off-by: line. If no Gerrit "Change-Id:"
> line is present it will automatically add this as well. The new
> "subsystem:" field is similar to those used for kernel commit messages
> all have, so that it is easier to decide what part of the code a patch
> affects.
If it is missing on some other wiki page, I'd like to correct it.
Cheers, Andreas
--
Andreas Dilger
Principal Engineer
Whamcloud, Inc.
_______________________________________________
James Simmons
> >> An earlier version of the commit-msg hook used "git hash-object -t commit" and it caused an empty Change-Id string for older versions of Git. The "-t commit" option was removed from the latest version and solved the problem for the people that used it.
> >
> > Yes your right. I updated the hooks and it works now. I also had to learn
> > you need a subsystem: field for commits now. Info about the subsystem
> > field is not the wiki.
>
> Hmm, which wiki page are you referring to? The "Using Gerrit" page has had
> the "component:" tag since june, long before I sent out the email,
> and it was in the original email as well:
http://wiki.whamcloud.com/display/PUB/Using+Gerrit
Looking at the page I do see the "component:" tag, but its because I
looked for it. It needs to be more clear what is required to push a
patch. Just a outsider's view.
Prakash Surya
>
> > >> An earlier version of the commit-msg hook used "git hash-object -t commit" and it caused an empty Change-Id string for older versions of Git. The "-t commit" option was removed from the latest version and solved the problem for the people that used it.
> > >
> > > Yes your right. I updated the hooks and it works now. I also had to learn
> > > you need a subsystem: field for commits now. Info about the subsystem
> > > field is not the wiki.
> >
> > Hmm, which wiki page are you referring to? The "Using Gerrit" page has had
> > the "component:" tag since june, long before I sent out the email,
> > and it was in the original email as well:
>
> http://wiki.whamcloud.com/display/PUB/Using+Gerrit
>
> Looking at the page I do see the "component:" tag, but its because I
> looked for it. It needs to be more clear what is required to push a
> patch. Just a outsider's view.
Would a more detailed explanation of each "part" of the commit message
help? For example, under the sample commit message:
* LU-000: Issue number referencing an open issue at jira.whamcloud.com
* component: The component(s) affected by this patch (i.e. osd-ldiskfs,
lnet, etc.)
* Short Description: ...
* etc...
Looking at the wiki page referenced above, it's a little hard to
distinguish each section from one another. They all seem to blend into a
single wall of text, IMO. Splitting them up into separate pages, or
simply using a better color scheme to distinguish the sections apart,
might help the page's readability as a whole.
--
Cheers,
Prakash
James Simmons
> > > >> An earlier version of the commit-msg hook used "git hash-object -t commit" and it caused an empty Change-Id string for older versions of Git. The "-t commit" option was removed from the latest version and solved the problem for the people that used it.
> > > >
> > > > Yes your right. I updated the hooks and it works now. I also had to learn
> > > > you need a subsystem: field for commits now. Info about the subsystem
> > > > field is not the wiki.
> > >
> > > Hmm, which wiki page are you referring to? The "Using Gerrit" page has had
> > > the "component:" tag since june, long before I sent out the email,
> > > and it was in the original email as well:
> >
> > http://wiki.whamcloud.com/display/PUB/Using+Gerrit
> >
> > Looking at the page I do see the "component:" tag, but its because I
> > looked for it. It needs to be more clear what is required to push a
> > patch. Just a outsider's view.
>
> Would a more detailed explanation of each "part" of the commit message
> help? For example, under the sample commit message:
>
> * LU-000: Issue number referencing an open issue at jira.whamcloud.com
>
> * component: The component(s) affected by this patch (i.e. osd-ldiskfs,
> lnet, etc.)
>
> * Short Description: ...
>
> * etc...
>
> Looking at the wiki page referenced above, it's a little hard to
> distinguish each section from one another. They all seem to blend into a
> single wall of text, IMO. Splitting them up into separate pages, or
> simply using a better color scheme to distinguish the sections apart,
> might help the page's readability as a whole.
Absolutely agree. I like the idea of a color scheme with the above format
under the simple commit message.
Andreas Dilger
>>> http://wiki.whamcloud.com/display/PUB/Using+Gerrit
>>>
>>> Looking at the page I do see the "component:" tag, but its because I
>>> looked for it. It needs to be more clear what is required to push a
>>> patch. Just a outsider's view.
>>
>> Would a more detailed explanation of each "part" of the commit message
>> help? For example, under the sample commit message:
>>
>> * LU-000: Issue number referencing an open issue at jira.whamcloud.com
>>
>> * component: The component(s) affected by this patch (i.e. osd-ldiskfs,
>> lnet, etc.)
>>
>> * Short Description: ...
Have a look at: http://wiki.whamcloud.com/display/PUB/Commit+Comments
It has an updated description of the commit comment, and why having a
good one is important, as well as an improved commit comment template.
I'm submitting this va change http://review.whamcloud.com/1688, and it
is reproduced below:
LU-nnn component: short description of change under 64 columns
The "component:" should be a lower-case single-word subsystem of the
Lustre code that best encompasses the change being made. Examples of
components include modules like: llite, lov, lmv, osc, mdc, ldlm, lnet,
ptlrpc, mds, oss, osd, ldiskfs, libcfs, socklnd, o2iblnd; functional
subsystems like: recovery, quota, grant; and auxilliary areas like:
build, tests, docs. This list is not exhaustive, but is a guideline.
The commit comment should contain a detailed explanation of the change
being made. This can be as long as you'd like. Please give details
of what problem was solved (including error messages or problems that
were seen), a good high-level description of how it was solved, and
which parts of the code were changed (including important functions
that were changed, if this is useful to understand the patch, and
for easier searching). Wrap lines at/under 70 columns.
This is the error message that you would get if you try to submit an
incorrect commit. If people think it would be more useful, the
prepare-commit-msg hook could go whole-hog and just replace the basic
commit comment (which basically shows "git status") with a full sample
commit template that has (commented out) the above template and a
Signed-off-by: line.
>> Looking at the wiki page referenced above, it's a little hard to
>> distinguish each section from one another. They all seem to blend into a
>> single wall of text, IMO. Splitting them up into separate pages, or
>> simply using a better color scheme to distinguish the sections apart,
>> might help the page's readability as a whole.
>
> Absolutely agree. I like the idea of a color scheme with the above format
> under the simple commit message.
Yes, we started to split up these pages and improve the content, but
SC has interrupted this process.
Cheers, Andreas
--
Andreas Dilger
Principal Engineer
Whamcloud, Inc.
_______________________________________________
Bruce Korb
On 11/15/11 12:55 AM, Andreas Dilger wrote:
>On 2011-11-14, at 11:14 AM, James Simmons wrote:
>This is the error message that you would get if you try to submit an
>incorrect commit. If people think it would be more useful, the
>prepare-commit-msg hook could go whole-hog and just replace the basic
>commit comment (which basically shows "git status") with a full sample
>commit template that has (commented out) the above template and a
>Signed-off-by: line.
Just a brief synopsis of the "transgression" and a URL to the full
description would be excellent. (I don't like having to Google an
error message to find the full docs and the full docs in an error
message is overly verbose.)
Thank you!