Patchwork [RFC] check-commit: add magic string to bypass check-commit

login
register
mail settings
Submitter David Soria Parra
Date Dec. 21, 2016, 4:05 a.m.
Message ID <ff1014e78d5a3084eda3.1482293109@devbig415.prn1.facebook.com>
Download mbox | patch
Permalink /patch/17986/
State Changes Requested
Headers show

Comments

David Soria Parra - Dec. 21, 2016, 4:05 a.m.
# HG changeset patch
# User David Soria Parra <davidsp@fb.com>
# Date 1482293000 28800
#      Tue Dec 20 20:03:20 2016 -0800
# Node ID ff1014e78d5a3084eda379ec51b1c0a1fc4f5b2a
# Parent  11deeb8980886f1bd45c56e8ac3ab662b4fdb252
check-commit: add magic string to bypass check-commit

Allow bypassing check-commit runs by specifying '# no-check-commit' at the
beginning of a line. This should be avoided but is useful for upstream imports
such as pywatchman which will cause check-code to fail otherwise.
Pierre-Yves David - Dec. 21, 2016, 11:36 a.m.
On 12/21/2016 05:05 AM, David Soria Parra wrote:
> # HG changeset patch
> # User David Soria Parra <davidsp@fb.com>
> # Date 1482293000 28800
> #      Tue Dec 20 20:03:20 2016 -0800
> # Node ID ff1014e78d5a3084eda379ec51b1c0a1fc4f5b2a
> # Parent  11deeb8980886f1bd45c56e8ac3ab662b4fdb252
> check-commit: add magic string to bypass check-commit
>
> Allow bypassing check-commit runs by specifying '# no-check-commit' at the
> beginning of a line. This should be avoided but is useful for upstream imports
> such as pywatchman which will cause check-code to fail otherwise.

I would prefer to have that exclusion logic in 'test-check-commit.t', 
These commit still fails our commit checks, but we skip them from 
automated testing.

Just add "and not desc('#no-check-commit')" to the revset there
https://www.mercurial-scm.org/repo/hg/file/tip/tests/test-check-commit.t#l11

> diff --git a/contrib/check-commit b/contrib/check-commit
> --- a/contrib/check-commit
> +++ b/contrib/check-commit
> @@ -25,6 +25,7 @@
>  afterheader = commitheader + r"(?!#)"
>  beforepatch = afterheader + r"(?!\n(?!@@))"
>
> +bypass = r"^# no-check-commit"
>  errors = [
>      (beforepatch + r".*[(]bc[)]", "(BC) needs to be uppercase"),
>      (beforepatch + r".*[(]issue \d\d\d",
> @@ -59,6 +60,9 @@
>      exitcode = 0
>      printed = node is None
>      hits = []
> +    if re.search(bypass, commit, re.MULTILINE):
> +        return 0
> +
>      for exp, msg in errors:
>          for m in re.finditer(exp, commit):
>              end = m.end()

Cheers,
Pierre-Yves David - Dec. 21, 2016, 11:36 a.m.
On 12/21/2016 12:36 PM, Pierre-Yves David wrote:
>
>
> On 12/21/2016 05:05 AM, David Soria Parra wrote:
>> # HG changeset patch
>> # User David Soria Parra <davidsp@fb.com>
>> # Date 1482293000 28800
>> #      Tue Dec 20 20:03:20 2016 -0800
>> # Node ID ff1014e78d5a3084eda379ec51b1c0a1fc4f5b2a
>> # Parent  11deeb8980886f1bd45c56e8ac3ab662b4fdb252
>> check-commit: add magic string to bypass check-commit
>>
>> Allow bypassing check-commit runs by specifying '# no-check-commit' at
>> the
>> beginning of a line. This should be avoided but is useful for upstream
>> imports
>> such as pywatchman which will cause check-code to fail otherwise.
>
> I would prefer to have that exclusion logic in 'test-check-commit.t',
> These commit still fails our commit checks, but we skip them from
> automated testing.

But, to be clear, I'm all for this feature otherwise !

> Just add "and not desc('#no-check-commit')" to the revset there
> https://www.mercurial-scm.org/repo/hg/file/tip/tests/test-check-commit.t#l11
>
>
>> diff --git a/contrib/check-commit b/contrib/check-commit
>> --- a/contrib/check-commit
>> +++ b/contrib/check-commit
>> @@ -25,6 +25,7 @@
>>  afterheader = commitheader + r"(?!#)"
>>  beforepatch = afterheader + r"(?!\n(?!@@))"
>>
>> +bypass = r"^# no-check-commit"
>>  errors = [
>>      (beforepatch + r".*[(]bc[)]", "(BC) needs to be uppercase"),
>>      (beforepatch + r".*[(]issue \d\d\d",
>> @@ -59,6 +60,9 @@
>>      exitcode = 0
>>      printed = node is None
>>      hits = []
>> +    if re.search(bypass, commit, re.MULTILINE):
>> +        return 0
>> +
>>      for exp, msg in errors:
>>          for m in re.finditer(exp, commit):
>>              end = m.end()
>
> Cheers,
>
via Mercurial-devel - Dec. 21, 2016, 9:47 p.m.
> -----Original Message-----

> From: Pierre-Yves David [mailto:pierre-yves.david@ens-lyon.org]

> 

> I would prefer to have that exclusion logic in 'test-check-commit.t',

> These commit still fails our commit checks, but we skip them from

> automated testing.

> 

> Just add "and not desc('#no-check-commit')" to the revset there


Seems reasonable, note however it changes the behavior as `# no-check-commit`
can appear anywhere when using `desc()` while my implementation searches for it
at the starting of a line.
Pierre-Yves David - Dec. 21, 2016, 10:22 p.m.
On 12/21/2016 10:47 PM, David Soria Parra wrote:
>
>
>> -----Original Message-----
>> From: Pierre-Yves David [mailto:pierre-yves.david@ens-lyon.org]
>>
>> I would prefer to have that exclusion logic in 'test-check-commit.t',
>> These commit still fails our commit checks, but we skip them from
>> automated testing.
>>
>> Just add "and not desc('#no-check-commit')" to the revset there
>
> Seems reasonable, note however it changes the behavior as `# no-check-commit`
> can appear anywhere when using `desc()` while my implementation searches for it
> at the starting of a line.

The regex to feed 'desc()' with in order to mark start of line only is 
left as an exercise with the reader ;-)

Cheers,
via Mercurial-devel - Dec. 21, 2016, 10:32 p.m.
> On 12/21/2016 10:47 PM, David Soria Parra wrote:

> >

> >

> >> -----Original Message-----

> >> From: Pierre-Yves David [mailto:pierre-yves.david@ens-lyon.org]

> >>

> >> I would prefer to have that exclusion logic in 'test-check-commit.t',

> >> These commit still fails our commit checks, but we skip them from

> >> automated testing.

> >>

> >> Just add "and not desc('#no-check-commit')" to the revset there

> >

> > Seems reasonable, note however it changes the behavior as `# no-check-

> commit`

> > can appear anywhere when using `desc()` while my implementation

> searches for it

> > at the starting of a line.

> 

> The regex to feed 'desc()' with in order to mark start of line only is

> left as an exercise with the reader ;-)


Please read the code before making bold statements:

From hg help revsets
    "desc(string)"
      Search commit message for string. The match is case-insensitive.

Note "string" not pattern. If we look into the implementation:

  @predicate('desc(string)', safe=True)
  def desc(repo, subset, x):
      # ...
      def matches(x):
          c = repo[x]
          return ds in encoding.lower(c.description())

note `ds in encoding.lower(c.description())`. Maybe my Python is not as strong as yours but last time
I checked this does not support regex unless I am mistaken.

I leave it to the reviewer to decide if we are taking the v1 with proper regex matching on multiline or settle
with desc() which does not support patterns.

-D

Patch

diff --git a/contrib/check-commit b/contrib/check-commit
--- a/contrib/check-commit
+++ b/contrib/check-commit
@@ -25,6 +25,7 @@ 
 afterheader = commitheader + r"(?!#)"
 beforepatch = afterheader + r"(?!\n(?!@@))"
 
+bypass = r"^# no-check-commit"
 errors = [
     (beforepatch + r".*[(]bc[)]", "(BC) needs to be uppercase"),
     (beforepatch + r".*[(]issue \d\d\d",
@@ -59,6 +60,9 @@ 
     exitcode = 0
     printed = node is None
     hits = []
+    if re.search(bypass, commit, re.MULTILINE):
+        return 0
+
     for exp, msg in errors:
         for m in re.finditer(exp, commit):
             end = m.end()