On Thu, Nov 15, 2012 at 08:01:34PM +0100, Jakub Hrozek wrote:
On Thu, Nov 15, 2012 at 10:47:10AM -0800, Paul B. Henson wrote:
> On 11/15/2012 5:45 AM, Stephen Gallagher wrote:
>
> >This is obviously already a significant enhancement, and of course the
> >difference will be more pronounced for much larger environments. I'm
> >prepared to give this an ack, with one comment to whoever pushes the
> >patch upstream: please reflow the changes in nsssrv_cmd.c's fill_grent()
> >that are now exceeding the 80-character line limit due to the new
> >indentation.
>
> Hmm, I see a few other lines in that file that exceed 80 characters;
> are there scenarios where there is an exception to that limit or
> were those just not caught ;)?
They slipped through the review, most probably. There's no hard rule,
the 80-char limit just makes the code more readable for people like me
who like to split their terminals vertically.
>
> It looks like there were only two lines in my diff for that file
> that exceeded 80 characters, the attached updated patch should fix
> that.
>
Looks good to me, now. There's one more log line in groups_get_send but
that one is a) hard to split, so it's a reasonable exception and b) that
whole function doesn't respect the limit.
I tested the patch as well, works fine, the code looks great to me, to..
Thank you very much for the contribution!
I included the ticket URL in the commit message and pushed the patch to
master.