You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2016/04/10 08:41:24 UTC
Review Request 45984: Fixed the commit message hook to wrap the
variables in quotes.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45984/
-----------------------------------------------------------
Review request for mesos, Joerg Schad, Kevin Klues, and Vinod Kone.
Bugs: MESOS-5162
https://issues.apache.org/jira/browse/MESOS-5162
Repository: mesos
Description
-------
See summary.
Diffs
-----
support/hooks/commit-msg d3f10c886a654aa8d8364faddf2e50dc7fe82058
Diff: https://reviews.apache.org/r/45984/diff/
Testing
-------
Thanks,
Michael Park
Re: Review Request 45984: Fixed the commit message hook to wrap the
variables in quotes.
Posted by Michael Park <mp...@apache.org>.
> On April 11, 2016, 4:48 p.m., Benjamin Bannier wrote:
> > support/hooks/commit-msg, lines 26-27
> > <https://reviews.apache.org/r/45984/diff/1/?file=1338419#file1338419line26>
> >
> > Not part of this patch, but since we `wc` here we manually need to correct for it counting the newline implicit in the use of `echo` (I am not entirely convinced by the argument here: https://reviews.apache.org/r/45768/#review127198).
> >
> > Following @haosdent's original suggestion of just using shell builtins would not only entirely have eliminated the problem this patch here fixes, but also have led to the same magic max line length number appearing in both the check and the diagnostic.
> >
> > LENGTH=${#LINE}
> > if [ "$LENGTH" -gt "72" ]; then
> > echo >&2 "Error: No line in the commit message summary may exceed 72 characters."
> > fi
We use `echo -n` everywhere else except here currently. https://reviews.apache.org/r/45985 changes it to `echo -n`.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45984/#review128158
-----------------------------------------------------------
On April 11, 2016, 5:45 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45984/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 5:45 p.m.)
>
>
> Review request for mesos, Joerg Schad, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5162
> https://issues.apache.org/jira/browse/MESOS-5162
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Some symbols, e.g., '*' can be expanded by the shell altering the
> original line and hence leading to false positives.
>
>
> Diffs
> -----
>
> support/hooks/commit-msg d3f10c886a654aa8d8364faddf2e50dc7fe82058
>
> Diff: https://reviews.apache.org/r/45984/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 45984: Fixed the commit message hook to wrap the
variables in quotes.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45984/#review128158
-----------------------------------------------------------
Ship it!
support/hooks/commit-msg (lines 26 - 27)
<https://reviews.apache.org/r/45984/#comment191521>
Not part of this patch, but since we `wc` here we manually need to correct for it counting the newline implicit in the use of `echo` (I am not entirely convinced by the argument here: https://reviews.apache.org/r/45768/#review127198).
Following @haosdent's original suggestion of just using shell builtins would not only entirely have eliminated the problem this patch here fixes, but also have led to the same magic max line length number appearing in both the check and the diagnostic.
LENGTH=${#LINE}
if [ "$LENGTH" -gt "72" ]; then
echo >&2 "Error: No line in the commit message summary may exceed 72 characters."
fi
- Benjamin Bannier
On April 10, 2016, 8:41 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45984/
> -----------------------------------------------------------
>
> (Updated April 10, 2016, 8:41 a.m.)
>
>
> Review request for mesos, Joerg Schad, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5162
> https://issues.apache.org/jira/browse/MESOS-5162
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> support/hooks/commit-msg d3f10c886a654aa8d8364faddf2e50dc7fe82058
>
> Diff: https://reviews.apache.org/r/45984/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 45984: Fixed the commit message hook to wrap the
variables in quotes.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45984/#review128130
-----------------------------------------------------------
Ship it!
Ship It!
- Joerg Schad
On April 10, 2016, 6:41 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45984/
> -----------------------------------------------------------
>
> (Updated April 10, 2016, 6:41 a.m.)
>
>
> Review request for mesos, Joerg Schad, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5162
> https://issues.apache.org/jira/browse/MESOS-5162
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> support/hooks/commit-msg d3f10c886a654aa8d8364faddf2e50dc7fe82058
>
> Diff: https://reviews.apache.org/r/45984/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 45984: Fixed the commit message hook to wrap the
variables in quotes.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45984/
-----------------------------------------------------------
(Updated April 11, 2016, 5:45 p.m.)
Review request for mesos, Joerg Schad, Kevin Klues, and Vinod Kone.
Bugs: MESOS-5162
https://issues.apache.org/jira/browse/MESOS-5162
Repository: mesos
Description (updated)
-------
Some symbols, e.g., '*' can be expanded by the shell altering the
original line and hence leading to false positives.
Diffs
-----
support/hooks/commit-msg d3f10c886a654aa8d8364faddf2e50dc7fe82058
Diff: https://reviews.apache.org/r/45984/diff/
Testing
-------
Thanks,
Michael Park
Re: Review Request 45984: Fixed the commit message hook to wrap the
variables in quotes.
Posted by Michael Park <mp...@apache.org>.
> On April 11, 2016, 3:19 p.m., Alexander Rukletsov wrote:
> > Could you please briefly explain the problem with the globbing in the description?
> >
> > Btw, I was unaware of that patch and filed https://reviews.apache.org/r/46034/ . I'll discard it in favour of your patch.
I had a more detailed description in the JIRA ticket, but I've copied the description from your review.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45984/#review128134
-----------------------------------------------------------
On April 11, 2016, 5:45 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45984/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 5:45 p.m.)
>
>
> Review request for mesos, Joerg Schad, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5162
> https://issues.apache.org/jira/browse/MESOS-5162
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Some symbols, e.g., '*' can be expanded by the shell altering the
> original line and hence leading to false positives.
>
>
> Diffs
> -----
>
> support/hooks/commit-msg d3f10c886a654aa8d8364faddf2e50dc7fe82058
>
> Diff: https://reviews.apache.org/r/45984/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 45984: Fixed the commit message hook to wrap the
variables in quotes.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45984/#review128134
-----------------------------------------------------------
Ship it!
Could you please briefly explain the problem with the globbing in the description?
Btw, I was unaware of that patch and filed https://reviews.apache.org/r/46034/ . I'll discard it in favour of your patch.
- Alexander Rukletsov
On April 10, 2016, 6:41 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45984/
> -----------------------------------------------------------
>
> (Updated April 10, 2016, 6:41 a.m.)
>
>
> Review request for mesos, Joerg Schad, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-5162
> https://issues.apache.org/jira/browse/MESOS-5162
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> support/hooks/commit-msg d3f10c886a654aa8d8364faddf2e50dc7fe82058
>
> Diff: https://reviews.apache.org/r/45984/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>