[RFC,v1] utils: checkstyle.py: Accept `Closes` commit trailer
diff mbox series

Message ID 20251029100847.730477-1-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [RFC,v1] utils: checkstyle.py: Accept `Closes` commit trailer
Related show

Commit Message

Barnabás Pőcze Oct. 29, 2025, 10:08 a.m. UTC 
`Closes: <url>` is recognized by gitlab and closes the referenced issue
automatically when a commit is made[0] to the project's default branch.
Now that issues are hosted on gitlab, it will be the preferred way to
reference the fixed issue from a commit, so add it to the checkstlye.py script.

[0]: https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
Two questions remain:

(1) should the regex be stricter? e.g.
    r"https:\/\/gitlab\.freedesktop\.org\/camera\/libcamera\/-\/issues\/[0-9]+"

(2) what should happen to `Bug` ?
---
 utils/checkstyle.py | 1 +
 1 file changed, 1 insertion(+)

--
2.51.1

Comments

Laurent Pinchart Oct. 29, 2025, 10:14 a.m. UTC | #1 
On Wed, Oct 29, 2025 at 11:08:47AM +0100, Barnabás Pőcze wrote:
> `Closes: <url>` is recognized by gitlab and closes the referenced issue
> automatically when a commit is made[0] to the project's default branch.
> Now that issues are hosted on gitlab, it will be the preferred way to
> reference the fixed issue from a commit, so add it to the checkstlye.py script.
> 
> [0]: https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
> Two questions remain:

I read the commit message first, then glanced at the code, and had the
exact same two questions :-)

> (1) should the regex be stricter? e.g.
>     r"https:\/\/gitlab\.freedesktop\.org\/camera\/libcamera\/-\/issues\/[0-9]+"

I don't see a reason not to make it strict.

> (2) what should happen to `Bug` ?

I think we can deprecate it, and drop it from checkstyle.py.

> ---
>  utils/checkstyle.py | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index f6229bbd86..219921bc9d 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -542,6 +542,7 @@ class TrailersChecker(CommitChecker):
>      known_trailers = {
>          'Acked-by': email_regex,
>          'Bug': link_regex,
> +        'Closes': link_regex,
>          'Co-developed-by': email_regex,
>          'Fixes': commit_regex,
>          'Link': link_regex,
Kieran Bingham Oct. 29, 2025, 11:21 a.m. UTC | #2 
Quoting Laurent Pinchart (2025-10-29 10:14:54)
> On Wed, Oct 29, 2025 at 11:08:47AM +0100, Barnabás Pőcze wrote:
> > `Closes: <url>` is recognized by gitlab and closes the referenced issue
> > automatically when a commit is made[0] to the project's default branch.
> > Now that issues are hosted on gitlab, it will be the preferred way to
> > reference the fixed issue from a commit, so add it to the checkstlye.py script.
> > 
> > [0]: https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern
> > 
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> > Two questions remain:
> 
> I read the commit message first, then glanced at the code, and had the
> exact same two questions :-)
> 
> > (1) should the regex be stricter? e.g.
> >     r"https:\/\/gitlab\.freedesktop\.org\/camera\/libcamera\/-\/issues\/[0-9]+"
> 
> I don't see a reason not to make it strict.
> 

Would we ever want to reference that we're closing bugs reported in
other projects ?

Like:

Closes: https://github.com/raspberrypi/libcamera/issues/141

I could imagine having a commit that 'closes' multiple issue reports in
multiple locations.




> > (2) what should happen to `Bug` ?
> 
> I think we can deprecate it, and drop it from checkstyle.py.

I think it can be removed too.



> 
> > ---
> >  utils/checkstyle.py | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > index f6229bbd86..219921bc9d 100755
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -542,6 +542,7 @@ class TrailersChecker(CommitChecker):
> >      known_trailers = {
> >          'Acked-by': email_regex,
> >          'Bug': link_regex,
> > +        'Closes': link_regex,
> >          'Co-developed-by': email_regex,
> >          'Fixes': commit_regex,
> >          'Link': link_regex,
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Kieran Bingham Oct. 30, 2025, 1:31 p.m. UTC | #3 
Quoting Kieran Bingham (2025-10-29 11:21:33)
> Quoting Laurent Pinchart (2025-10-29 10:14:54)
> > On Wed, Oct 29, 2025 at 11:08:47AM +0100, Barnabás Pőcze wrote:
> > > `Closes: <url>` is recognized by gitlab and closes the referenced issue
> > > automatically when a commit is made[0] to the project's default branch.
> > > Now that issues are hosted on gitlab, it will be the preferred way to
> > > reference the fixed issue from a commit, so add it to the checkstlye.py script.
> > > 
> > > [0]: https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern
> > > 
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > ---
> > > Two questions remain:
> > 
> > I read the commit message first, then glanced at the code, and had the
> > exact same two questions :-)
> > 
> > > (1) should the regex be stricter? e.g.
> > >     r"https:\/\/gitlab\.freedesktop\.org\/camera\/libcamera\/-\/issues\/[0-9]+"
> > 
> > I don't see a reason not to make it strict.
> > 
> 
> Would we ever want to reference that we're closing bugs reported in
> other projects ?
> 
> Like:
> 
> Closes: https://github.com/raspberrypi/libcamera/issues/141
> 
> I could imagine having a commit that 'closes' multiple issue reports in
> multiple locations.
> 
> 

Which actually means I think this patch is correct as it is so:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> 
> > > (2) what should happen to `Bug` ?
> > 
> > I think we can deprecate it, and drop it from checkstyle.py.
> 
> I think it can be removed too.
> 
> 
> 
> > 
> > > ---
> > >  utils/checkstyle.py | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > > index f6229bbd86..219921bc9d 100755
> > > --- a/utils/checkstyle.py
> > > +++ b/utils/checkstyle.py
> > > @@ -542,6 +542,7 @@ class TrailersChecker(CommitChecker):
> > >      known_trailers = {
> > >          'Acked-by': email_regex,
> > >          'Bug': link_regex,
> > > +        'Closes': link_regex,
> > >          'Co-developed-by': email_regex,
> > >          'Fixes': commit_regex,
> > >          'Link': link_regex,
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
Laurent Pinchart Oct. 30, 2025, 1:55 p.m. UTC | #4 
On Thu, Oct 30, 2025 at 01:31:31PM +0000, Kieran Bingham wrote:
> Quoting Kieran Bingham (2025-10-29 11:21:33)
> > Quoting Laurent Pinchart (2025-10-29 10:14:54)
> > > On Wed, Oct 29, 2025 at 11:08:47AM +0100, Barnabás Pőcze wrote:
> > > > `Closes: <url>` is recognized by gitlab and closes the referenced issue
> > > > automatically when a commit is made[0] to the project's default branch.
> > > > Now that issues are hosted on gitlab, it will be the preferred way to
> > > > reference the fixed issue from a commit, so add it to the checkstlye.py script.
> > > > 
> > > > [0]: https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern
> > > > 
> > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > > ---
> > > > Two questions remain:
> > > 
> > > I read the commit message first, then glanced at the code, and had the
> > > exact same two questions :-)
> > > 
> > > > (1) should the regex be stricter? e.g.
> > > >     r"https:\/\/gitlab\.freedesktop\.org\/camera\/libcamera\/-\/issues\/[0-9]+"
> > > 
> > > I don't see a reason not to make it strict.
> > > 
> > 
> > Would we ever want to reference that we're closing bugs reported in
> > other projects ?
> > 
> > Like:
> > 
> > Closes: https://github.com/raspberrypi/libcamera/issues/141
> > 
> > I could imagine having a commit that 'closes' multiple issue reports in
> > multiple locations.

I think I would still prefer restricting the usage to gitlab.fdo, and
expand it if we find that it's useful. Having a regex that checks the
URL is useful to avoid typos. It's a slight preference though.

> Which actually means I think this patch is correct as it is so:

The Bug trailer should be removed.

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > > > (2) what should happen to `Bug` ?
> > > 
> > > I think we can deprecate it, and drop it from checkstyle.py.
> > 
> > I think it can be removed too.
> > 
> > > > ---
> > > >  utils/checkstyle.py | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > > > index f6229bbd86..219921bc9d 100755
> > > > --- a/utils/checkstyle.py
> > > > +++ b/utils/checkstyle.py
> > > > @@ -542,6 +542,7 @@ class TrailersChecker(CommitChecker):
> > > >      known_trailers = {
> > > >          'Acked-by': email_regex,
> > > >          'Bug': link_regex,
> > > > +        'Closes': link_regex,
> > > >          'Co-developed-by': email_regex,
> > > >          'Fixes': commit_regex,
> > > >          'Link': link_regex,
Barnabás Pőcze Oct. 30, 2025, 4:03 p.m. UTC | #5 
2025. 10. 30. 14:55 keltezéssel, Laurent Pinchart írta:
> On Thu, Oct 30, 2025 at 01:31:31PM +0000, Kieran Bingham wrote:
>> Quoting Kieran Bingham (2025-10-29 11:21:33)
>>> Quoting Laurent Pinchart (2025-10-29 10:14:54)
>>>> On Wed, Oct 29, 2025 at 11:08:47AM +0100, Barnabás Pőcze wrote:
>>>>> `Closes: <url>` is recognized by gitlab and closes the referenced issue
>>>>> automatically when a commit is made[0] to the project's default branch.
>>>>> Now that issues are hosted on gitlab, it will be the preferred way to
>>>>> reference the fixed issue from a commit, so add it to the checkstlye.py script.
>>>>>
>>>>> [0]: https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern
>>>>>
>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>>> ---
>>>>> Two questions remain:
>>>>
>>>> I read the commit message first, then glanced at the code, and had the
>>>> exact same two questions :-)
>>>>
>>>>> (1) should the regex be stricter? e.g.
>>>>>      r"https:\/\/gitlab\.freedesktop\.org\/camera\/libcamera\/-\/issues\/[0-9]+"
>>>>
>>>> I don't see a reason not to make it strict.
>>>>
>>>
>>> Would we ever want to reference that we're closing bugs reported in
>>> other projects ?
>>>
>>> Like:
>>>
>>> Closes: https://github.com/raspberrypi/libcamera/issues/141
>>>
>>> I could imagine having a commit that 'closes' multiple issue reports in
>>> multiple locations.
> 
> I think I would still prefer restricting the usage to gitlab.fdo, and
> expand it if we find that it's useful. Having a regex that checks the
> URL is useful to avoid typos. It's a slight preference though.
> 
>> Which actually means I think this patch is correct as it is so:
> 
> The Bug trailer should be removed.

Okay, so I will remove "Bug". But it's not clear to me whether "Closes"
should accept any link or not. I haven't really considered the option
of putting e.g. rpi libcamera issue urls there, but I think it makes
sense to do that. There is still "Link" for those, but I think having
a single way to refer to issue trackers is better.


Regards,
Barnabás Pőcze

> 
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>>>>> (2) what should happen to `Bug` ?
>>>>
>>>> I think we can deprecate it, and drop it from checkstyle.py.
>>>
>>> I think it can be removed too.
>>>
>>>>> ---
>>>>>   utils/checkstyle.py | 1 +
>>>>>   1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
>>>>> index f6229bbd86..219921bc9d 100755
>>>>> --- a/utils/checkstyle.py
>>>>> +++ b/utils/checkstyle.py
>>>>> @@ -542,6 +542,7 @@ class TrailersChecker(CommitChecker):
>>>>>       known_trailers = {
>>>>>           'Acked-by': email_regex,
>>>>>           'Bug': link_regex,
>>>>> +        'Closes': link_regex,
>>>>>           'Co-developed-by': email_regex,
>>>>>           'Fixes': commit_regex,
>>>>>           'Link': link_regex,
>
Kieran Bingham Oct. 30, 2025, 4:29 p.m. UTC | #6 
Quoting Barnabás Pőcze (2025-10-30 16:03:21)
> 2025. 10. 30. 14:55 keltezéssel, Laurent Pinchart írta:
> > On Thu, Oct 30, 2025 at 01:31:31PM +0000, Kieran Bingham wrote:
> >> Quoting Kieran Bingham (2025-10-29 11:21:33)
> >>> Quoting Laurent Pinchart (2025-10-29 10:14:54)
> >>>> On Wed, Oct 29, 2025 at 11:08:47AM +0100, Barnabás Pőcze wrote:
> >>>>> `Closes: <url>` is recognized by gitlab and closes the referenced issue
> >>>>> automatically when a commit is made[0] to the project's default branch.
> >>>>> Now that issues are hosted on gitlab, it will be the preferred way to
> >>>>> reference the fixed issue from a commit, so add it to the checkstlye.py script.
> >>>>>
> >>>>> [0]: https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern
> >>>>>
> >>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >>>>> ---
> >>>>> Two questions remain:
> >>>>
> >>>> I read the commit message first, then glanced at the code, and had the
> >>>> exact same two questions :-)
> >>>>
> >>>>> (1) should the regex be stricter? e.g.
> >>>>>      r"https:\/\/gitlab\.freedesktop\.org\/camera\/libcamera\/-\/issues\/[0-9]+"
> >>>>
> >>>> I don't see a reason not to make it strict.
> >>>>
> >>>
> >>> Would we ever want to reference that we're closing bugs reported in
> >>> other projects ?
> >>>
> >>> Like:
> >>>
> >>> Closes: https://github.com/raspberrypi/libcamera/issues/141
> >>>
> >>> I could imagine having a commit that 'closes' multiple issue reports in
> >>> multiple locations.
> > 
> > I think I would still prefer restricting the usage to gitlab.fdo, and
> > expand it if we find that it's useful. Having a regex that checks the
> > URL is useful to avoid typos. It's a slight preference though.
> > 
> >> Which actually means I think this patch is correct as it is so:
> > 
> > The Bug trailer should be removed.
> 
> Okay, so I will remove "Bug". But it's not clear to me whether "Closes"
> should accept any link or not. I haven't really considered the option
> of putting e.g. rpi libcamera issue urls there, but I think it makes
> sense to do that. There is still "Link" for those, but I think having
> a single way to refer to issue trackers is better.
> 

Link: <url> means - this commit is related to something at url.

Closes: is a clear way to say "This resolves <url>" and I agree it would
be good to be consistent on this.

Bugs in libcamera aren't only stored at bugs.libcamera.org or
gitlab.freedesktop.org/camera/libcamera/issues/

They might also be logged in Yocto or Debian or ...

I think if we are submitted a patch that closes an issue logged in
multiple places we should say that the patch closes those issues. (I'm
not going to assume we can expect them all to automatically close...)

We might want to be able to reference any of:
 https://bugs.launchpad.net/ubuntu/+source/libcamera

or 
 https://bugzilla.redhat.com/show_bug.cgi?id=2368538
 https://bugzilla.redhat.com/show_bug.cgi?id=2329365

Would be valid issues to represent.

> 
> Regards,
> Barnabás Pőcze
> 
> > 
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>
> >>>>> (2) what should happen to `Bug` ?
> >>>>
> >>>> I think we can deprecate it, and drop it from checkstyle.py.
> >>>
> >>> I think it can be removed too.
> >>>
> >>>>> ---
> >>>>>   utils/checkstyle.py | 1 +
> >>>>>   1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> >>>>> index f6229bbd86..219921bc9d 100755
> >>>>> --- a/utils/checkstyle.py
> >>>>> +++ b/utils/checkstyle.py
> >>>>> @@ -542,6 +542,7 @@ class TrailersChecker(CommitChecker):
> >>>>>       known_trailers = {
> >>>>>           'Acked-by': email_regex,
> >>>>>           'Bug': link_regex,
> >>>>> +        'Closes': link_regex,
> >>>>>           'Co-developed-by': email_regex,
> >>>>>           'Fixes': commit_regex,
> >>>>>           'Link': link_regex,
> > 
>

Patch
diff mbox series 

diff --git a/utils/checkstyle.py b/utils/checkstyle.py
index f6229bbd86..219921bc9d 100755
--- a/utils/checkstyle.py
+++ b/utils/checkstyle.py
@@ -542,6 +542,7 @@  class TrailersChecker(CommitChecker):
     known_trailers = {
         'Acked-by': email_regex,
         'Bug': link_regex,
+        'Closes': link_regex,
         'Co-developed-by': email_regex,
         'Fixes': commit_regex,
         'Link': link_regex,