Comment 6 for bug 741088

Revision history for this message
Dan Scott (denials) wrote :

Thanks again, Joseph. I've looked over your patch, and there are a few bits left to clean up - but it's very close.

1) Please don't change "prefix=@prefix@" to "prefix=/openils"; this breaks autoconf's ability to specify a different --prefix option to configure and have it affect the osrf_ctl.sh script that is produced from osrf_ctl.sh.in. Same for exec_prefix and OSRF_CONFIG.

2) Please don't mix changes to whitespace (removing trailing spaces at the end of lines, or converting tabs to spaces or vice versa) with actual code changes. You're welcome to submit a separate patch with the whitespace changes, ideally after the functional patch. We want to be able to focus on what changed in a given line without having to determine if it was just a whitespace change.

3) A very minor nit - I'd prefer to see PID in all caps in comments and user-visible text.

4) The spacing on the clear_pid action at the bottom of the patch is off; all the other options are prefixed by a tab. Again, minor.

Functionally, the patch works; I've given it a test run with no visible problems. If you can address these minor points, I think we're golden!