You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by vijay <vi...@collab.net> on 2012/12/07 14:37:05 UTC
[PATCH] Replay report logs the entire session url in httpd access
log
Hi,
The REPORT request during replay is being logged in httpd access log
with the entire session url instead of the request path portion of the url.
E.g,
Now the REPORT request during 'svnsync sync' or 'svnrdump dump' is
logged in httpd access log as follows.
<snip>
127.0.0.1 - jrandom [07/Dec/2012:17:26:57 +0530] "REPORT
http://localhost:25001/svn-test-work/repositories/svnsync_tests-1
HTTP/1.1" 200 303
</snip>
After applying this patch, it will look like,
<snip>
127.0.0.1 - jrandom [07/Dec/2012:17:24:21 +0530] "REPORT
/svn-test-work/repositories/svnsync_tests-1 HTTP/1.1" 200 303
</snip>
Attached the patch and log message.
Thanks & Regards,
Vijayaguru
Re: [PATCH] Replay report logs the entire session url in httpd access log
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>> 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.
>
> The logging is really just a side-effect. We setup this structure in
> several places and set the path member to a path. In one place we put a
> full URL in by mistake. As far as I can tell the member is supposed to
> be a path and the URL is wrong. The fact that this field appears in a
> log file simply makes the error visible. The error is that the field
> contains an URL when it should contain a path.
Great, thanks - that's the level of understanding I was looking for. I would have got that if the log message said something like,
[[[
Fix a bug whereby, during a 'replay' using Serf, a URL was being
sent to the server instead of a path. The only effect we know of
is that the entire session URL was written to the httpd access log.
* subversion/libsvn_ra_serf/replay.c
(svn_ra_serf__replay, svn_ra_serf__replay_range): Initialize the
handler's 'path' field to the request path portion of the URL,
not to the full session URL.
]]]
Does that reflect it correctly?
- Julian
Re: [PATCH] Replay report logs the entire session url in httpd access
log
Posted by vijay <vi...@collab.net>.
Thanks, Philip and Julian.
I will explain things clearly in future patches.
Thanks & Regards,
Vijayaguru
On Friday 07 December 2012 10:02 PM, Julian Foad wrote:
> Philip Martin wrote:
>
>> Philip Martin <ph...@wandisco.com> writes:
>>> The logging is really just a side-effect. We setup this structure in
>>> several places and set the path member to a path. In one place we put a
>>> full URL in by mistake. As far as I can tell the member is supposed to
>>> be a path and the URL is wrong. The fact that this field appears in a
>>> log file simply makes the error visible. The error is that the field
>>> contains an URL when it should contain a path.
>>
>> I changed the log to:
>>
>> * subversion/libsvn_ra_serf/replay.c
>> (svn_ra_serf__replay, svn_ra_serf__replay_range): Use a request path,
>> rather than a full URL, where a path is expected. This bug showed
>> up as a full URL in the httpd access log.
>
> That's fine. Thanks, Philip.
>
> - Julian
>
Re: [PATCH] Replay report logs the entire session url in httpd access log
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>> The logging is really just a side-effect. We setup this structure in
>> several places and set the path member to a path. In one place we put a
>> full URL in by mistake. As far as I can tell the member is supposed to
>> be a path and the URL is wrong. The fact that this field appears in a
>> log file simply makes the error visible. The error is that the field
>> contains an URL when it should contain a path.
>
> I changed the log to:
>
> * subversion/libsvn_ra_serf/replay.c
> (svn_ra_serf__replay, svn_ra_serf__replay_range): Use a request path,
> rather than a full URL, where a path is expected. This bug showed
> up as a full URL in the httpd access log.
That's fine. Thanks, Philip.
- Julian
Re: [PATCH] Replay report logs the entire session url in httpd access log
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Julian Foad <ju...@btopenworld.com> writes:
>
>> 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.
>
> The logging is really just a side-effect. We setup this structure in
> several places and set the path member to a path. In one place we put a
> full URL in by mistake. As far as I can tell the member is supposed to
> be a path and the URL is wrong. The fact that this field appears in a
> log file simply makes the error visible. The error is that the field
> contains an URL when it should contain a path.
I changed the log to:
* subversion/libsvn_ra_serf/replay.c
(svn_ra_serf__replay, svn_ra_serf__replay_range): Use a request path,
rather than a full URL, where a path is expected. This bug showed
up as a full URL in the httpd access log.
Patch by: Vijayaguru G <vijay{_AT_}collab.net>
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: [PATCH] Replay report logs the entire session url in httpd access log
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
> 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.
The logging is really just a side-effect. We setup this structure in
several places and set the path member to a path. In one place we put a
full URL in by mistake. As far as I can tell the member is supposed to
be a path and the URL is wrong. The fact that this field appears in a
log file simply makes the error visible. The error is that the field
contains an URL when it should contain a path.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: [PATCH] Replay report logs the entire session url in httpd access log
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Julian Foad writes:
>> Philip Martin <ph...@wandisco.com>
>>> vijay <vi...@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
Re: [PATCH] Replay report logs the entire session url in httpd access log
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
> Philip Martin <ph...@wandisco.com>
>> vijay <vi...@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.
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.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: [PATCH] Replay report logs the entire session url in httpd access log
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin <ph...@wandisco.com>
> vijay <vi...@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.).
- Julian
Re: [PATCH] Replay report logs the entire session url in httpd access log
Posted by Philip Martin <ph...@wandisco.com>.
vijay <vi...@collab.net> writes:
> Attached the patch and log message.
Committed in r1418322. Thanks!
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download