linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* linux-next: Fixes tags need some work in the pm tree
@ 2019-01-15 20:55 Stephen Rothwell
  2019-01-15 21:10 ` Sinan Kaya
  2019-01-15 22:13 ` Rafael J. Wysocki
  0 siblings, 2 replies; 10+ messages in thread
From: Stephen Rothwell @ 2019-01-15 20:55 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux Next Mailing List, Linux Kernel Mailing List, Sinan Kaya

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

[I am experimenting with checking the Fixes tags in commits in linux-next.
Please let me know if you think I am being too strict.]

Hi Rafael,

Commits

  62b33d57c534 ("drivers: thermal: int340x_thermal: Make PCI dependency explicit")
  cd793ab22a93 ("x86/intel/lpss: Make PCI dependency explicit")
  42ac19e7b81e ("ACPI: EC: Look for ECDT EC after calling acpi_load_tables()")
  6c29b81b5695 ("platform/x86: apple-gmux: Make PCI dependency explicit")
  34783dc0182a ("platform/x86: intel_pmc: Make PCI dependency explicit")
  704658d1d3ae ("platform/x86: intel_ips: make PCI dependency explicit")
  5df37f3a1aa9 ("vga-switcheroo: make PCI dependency explicit")
  da1df6ee4296 ("ata: pata_acpi: Make PCI dependency explicit")
  ce97a22a596b ("ACPI / LPSS: Make PCI dependency explicit")

Have malformed Fixes tags:

There should be double quotes around the commit subject.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: Fixes tags need some work in the pm tree
  2019-01-15 20:55 linux-next: Fixes tags need some work in the pm tree Stephen Rothwell
@ 2019-01-15 21:10 ` Sinan Kaya
  2019-01-15 22:44   ` Stephen Rothwell
  2019-01-15 22:13 ` Rafael J. Wysocki
  1 sibling, 1 reply; 10+ messages in thread
From: Sinan Kaya @ 2019-01-15 21:10 UTC (permalink / raw)
  To: Stephen Rothwell, Rafael J. Wysocki
  Cc: Linux Next Mailing List, Linux Kernel Mailing List

On 1/15/2019 3:55 PM, Stephen Rothwell wrote:
> [I am experimenting with checking the Fixes tags in commits in linux-next.
> Please let me know if you think I am being too strict.]
> 
> Hi Rafael,
> 
> Commits
> 
>    62b33d57c534 ("drivers: thermal: int340x_thermal: Make PCI dependency explicit")
>    cd793ab22a93 ("x86/intel/lpss: Make PCI dependency explicit")
>    42ac19e7b81e ("ACPI: EC: Look for ECDT EC after calling acpi_load_tables()")
>    6c29b81b5695 ("platform/x86: apple-gmux: Make PCI dependency explicit")
>    34783dc0182a ("platform/x86: intel_pmc: Make PCI dependency explicit")
>    704658d1d3ae ("platform/x86: intel_ips: make PCI dependency explicit")
>    5df37f3a1aa9 ("vga-switcheroo: make PCI dependency explicit")
>    da1df6ee4296 ("ata: pata_acpi: Make PCI dependency explicit")
>    ce97a22a596b ("ACPI / LPSS: Make PCI dependency explicit")
> 
> Have malformed Fixes tags:
> 
> There should be double quotes around the commit subject.
> 

Interesting, can you add this to the checkpatch.pl script so that it doesn't
happen again?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: Fixes tags need some work in the pm tree
  2019-01-15 20:55 linux-next: Fixes tags need some work in the pm tree Stephen Rothwell
  2019-01-15 21:10 ` Sinan Kaya
@ 2019-01-15 22:13 ` Rafael J. Wysocki
  2019-01-15 22:43   ` Stephen Rothwell
  1 sibling, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2019-01-15 22:13 UTC (permalink / raw)
  To: Stephen Rothwell
  Cc: Linux Next Mailing List, Linux Kernel Mailing List, Sinan Kaya

On Tuesday, January 15, 2019 9:55:40 PM CET Stephen Rothwell wrote:
> [I am experimenting with checking the Fixes tags in commits in linux-next.
> Please let me know if you think I am being too strict.]
> 
> Hi Rafael,
> 
> Commits
> 
>   62b33d57c534 ("drivers: thermal: int340x_thermal: Make PCI dependency explicit")
>   cd793ab22a93 ("x86/intel/lpss: Make PCI dependency explicit")
>   42ac19e7b81e ("ACPI: EC: Look for ECDT EC after calling acpi_load_tables()")
>   6c29b81b5695 ("platform/x86: apple-gmux: Make PCI dependency explicit")
>   34783dc0182a ("platform/x86: intel_pmc: Make PCI dependency explicit")
>   704658d1d3ae ("platform/x86: intel_ips: make PCI dependency explicit")
>   5df37f3a1aa9 ("vga-switcheroo: make PCI dependency explicit")
>   da1df6ee4296 ("ata: pata_acpi: Make PCI dependency explicit")
>   ce97a22a596b ("ACPI / LPSS: Make PCI dependency explicit")
> 
> Have malformed Fixes tags:
> 
> There should be double quotes around the commit subject.

Well, where does this requirement come from?

It hasn't been there before AFAICS.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: Fixes tags need some work in the pm tree
  2019-01-15 22:13 ` Rafael J. Wysocki
@ 2019-01-15 22:43   ` Stephen Rothwell
  2019-01-15 23:06     ` Rafael J. Wysocki
  0 siblings, 1 reply; 10+ messages in thread
From: Stephen Rothwell @ 2019-01-15 22:43 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux Next Mailing List, Linux Kernel Mailing List, Sinan Kaya,
	Paul Gortmaker

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

Hi Rafael,

On Tue, 15 Jan 2019 23:13:16 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
>
> On Tuesday, January 15, 2019 9:55:40 PM CET Stephen Rothwell wrote:
> > [I am experimenting with checking the Fixes tags in commits in linux-next.
> > Please let me know if you think I am being too strict.]
> > 
> > Hi Rafael,
> > 
> > Commits
> > 
> >   62b33d57c534 ("drivers: thermal: int340x_thermal: Make PCI dependency explicit")
> >   cd793ab22a93 ("x86/intel/lpss: Make PCI dependency explicit")
> >   42ac19e7b81e ("ACPI: EC: Look for ECDT EC after calling acpi_load_tables()")
> >   6c29b81b5695 ("platform/x86: apple-gmux: Make PCI dependency explicit")
> >   34783dc0182a ("platform/x86: intel_pmc: Make PCI dependency explicit")
> >   704658d1d3ae ("platform/x86: intel_ips: make PCI dependency explicit")
> >   5df37f3a1aa9 ("vga-switcheroo: make PCI dependency explicit")
> >   da1df6ee4296 ("ata: pata_acpi: Make PCI dependency explicit")
> >   ce97a22a596b ("ACPI / LPSS: Make PCI dependency explicit")
> > 
> > Have malformed Fixes tags:
> > 
> > There should be double quotes around the commit subject.  
> 
> Well, where does this requirement come from?
> 
> It hasn't been there before AFAICS.

Documentation/process/submitting-patches.rst has the following, but I
am sure people are happy to discuss changes and it does say "For
example", so maybe I am being to strict?  The counter argument is that
there are various (semi-)automated processes that use these tags and
being consistent probably makes those processes (and life for those who
run them) easier.

------------------------------------------------------------------------
If your patch fixes a bug in a specific commit, e.g. you found an issue using
``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
the SHA-1 ID, and the one line summary.  For example::

        Fixes: e21d2170f366 ("video: remove unnecessary platform_set_drvdata()")

The following ``git config`` settings can be used to add a pretty format for
outputting the above style in the ``git log`` or ``git show`` commands::

        [core]
                abbrev = 12
        [pretty]
                fixes = Fixes: %h (\"%s\")
------------------------------------------------------------------------

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: Fixes tags need some work in the pm tree
  2019-01-15 21:10 ` Sinan Kaya
@ 2019-01-15 22:44   ` Stephen Rothwell
  0 siblings, 0 replies; 10+ messages in thread
From: Stephen Rothwell @ 2019-01-15 22:44 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: Rafael J. Wysocki, Linux Next Mailing List,
	Linux Kernel Mailing List, Paul Gortmaker

[-- Attachment #1: Type: text/plain, Size: 268 bytes --]

Hi Sinan,

On Tue, 15 Jan 2019 16:10:07 -0500 Sinan Kaya <okaya@kernel.org> wrote:
>
> Interesting, can you add this to the checkpatch.pl script so that it doesn't
> happen again?

Probably a good idea ... (cc'ing Paul G :-))

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: Fixes tags need some work in the pm tree
  2019-01-15 22:43   ` Stephen Rothwell
@ 2019-01-15 23:06     ` Rafael J. Wysocki
  2019-01-15 23:43       ` Michael Ellerman
  2019-01-16  0:22       ` Paul Gortmaker
  0 siblings, 2 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2019-01-15 23:06 UTC (permalink / raw)
  To: Stephen Rothwell
  Cc: Linux Next Mailing List, Linux Kernel Mailing List, Sinan Kaya,
	Paul Gortmaker

On Tuesday, January 15, 2019 11:43:05 PM CET Stephen Rothwell wrote:
> Hi Rafael,
> 
> On Tue, 15 Jan 2019 23:13:16 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> >
> > On Tuesday, January 15, 2019 9:55:40 PM CET Stephen Rothwell wrote:
> > > [I am experimenting with checking the Fixes tags in commits in linux-next.
> > > Please let me know if you think I am being too strict.]
> > > 
> > > Hi Rafael,
> > > 
> > > Commits
> > > 
> > >   62b33d57c534 ("drivers: thermal: int340x_thermal: Make PCI dependency explicit")
> > >   cd793ab22a93 ("x86/intel/lpss: Make PCI dependency explicit")
> > >   42ac19e7b81e ("ACPI: EC: Look for ECDT EC after calling acpi_load_tables()")
> > >   6c29b81b5695 ("platform/x86: apple-gmux: Make PCI dependency explicit")
> > >   34783dc0182a ("platform/x86: intel_pmc: Make PCI dependency explicit")
> > >   704658d1d3ae ("platform/x86: intel_ips: make PCI dependency explicit")
> > >   5df37f3a1aa9 ("vga-switcheroo: make PCI dependency explicit")
> > >   da1df6ee4296 ("ata: pata_acpi: Make PCI dependency explicit")
> > >   ce97a22a596b ("ACPI / LPSS: Make PCI dependency explicit")
> > > 
> > > Have malformed Fixes tags:
> > > 
> > > There should be double quotes around the commit subject.  
> > 
> > Well, where does this requirement come from?
> > 
> > It hasn't been there before AFAICS.
> 
> Documentation/process/submitting-patches.rst has the following, but I
> am sure people are happy to discuss changes and it does say "For
> example", so maybe I am being to strict?

If that's the source of it, then it's rather weak IMO.

Formal requirements should be documented as such and I would expect that
to happen through the usual process: patch submission, review, acceptance etc.

Moreover, extending advice on to how submit paches to formatting requirements
for commits feels like a bit of a stretch to me.

> The counter argument is that
> there are various (semi-)automated processes that use these tags and
> being consistent probably makes those processes (and life for those who
> run them) easier.

And frankly I wouldn't expect any of these to even look at the summary
lines as they have not been consistent historically and the SHA-1 ID should
be sufficient to identify the commit in question.

Anyway, I'm not against formalizing the Fixes: tags, but I would rather expect
that to be done in a, well, more formal way.

> ------------------------------------------------------------------------
> If your patch fixes a bug in a specific commit, e.g. you found an issue using
> ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
> the SHA-1 ID, and the one line summary.  For example::
> 
>         Fixes: e21d2170f366 ("video: remove unnecessary platform_set_drvdata()")
> 
> The following ``git config`` settings can be used to add a pretty format for
> outputting the above style in the ``git log`` or ``git show`` commands::
> 
>         [core]
>                 abbrev = 12
>         [pretty]
>                 fixes = Fixes: %h (\"%s\")
> ------------------------------------------------------------------------

Cheers,
Rafael


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: Fixes tags need some work in the pm tree
  2019-01-15 23:06     ` Rafael J. Wysocki
@ 2019-01-15 23:43       ` Michael Ellerman
  2019-01-16 11:34         ` Rafael J. Wysocki
  2019-01-16  0:22       ` Paul Gortmaker
  1 sibling, 1 reply; 10+ messages in thread
From: Michael Ellerman @ 2019-01-15 23:43 UTC (permalink / raw)
  To: Rafael J. Wysocki, Stephen Rothwell
  Cc: Linux Next Mailing List, Linux Kernel Mailing List, Sinan Kaya,
	Paul Gortmaker

"Rafael J. Wysocki" <rjw@rjwysocki.net> writes:
> On Tuesday, January 15, 2019 11:43:05 PM CET Stephen Rothwell wrote:
>> Hi Rafael,
>> 
>> On Tue, 15 Jan 2019 23:13:16 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
>> >
>> > On Tuesday, January 15, 2019 9:55:40 PM CET Stephen Rothwell wrote:
>> > > [I am experimenting with checking the Fixes tags in commits in linux-next.
>> > > Please let me know if you think I am being too strict.]
>> > > 
>> > > Hi Rafael,
>> > > 
>> > > Commits
>> > > 
>> > >   62b33d57c534 ("drivers: thermal: int340x_thermal: Make PCI dependency explicit")
>> > >   cd793ab22a93 ("x86/intel/lpss: Make PCI dependency explicit")
>> > >   42ac19e7b81e ("ACPI: EC: Look for ECDT EC after calling acpi_load_tables()")
>> > >   6c29b81b5695 ("platform/x86: apple-gmux: Make PCI dependency explicit")
>> > >   34783dc0182a ("platform/x86: intel_pmc: Make PCI dependency explicit")
>> > >   704658d1d3ae ("platform/x86: intel_ips: make PCI dependency explicit")
>> > >   5df37f3a1aa9 ("vga-switcheroo: make PCI dependency explicit")
>> > >   da1df6ee4296 ("ata: pata_acpi: Make PCI dependency explicit")
>> > >   ce97a22a596b ("ACPI / LPSS: Make PCI dependency explicit")
>> > > 
>> > > Have malformed Fixes tags:
>> > > 
>> > > There should be double quotes around the commit subject.  
>> > 
>> > Well, where does this requirement come from?
>> > 
>> > It hasn't been there before AFAICS.
>> 
>> Documentation/process/submitting-patches.rst has the following, but I
>> am sure people are happy to discuss changes and it does say "For
>> example", so maybe I am being to strict?
>
> If that's the source of it, then it's rather weak IMO.
>
> Formal requirements should be documented as such and I would expect that
> to happen through the usual process: patch submission, review, acceptance etc.

It is documented, in submitting-patches.rst.

That was submitted to lkml:

  https://lore.kernel.org/lkml/1396949135-27122-1-git-send-email-jeffrey.t.kirsher@intel.com/

And committed by Linus:

  8401aa1f5997 ("Documentation/SubmittingPatches: describe the Fixes: tag")

How would we make it more formal than that?

> Moreover, extending advice on to how submit paches to formatting requirements
> for commits feels like a bit of a stretch to me.
>
>> The counter argument is that
>> there are various (semi-)automated processes that use these tags and
>> being consistent probably makes those processes (and life for those who
>> run them) easier.
>
> And frankly I wouldn't expect any of these to even look at the summary
> lines as they have not been consistent historically and the SHA-1 ID should
> be sufficient to identify the commit in question.

It usually is, but it's still a good sanity check to have the subject in
there, especially for cases where the SHA is wrong (though that should
be less of a problem in future due to Stephen doing these checks).

cheers

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: Fixes tags need some work in the pm tree
  2019-01-15 23:06     ` Rafael J. Wysocki
  2019-01-15 23:43       ` Michael Ellerman
@ 2019-01-16  0:22       ` Paul Gortmaker
  2019-01-16 11:50         ` Rafael J. Wysocki
  1 sibling, 1 reply; 10+ messages in thread
From: Paul Gortmaker @ 2019-01-16  0:22 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Stephen Rothwell, Linux Next Mailing List,
	Linux Kernel Mailing List, Sinan Kaya

[Re: linux-next: Fixes tags need some work in the pm tree] On 16/01/2019 (Wed 00:06) Rafael J. Wysocki wrote:

> On Tuesday, January 15, 2019 11:43:05 PM CET Stephen Rothwell wrote:
> > Hi Rafael,
> > 
> > On Tue, 15 Jan 2019 23:13:16 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> > >
> > > On Tuesday, January 15, 2019 9:55:40 PM CET Stephen Rothwell wrote:
> > > > [I am experimenting with checking the Fixes tags in commits in linux-next.
> > > > Please let me know if you think I am being too strict.]
> > > > 
> > > > Hi Rafael,
> > > > 
> > > > Commits
> > > > 
> > > >   62b33d57c534 ("drivers: thermal: int340x_thermal: Make PCI dependency explicit")
> > > >   cd793ab22a93 ("x86/intel/lpss: Make PCI dependency explicit")
> > > >   42ac19e7b81e ("ACPI: EC: Look for ECDT EC after calling acpi_load_tables()")
> > > >   6c29b81b5695 ("platform/x86: apple-gmux: Make PCI dependency explicit")
> > > >   34783dc0182a ("platform/x86: intel_pmc: Make PCI dependency explicit")
> > > >   704658d1d3ae ("platform/x86: intel_ips: make PCI dependency explicit")
> > > >   5df37f3a1aa9 ("vga-switcheroo: make PCI dependency explicit")
> > > >   da1df6ee4296 ("ata: pata_acpi: Make PCI dependency explicit")
> > > >   ce97a22a596b ("ACPI / LPSS: Make PCI dependency explicit")
> > > > 
> > > > Have malformed Fixes tags:
> > > > 
> > > > There should be double quotes around the commit subject.  
> > > 
> > > Well, where does this requirement come from?
> > > 
> > > It hasn't been there before AFAICS.
> > 
> > Documentation/process/submitting-patches.rst has the following, but I
> > am sure people are happy to discuss changes and it does say "For
> > example", so maybe I am being to strict?
> 
> If that's the source of it, then it's rather weak IMO.

Rafael, allow me to rewind a bit, and add context you would not have...

A quick on-the-fly script shows we have a lot of "Fixes:" tags that
either have bad (rebased/gone) commit IDs and/or non-standard
formatting.

The biggest issue is having commits in mainline that reference a commit
ID in a Fixes: tag that doesn't exist - for one reason or another.  Once
that is in mainline, we can't correct it.  It is there forever.

Doing a sanity check for that in linux-next seems like a good place to
check for this and prevent it.  And if we sanity check for one thing, we
can sanity check for other common issues.  Hence the less important
formatting checks.

> Formal requirements should be documented as such and I would expect that
> to happen through the usual process: patch submission, review, acceptance etc.

I'd rather not misinterpret this as a formal requirement.  We just want
to catch bad SHA IDs in Fixes: and also help our friends doing the
linux-stable releases so they can parse those Fixes: fields more easily
and reliably.

I'd like to think we all can support the work the linux-stable people do
and stand behind doing whatever we can to help them.  At the same time,
people (maintainers and submitters) have the choice to ignore the
e-mails that suggest "Fixes:" changes, if they feel they are invalid.
And suggestions for improvements in parsing etc etc are always welcome.

Thanks,
Paul.
--

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: Fixes tags need some work in the pm tree
  2019-01-15 23:43       ` Michael Ellerman
@ 2019-01-16 11:34         ` Rafael J. Wysocki
  0 siblings, 0 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2019-01-16 11:34 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Stephen Rothwell, Linux Next Mailing List,
	Linux Kernel Mailing List, Sinan Kaya, Paul Gortmaker

On Wednesday, January 16, 2019 12:43:31 AM CET Michael Ellerman wrote:
> "Rafael J. Wysocki" <rjw@rjwysocki.net> writes:
> > On Tuesday, January 15, 2019 11:43:05 PM CET Stephen Rothwell wrote:
> >> Hi Rafael,
> >> 
> >> On Tue, 15 Jan 2019 23:13:16 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> >> >
> >> > On Tuesday, January 15, 2019 9:55:40 PM CET Stephen Rothwell wrote:
> >> > > [I am experimenting with checking the Fixes tags in commits in linux-next.
> >> > > Please let me know if you think I am being too strict.]
> >> > > 
> >> > > Hi Rafael,
> >> > > 
> >> > > Commits
> >> > > 
> >> > >   62b33d57c534 ("drivers: thermal: int340x_thermal: Make PCI dependency explicit")
> >> > >   cd793ab22a93 ("x86/intel/lpss: Make PCI dependency explicit")
> >> > >   42ac19e7b81e ("ACPI: EC: Look for ECDT EC after calling acpi_load_tables()")
> >> > >   6c29b81b5695 ("platform/x86: apple-gmux: Make PCI dependency explicit")
> >> > >   34783dc0182a ("platform/x86: intel_pmc: Make PCI dependency explicit")
> >> > >   704658d1d3ae ("platform/x86: intel_ips: make PCI dependency explicit")
> >> > >   5df37f3a1aa9 ("vga-switcheroo: make PCI dependency explicit")
> >> > >   da1df6ee4296 ("ata: pata_acpi: Make PCI dependency explicit")
> >> > >   ce97a22a596b ("ACPI / LPSS: Make PCI dependency explicit")
> >> > > 
> >> > > Have malformed Fixes tags:
> >> > > 
> >> > > There should be double quotes around the commit subject.  
> >> > 
> >> > Well, where does this requirement come from?
> >> > 
> >> > It hasn't been there before AFAICS.
> >> 
> >> Documentation/process/submitting-patches.rst has the following, but I
> >> am sure people are happy to discuss changes and it does say "For
> >> example", so maybe I am being to strict?
> >
> > If that's the source of it, then it's rather weak IMO.
> >
> > Formal requirements should be documented as such and I would expect that
> > to happen through the usual process: patch submission, review, acceptance etc.
> 
> It is documented, in submitting-patches.rst.
> 
> That was submitted to lkml:
> 
>   https://lore.kernel.org/lkml/1396949135-27122-1-git-send-email-jeffrey.t.kirsher@intel.com/
> 
> And committed by Linus:
> 
>   8401aa1f5997 ("Documentation/SubmittingPatches: describe the Fixes: tag")

Stephen has already quoted from that doc, but it only gives the format of the
summary line as an example.

> How would we make it more formal than that?

Say somewhere that this particular summary formatting is required?

Also, tags are more of a maintainers' thing and SubmittingPatches doesn't look
like the best place for documenting how the maintainers are expected to format
their commits.

> > Moreover, extending advice on to how submit paches to formatting requirements
> > for commits feels like a bit of a stretch to me.
> >
> >> The counter argument is that
> >> there are various (semi-)automated processes that use these tags and
> >> being consistent probably makes those processes (and life for those who
> >> run them) easier.
> >
> > And frankly I wouldn't expect any of these to even look at the summary
> > lines as they have not been consistent historically and the SHA-1 ID should
> > be sufficient to identify the commit in question.
> 
> It usually is, but it's still a good sanity check to have the subject in
> there, especially for cases where the SHA is wrong (though that should
> be less of a problem in future due to Stephen doing these checks).

A human can look at the summary after a script has not found the commit by
ID, but the human then doesn't care about the quoting characters.

Also it is rather straightforward to strip the quoting characters in a script
whatever they are.

My point basically is that in order to call something "malformed", you need
to provide a formal definition of what is expected.  An example in
SubmittingPatches doesn't seem quite sufficient to me for that role.

Cheers,
Rafael


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: Fixes tags need some work in the pm tree
  2019-01-16  0:22       ` Paul Gortmaker
@ 2019-01-16 11:50         ` Rafael J. Wysocki
  0 siblings, 0 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2019-01-16 11:50 UTC (permalink / raw)
  To: Paul Gortmaker
  Cc: Stephen Rothwell, Linux Next Mailing List,
	Linux Kernel Mailing List, Sinan Kaya

On Wednesday, January 16, 2019 1:22:57 AM CET Paul Gortmaker wrote:
> [Re: linux-next: Fixes tags need some work in the pm tree] On 16/01/2019 (Wed 00:06) Rafael J. Wysocki wrote:
> 
> > On Tuesday, January 15, 2019 11:43:05 PM CET Stephen Rothwell wrote:
> > > Hi Rafael,
> > > 
> > > On Tue, 15 Jan 2019 23:13:16 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> > > >
> > > > On Tuesday, January 15, 2019 9:55:40 PM CET Stephen Rothwell wrote:
> > > > > [I am experimenting with checking the Fixes tags in commits in linux-next.
> > > > > Please let me know if you think I am being too strict.]
> > > > > 
> > > > > Hi Rafael,
> > > > > 
> > > > > Commits
> > > > > 
> > > > >   62b33d57c534 ("drivers: thermal: int340x_thermal: Make PCI dependency explicit")
> > > > >   cd793ab22a93 ("x86/intel/lpss: Make PCI dependency explicit")
> > > > >   42ac19e7b81e ("ACPI: EC: Look for ECDT EC after calling acpi_load_tables()")
> > > > >   6c29b81b5695 ("platform/x86: apple-gmux: Make PCI dependency explicit")
> > > > >   34783dc0182a ("platform/x86: intel_pmc: Make PCI dependency explicit")
> > > > >   704658d1d3ae ("platform/x86: intel_ips: make PCI dependency explicit")
> > > > >   5df37f3a1aa9 ("vga-switcheroo: make PCI dependency explicit")
> > > > >   da1df6ee4296 ("ata: pata_acpi: Make PCI dependency explicit")
> > > > >   ce97a22a596b ("ACPI / LPSS: Make PCI dependency explicit")
> > > > > 
> > > > > Have malformed Fixes tags:
> > > > > 
> > > > > There should be double quotes around the commit subject.  
> > > > 
> > > > Well, where does this requirement come from?
> > > > 
> > > > It hasn't been there before AFAICS.
> > > 
> > > Documentation/process/submitting-patches.rst has the following, but I
> > > am sure people are happy to discuss changes and it does say "For
> > > example", so maybe I am being to strict?
> > 
> > If that's the source of it, then it's rather weak IMO.
> 
> Rafael, allow me to rewind a bit, and add context you would not have...
> 
> A quick on-the-fly script shows we have a lot of "Fixes:" tags that
> either have bad (rebased/gone) commit IDs and/or non-standard
> formatting.
> 
> The biggest issue is having commits in mainline that reference a commit
> ID in a Fixes: tag that doesn't exist - for one reason or another.  Once
> that is in mainline, we can't correct it.  It is there forever.
> 
> Doing a sanity check for that in linux-next seems like a good place to
> check for this and prevent it.  And if we sanity check for one thing, we
> can sanity check for other common issues.  Hence the less important
> formatting checks.

I agree with that in general, and I'd really appreciate telling me that I
have a non-existing SHA-ID in a Fixes: tag, but is it really a good idea to
make it a fail if someone uses "non-canonical" quoting style in the summary
part?

> > Formal requirements should be documented as such and I would expect that
> > to happen through the usual process: patch submission, review, acceptance etc.
> 
> I'd rather not misinterpret this as a formal requirement.  We just want
> to catch bad SHA IDs in Fixes: and also help our friends doing the
> linux-stable releases so they can parse those Fixes: fields more easily
> and reliably.
> 
> I'd like to think we all can support the work the linux-stable people do
> and stand behind doing whatever we can to help them.  At the same time,
> people (maintainers and submitters) have the choice to ignore the
> e-mails that suggest "Fixes:" changes, if they feel they are invalid.
> And suggestions for improvements in parsing etc etc are always welcome.

IMO, if you don't find the SHA ID given in the tag, it is a fail already.

If you find it, you should at least be able to get a partial match of the
initial part of the original commit summary with what is given in the
summary part of the tag except for some possible quoting characters at
the beginning of it.  That should be reasonably straightforward to implement.

Cheers,
Rafael


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-01-16 11:51 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-15 20:55 linux-next: Fixes tags need some work in the pm tree Stephen Rothwell
2019-01-15 21:10 ` Sinan Kaya
2019-01-15 22:44   ` Stephen Rothwell
2019-01-15 22:13 ` Rafael J. Wysocki
2019-01-15 22:43   ` Stephen Rothwell
2019-01-15 23:06     ` Rafael J. Wysocki
2019-01-15 23:43       ` Michael Ellerman
2019-01-16 11:34         ` Rafael J. Wysocki
2019-01-16  0:22       ` Paul Gortmaker
2019-01-16 11:50         ` Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).