[naviserver-devel] Re: [naviserver-commits] naviserver/nslog nslog.c,1.1.1.1,1.2

[Stephen D. <sd...@gm...>]

I've noticed some problems with this recent change.


The log message doesn't say what has changed, just that something has
changed, although the ChangeLog entry is better.  The X-Forwarded-For
change is a bug fix and is not mentioned in either.  I thought we'd
already agreed that non-trivial changes should have an RFE or BUG
posted to the tracker?


It's unreasonable to expect end users to examine the source code for
documentation, so we should not clutter up the code with it.  There
are no other source files I'm aware of which do this.


The members of the Log struct have been re-arranged.  Variable
declarations beginning a function have moved around.  Many of the
comments have been reformatted.  Correctly formatted code has had
braces and spacing removed, etc.  This all makes it very tricky to
read diffs to figure out what has logically changed.


Code like the following:

logPtr->file = Ns_ConfigGetValue(path, "file");
if (logPtr->file == NULL) {
    logPtr->file = "access.log";
}

Has been changed into code like this:

if(!(logPtr->file = Ns_ConfigGetValue(path,"file"))) logPtr->file =
"access.log";
logPtr->file = ns_strdup(logPtr->file);

This is not an improvement in readability, and doesn't follow our code
formatting conventions.


The RCSID static variable is missing.



On Tue, 01 Mar 2005 21:39:59 +0000, Vlad Seryakov
<ser...@us...> wrote:
> Update of /cvsroot/naviserver/naviserver/nslog
> In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv2963/nslog
> 
> Modified Files:
>         nslog.c
> Log Message:
> Added more options to the module and to the command ns_accesslog
> 
> Index: nslog.c
> ===================================================================
> RCS file: /cvsroot/naviserver/naviserver/nslog/nslog.c,v
> retrieving revision 1.1.1.1
> retrieving revision 1.2
> diff -C2 -d -r1.1.1.1 -r1.2
> *** nslog.c     16 Feb 2005 08:41:00 -0000      1.1.1.1
> --- nslog.c     1 Mar 2005 21:39:57 -0000       1.2
> ***************
> *** 1,10 ****
>   /*
> !  * The contents of this file are subject to the AOLserver Public License
> !  * Version 1.1 (the "License"); you may not use this file except in
>    * compliance with the License. You may obtain a copy of the License at
> !  * http://aolserver.com/.
>    *
>    * Software distributed under the License is distributed on an "AS IS"
> !  * basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See
>    * the License for the specific language governing rights and limitations
>    * under the License.
> [...1216 lines suppressed...]
> !     logPtr->extheaders = ns_calloc((size_t)(i + 1), sizeof *logPtr->extheaders);
> !
> !     logPtr->extheaders[0] = config;
> !
> !     for (i = 1, p = config; *p; p++) {
> !       if (*p == ',') {
> !           *p = '\000';
> !           logPtr->extheaders[i++] = p + 1;
> !       }
> !     }
> !
> !     logPtr->extheaders[i] = NULL;
> !
>   }
> -
> --- 736,739 ----
>   LogRollCallback(void *arg)
>   {
> !     LogCallback(LogRoll,arg,"roll");
>   }