Dear Mr. Hrozek, I really appreciate all your advice. I tried to correct mentioned mistakes, changed the code to call the fprintf() only once and reposted the patch anew with now hopefully correct subject format and with ticket URL included.
Best Regards, Milan Cejnar On Mon, Feb 18, 2013 at 12:44 PM, Jakub Hrozek <jhro...@redhat.com> wrote: > 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@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel >
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel