You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Gavin Baumanis <ga...@thespidernet.com> on 2013/03/07 15:58:25 UTC
RE: Re[2]: [PATCH] implement keywords substitution in mod_dav_svn
Ping.
This thread has received no new comments.
> -----Original Message-----
> From: jinfroster [mailto:jinfroster@mail.ru]
> Sent: Sunday, 10 February 2013 11:20
> To: C. Michael Pilato
> Cc: dev@subversion.apache.org
> Subject: Re[2]: [PATCH] implement keywords substitution in mod_dav_svn
>
> Hello,
>
> CMP> On 02/04/2013 09:55 AM, C. Michael Pilato wrote:
> >> On 02/04/2013 06:30 AM, Philip Martin wrote:
> >>> jinfroster <ji...@mail.ru> writes:
> >>>
> >>>> Add "SVNKeywordSubstitution" per-directory (repository) mod_dav_svn
> >>>> configuration parameter (default is "Off"). Implement keywords
> >>>> substitution.
> >>>
> >>>> * subversion/mod_dav_svn/repos.c
> >>>> (set_headers):
> >>>> If parameter SVNKeywordSubstitution is On, don't send
> >>>> "Content-length". We can't guess the size of expanded stream at
> >>>> the moment (..is that bad?)
> >>>> (deliver):
> >>>> If parameter SVNKeywordSubstitution is On, perform keywords
> >>>> substitution, like client-side utilities do.
> >>>
> >>> Does your Subversion client use neon? I think this causes the
> >>> server to send expanded keywords in response to GET requests and so
> >>> will break Subversion clients using serf.
> >>
> >> Yes, that's precisely what it does. But the problem isn't limited to
> >> Serf clients. Any call to svn_ra_get_file() -- for example, to support
'svn
> cat'
> >> -- will suffer. So, yeah, cool idea, but unfortunately the patch is
> >> unacceptable as-is.
>
> CMP> Sorry, hit send too fast. That should be "... Any call to
> CMP> svn_ra_get_file() with Neon ..."
>
> You are right! With this patch SVN client complains on bad checksums...
> Sorry, didn't test that well.
> But SVN clients do keywords substitution themselves. The idea was to
> implement substitution for dumb HTTP clients which are missing it.
>
> Would it be sufficent to check 'is_svn_client'?
>
> if (dav_svn__get_keyword_substitution_flag(resource->info->r)
> && !resource->info->repos->is_svn_client)
> {
> ...
>
> If there are more troubles/questions, please point on them to me.
> I'm willing to work on this patch if it has chances to be accepted :)
>
> --
> Best regards,
> jinfroster mailto:jinfroster@mail.ru
Re: [PATCH] perform keywords substitution in mod_dav_svn
Posted by Daniel Shahaf <da...@elego.de>.
jinfroster wrote on Sun, Apr 07, 2013 at 21:38:07 +0400:
> Hello, dev!
> Here is my second attempt:
>
> [[[
> Accept "kw" parameter in query string. Treat any value except "0" as a
> request to perform server-side keywords substitution in file contents.
I would suggest requiring "1" as the value, so that 1.9 can assign
a different meaning to kw=2 etc.
Also please send patches as text/plain (*.txt extension usually causes
that).
Re: Re[2]: [PATCH] perform keywords substitution in mod_dav_svn
Posted by Julian Foad <ju...@btopenworld.com>.
jinfroster wrote:
> CMP> Finished.
>
> CMP> Sending subversion/mod_dav_svn/dav_svn.h
> CMP> Sending subversion/mod_dav_svn/repos.c
> CMP> Transmitting file data ..
> CMP> Committed revision 1466055.
>
> Great! Thanks a lot, Michael!
I'm just curious... Do you have any desire for a similar feature for enabling end-of-line translation on the server? I ask because keyword expansion and end-of-line translation are the two transforms that svn clients typically apply to text from the repository, and when one is useful, often both are useful.
- Julian
Re[2]: [PATCH] perform keywords substitution in mod_dav_svn
Posted by jinfroster <ji...@mail.ru>.
CMP> Finished.
CMP> Sending subversion/mod_dav_svn/dav_svn.h
CMP> Sending subversion/mod_dav_svn/repos.c
CMP> Transmitting file data ..
CMP> Committed revision 1466055.
Great! Thanks a lot, Michael!
--
Best regards,
jinfroster mailto:jinfroster@mail.ru
Re: [PATCH] perform keywords substitution in mod_dav_svn
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 04/09/2013 09:46 AM, C. Michael Pilato wrote:
> On 04/07/2013 01:38 PM, jinfroster wrote:
>> + keyword_subst = apr_table_get(pairs, "kw");
>> + if (keyword_subst && *keyword_subst != '0')
>> + comb->priv.keyword_subst = TRUE;
>> +
>> return NULL;
>> }
>
> I'm with Daniel -- this should check for "1" explicitly.
In this function, I also caused mod_dav_svn to preserve the "kw=1" flag when
redirecting. Without this change, a request like:
.../path/to/file?r=100&kw=1
would get redirected to:
.../path/to/file?p=100
(Note the missing "kw" flag.) That's fixed in the version I committed.
>> + str_uri = apr_psprintf(resource->pool,
>> + "%s%s", resource->info->repos->base_url,
>> + resource->info->r->uri);
>
> This URL construction doesn't feel quite right. I *think* you want:
Turns out you were mostly correct after all. Just needed to ap_escape_uri()
the r->uri bit.
> All that said, I think you've given us enough to work with here, so I'll try
> to patch up your patch, test, and commit.
Finished.
Sending subversion/mod_dav_svn/dav_svn.h
Sending subversion/mod_dav_svn/repos.c
Transmitting file data ..
Committed revision 1466055.
Thanks for the patch and for patiently working through our patch submission
process.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: [PATCH] perform keywords substitution in mod_dav_svn
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 04/07/2013 01:38 PM, jinfroster wrote:
> Index: subversion/mod_dav_svn/dav_svn.h
> ===================================================================
> --- subversion/mod_dav_svn/dav_svn.h (revision 1465320)
> +++ subversion/mod_dav_svn/dav_svn.h (working copy)
> @@ -289,6 +289,9 @@
>
> /* Cache any revprop change error */
> svn_error_t *revprop_error;
> +
> + /* was keywords subsitution requested by client? (ie: /path/to/item?kw=1)*/
> + svn_boolean_t keyword_subst;
> };
Some typos there. "substitution".
> Index: subversion/mod_dav_svn/repos.c
> ===================================================================
> --- subversion/mod_dav_svn/repos.c (revision 1465320)
> +++ subversion/mod_dav_svn/repos.c (working copy)
[...]
> @@ -1924,6 +1926,10 @@
> 0, "redirecting to canonical location");
> }
>
> + keyword_subst = apr_table_get(pairs, "kw");
> + if (keyword_subst && *keyword_subst != '0')
> + comb->priv.keyword_subst = TRUE;
> +
> return NULL;
> }
I'm with Daniel -- this should check for "1" explicitly.
> @@ -3026,18 +3032,23 @@
>
>
> /* if we aren't sending a diff, then we know the length of the file,
> - so set up the Content-Length header */
> - serr = svn_fs_file_length(&length,
> - resource->info->root.root,
> - resource->info->repos_path,
> - resource->pool);
> - if (serr != NULL)
> + so set up the Content-Length header.
> + Except when keywords substitution was requested (length may increase
> + during deliver) */
> + if (resource->info->keyword_subst == FALSE)
Just use "if (!resource->info->keyword_subst)" -- no need to explicitly test
for the value "FALSE".
> @@ -3563,6 +3574,60 @@
> resource->pool);
> }
>
> + /* Perform keywords substitution if requested by client */
> + if (resource->info->keyword_subst == TRUE)
Same here, but the reverse.
> + {
> + const char *err_msg_props = "could not get revision properties"
> + " for keywords substitution";
> + svn_string_t *keywords;
> + serr = svn_fs_node_prop(&keywords,
> + resource->info->root.root,
> + resource->info->repos_path,
> + SVN_PROP_KEYWORDS,
> + resource->pool);
> + if (serr != NULL)
> + return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> + err_msg_props, resource->pool);
Here, you return an error message about revision properties when you've just
tried to fetch *node* properties. In fact, I'm not thrilled about this
reused error message at all. Why not take the time to be give specific
errors for each specific problem we might hit?
> +
> + if (keywords)
> + {
> + apr_hash_t *kw;
> + svn_revnum_t cmt_rev;
> + char *str_cmt_rev, *str_uri;
> + const char *cmt_date, *cmt_author;
> + apr_time_t when = 0;
> +
> + serr = svn_repos_get_committed_info(&cmt_rev, &cmt_date, &cmt_author,
> + resource->info->root.root,
> + resource->info->repos_path,
> + resource->pool);
> + if (serr != NULL)
> + return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> + err_msg_props, resource->pool);
> +
> + str_cmt_rev = apr_psprintf(resource->pool,
> + "%ld", cmt_rev);
> + str_uri = apr_psprintf(resource->pool,
> + "%s%s", resource->info->repos->base_url,
> + resource->info->r->uri);
This URL construction doesn't feel quite right. I *think* you want:
svn_path_url_add_component2(resource->info->repos->base_url,
resource->info->repos_path,
resource->pool);
> + serr = svn_time_from_cstring(&when, cmt_date, resource->pool);
> + if (serr != NULL)
> + return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> + err_msg_props, resource->pool);
Again, more sloppy error construction.
All that said, I think you've given us enough to work with here, so I'll try
to patch up your patch, test, and commit.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
[PATCH] perform keywords substitution in mod_dav_svn
Posted by jinfroster <ji...@mail.ru>.
Hello, dev!
Here is my second attempt:
[[[
Accept "kw" parameter in query string. Treat any value except "0" as a
request to perform server-side keywords substitution in file contents.
* subversion/mod_dav_svn/dav_svn.h
(struct dav_resource_private): add keyword_subst flag.
* subversion/mod_dav_svn/repos.c
(svn_subst.h): Add #include.
(parse_querystring): Set keyword_subst flag if query string contains
"kw" parameter with any value except "0".
(set_headers):
If keyword_subst flag is set, don't send "Content-length". We
can't guess the size of expanded stream at the moment.
(deliver):
If keyword_subst flag is set, perform keywords substitution like
client-side utilities do.
]]]
The URL looks like this:
http://example.com/svn/project/file.txt?p=123&kw=1
VB> On Fri, Mar 8, 2013 at 2:24 PM, Ben Reser <be...@reser.org> wrote:
VB> On Fri, Mar 8, 2013 at 11:12 AM, Vladimir Berezniker <vm...@hitechman.com> wrote:
>> If someone does not mind explaining. ═What would be the benefit of having
>> this decision be made by the server vs the client having to request that via
>> a explicit parameter on the request?
VB> It's not so much of a benefit as it is a compatibility problem. ═We
VB> support WebDAV clients with auto-versioning. ═WebDAV+auto-versioning
VB> lets you mount a SVN repository as though it was a regular file system
VB> and just start making changes with each change automatically committed
VB> as a revision. ═These clients are at current indistinguishable from a
VB> web browser, they make similar requests that a web browser would and
VB> don't have some pattern we can key off in the GET request.
VB> When using these clients it's important that we don't present to them
VB> a different version of the file than what the repository actually
VB> stores. ═Otherwise we break this functionality.
VB> Using a query parameter means you can ask for the keyword expansion
VB> without breaking these clients.
VB> Query parameter makes perfect sense to me. ═Thank you.═
--
Best regards,
jinfroster mailto:jinfroster@mail.ru
Re: [PATCH] implement keywords substitution in mod_dav_svn
Posted by Vladimir Berezniker <vm...@hitechman.com>.
On Fri, Mar 8, 2013 at 2:24 PM, Ben Reser <be...@reser.org> wrote:
> On Fri, Mar 8, 2013 at 11:12 AM, Vladimir Berezniker <vm...@hitechman.com>
> wrote:
> > If someone does not mind explaining. What would be the benefit of having
> > this decision be made by the server vs the client having to request that
> via
> > a explicit parameter on the request?
>
> It's not so much of a benefit as it is a compatibility problem. We
> support WebDAV clients with auto-versioning. WebDAV+auto-versioning
> lets you mount a SVN repository as though it was a regular file system
> and just start making changes with each change automatically committed
> as a revision. These clients are at current indistinguishable from a
> web browser, they make similar requests that a web browser would and
> don't have some pattern we can key off in the GET request.
>
> When using these clients it's important that we don't present to them
> a different version of the file than what the repository actually
> stores. Otherwise we break this functionality.
>
> Using a query parameter means you can ask for the keyword expansion
> without breaking these clients.
>
Query parameter makes perfect sense to me. Thank you.
Re: [PATCH] implement keywords substitution in mod_dav_svn
Posted by Ben Reser <be...@reser.org>.
On Fri, Mar 8, 2013 at 11:12 AM, Vladimir Berezniker <vm...@hitechman.com> wrote:
> If someone does not mind explaining. What would be the benefit of having
> this decision be made by the server vs the client having to request that via
> a explicit parameter on the request?
It's not so much of a benefit as it is a compatibility problem. We
support WebDAV clients with auto-versioning. WebDAV+auto-versioning
lets you mount a SVN repository as though it was a regular file system
and just start making changes with each change automatically committed
as a revision. These clients are at current indistinguishable from a
web browser, they make similar requests that a web browser would and
don't have some pattern we can key off in the GET request.
When using these clients it's important that we don't present to them
a different version of the file than what the repository actually
stores. Otherwise we break this functionality.
Using a query parameter means you can ask for the keyword expansion
without breaking these clients.
Re: [PATCH] implement keywords substitution in mod_dav_svn
Posted by Vladimir Berezniker <vm...@hitechman.com>.
If someone does not mind explaining. What would be the benefit of having
this decision be made by the server vs the client having to request that
via a explicit parameter on the request?
Thank you,
Vladimir
On Thu, Mar 7, 2013 at 3:41 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> C. Michael Pilato wrote on Thu, Mar 07, 2013 at 13:55:15 -0500:
> > [1] What happens if [a DAV] client screws up our "repository normal
> > format" -- expanding keywords or futzing with newlines -- when
> > PUTting a new version today?
>
> The non-RNF form gets committed, I think. Or did we install server-side
> validation of these things when I wasn't looking?
>
Re: [PATCH] implement keywords substitution in mod_dav_svn
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Thu, Mar 07, 2013 at 13:55:15 -0500:
> [1] What happens if [a DAV] client screws up our "repository normal
> format" -- expanding keywords or futzing with newlines -- when
> PUTting a new version today?
The non-RNF form gets committed, I think. Or did we install server-side
validation of these things when I wasn't looking?
Re: [PATCH] implement keywords substitution in mod_dav_svn
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 03/07/2013 12:21 PM, Bert Huijben wrote:
>> Then those proxy servers are already interfering with existing clients,
>> and preventing those clients from reporting capabilities, from storing
>> and fetch file lock metadata correctly, etc.
>
> I think we use different headers for the user agent and the capabilities
> and most other things.
>
> Proxies suppressing all non-default headers would have problems, but the
> user agent is sometimes an easy tweak to reduce the attack surface.
What I meant was that mod_dav_svn only bothers to parse a capabilities
header at all if the User-Agent string has "SVN/". If a proxy is stripping
User-Agent out, then I daresay that client is mergeinfo-disabled as a result
of this.
> Another possible issue: What about standard DAV clients?
> Should these obtain the keywords collapsed or expanded.
Ah! Now that's the rub! (Good catch, Bert.) We do *not* want a standard
DAV client GETting a resource with keywords expanded, tweaking it, and then
PUTting it back into the repository with expanded keywords.[1]
So it would seem that we would not want this behavior to be the default for
a GET request, regardless of the client requesting it. We could make it an
option toggleable via the query string portion of the URL -- even
automatically add that flag in the URLs presented by a GET of the containing
directory. But no, a standard GET request against the public URL should not
expand keywords.
-- C-Mike
[1] What happens if such a client screws up our "repository normal
format" -- expanding keywords or futzing with newlines -- when
PUTting a new version today?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
RE: [PATCH] implement keywords substitution in mod_dav_svn
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: C. Michael Pilato [mailto:cmpilato@collab.net]
> Sent: donderdag 7 maart 2013 17:32
> To: Bert Huijben
> Cc: 'Gavin Baumanis'; 'jinfroster'; dev@subversion.apache.org
> Subject: Re: [PATCH] implement keywords substitution in mod_dav_svn
>
> On 03/07/2013 11:27 AM, Bert Huijben wrote:
> >> Yeah, I *think* using the "is_svn_client" flag is acceptable. I can't
> >> remember now how much weight we attributed to that flag (which is set
> >> based on a grep of the User-Agent string). The downside here is that
> >> if there happens to be a Subversion client that doesn't report itself
> >> as such in this way, it will presumably run into the same sorts of
> >> issues we've already discussed. But I know of no such client, and
> >> maybe we as a community are willing to say, "Look, if you are to be a
> >> well-behaved Subversion client, you've gotta slap those four characters
> >> "SVN/" in your User-Agent header value.
> >
> > What about proxy servers?
> >
> > There used to be privacy features in several proxies that suppressed the
> > user agent. (Not sure if they still use that trick).
>
> Then those proxy servers are already interfering with existing clients, and
> preventing those clients from reporting capabilities, from storing and fetch
> file lock metadata correctly, etc.
I think we use different headers for the user agent and the capabilities and most other things.
Proxies suppressing all non-default headers would have problems, but the user agent is sometimes an easy tweak to reduce the attack surface.
Another possible issue: What about standard DAV clients?
Should these obtain the keywords collapsed or expanded.
Bert
Re: [PATCH] implement keywords substitution in mod_dav_svn
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 03/07/2013 11:27 AM, Bert Huijben wrote:
>> Yeah, I *think* using the "is_svn_client" flag is acceptable. I can't
>> remember now how much weight we attributed to that flag (which is set
>> based on a grep of the User-Agent string). The downside here is that
>> if there happens to be a Subversion client that doesn't report itself
>> as such in this way, it will presumably run into the same sorts of
>> issues we've already discussed. But I know of no such client, and
>> maybe we as a community are willing to say, "Look, if you are to be a
>> well-behaved Subversion client, you've gotta slap those four characters
>> "SVN/" in your User-Agent header value.
>
> What about proxy servers?
>
> There used to be privacy features in several proxies that suppressed the
> user agent. (Not sure if they still use that trick).
Then those proxy servers are already interfering with existing clients, and
preventing those clients from reporting capabilities, from storing and fetch
file lock metadata correctly, etc.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
RE: [PATCH] implement keywords substitution in mod_dav_svn
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: C. Michael Pilato [mailto:cmpilato@collab.net]
> Sent: donderdag 7 maart 2013 17:23
> To: Gavin Baumanis
> Cc: 'jinfroster'; dev@subversion.apache.org
> Subject: Re: [PATCH] implement keywords substitution in mod_dav_svn
>
> On 03/07/2013 09:58 AM, Gavin Baumanis wrote:
> > Ping.
> > This thread has received no new comments.
>
> (Thanks, Gavin.)
>
> >> You are right! With this patch SVN client complains on bad checksums...
> >> Sorry, didn't test that well.
> >> But SVN clients do keywords substitution themselves. The idea was to
> >> implement substitution for dumb HTTP clients which are missing it.
> >>
> >> Would it be sufficent to check 'is_svn_client'?
> >>
> >> if (dav_svn__get_keyword_substitution_flag(resource->info->r)
> >> && !resource->info->repos->is_svn_client)
> >> {
> >> ...
> >>
> >> If there are more troubles/questions, please point on them to me.
> >> I'm willing to work on this patch if it has chances to be accepted :)
>
> Yeah, I *think* using the "is_svn_client" flag is acceptable. I can't
> remember now how much weight we attributed to that flag (which is set
> based
> on a grep of the User-Agent string). The downside here is that if there
> happens to be a Subversion client that doesn't report itself as such in this
> way, it will presumably run into the same sorts of issues we've already
> discussed. But I know of no such client, and maybe we as a community are
> willing to say, "Look, if you are to be a well-behaved Subversion client,
> you've gotta slap those four characters "SVN/" in your User-Agent header
> value.
What about proxy servers?
There used to be privacy features in several proxies that suppressed the user agent. (Not sure if they still use that trick).
Bert
Re: [PATCH] implement keywords substitution in mod_dav_svn
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 03/07/2013 09:58 AM, Gavin Baumanis wrote:
> Ping.
> This thread has received no new comments.
(Thanks, Gavin.)
>> You are right! With this patch SVN client complains on bad checksums...
>> Sorry, didn't test that well.
>> But SVN clients do keywords substitution themselves. The idea was to
>> implement substitution for dumb HTTP clients which are missing it.
>>
>> Would it be sufficent to check 'is_svn_client'?
>>
>> if (dav_svn__get_keyword_substitution_flag(resource->info->r)
>> && !resource->info->repos->is_svn_client)
>> {
>> ...
>>
>> If there are more troubles/questions, please point on them to me.
>> I'm willing to work on this patch if it has chances to be accepted :)
Yeah, I *think* using the "is_svn_client" flag is acceptable. I can't
remember now how much weight we attributed to that flag (which is set based
on a grep of the User-Agent string). The downside here is that if there
happens to be a Subversion client that doesn't report itself as such in this
way, it will presumably run into the same sorts of issues we've already
discussed. But I know of no such client, and maybe we as a community are
willing to say, "Look, if you are to be a well-behaved Subversion client,
you've gotta slap those four characters "SVN/" in your User-Agent header value.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development