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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
