On Mon, Feb 18, 2013 at 10:40:23AM +0100, Milan Cejnar wrote: > Dear SSSD Developers, > please review attached patch for issue 1756 > > Description: > > [PATCH] Patch for bug #1756, Append new line to string from poptStrerror() > > This patch changes function usage() in tools_util.c which is used by > BAD_POPT_PARAMS macro. > Since BAD_POPT_PARAMS macro is called by multiple functions in multiple > files with both custom messages termited with \n as well as popt messages > which are not terminated by default, this patch offers a correction by > checking the string just after printing out to console and printing out > additional \n if new line wasn't present. > > Best wishes > Milan Cejnar Hi Milan, Thank you for the contribution! In general I like the patch and it's good that it covers both the cases where the caller already appends \n and cases where you want the usage() to append the \n itself. Normally, it might be more efficient to call fprintf just once with a string pointer set to either "" or "\n" depending on the condition. But because this is just an error handler, it's not a blocker issue. I have a couple of very minor comments: * Our coding style recommends to use {} brackets even for one-line conditions * In general when sending patches we prefer to add a [PATCH] tag into the subject tag of the e-mail, too. This is done to make it easier for outside developers to follow the discussions but not the patch reviews. * We also usually put the short description as the first line, an empty line, the full URL of the ticket fixed, an empty line and then a short description of the problem fixed. It's somewhat similar to kernel's process (see http://www.kernel.org/doc/Documentation/SubmittingPatches bullet point 15) for example) I realize this info is missing from https://fedorahosted.org/sssd/wiki/DevelTutorials and I'll add it there. If you fix the above small problems, I'll apply the patch. _______________________________________________ sssd-devel mailing list sssd-devel(a)lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

* We also usually put the short description as the first line, an empty line, the full URL of the ticket fixed, an empty line and then a short description of the problem fixed. It's somewhat similar to kernel's process (see http://www.kernel.org/doc/Documentation/SubmittingPatches bullet point 15) for example) I realize this info is missing from https://fedorahosted.org/sssd/wiki/DevelTutorials and I'll add it there.

On 02/18/2013 05:20 PM, Jakub Hrozek wrote: >On Mon, Feb 18, 2013 at 12:44:10PM +0100, Jakub Hrozek wrote: >> * We also usually put the short description as the first line, an empty >> line, the full URL of the ticket fixed, an empty line and then a >> short description of the problem fixed. It's somewhat similar to >> kernel's process (see >> http://www.kernel.org/doc/Documentation/SubmittingPatches bullet point >> 15) for example) I realize this info is missing from >> https://fedorahosted.org/sssd/wiki/DevelTutorials and I'll add it >> there. >I added a loose guideline and an example to: >https://fedorahosted.org/sssd/wiki/DevelTutorials#Patchmetadata >_______________________________________________ >sssd-devel mailing list >sssd-devel(a)lists.fedorahosted.org >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel Hello, I'm sending the second patch to the ticket https://fedorahosted.org/sssd/ticket/1756 - sorry for starting the second thread earlier this day - commit message changed (hopefully) according to your recommendations - the second patch is squashed onto the first one, hope it's alright this way

On Thu, Feb 21, 2013 at 10:35:36AM +0100, Jakub Hrozek wrote: > On Mon, Feb 18, 2013 at 07:13:00PM +0100, Milan Cejnar wrote: >> On 02/18/2013 05:20 PM, Jakub Hrozek wrote: >>> On Mon, Feb 18, 2013 at 12:44:10PM +0100, Jakub Hrozek wrote: >>>> * We also usually put the short description as the first line, an empty >>>> line, the full URL of the ticket fixed, an empty line and then a >>>> short description of the problem fixed. It's somewhat similar to >>>> kernel's process (see >>>> http://www.kernel.org/doc/Documentation/SubmittingPatches bullet point >>>> 15) for example) I realize this info is missing from >>>> https://fedorahosted.org/sssd/wiki/DevelTutorials and I'll add it >>>> there. >>> I added a loose guideline and an example to: >>> https://fedorahosted.org/sssd/wiki/DevelTutorials#Patchmetadata >>> _______________________________________________ >>> sssd-devel mailing list >>> sssd-devel(a)lists.fedorahosted.org >>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel >> Hello, >> I'm sending the second patch to the ticket >> https://fedorahosted.org/sssd/ticket/1756 >> >> - sorry for starting the second thread earlier this day >> - commit message changed (hopefully) according to your recommendations >> - the second patch is squashed onto the first one, hope it's alright >> this way >> > The code looks good and works as advertised. I tested a case where the > error message was missing the newline previously (sss_cache) and one > that had it (sss_debuglevel). Both error messages look good to me. > > Ack Pushed to master. _______________________________________________ sssd-devel mailing list sssd-devel(a)lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel