Hi, I creating my first patch to the kernel. I followed the instruction on the newbies wiki and everything went smoothly. I got feedback from the maintainer to fix all the alignment issues in the file instead of just the one i picked. My question is if I fix 10 alignment issues identified by checkpatch,pl should each fix be a separate commit? Should I have a 10 commit patch set for the 10 alignment issues or is it better to make them all one commit since all the commits are fixing alignment issues? Thanks for our time, Greg
On Tue, Aug 07, 2018 at 12:15:04AM -0400, Greg Gallagher wrote:
Hi, I creating my first patch to the kernel. I followed the instruction on the newbies wiki and everything went smoothly. I got feedback from the maintainer to fix all the alignment issues in the file instead of just the one i picked. My question is if I fix 10 alignment issues identified by checkpatch,pl should each fix be a separate commit? Should I have a 10 commit patch set for the 10 alignment issues or is it better to make them all one commit since all the commits are fixing alignment issues?
The main criteria I would say is reviewability - so there should be no need to split it into 10 patches if it is aligment fixes. If the 10 fixes are in one file I would make one patch out of it. If it is in files that scripts/get_maintainer.pl suggests different imaintainer/contributors/reviewers for then put it in seperate patches - one for each group of maintainers/conributors/reviewers. thx! hofrat
On 2018-08-07 12:47 AM, Nicholas Mc Guire wrote:
On Tue, Aug 07, 2018 at 12:15:04AM -0400, Greg Gallagher wrote:
Hi, I creating my first patch to the kernel. I followed the instruction on the newbies wiki and everything went smoothly. I got feedback from the maintainer to fix all the alignment issues in the file instead of just the one i picked. My question is if I fix 10 alignment issues identified by checkpatch,pl should each fix be a separate commit? Should I have a 10 commit patch set for the 10 alignment issues or is it better to make them all one commit since all the commits are fixing alignment issues?
The main criteria I would say is reviewability - so there should be no need to split it into 10 patches if it is aligment fixes. If the 10 fixes are in one file I would make one patch out of it. If it is in files that scripts/get_maintainer.pl suggests different imaintainer/contributors/reviewers for then put it in seperate patches - one for each group of maintainers/conributors/reviewers.
thx! hofrat Hi, Thanks very much for replying. I took your advice and after an unrelated fat finger error I received an acked-by. Do I have to do anything else at this point? Should I add the acked-by to the change log? I've read the documentation in the Documents/process directory and I can't find a clear answer.
Thanks again for everyone's time, Greg
On Thu, Aug 09, 2018 at 01:19:53PM -0400, greg gallagher wrote:
On 2018-08-07 12:47 AM, Nicholas Mc Guire wrote:
On Tue, Aug 07, 2018 at 12:15:04AM -0400, Greg Gallagher wrote:
Hi, I creating my first patch to the kernel. I followed the instruction on the newbies wiki and everything went smoothly. I got feedback from the maintainer to fix all the alignment issues in the file instead of just the one i picked. My question is if I fix 10 alignment issues identified by checkpatch,pl should each fix be a separate commit? Should I have a 10 commit patch set for the 10 alignment issues or is it better to make them all one commit since all the commits are fixing alignment issues?
The main criteria I would say is reviewability - so there should be no need to split it into 10 patches if it is aligment fixes. If the 10 fixes are in one file I would make one patch out of it. If it is in files that scripts/get_maintainer.pl suggests different imaintainer/contributors/reviewers for then put it in seperate patches - one for each group of maintainers/conributors/reviewers.
thx! hofrat Hi, Thanks very much for replying. I took your advice and after an unrelated fat finger error I received an acked-by. Do I have to do anything else at this point? Should I add the acked-by to the change log? I've read the documentation in the Documents/process directory and I can't find a clear answer.
Basically all the Acked-by/Review-by/Tested-by are added by the maintainer that then queues your patch for upstream - so essentially you are done for this round of fun. thx! hofrat
On 2018-08-10 01:04 AM, Nicholas Mc Guire wrote:
On Thu, Aug 09, 2018 at 01:19:53PM -0400, greg gallagher wrote:
On 2018-08-07 12:47 AM, Nicholas Mc Guire wrote:
On Tue, Aug 07, 2018 at 12:15:04AM -0400, Greg Gallagher wrote:
Hi, I creating my first patch to the kernel. I followed the instruction on the newbies wiki and everything went smoothly. I got feedback from the maintainer to fix all the alignment issues in the file instead of just the one i picked. My question is if I fix 10 alignment issues identified by checkpatch,pl should each fix be a separate commit? Should I have a 10 commit patch set for the 10 alignment issues or is it better to make them all one commit since all the commits are fixing alignment issues?
The main criteria I would say is reviewability - so there should be no need to split it into 10 patches if it is aligment fixes. If the 10 fixes are in one file I would make one patch out of it. If it is in files that scripts/get_maintainer.pl suggests different imaintainer/contributors/reviewers for then put it in seperate patches - one for each group of maintainers/conributors/reviewers.
thx! hofrat Hi, Thanks very much for replying. I took your advice and after an unrelated fat finger error I received an acked-by. Do I have to do anything else at this point? Should I add the acked-by to the change log? I've read the documentation in the Documents/process directory and I can't find a clear answer.
Basically all the Acked-by/Review-by/Tested-by are added by the maintainer that then queues your patch for upstream - so essentially you are done for this round of fun.
thx! hofrat Awesome, thanks very much for you reply. Greatly appreciate the help:)
participants (3)
-
Greg Gallagher -
greg gallagher
-
Nicholas Mc Guire