[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
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?
Hi Rafael,
On Tue, 15 Jan 2019 23:13:16 +0100 "Rafael J. Wysocki" <[email protected]> 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
Hi Sinan,
On Tue, 15 Jan 2019 16:10:07 -0500 Sinan Kaya <[email protected]> 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
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" <[email protected]> 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
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.
"Rafael J. Wysocki" <[email protected]> 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" <[email protected]> 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/[email protected]/
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
On Wednesday, January 16, 2019 12:43:31 AM CET Michael Ellerman wrote:
> "Rafael J. Wysocki" <[email protected]> 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" <[email protected]> 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/[email protected]/
>
> 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
[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" <[email protected]> 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.
--
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" <[email protected]> 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