Re: [gentoo-portage-dev] [PATCH] repoman: Add commit message verification
W dniu nie, 04.02.2018 o godzinie 22∶15 +0100, użytkownik Ulrich Mueller napisał: > > > > > > On Sun, 04 Feb 2018, Michał Górny wrote: > > W dniu sob, 03.02.2018 o godzinie 09∶58 +0100, użytkownik Ulrich Mueller > > napisał: > > > > Add a check for common mistakes in commit messages. For now, it > > > > is pretty rough and exits immediately but it should be integrated > > > > with the editor in the future. > > > > > > Have you tested this against existing commits in the gentoo repo? > > After checking the 1 most recent commits, I have the following > > stats on invalid commit messages: > > [...] > > Most of the violations are uses of Gentoo-Bug and very long > > single-line commit messages; most likely people using '-m "very long > > message because I am lazy and can't use editor properly"'. > > Full list of violations in the sample: > > https://gist.github.com/mgorny/fa81ffe07ca9565198e4e44fb040bb19 > > IMHO this shows that some of the tests are too rigid. Especially, > "body lines should be wrapped at 72 characters" often triggers in > cases where rewrapping wouldn't improve readability. > > Since GLEP 66 says that "the body *should* be wrapped at 72 > characters" (my emphasis), I would suggest to relax this test a bit > and only flag body lines longer than 80 characters. WFM. > Also, any lines > containing long URIs should be excluded from the test. > For that specific purpose, I've excluded footer from test. Maybe I'll also be able to exclude long lines that look like URL. -- Best regards, Michał Górny
Re: [gentoo-portage-dev] [PATCH] repoman: Add commit message verification
> On Sun, 04 Feb 2018, Michał Górny wrote: > W dniu sob, 03.02.2018 o godzinie 09∶58 +0100, użytkownik Ulrich Mueller > napisał: >> > Add a check for common mistakes in commit messages. For now, it >> > is pretty rough and exits immediately but it should be integrated >> > with the editor in the future. >> >> Have you tested this against existing commits in the gentoo repo? > After checking the 1 most recent commits, I have the following > stats on invalid commit messages: > [...] > Most of the violations are uses of Gentoo-Bug and very long > single-line commit messages; most likely people using '-m "very long > message because I am lazy and can't use editor properly"'. > Full list of violations in the sample: > https://gist.github.com/mgorny/fa81ffe07ca9565198e4e44fb040bb19 IMHO this shows that some of the tests are too rigid. Especially, "body lines should be wrapped at 72 characters" often triggers in cases where rewrapping wouldn't improve readability. Since GLEP 66 says that "the body *should* be wrapped at 72 characters" (my emphasis), I would suggest to relax this test a bit and only flag body lines longer than 80 characters. Also, any lines containing long URIs should be excluded from the test. Ulrich pgpfHhBhi2Jyg.pgp Description: PGP signature
Re: [gentoo-portage-dev] [PATCH] repoman: Add commit message verification
W dniu sob, 03.02.2018 o godzinie 09∶58 +0100, użytkownik Ulrich Mueller napisał: > > > > > > On Fri, 2 Feb 2018, Michał Górny wrote: > > Add a check for common mistakes in commit messages. For now, it is > > pretty rough and exits immediately but it should be integrated with > > the editor in the future. > > Have you tested this against existing commits in the gentoo repo? > After checking the 1 most recent commits, I have the following stats on invalid commit messages: 138 klaus...@gentoo.org 99 sly...@gentoo.org 61 l...@gentoo.org 20 dilfri...@gentoo.org 18 monsie...@gentoo.org 15 pa...@gentoo.org 15 whi...@gentoo.org 13 ali...@gentoo.org 12 flop...@gentoo.org 12 x...@gentoo.org 12 j...@gentoo.org 11 a...@gentoo.org 11 amy...@gentoo.org 10 polynomia...@gentoo.org ... Most of the violations are uses of Gentoo-Bug and very long single-line commit messages; most likely people using '-m "very long message because I am lazy and can't use editor properly"'. Full list of violations in the sample: https://gist.github.com/mgorny/fa81ffe07ca9565198e4e44fb040bb19 -- Best regards, Michał Górny
Re: [gentoo-portage-dev] [PATCH] repoman: Add commit message verification
On Sun, 04 Feb 2018 18:58:21 +0100 Michał Górny wrote: > W dniu nie, 04.02.2018 o godzinie 09∶51 -0800, użytkownik Brian Dolbec > napisał: > > > > I know there are not a lot of repoman unit tests, but, can you > > please create some to test this? That way it doesn't get any worse. > > > > In the PR, you mentioned adding in an editor call to re-edit the > > message. This would be a good thing and I know is not that > > difficult to accomplish. Is this coming in another patch? > > > > I'm still wondering how to implement it best. In particular, how to > handle the non-EDITOR (stdin) branch? > hmm, yeah, that was not an option for the work tool I wrote and added the re-edit to. It had 2 files it needed to edit or re-edit. In the case of passing in a pre-saved message, that should be left for the user to fix. Perhaps in the case of a stdin -m "foo" message we could just let the user rely on bash their history to bring up that previous command, to edit before hitting enter again. -- Brian Dolbec
Re: [gentoo-portage-dev] [PATCH] repoman: Add commit message verification
W dniu nie, 04.02.2018 o godzinie 09∶51 -0800, użytkownik Brian Dolbec
napisał:
> On Fri, 2 Feb 2018 23:53:05 +0100
> Michał Górny wrote:
>
> > Add a check for common mistakes in commit messages. For now, it is
> > pretty rough and exits immediately but it should be integrated with
> > the editor in the future.
> > ---
> > repoman/pym/repoman/actions.py | 68
> > ++ 1 file changed, 68
> > insertions(+)
> >
> > diff --git a/repoman/pym/repoman/actions.py
> > b/repoman/pym/repoman/actions.py index b76a6e466..97a71d0f5 100644
> > --- a/repoman/pym/repoman/actions.py
> > +++ b/repoman/pym/repoman/actions.py
> > @@ -124,6 +124,15 @@ class Actions(object):
> >
> > commitmessage = commitmessage.rstrip()
> >
> > + res, expl = self.verify_commit_message(commitmessage)
> > + if not res:
> > + print(bad("RepoMan does not like your commit
> > message:"))
> > + print(expl)
> > + if self.options.force:
> > + print('(but proceeding due to
> > --force)')
> > + else:
> > + sys.exit(1)
> > +
> > # Update copyright for new and changed files
> > year = time.strftime('%Y', time.gmtime())
> > for fn in chain(mynew, mychanged):
> > @@ -585,3 +594,62 @@ class Actions(object):
> > print("* no commit message? aborting
> > commit.") sys.exit(1)
> > return commitmessage
> > +
> > + footer_re = re.compile(r'^[\w-]+:')
> > +
> > + def verify_commit_message(self, msg):
> > + """
> > + Check whether the message roughly complies with
> > GLEP66. Returns
> > + (True, None) if it does, (False, ) if
> > it does not.
> > + """
> > +
> > + problems = []
> > + paras = msg.strip().split('\n\n')
> > + summary = paras.pop(0)
> > +
> > + if '\n' in summary.strip():
> > + problems.append('commit message must start
> > with a *single* line of summary, followed by empty line')
> > + if ':' not in summary:
> > + problems.append('summary line must start
> > with a logical unit name, e.g. "cat/pkg:"')
> > + # accept 69 overall or unit+50, in case of very long
> > package names
> > + if len(summary.strip()) > 69 and
> > len(summary.split(':', 1)[-1]) > 50:
> > + problems.append('summary line is too long
> > (max 69 characters)') +
> > + multiple_footers = False
> > + gentoo_bug_used = False
> > + bug_closes_without_url = False
> > + body_too_long = False
> > +
> > + found_footer = False
> > + for p in paras:
> > + lines = p.splitlines()
> > + # if all lines look like footer material, we
> > assume it's footer
> > + # else, we assume it's body text
> > + if all(self.footer_re.match(l) for l in
> > lines if l.strip()):
> > + # multiple footer-like blocks?
> > + if found_footer:
> > + multiple_footers = True
> > + found_footer = True
> > + for l in lines:
> > + if
> > l.startswith('Gentoo-Bug'):
> > + gentoo_bug_used =
> > True
> > + elif l.startswith('Bug:') or
> > l.startswith('Closes:'):
> > + if 'http://' not in
> > l and 'https://' not in l:
> > +
> > bug_closes_without_url = True
> > + else:
> > + for l in lines:
> > + if len(l.strip()) > 72:
> > + body_too_long = True
> > +
> > + if multiple_footers:
> > + problems.append('multiple footer-style
> > blocks found, please combine them into one')
> > + if gentoo_bug_used:
> > + problems.append('please replace Gentoo-Bug
> > with GLEP 66-compliant Bug/Closes')
> > + if bug_closes_without_url:
> > + problems.append('Bug/Closes footers require
> > full URL')
> > + if body_too_long:
> > + problems.append('body lines should be
> > wrapped at 72 characters') +
> > + if problems:
> > + return (False, '\n'.join('- %s' % x for x in
> > problems))
> > + return (True, None)
>
>
> I know there are not a lot of repoman unit tests, but, can you please
> create some to test this? That way it doesn't get any worse.
>
> In the PR, you mentioned adding in an editor call to re-edit the
> message. This would be a good thing and I know is not that difficult
> to accomplish. Is this coming in another patch?
>
Re: [gentoo-portage-dev] [PATCH] repoman: Add commit message verification
On Fri, 2 Feb 2018 23:53:05 +0100
Michał Górny wrote:
> Add a check for common mistakes in commit messages. For now, it is
> pretty rough and exits immediately but it should be integrated with
> the editor in the future.
> ---
> repoman/pym/repoman/actions.py | 68
> ++ 1 file changed, 68
> insertions(+)
>
> diff --git a/repoman/pym/repoman/actions.py
> b/repoman/pym/repoman/actions.py index b76a6e466..97a71d0f5 100644
> --- a/repoman/pym/repoman/actions.py
> +++ b/repoman/pym/repoman/actions.py
> @@ -124,6 +124,15 @@ class Actions(object):
>
> commitmessage = commitmessage.rstrip()
>
> + res, expl = self.verify_commit_message(commitmessage)
> + if not res:
> + print(bad("RepoMan does not like your commit
> message:"))
> + print(expl)
> + if self.options.force:
> + print('(but proceeding due to
> --force)')
> + else:
> + sys.exit(1)
> +
> # Update copyright for new and changed files
> year = time.strftime('%Y', time.gmtime())
> for fn in chain(mynew, mychanged):
> @@ -585,3 +594,62 @@ class Actions(object):
> print("* no commit message? aborting
> commit.") sys.exit(1)
> return commitmessage
> +
> + footer_re = re.compile(r'^[\w-]+:')
> +
> + def verify_commit_message(self, msg):
> + """
> + Check whether the message roughly complies with
> GLEP66. Returns
> + (True, None) if it does, (False, ) if
> it does not.
> + """
> +
> + problems = []
> + paras = msg.strip().split('\n\n')
> + summary = paras.pop(0)
> +
> + if '\n' in summary.strip():
> + problems.append('commit message must start
> with a *single* line of summary, followed by empty line')
> + if ':' not in summary:
> + problems.append('summary line must start
> with a logical unit name, e.g. "cat/pkg:"')
> + # accept 69 overall or unit+50, in case of very long
> package names
> + if len(summary.strip()) > 69 and
> len(summary.split(':', 1)[-1]) > 50:
> + problems.append('summary line is too long
> (max 69 characters)') +
> + multiple_footers = False
> + gentoo_bug_used = False
> + bug_closes_without_url = False
> + body_too_long = False
> +
> + found_footer = False
> + for p in paras:
> + lines = p.splitlines()
> + # if all lines look like footer material, we
> assume it's footer
> + # else, we assume it's body text
> + if all(self.footer_re.match(l) for l in
> lines if l.strip()):
> + # multiple footer-like blocks?
> + if found_footer:
> + multiple_footers = True
> + found_footer = True
> + for l in lines:
> + if
> l.startswith('Gentoo-Bug'):
> + gentoo_bug_used =
> True
> + elif l.startswith('Bug:') or
> l.startswith('Closes:'):
> + if 'http://' not in
> l and 'https://' not in l:
> +
> bug_closes_without_url = True
> + else:
> + for l in lines:
> + if len(l.strip()) > 72:
> + body_too_long = True
> +
> + if multiple_footers:
> + problems.append('multiple footer-style
> blocks found, please combine them into one')
> + if gentoo_bug_used:
> + problems.append('please replace Gentoo-Bug
> with GLEP 66-compliant Bug/Closes')
> + if bug_closes_without_url:
> + problems.append('Bug/Closes footers require
> full URL')
> + if body_too_long:
> + problems.append('body lines should be
> wrapped at 72 characters') +
> + if problems:
> + return (False, '\n'.join('- %s' % x for x in
> problems))
> + return (True, None)
I know there are not a lot of repoman unit tests, but, can you please
create some to test this? That way it doesn't get any worse.
In the PR, you mentioned adding in an editor call to re-edit the
message. This would be a good thing and I know is not that difficult
to accomplish. Is this coming in another patch?
--
Brian Dolbec
Re: [gentoo-portage-dev] [PATCH] repoman: Add commit message verification
W dniu sob, 03.02.2018 o godzinie 09∶58 +0100, użytkownik Ulrich Mueller napisał: > > > > > > On Fri, 2 Feb 2018, Michał Górny wrote: > > Add a check for common mistakes in commit messages. For now, it is > > pretty rough and exits immediately but it should be integrated with > > the editor in the future. > > Have you tested this against existing commits in the gentoo repo? > Nah, just hand-tested all of the conditionals. -- Best regards, Michał Górny
Re: [gentoo-portage-dev] [PATCH] repoman: Add commit message verification
> On Fri, 2 Feb 2018, Michał Górny wrote: > Add a check for common mistakes in commit messages. For now, it is > pretty rough and exits immediately but it should be integrated with > the editor in the future. Have you tested this against existing commits in the gentoo repo? Ulrich pgp9WrL5Q8VBT.pgp Description: PGP signature
[gentoo-portage-dev] [PATCH] repoman: Add commit message verification
Add a check for common mistakes in commit messages. For now, it is
pretty rough and exits immediately but it should be integrated with
the editor in the future.
---
repoman/pym/repoman/actions.py | 68 ++
1 file changed, 68 insertions(+)
diff --git a/repoman/pym/repoman/actions.py b/repoman/pym/repoman/actions.py
index b76a6e466..97a71d0f5 100644
--- a/repoman/pym/repoman/actions.py
+++ b/repoman/pym/repoman/actions.py
@@ -124,6 +124,15 @@ class Actions(object):
commitmessage = commitmessage.rstrip()
+ res, expl = self.verify_commit_message(commitmessage)
+ if not res:
+ print(bad("RepoMan does not like your commit message:"))
+ print(expl)
+ if self.options.force:
+ print('(but proceeding due to --force)')
+ else:
+ sys.exit(1)
+
# Update copyright for new and changed files
year = time.strftime('%Y', time.gmtime())
for fn in chain(mynew, mychanged):
@@ -585,3 +594,62 @@ class Actions(object):
print("* no commit message? aborting commit.")
sys.exit(1)
return commitmessage
+
+ footer_re = re.compile(r'^[\w-]+:')
+
+ def verify_commit_message(self, msg):
+ """
+ Check whether the message roughly complies with GLEP66. Returns
+ (True, None) if it does, (False, ) if it does not.
+ """
+
+ problems = []
+ paras = msg.strip().split('\n\n')
+ summary = paras.pop(0)
+
+ if '\n' in summary.strip():
+ problems.append('commit message must start with a
*single* line of summary, followed by empty line')
+ if ':' not in summary:
+ problems.append('summary line must start with a logical
unit name, e.g. "cat/pkg:"')
+ # accept 69 overall or unit+50, in case of very long package
names
+ if len(summary.strip()) > 69 and len(summary.split(':', 1)[-1])
> 50:
+ problems.append('summary line is too long (max 69
characters)')
+
+ multiple_footers = False
+ gentoo_bug_used = False
+ bug_closes_without_url = False
+ body_too_long = False
+
+ found_footer = False
+ for p in paras:
+ lines = p.splitlines()
+ # if all lines look like footer material, we assume
it's footer
+ # else, we assume it's body text
+ if all(self.footer_re.match(l) for l in lines if
l.strip()):
+ # multiple footer-like blocks?
+ if found_footer:
+ multiple_footers = True
+ found_footer = True
+ for l in lines:
+ if l.startswith('Gentoo-Bug'):
+ gentoo_bug_used = True
+ elif l.startswith('Bug:') or
l.startswith('Closes:'):
+ if 'http://' not in l and
'https://' not in l:
+ bug_closes_without_url
= True
+ else:
+ for l in lines:
+ if len(l.strip()) > 72:
+ body_too_long = True
+
+ if multiple_footers:
+ problems.append('multiple footer-style blocks found,
please combine them into one')
+ if gentoo_bug_used:
+ problems.append('please replace Gentoo-Bug with GLEP
66-compliant Bug/Closes')
+ if bug_closes_without_url:
+ problems.append('Bug/Closes footers require full URL')
+ if body_too_long:
+ problems.append('body lines should be wrapped at 72
characters')
+
+ if problems:
+ return (False, '\n'.join('- %s' % x for x in problems))
+ return (True, None)
--
2.16.1
