-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41586/#review111839
-----------------------------------------------------------


Nice work. Just a couple of tweaks.


bootstrap (lines 22 - 23)
<https://reviews.apache.org/r/41586/#comment172120>

    Our general policy in C++ variable names is to avoid abbreviations, and I 
imagine that could apply to shell scripts too.
    Please s/msg/message/



support/hooks/commit-msg (line 3)
<https://reviews.apache.org/r/41586/#comment172121>

    "A hook script to verify commit message format. Called by..." so that the 
first sentence explains what the script does, rather than how it's called.



support/hooks/commit-msg (line 4)
<https://reviews.apache.org/r/41586/#comment172123>

    "should"? Are you unsure if it will?



support/hooks/commit-msg (lines 6 - 7)
<https://reviews.apache.org/r/41586/#comment172124>

    # To enable this hook, do this from the root of the repo:
    #
    # $ ln -s ../../support/hooks/commit-message .git/hooks/commit-message



support/hooks/commit-msg (line 18)
<https://reviews.apache.org/r/41586/#comment172126>

    Isn't it more reasonable to say `if [ "$LENGTH" -gt 72 ];`?



support/hooks/commit-msg (line 19)
<https://reviews.apache.org/r/41586/#comment172127>

    None of the lines should exceed 72 chars, and the first line should ideally 
be even less (~50). Ideally this hook would error for a too-long summary line, 
and wrap the description.



support/hooks/commit-msg (line 23)
<https://reviews.apache.org/r/41586/#comment172130>

    How about `!=`?



support/hooks/commit-msg (line 24)
<https://reviews.apache.org/r/41586/#comment172128>

    You're actually only testing the commit message summary, not the entire 
"commit message"



support/hooks/commit-msg (line 28)
<https://reviews.apache.org/r/41586/#comment172129>

    Pretty sure "exit 0" is the default if you successfully reach the end of 
the script. You can leave it out.


- Adam B


On Dec. 19, 2015, 12:21 a.m., Artem Harutyunyan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41586/
> -----------------------------------------------------------
> 
> (Updated Dec. 19, 2015, 12:21 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Partially enforced commit message guidelines with a hook.
> 
> 
> Diffs
> -----
> 
>   bootstrap 89d986fd95dc16bbb79623ef92e3b14a2e7009f9 
>   support/hooks/commit-msg PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41586/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Reply via email to