naviserver-devel

[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");
>   }

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

[Vlad S. <vl...@cr...>]

I've been using that new module for so long time that i did not realized 
that the code is so different than
current style, i am sorry.

> 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?

No, i did not add anythig new,  X-Forwarded-For was handled by old nslog 
as well,
i just restructure the module to support more options

> 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.

nslog used to have index.html, in the CVS i do not see it, so i put the 
docs in the nslog.c for now and
we do not have docs for modules yet.


> 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.
> 
Yes, that's my fault, i will change it to follow naviserver code style.


> 
> 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");
>>  }
> 
> 
> 
> -------------------------------------------------------
> SF email is sponsored by - The IT Product Guide
> Read honest & candid reviews on hundreds of IT Products from real users.
> Discover which products truly live up to the hype. Start reading now.
> http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
> _______________________________________________
> naviserver-devel mailing list
> nav...@li...
> https://lists.sourceforge.net/lists/listinfo/naviserver-devel

-- 
Vlad Seryakov
571 262-8608 office
vl...@cr...
http://www.crystalballinc.com/vlad/

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

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

http://cvs.sourceforge.net/viewcvs.py/naviserver/naviserver/nslog/nslog.c?rev=1.3&view=log

The cvs commit message for nslog/nslog.c now reads:

"Add Ns_ConnSetResponseStatus command to be able to se response status
without touching headers.  Also ns_conn status has been extended with
ability to set response status if third argument is provided. ns_conn
status ?newStatus?"

Which will thoroughly confuse someone in the future.  Please be
careful about combining changes in a single commit.



On Thu, 03 Mar 2005 10:08:33 -0500, Vlad Seryakov
<vl...@cr...> wrote:
> I've been using that new module for so long time that i did not realized
> that the code is so different than
> current style, i am sorry.
> 
> > 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?
> 
> No, i did not add anythig new,  X-Forwarded-For was handled by old nslog
> as well,
> i just restructure the module to support more options
> 
> > 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.
> 
> nslog used to have index.html, in the CVS i do not see it, so i put the
> docs in the nslog.c for now and
> we do not have docs for modules yet.
> 
> 
> > 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.
> >
> Yes, that's my fault, i will change it to follow naviserver code style.

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

[Zoran V. <zv...@ar...>]

On Thursday 03 March 2005 09:43, Stephen Deasey wrote:
> 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.

Suggestion: we should try spearate these two steps: reformatting
and actual code changes.

I quite often merge these two in one, which makes the real changes
pretty obscure. But this is really wrong. One should first cleanup
then commit with the log clearly stated that no functional code 
changes have been done. Then one should go and add real changes,
of course by trying to follow the style guidelines. The latter are
quite obvious when skimming thru the rest of the code and eventually
the Tcl code (both styles are very similar). 

And, yes. I would also preffer to make an RFE or bug report in SF
before doing the change. This is for the sole purpose of trails 
and documentation. Otherwise, we will get lost very soon.

Zoran