[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160225084939.6504c412@jclayton-pc>
Date: Thu, 25 Feb 2016 08:49:39 -0800
From: Joshua Clayton <stillcompiling@...il.com>
To: Troy Kisky <troy.kisky@...ndarydevices.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, B38611@...escale.com,
fabio.estevam@...escale.com, l.stach@...gutronix.de,
andrew@...n.ch, tremyfr@...il.com, linux@....linux.org.uk,
linux-arm-kernel@...ts.infradead.org, laci@...ndarydevices.com,
shawnguo@...nel.org, johannes@...solutions.net,
sergei.shtylyov@...entembedded.com, arnd@...db.de
Subject: Re: [PATCH net-next V2 00/16] net: fec: cleanup and fixes
On Thu, 25 Feb 2016 09:05:34 -0700
Troy Kisky <troy.kisky@...ndarydevices.com> wrote:
> On 2/24/2016 7:52 PM, Joshua Clayton wrote:
> > Hello Troy,
> > I'm replying here instead of to a particular commit because several
> > of the commit messages seem inadequate.
> >
> > The first line summaries all look good.
> >
> > The descriptions should each also include the "user visible impact"
> > of the patch and the justification for it (i.e. why you made the
> > change).
> >
> > For instance, patch 3 doesn't include either what will change
> > (nothing, I'm guessing?) or why we now pass in the structures
> > instead of a queue_id.
>
> I can add to the commit message, that this is in preparation for
> patch 4 which depends on it. Or I could squash patches 2/3/4
> together, but I think it is easier to review smaller patches.
>
I agree that the smaller patches are better. Mentioning that a future
patch depends on the cleanup, (or the specific structure that is
depended on) is good.
>
> >
> > You've also got a few (e.g. patch 9, patch 14) where the substance
> > of the patch is in the summary,
> >
> > but missing from the message.
> >
> > These kind of descriptions are very hard to review since the
> > expression is split between the subject of the email and the body
> > of the email, which are not close
> > together in some email programs.
> >
> > Better to reiterate or elaborate on the summary in the message.
> > In patch 9, for instance, it would be more clear to say:
> >
> > Move restart test to earlier in fec_txq() which saves one
> > comparison.
>
>
> I can do this. And change patch 14 to read
>
Ok.
>
> Create subroutine reset_tx_queue to have one place
> to release any queued tx skbs.
>
That looks like a good message.
> Any other commit messages you'd like to improve?
>
I'm trying to give guidance in keeping with
Documentation/SubmittingPatches
What I might rather suggest is to do a quick once over for
each commit message to make sure they are each in harmony
with that document.
You can do it with git rebase --interactive, or directly in the
patches.
>
> > P.S I'm a little confused, as I came looking for a v3 of the first
> > 8 patches and found these instead. I'll try to give your first 8 a
> > look when they show up.
>
> The 1st 8 patches have already been applied. I added a patch to
> address your review there at the end of the series. So, that patch
> will show up in my next set.
>
Heh. I didn't see that. I'm used to maintainers waiting for comments to
be responded to before merging changes. That however is not your fault.
Thanks for considering my suggestion anyway.
>
> Thanks for the review
>
> Troy
You are welcome. Thanks for upstreaming these improvements.
Joshua
Powered by blists - more mailing lists