Re: [gentoo-portage-dev] [PATCH v2 1/3] repoman: Add commit message verification

[Michał Górny]

W dniu sob, 17.02.2018 o godzinie 13∶56 +0100, użytkownik Michał Górny
napisał:
> Add a check for common mistakes in commit messages. For now, it is
> pretty rough and works only for -m/-F. It will be extended to work
> in the interactive mode in the future.
> ---
>  repoman/pym/repoman/actions.py  | 74 
> -
>  repoman/pym/repoman/tests/commit/__init__.py|  2 +
>  repoman/pym/repoman/tests/commit/__test__.py|  1 +

Oh, great. Now I see that I haven't added the new tests, and the file is
gone now, and now I'm going to waste another hour writing them all
again...

>  repoman/pym/repoman/tests/simple/test_simple.py |  8 +--
>  4 files changed, 80 insertions(+), 5 deletions(-)
>  create mode 100644 repoman/pym/repoman/tests/commit/__init__.py
>  create mode 100644 repoman/pym/repoman/tests/commit/__test__.py
> 
> diff --git a/repoman/pym/repoman/actions.py b/repoman/pym/repoman/actions.py
> index b76a6e466..91603865c 100644
> --- a/repoman/pym/repoman/actions.py
> +++ b/repoman/pym/repoman/actions.py
> @@ -119,7 +119,16 @@ class Actions(object):
>   if commitmessage[:9].lower() in ("cat/pkg: ",):
>   commitmessage = self.msg_prefix() + 
> commitmessage[9:]
>  
> - if not commitmessage or not commitmessage.strip():
> + if commitmessage is not None and commitmessage.strip():
> + 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)
> + else:
>   commitmessage = self.get_new_commit_message(qa_output)
>  
>   commitmessage = commitmessage.rstrip()
> @@ -585,3 +594,66 @@ class Actions(object):
>   print("* no commit message?  aborting commit.")
>   sys.exit(1)
>   return commitmessage
> +
> + footer_re = re.compile(r'^[\w-]+:')
> +
> + @classmethod
> + def verify_commit_message(cls, 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 ':' not in summary:
> + problems.append('summary line must start with a logical 
> unit name, e.g. "cat/pkg:"')
> + if '\n' in summary.strip():
> + problems.append('commit message must start with a 
> *single* line of summary, followed by empty line')
> + # accept 69 overall or unit+50, in case of very long package 
> names
> + elif 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(cls.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:
> + # we recommend wrapping at 72 but 
> accept up to 80;
> + # do not complain if we have single 
> word (usually
> + # it means very long URL)
> + if len(l.strip()) > 80 and 
> len(l.split()) > 1:
> + body_too_long = True
> +
> + 

Re: [gentoo-portage-dev] [PATCH v2 1/3] repoman: Add commit message verification

[Michał Górny]

W dniu nie, 18.02.2018 o godzinie 09∶11 -0800, użytkownik Brian Dolbec
napisał:
> On Sat, 17 Feb 2018 13:56:46 +0100
> Michał Górny  wrote:
> 
> > +
> > +   footer_re = re.compile(r'^[\w-]+:')
> > +
> > +   @classmethod
> > +   def verify_commit_message(cls, msg):
> 
> ...
> > +  if all(cls.footer_re.match(l) for l in lines if l.strip()):
> 
> 
> Why declare the footer_re in the class space?  It is only used in this
> new function.  Then the function could be @staticmethod instead.  I
> don't see the advantage of it this way.
> 
> If it is for re-use in other possible check functions, then perhaps it
> would be best to split this out to it's own class/file that can be
> added to easily.  And just run it from the Actions class as M.J.
> Everitt suggested.

I've declared it outside the function to not have to re-compile it every
time the function is run (well, yes, now that I think of it, it won't
happen more than once most of the time...). Used class space to avoid
moving it too far from code.

-- 
Best regards,
Michał Górny

Re: [gentoo-portage-dev] [PATCH v2 1/3] repoman: Add commit message verification

[Brian Dolbec]

On Sat, 17 Feb 2018 13:56:46 +0100
Michał Górny  wrote:

> +
> + footer_re = re.compile(r'^[\w-]+:')
> +
> + @classmethod
> + def verify_commit_message(cls, msg):
...
> +  if all(cls.footer_re.match(l) for l in lines if l.strip()):


Why declare the footer_re in the class space?  It is only used in this
new function.  Then the function could be @staticmethod instead.  I
don't see the advantage of it this way.

If it is for re-use in other possible check functions, then perhaps it
would be best to split this out to it's own class/file that can be
added to easily.  And just run it from the Actions class as M.J.
Everitt suggested.
-- 
Brian Dolbec 

Re: [gentoo-portage-dev] [PATCH v2 1/3] repoman: Add commit message verification

[M. J. Everitt]

On 18/02/18 16:28, Brian Dolbec wrote:
> On Sat, 17 Feb 2018 13:18:03 +
> "M. J. Everitt"  wrote:
>
>> Might I suggest breaking out checks into a separate module? I think
>> that hard-coding it all is likely to become a pain as time goes on,
>> more checks get added, etc  ...
>>
> So, you would like the commit message checks to be  a plug-in system
> like the ebuild checks are now.
>
> Yes that is possible,  that would also allow overlays and sub-distros
> to customize it to their liking.  But you do realize it would be a
> module system with only the one plug-in.  It would add a slight amount
> of overhead and subsequently a tiny bit more time.  Although it is not
> much as I remember when I first developed the plug-in system for emaint.
>
> I will consider that for the stage3 work in progress, but for the
> master branch of repoman, the hard-coded version above would be fine.
>
That's probably a better implementation, but for the time being I was
thinking more of an 'actions_commit' module to break out the extra code
from the generic 'actions' module.
Regards,
Michael/veremitz.



signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH v2 1/3] repoman: Add commit message verification

[Brian Dolbec]

On Sat, 17 Feb 2018 13:18:03 +
"M. J. Everitt"  wrote:

> On 17/02/18 12:56, Michał Górny wrote:
> > Add a check for common mistakes in commit messages. For now, it is
> > pretty rough and works only for -m/-F. It will be extended to work
> > in the interactive mode in the future.
> > ---
> >  repoman/pym/repoman/actions.py  | 74
> > -
> > repoman/pym/repoman/tests/commit/__init__.py|  2 +
> > repoman/pym/repoman/tests/commit/__test__.py|  1 +
> > repoman/pym/repoman/tests/simple/test_simple.py |  8 +-- 4 files
> > changed, 80 insertions(+), 5 deletions(-) create mode 100644
> > repoman/pym/repoman/tests/commit/__init__.py create mode 100644
> > repoman/pym/repoman/tests/commit/__test__.py
> >
> > diff --git a/repoman/pym/repoman/actions.py
> > b/repoman/pym/repoman/actions.py index b76a6e466..91603865c 100644
> > --- a/repoman/pym/repoman/actions.py
> > +++ b/repoman/pym/repoman/actions.py
> > @@ -119,7 +119,16 @@ class Actions(object):
> > if commitmessage[:9].lower() in ("cat/pkg:
> > ",): commitmessage = self.msg_prefix() + commitmessage[9:]
> >  
> > -   if not commitmessage or not commitmessage.strip():
> > +   if commitmessage is not None and
> > commitmessage.strip():
> > +   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)
> > +   else:
> > commitmessage =
> > self.get_new_commit_message(qa_output) 
> > commitmessage = commitmessage.rstrip()
> > @@ -585,3 +594,66 @@ class Actions(object):
> > print("* no commit message?  aborting
> > commit.") sys.exit(1)
> > return commitmessage
> > +
> > +   footer_re = re.compile(r'^[\w-]+:')
> > +
> > +   @classmethod
> > +   def verify_commit_message(cls, 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 ':' not in summary:
> > +   problems.append('summary line must start
> > with a logical unit name, e.g. "cat/pkg:"')
> > +   if '\n' in summary.strip():
> > +   problems.append('commit message must start
> > with a *single* line of summary, followed by empty line')
> > +   # accept 69 overall or unit+50, in case of very
> > long package names
> > +   elif 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(cls.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:
> > +   # we recommend wrapping at
> > 72 but accept up to 80;
> > +   # do not complain if we
> > have single word (usually
> > +   # it means very long URL)
> > +   if len(l.strip()) > 80 and
> > len(l.split()) > 1:
> > +   body_too_long =
> > True +
> > +   if multiple_footers:
> > +   problems.append('multiple footer-style
> > blocks found, please combine them into 

Re: [gentoo-portage-dev] [PATCH v2 1/3] repoman: Add commit message verification

[M. J. Everitt]

On 17/02/18 12:56, Michał Górny wrote:
> Add a check for common mistakes in commit messages. For now, it is
> pretty rough and works only for -m/-F. It will be extended to work
> in the interactive mode in the future.
> ---
>  repoman/pym/repoman/actions.py  | 74 
> -
>  repoman/pym/repoman/tests/commit/__init__.py|  2 +
>  repoman/pym/repoman/tests/commit/__test__.py|  1 +
>  repoman/pym/repoman/tests/simple/test_simple.py |  8 +--
>  4 files changed, 80 insertions(+), 5 deletions(-)
>  create mode 100644 repoman/pym/repoman/tests/commit/__init__.py
>  create mode 100644 repoman/pym/repoman/tests/commit/__test__.py
>
> diff --git a/repoman/pym/repoman/actions.py b/repoman/pym/repoman/actions.py
> index b76a6e466..91603865c 100644
> --- a/repoman/pym/repoman/actions.py
> +++ b/repoman/pym/repoman/actions.py
> @@ -119,7 +119,16 @@ class Actions(object):
>   if commitmessage[:9].lower() in ("cat/pkg: ",):
>   commitmessage = self.msg_prefix() + 
> commitmessage[9:]
>  
> - if not commitmessage or not commitmessage.strip():
> + if commitmessage is not None and commitmessage.strip():
> + 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)
> + else:
>   commitmessage = self.get_new_commit_message(qa_output)
>  
>   commitmessage = commitmessage.rstrip()
> @@ -585,3 +594,66 @@ class Actions(object):
>   print("* no commit message?  aborting commit.")
>   sys.exit(1)
>   return commitmessage
> +
> + footer_re = re.compile(r'^[\w-]+:')
> +
> + @classmethod
> + def verify_commit_message(cls, 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 ':' not in summary:
> + problems.append('summary line must start with a logical 
> unit name, e.g. "cat/pkg:"')
> + if '\n' in summary.strip():
> + problems.append('commit message must start with a 
> *single* line of summary, followed by empty line')
> + # accept 69 overall or unit+50, in case of very long package 
> names
> + elif 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(cls.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:
> + # we recommend wrapping at 72 but 
> accept up to 80;
> + # do not complain if we have single 
> word (usually
> + # it means very long URL)
> + if len(l.strip()) > 80 and 
> len(l.split()) > 1:
> + body_too_long = True
> +
> + if multiple_footers:
> + problems.append('multiple footer-style blocks found, 
> please combine them into one')
> + if gentoo_bug_used:
> +