Date: Tue, 16 Mar 2010 19:53:11 -0700 From: Doug Barton <dougb@FreeBSD.org> To: Martin Pala <martinp@tildeslash.com> Cc: Wen Heping <wen@FreeBSD.org>, cvs-ports@FreeBSD.org, cvs-all@FreeBSD.org, ports-committers@FreeBSD.org Subject: Re: cvs commit: ports/sysutils/monit/files monit.sh.in Message-ID: <4BA04417.5000309@FreeBSD.org> In-Reply-To: <0E8E3FB7-40B8-4666-A3B6-462621F43B98@tildeslash.com> References: <201003160209.o2G29ieE041601@repoman.freebsd.org> <4B9EEFA3.80106@FreeBSD.org> <0E8E3FB7-40B8-4666-A3B6-462621F43B98@tildeslash.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 03/16/10 19:12, Martin Pala wrote: > > On Mar 16, 2010, at 3:40 AM, Doug Barton wrote: >>> http://www.FreeBSD.org/cgi/cvsweb.cgi/ports/sysutils/monit/files/monit.sh.in.diff?&r1=1.1&r2=1.2&f=h >> >> >>> Documenting the _flags option in comments is an improvement, yes. >> Everything else is a pessimization; particularly since the default >> assignment for monit_enable is now broken. >> >> I've attached a patch that fixes the following issues: 1. General >> re-sorting to match conventions. 2. Change the default for _enable >> to the conventional method, and fix it as a side effect. Also move >> it down past load_rc_config. Otherwise testing for a value first is >> meaningless. 3. Eliminate the need for $default_config >> >> Martin, please test this and respond ASAP. Since the script as >> committed is now broken, it needs to be fixed. >> > > Hi, > > no, the script is not broken - it works the same as before, i have > tested it and found no issues - if you can reproduce any problem, > please provide details. The monit_enable works OK with both values. Remove or comment out monit_enable from your rc.conf file, and test it again. Also, look carefully at the test and you should be able to spot the error. > The testing of monit_enable value before load_rc_config works OK - it > should always fail and the monit_enable is set to "NO", which can be > overridden by load_rc_config (is executed post this setting). Doing the test before load_rc_config is pointless since the value will always be empty. You might just as well set the default value unconditionally since that's what's going to happen anyway. > I think your patch is not necessary - it does cosmetic modifications > only. For example apache rc script has very similar structure as > monit rc script. Yes, I agree that _some_ of the modifications are cosmetic, and said so. However, fixing the test for monit_enable and removing the empty assignment to monit_flags are not cosmetic. I've committed those two changes just now since the port can't continue to be broken. If you choose not to accept the other changes, that's up to you, no hard feelings either way. :) Meanwhile, a quick word about cutting and pasting from other people's examples. There is more or less a "default" style for the rc.d scripts as exemplified in the scripts in the base, and the documentation, such as http://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/rc-scripts.html. Having a default style makes it easier for people to review existing code, new code, and proposed changes. It also makes debugging easier. Existing examples of code that doesn't follow the conventions should be considered an example of how we are generally "liberal in what we accept," not as an example or justification of how to do things in other scripts. Doug -- ... and that's just a little bit of history repeating. -- Propellerheads Improve the effectiveness of your Internet presence with a domain name makeover! http://SupersetSolutions.com/
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4BA04417.5000309>