Hi, Amos, Alex, Thanks a lot for your comments! They are really good advice.
The updated patch is attached. It addresses all the issues you mentioned except whether to deprecate the "enable"/"disable" options, which is worth of discussion. Thanks, Tianyin On Thu, Jan 10, 2013 at 8:14 AM, Alex Rousskov <[email protected]> wrote: > On 01/05/2013 05:12 PM, Tianyin Xu wrote: > >> @@ -193,6 +290,8 @@ >> return false; >> } else if ((port = strtol(token, &tmp, 10)), !*tmp) { >> /* port */ >> + port = xatos(token); >> + >> } else { >> host = token; >> port = 0; > > Please do not set "port" twice because it is confusing and the comma > operator in the expression only makes things worse. Do something like > this instead: > > else if (strtol(token, &tmp, 10) && !*tmp) { > port = xatos(token); > } > > >> + debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: 'enable' and >> 'disable' are deprecated. Please update to use value 'on' and 'off'."); > > s/value/values/ > > or simply remove the word "value". > > IMO, it would be better to make the above message specific to 'enable' > or 'disable' case, so that the admin knows which value to grep for in > the config file. > > Have others discussed deprecation of enable/disable? If not, perhaps > they do not need to be deprecated at all? > > > Thank you, > > Alex. > -- Tianyin XU, http://cseweb.ucsd.edu/~tixu/
config.patch
Description: Binary data
