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

[Dan Carpenter]

On Fri, Mar 16, 2018 at 07:02:51AM +1100, NeilBrown wrote:
> 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.
> 

Sorry.  Yeah.  I do use them interchangeably which is probably not the
right thing.


> 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).
> 

First of all, you're a writer.  If you don't like to do things then you
need to keep those skills hidden away.  I never tell anyone that I know
how to program in COBOL (True story.  COBOL is great for formatting text
on dot matrix printers and for doing decimal math.)

This is a hard requirement from Greg, not something that specific to
*me* although I do agree with the requirement as well.  The idea of
forcing everyone to write a commit message is that we're hoping they
take a little time to tell a story about what the patch is.  Sometimes
they're not English majors or whatever and they just restate the subject
and whatever, that's fine.

The first patch I reviewed in this series was:
[PATCH 03/13] staging: mt7621-gpio: ralink: add mt7621 gpio controller
A good commit message might be:

    This adds Mediatek GPIO support for the mt7621 chip.  It's used on
    the Gnubee NAS hardware and a couple other MIPS SoCs.  This code
    originally came from the OpenWRT project.

That information was all there in the patch 0 commit but that gets
dropped.  It's feels a bit weird to put that boilerplate information in
every commit, but it means that it's there when we do a git log on the
file so I think it's a good idea.

Also, and this is my fault not yours of course, but my email client is
all text and looks exactly like marc.info:
https://marc.info/?l=linux-driver-devel&m=152105965413484&w=2

It's hard to see the subject so I normally don't even read it when I'm
looking at patches.

regards,
dan carpenter