Re: [PATCH v3 4/14] staging/media/as102: checkpatch fixes

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

On Oct 30 Piotr Chmura wrote:
> Patch taken from http://kernellabs.com/hg/~dheitmueller/v4l-dvb-as102-2/
> 
> Original source and comment:
> # HG changeset patch
> # User Devin Heitmueller <dheitmueller@kernellabs.com>
> # Date 1267318701 18000
> # Node ID 69c8f5172790784738bcc18f8301919ef3d5373f
> # Parent  b91e96a07bee27c1d421b4c3702e33ee8075de83
> as102: checkpatch fixes
> 
> From: Devin Heitmueller <dheitmueller@kernellabs.com>
> 
> Fix make checkpatch issues reported against as10x_cmd.c.
> 
> Priority: normal
> 
> Signed-off-by: Devin Heitmueller <dheitmueller@kernellabs.com>
> Signed-off-by: Piotr Chmura <chmooreck@poczta.onet.pl>
> ----
> Added missing empty lines at end of patch
> 
> Sylwester could you check if it applies cleanly now ?
> 
> 
> diff --git linux/drivers/staging/media/as102/as10x_cmd.c linuxb/drivers/staging/media/as102/as10x_cmd.c
> --- linux/drivers/staging/media/as102/as10x_cmd.c
> +++ linuxb/drivers/staging/media/as102/as10x_cmd.c
[...]

"man git-am" says:  "'From: ' [...] lines *starting* the body
override the respective commit author name and title values taken from the
headers."  (Emphasis mine.)  So, if the author field of the commit is
meant to be set to Devin Heitmueller, the line "From: Devin [...]" needs
to be placed as the first body of the mail message body.

Or more correctly spoken, lines in a format like RFC 2822 mail headers may
appear in the mail body and are recognized by "git am" as input for commit
metadata
  - if they are "From: ", "Subject: ", or "Date: " header-like lines,
  - if they appear before any other kinds of lines of the mail body.

A pro pos, it would be good to not only preserve author name and address
that way, but also authorship date.  I.e. if possible transform the line
"# Date 1267318701 18000" into an RFC 2822 date-header like line and put
it together with the From: line at the beginning of the body.  I am not
sure which date-time specification formats "git am" understands, but at
least the one per RFC 2822 clause 3.3 works.

The date thing would be icing on the cake.  In current practice, not many
kernel developers who forward patches by mail seem to be aware of it.

On the rest of the changelog, the part that you added:
  - The URL of the original repo was of some interest in the first patch
    which adds all the entire driver.  But it is hardly interesting in a
    checkpatch cleanup.
  - The information that this was once a HG changeset (before it became a
    mailed patch and hopefully eventually becomes a git commit...) as well
    as the original commit ID and parent commit ID are not of interest for
    the git changelog.

On the rest of the changelog, the part written by Devin:
  - Well, that doesn't say a lot.  /What/ kinds of checkpatch issues were
    "fixed", and why?  In my opinion, the fact that a certain script called
    checkpatch was involved at all is one of the last interesting facts
    about this change.  There was nothing "fixed" here.  From a quick look
    at it, this patch reformats whitespace and changes perhaps some other
    code formatting to bring it closer to normal Linux code format.
  - We don't have "Priority" lines in mainline changelogs.

However, since the kind of changes done in this patch appears to be mostly
or entirely trivial, it is not a drama to have a less than perfect
changelog (to say it mildly).  But please consider for future
contributions:  When writing a changelog always describe the reason or
impact of the change.  E.g. here whitespace changes (and more?) without
(?) change of functionality (without change of generated machine code
even?). ---  This criticism goes primarily to Devin, but also to Piotr who
had the chance to improve the changelog.

A very brief guide about good changelogs can be found in Andrew Morton's
"The Perfect Patch", e.g. http://kerneltrap.org/node/3737.  It takes a bit
of consideration how to apply it to a patch like this one though.  Hint:
The answer to "why the kernel needed patching" in this case is *not*
"because checkpatch told so". ;-)

Take this criticism of the changelog not as a request to change this
particular one, but as something to keep in mind in future contributions.

On the part after changelog, before the diff:
  - The delimiter is supposed to be *three* dashes, immediately followed
    by end-of-line, not four dashes.  See "man git-am" for lines that are
    recognized as beginning the patch == ending the changelog.
  - Please always insert a diffstat here.  This is a help to anybody
    wanting to review or handle a patch posting.

On the diff:
  - In typical patches which only touch a small part of a file, it is very
    important to generate the patch like "diff --show-c-function [...]"
    a.k.a. "diff -p [...]" would do.  While this information is ignored by
    any tool which applies the patch, it highly increases the human-
    readability of the diff.
    Of course in this patch here which changes almost the entire file, this
    bit is not important for readability.  But keep it in mind for future
    patch postings.
-- 
Stefan Richter
-=====-==-== =-=- ====-
http://arcgraph.de/sr/