On Fri, Oct 11, 2013 at 09:42:05AM -0400, Simo Sorce wrote: > 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.
If Nikolai would agree to use something like Coccinelle (and perhaps, as Sumit said, compare the results), then I think we could be more certain that the patch doesn't break anything. Moreover, we have support for detecting formatting errors (-Wformat) now and all the existing warnings have been fixed. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
