[SSSD] [PATCH 3/4] Make DEBUG macro variadic

Simo Sorce simo at redhat.com
Fri Oct 11 13:42:05 UTC 2013


On Fri, 2013-10-11 at 15:21 +0200, Sumit Bose wrote:
> On Fri, Oct 11, 2013 at 08:53:11AM -0400, Simo Sorce wrote:
> > On Fri, 2013-10-11 at 11:09 +0300, Nikolai Kondrashov wrote:
> > > On 10/10/2013 11:27 PM, Simo Sorce wrote:
> > > > On Thu, 2013-10-10 at 15:46 -0400, Stephen Gallagher wrote:
> > > >> -----BEGIN PGP SIGNED MESSAGE-----
> > > >> Hash: SHA1
> > > >>
> > > >> On 10/10/2013 10:07 AM, Nikolai Kondrashov wrote:
> > > >>> Make DEBUG macro accept variable number of arguments, thus removing
> > > >>> the need to wrap the format string and its arguments into parens
> > > >>> on invocation.
> > > >>
> > > >> Nack.
> > > >>
> > > >> Please do us a favor and split this into two patches, one that makes
> > > >> the change to the macro definition and one that bulk-updates the
> > > >> existing messages. Having a 2.2MB patch file with a few small changes
> > > >> buried in it somewhere is very difficult to review. I'm not confident
> > > >> I can locate the real changes amidst the find-replace noise.
> > > >
> > > > I would not want to consider such a patch in any case. The churn is just
> > > > too extraordinarily big and I do not see enough value to warrant it.
> > > >
> > > > The patch is so immense that is it impossible to review, so we would
> > > > have to make a leap of faith in assuming every single change did not
> > > > introduce errors.
> > > 
> > > I'll try to reduce the amount of faith required. Maybe try a semantic patch
> > > [1], or do some metrics on the code and the changes.
> > > 
> > > > I am not against 'simplifying' the debug macro if possible, but then it
> > > > should probably be done in a backwards compatible way and debug
> > > > statements changed in time, just like we did when we switched from mere
> > > > numbers to error type definitions.
> > > >
> > > > For example we could call the new macro DBG, an start using it in place
> > > > of DEBUG in new code. DEBUG would be just a wrapper around DBG that
> > > > maintains the old interface.
> > > 
> > > I would advise against that. This will hurt maintainability, and I think
> > > having two ways to specify debug level is harmful as well.  This is how
> > > software rot happens. It is better to have a way to verify that such changes
> > > don't break anything (tests?) and do a change globally, or not change anything
> > > at all.
> > > 
> > > However, this is of course entirely up to you and I won't push this further as
> > > I'm only doing it for a sport, while waiting for Beaker job queue to unclog.
> > > 
> > > Sincerely,
> > > Nick
> > > 
> > > [1] http://coccinelle.lip6.fr/sp.php
> > 
> > I seriously like the simplification, but I am dead against such a
> > monstrous patch that touches *all* the code.
> > 
> > Although not ideal I am perfectly fine with changing code gradually like
> > we did (and are still doing) with the debug numbers.
> 
> I would say it is ok for master since 1.12 development is still in an
> early stage.
> 
> Would it help to make you more confident if someone does the same
> changes (maybe with some scripting magic) independently and then check
> if the patches differ?

Nope, there are 2 reasons I do not like such changes:
- automatic changes can break in subtle ways when you use sed-like
actions, and your eyes are not going to catch this stuff as we are not
going to manually review thousands upon thousands of changes like these.
Even if you did you'd miss stuff.
- history: every single git blame will bring up this patch in a lot of
places, so that you'll have to go through additional hoops to find out
the real change. Granted, given these are just debug messages, it may
not be a big deal in most cases, but it is a consideration I make in
general.

Finally if everybody agree they want to make this huge change, *then* I
absolutely want to see each change also change debug numbers from
numbers to the new macros. It makes no sense whatsoever to change the
DEBUG form and leave in old debug numbers that we have been
painstakingly changing little by little in the last few years.

The current patch does not convert numbers to debug macros. Moreover it
will not be really simple to do that in a way that will not screw up
indentation / 80 columns rule, but it is up to the proponent of the huge
patch to do that correctly :-) I am sure with enough dedication a script
to do the correction properly can probably be made.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list