> On May 30, 2014, 9:53 a.m., Matt Jordan wrote: > > /branches/1.8/main/config.c, lines 1552-1570 > > <https://reviewboard.asterisk.org/r/3575/diff/2/?file=59028#file59028line1552> > > > > The indentation here is weird. Is it intentional that the following do > > loop is included with the else of the glob statement? > > > > If so, a comment at least indicating that would be nice. > > rmudgett wrote: > Yes it is intentional and the do/while loop is actually in the body of > the for loop within the else clause. It has been this way from before > revision -r6000. We're talking about ancient code here where readability was > the next persons problem. This routine badly needs refactoring. At the > least it should be indented correctly rather than having a chunk of it > outdented/unindented by a few levels. > > The AST_INCLUDE_GLOB code could always be included because it is always > defined. > > Just adding a comment that the indentation is screwey rather than > actually fixing the indentation doesn't seem right. > > Matt Jordan wrote: > I don't think refactoring this is really warranted, but a comment that > says "yes, this does end up under the previous else statement" is warranted. > Please add something for future maintainers of this code.
Why don't I just indent this function properly as a separate whitespace only patch? That would be better than a comment saying the indentation is screwed up. - rmudgett ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3575/#review12012 ----------------------------------------------------------- On May 30, 2014, 12:39 p.m., rmudgett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3575/ > ----------------------------------------------------------- > > (Updated May 30, 2014, 12:39 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-23683 > https://issues.asterisk.org/jira/browse/ASTERISK-23683 > > > Repository: Asterisk > > > Description > ------- > > The twisted logic determining if a config file should be reloaded was mostly > broken and disabled. The incorrect test that ASTERISK-23383 fixed actually > reenabled the broken logic. The incorrect test was causing the timestamp to > always be cleared which caused config files with includes to always be > reloaded. > > * Made wildcard includes always cause a reload. Determining if a file was > deleted cannot be determined without restructuring the cache to determine if > any files are missing from the last files actually loaded. Also without > refactoring config_text_file_load(), the glob loop couldn't check more than > one file for changes anyway. > > * Made remove the cache entry if the file no longer exists when trying to get > its timestamp or it is no longer a regular file. This fixes the corner case > where the file was loaded, then deleted, then the config reloaded, then the > file restored with the same timestamp, and then the config reloaded again. > > * Made remove the cache entry include list when actually loading the file. > This gets rid of any stale includes the file had from the last time the file > was loaded. > > > Diffs > ----- > > /branches/1.8/main/config.c 414961 > > Diff: https://reviewboard.asterisk.org/r/3575/diff/ > > > Testing > ------- > > Made sip.conf include a wildcard pattern. The config was always reloaded > even if no files were touched. > > Made sip.conf include several specific files. The config was reloaded when > any file was touched. > > Edited sip.conf to change which files the file included. Verified with debug > statements added to determine that the old include files were no longer > checked on subsequent reload attempts. > > > Thanks, > > rmudgett > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
