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@xxxxxxxxxxxxxx> > # Date 1267318701 18000 > # Node ID 69c8f5172790784738bcc18f8301919ef3d5373f > # Parent b91e96a07bee27c1d421b4c3702e33ee8075de83 > as102: checkpatch fixes > > From: Devin Heitmueller <dheitmueller@xxxxxxxxxxxxxx> > > Fix make checkpatch issues reported against as10x_cmd.c. > > Priority: normal > > Signed-off-by: Devin Heitmueller <dheitmueller@xxxxxxxxxxxxxx> > Signed-off-by: Piotr Chmura <chmooreck@xxxxxxxxxxxxxx> > ---- > 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/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
