Re: [PATCH 00/13] staging: add drivers to support Mediatek mt7621 in gnubee-pc1

[NeilBrown]

On Thu, Mar 15 2018, Dan Carpenter wrote:

> On Thu, Mar 15, 2018 at 10:04:33PM +1100, NeilBrown wrote:
>> On Thu, Mar 15 2018, Dan Carpenter wrote:
>> 
>> > This all seems fine.  Generally the requirements for staging are that it
>> > has a TODO, someone to work on it, and it doesn't break the build.  But
>> > some of the patches don't have commit message and those are required and
>> > some of the commit messages are just the changes you have made not don't
>> > describe the actual code...
>> 
>> Thanks for having a look.
>> It seems odd to require detailed commit messages, when we don't require
>> the same level of quality in the code.
>> Naturally when the driver is moved out of staging a properly detailed
>> commit message should be added, but is that needed on the way in to
>> staging?  At this stage I don't know much more than is already there.
>> After I've cleaned up the code I probably will.
>> 
>> For patch 01/13 you asked "what kind of device this is".  The subject
>> line makes it clear that it is a "pcie driver".  What extra detail did
>> you want?  Would it be sufficient to just copy the subject line so that
>> it appears twice in the commit message?
>> 
>
> Ah...  Sorry.  It's literally a pcie driver.  For some reason I thought
> it was a device that ran over pcie.
>
> We don't require a detailed changelog, but you have to put something...
> Probably just restating the subject and adding that it's for the gnubee1
> is fine.

I'll resend sometime next week with more words.  However could you
please clarify a couple of things for me?

1/ Why do you (sometimes) call the commit message a "change log".  When
  I see the term "change log" in the context of a patch, my first
  thought is that it it means a log of changes that have been made to
  the patch - typically through the review cycle.  But that isn't what
  you mean.  This has confused me a couple of times.

2/ Why don't you consider the first line of the commit message to be
   part of the commit message?  Why is duplication required?
   (You said "some of the patches don't have commit message[s]",
   which isn't true, though some of the messages are only one line).

Maybe the requirements on the commit message (including this
duplication) could be included in the "Staging trees" section of
Documentation/process/2.process.rst.
That file only lists the TODO and "doesn't break the build"
requirements.  It doesn't metion the "someone to work on it" requirement.
That might seem obvious, but it doesn't hurt to be explicit.

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature