Re: Review Request 41586: Partially enforced commit message guidelines with a hook.

[Adam B]

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

Ship it!


Ship It!

- Adam B


On Dec. 30, 2015, 11:28 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41586/
> ---
> 
> (Updated Dec. 30, 2015, 11:28 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
> 
>

Re: Review Request 41586: Partially enforced commit message guidelines with a hook.

[Adam B]

> On Dec. 24, 2015, 2:27 a.m., Adam B wrote:
> > support/hooks/commit-msg, lines 6-7
> > 
> >
> > # To enable this hook, do this from the root of the repo:
> > #
> > # $ ln -s ../../support/hooks/commit-message .git/hooks/commit-message
> 
> Artem Harutyunyan wrote:
> In our case it's done automatically by the bootstrap script. It's part of 
> this same commit. Added a clarifying comment.

Seems like the other two hooks are enabled by bootstrap as well. Let's keep it 
consistent. I'll update this one to more closely resemble the others, "To 
enable this hook, run `bootstrap` or do this..."


> On Dec. 24, 2015, 2:27 a.m., Adam B wrote:
> > support/hooks/commit-msg, line 4
> > 
> >
> > "should"? Are you unsure if it will?
> 
> Artem Harutyunyan wrote:
> I believe that the `if it wants to stop the commit` part of the sentence 
> makes it pretty explicit, no? Again, this comment is part of the template 
> hook file. If you have a suggestion on how to rewrite it I'll gladly follow 
> it :-).

Edited to "The hook exits with non-zero status after..."


- Adam


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


On Dec. 30, 2015, 11:28 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41586/
> ---
> 
> (Updated Dec. 30, 2015, 11:28 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
> 
>

Re: Review Request 41586: Partially enforced commit message guidelines with a hook.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41584, 41586]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 30, 2015, 7:28 p.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41586/
> ---
> 
> (Updated Dec. 30, 2015, 7:28 p.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
> 
>

Re: Review Request 41586: Partially enforced commit message guidelines with a hook.

[Artem Harutyunyan]

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

(Updated Dec. 30, 2015, 11:28 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 (updated)
-

  bootstrap 89d986fd95dc16bbb79623ef92e3b14a2e7009f9 
  support/hooks/commit-msg PRE-CREATION 

Diff: https://reviews.apache.org/r/41586/diff/


Testing
---


Thanks,

Artem Harutyunyan

Re: Review Request 41586: Partially enforced commit message guidelines with a hook.

[Neil Conway]

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

Ship it!


Ship It!


support/hooks/commit-msg (line 22)


Not sure why we say "(the first line)" here but not in the other error 
messages.


- Neil Conway


On Dec. 26, 2015, 2:05 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41586/
> ---
> 
> (Updated Dec. 26, 2015, 2:05 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
> 
>

Re: Review Request 41586: Partially enforced commit message guidelines with a hook.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41584, 41586]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 26, 2015, 2:05 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41586/
> ---
> 
> (Updated Dec. 26, 2015, 2:05 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
> 
>

Re: Review Request 41586: Partially enforced commit message guidelines with a hook.

[Artem Harutyunyan]

> On Dec. 24, 2015, 2:27 a.m., Adam B wrote:
> > bootstrap, lines 22-23
> > 
> >
> > 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/

That's a standard git hook file name. 
https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks


> On Dec. 24, 2015, 2:27 a.m., Adam B wrote:
> > support/hooks/commit-msg, line 3
> > 
> >
> > "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.

Actually that comment came with the template hook file 
(.git/hooks/commit-msg.sample).


> On Dec. 24, 2015, 2:27 a.m., Adam B wrote:
> > support/hooks/commit-msg, line 4
> > 
> >
> > "should"? Are you unsure if it will?

I believe that the `if it wants to stop the commit` part of the sentence makes 
it pretty explicit, no? Again, this comment is part of the template hook file. 
If you have a suggestion on how to rewrite it I'll gladly follow it :-).


> On Dec. 24, 2015, 2:27 a.m., Adam B wrote:
> > support/hooks/commit-msg, lines 6-7
> > 
> >
> > # To enable this hook, do this from the root of the repo:
> > #
> > # $ ln -s ../../support/hooks/commit-message .git/hooks/commit-message

In our case it's done automatically by the bootstrap script. It's part of this 
same commit. Added a clarifying comment.


> On Dec. 24, 2015, 2:27 a.m., Adam B wrote:
> > support/hooks/commit-msg, line 19
> > 
> >
> > 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.

Makes sense. Added a JIRA https://issues.apache.org/jira/browse/MESOS-4250 to 
follow up.


- Artem


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


On Dec. 25, 2015, 6:05 p.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41586/
> ---
> 
> (Updated Dec. 25, 2015, 6:05 p.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
> 
>

Re: Review Request 41586: Partially enforced commit message guidelines with a hook.

[Artem Harutyunyan]

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

(Updated Dec. 25, 2015, 6:05 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, and Neil Conway.


Repository: mesos


Description
---

Partially enforced commit message guidelines with a hook.


Diffs (updated)
-

  bootstrap 89d986fd95dc16bbb79623ef92e3b14a2e7009f9 
  support/hooks/commit-msg PRE-CREATION 

Diff: https://reviews.apache.org/r/41586/diff/


Testing
---


Thanks,

Artem Harutyunyan

Re: Review Request 41586: Partially enforced commit message guidelines with a hook.

[Adam B]

---
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)


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)


"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)


"should"? Are you unsure if it will?



support/hooks/commit-msg (lines 6 - 7)


# 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)


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



support/hooks/commit-msg (line 19)


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)


How about `!=`?



support/hooks/commit-msg (line 24)


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



support/hooks/commit-msg (line 28)


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
> 
>

Review Request 41586: Partially enforced commit message guidelines with a hook.

[Artem Harutyunyan]

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

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

Re: Review Request 41586: Partially enforced commit message guidelines with a hook.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41584, 41586]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 19, 2015, 8: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, 8: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
> 
>