httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Roy T. Fielding" <field...@gbiv.com>
Subject Re: cvs commit: httpd-2.0/modules/generators mod_info.c
Date Sat, 04 Sep 2004 01:30:12 GMT
On Sep 3, 2004, at 6:06 PM, Paul Querna wrote:

> On Fri, 2004-09-03 at 16:06 -0700, Roy T. Fielding wrote:
>> -0.9.   This change needs review.  The coding style should stick to
>>          the httpd guidelines.
>
> Yes, but I did not commit a correct style patch, because that would of
> been *impossible* to review.  The style of mod_info does not follow the
> guidelines, and Rici's original patch was much larger (including style
> fixes).  My plan was to make his functional changes first, and come 
> back
> later this weekend for a style cleanup of mod_info.

okay, that's a reasonable plan -- feel free to mention that in the
cvs log next time.

>>          Also, we don't add comments to a credit log inside the file
>>          (instead of just CHANGES), and whether or not God intended
>>          recursion is not relevant (though that part of the changes
>>          does look good).
>
> What is the Policy on this?  Should we remove the old ones from 
> existing
> files?  Sort of like how @author tags have been removed from other ASF
> projects...

We keep around things like "Originally written by ..." for entire
files, but nothing else.  Feel free to send a patch to the list if
you are unsure about something -- stuff like that takes time to
figure out the unwritten policy.

>> It is unfortunate that the diff gets confused about the new functions
>> and treats them as changes -- it makes review a pain in the butt.
>
> Like I said, this is the minimal patch to get the functional 
> differences
> for mod_info. mod_info already did not fit the style guide, and making
> Functional and Style changes in the same commit is even harder to
> review.

Well, in this case the cvs diff screwed the review anyway, but I
was actually talking about the style of the new code.  Feel free
to fix that later, but I figured I should mention it just in case
you hadn't seen the guidelines yet.

Style is important because it makes it easier to review changes
like that one.  I actually prefer to commit the style fix first
and then the change, since things like the usage of sprintf are
easier to see under our normal style.  This part just makes me
uncomfortable, like walking down a dark alley at night:

   +    char buf[32];
   +    if (thisfn == NULL) thisfn = "*UNKNOWN*";
   +    if (prevfn == NULL || 0 != strcmp(prevfn, thisfn)) {
   +        thisfn = ap_escape_html(r->pool, thisfn);
   +        ap_rprintf(r, "<dd><tt><strong>In file: 
%s</strong></tt></dd>\n", thisfn);
   +        ap_set_module_config(r->request_config, &info_module, 
thisfn);
        }
   +    ap_rputs("<dd><tt>", r);
   +    if (linenum > 0) sprintf(buf, "%d", linenum);
   +    else             buf[0] = '\0';
   +    for (i = strlen(buf); i < 4; ++i) ap_rputs("&nbsp;", r);
   +    ap_rputs(buf, r);
   +    ap_rputs(":&nbsp;", r);
   +    for (i = 1; i <= nest; ++i) ap_rputs("&nbsp;&nbsp;", r);

....Roy


Mime
View raw message