Re: [PATCH] Replay report logs the entire session url in httpd access log

Philip Martin wrote:

> Julian Foad writes:
>> Philip Martin <philip.martin_at_wandisco.com>
>>> vijay <vijay_at_collab.net> writes:
>>>>   Attached the patch and log message.
>>>
>>> Committed in r1418322. Thanks!
>>
>> Hi Philip and Vijay.
>>
>> Your log message:
>> [[[
>> * subversion/libsvn_ra_serf/replay.c
>>   (svn_ra_serf__replay, svn_ra_serf__replay_range): Replace the
>>     session url string with the request path portion of the url.
>> ]]]
>>
>> leaves me thinking, "But *why* did you replace the session url string
> with the request path portion of the url?"
>>
>> Please edit the log message to say why this change was made and what
>> it means in functional terms (like, what behaviour does it change,
>> does it fix a bug, etc.).
>
> I don't think a log message is the place for a detailed explanation of
> why a member called 'path' should contain a path rather than a full URL.

I absolutely agree.

> That's the sort of documentation that should go in the code.  I suppose
> a log message like: "Set the handler's path member to a path instead of
> a complete URL" might be better.

That would be a little bit better because it gives a hint that it is correcting some sort of error.

But isn't the subject line of this email a relevant clue to what's happening?  I don't 100% grok it, but it sounds like the issue being adressed by this patch must be something like:

  The mod_dav_svn replay report was logging the entire session
  URL in the httpd access log.  Make it do what it is supposed
  to do, which is log just the path-within-repos.

No?

(Please sanity-check that because I'm just guessing.)

- Julian
Received on 2012-12-07 16:02:00 CET