[PATCH/RFC 0/2] Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.
Hi Eric,
Michael G Schwern wrote:
> On 2012.7.28 6:50 AM, Jonathan Nieder wrote:
>> Michael G Schwern wrote:
>>> --- a/perl/Git/SVN/Utils.pm
>>> +++ b/perl/Git/SVN/Utils.pm
>> [...]
>>> @@ -100,6 +102,20 @@ API as a URL.
>>> =cut
>>>
>>> sub canonicalize_url {
>>> + my $url = shift;
>>> +
>>> + # The 1.7 way to do it
>>> + if ( defined &SVN::_Core::svn_uri_canonicalize ) {
>>> + return SVN::_Core::svn_uri_canonicalize($url);
>>> + }
>>> + # There wasn't a 1.6 way to do it, so we do it ourself.
>>> + else {
>>> + return _canonicalize_url_ourselves($url);
[...]
>> Leaves me a bit nervous.
>
> As it should, SVN dumped a mess on us.
Here's a pair of patches that address some of the bugs I was alluding
to. Patch 1 makes _canonicalize_url_ourselves() match Subversion's
own canonicalization behavior more closely, though even with that
patch it still does not meet Subversion's requirements perfectly
(e.g., "%ab" is not canonicalized to "%AB"). Patch 2 makes that not
matter by using svn_path_canonicalize() when possible, which is the
standard way to do this kind of thing.
Sorry for the lack of clarity before.
Jonathan Nieder (2):
git svn: do not overescape URLs (fallback case)
Git::SVN::Utils::canonicalize_url: use svn_path_canonicalize when
available
perl/Git/SVN/Utils.pm | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.
On 2012.7.28 1:02 PM, Jonathan Nieder wrote: > Jonathan Nieder wrote: >> Michael G Schwern wrote: > >>> I would suggest that worrying whether a few lines of code are introduced now >>> or 10 patches later in the same branch which is all going to be merged in >>> one >>> go (and retesting the patches after it) is not the most important thing. > [...] >> In that case they should be one patch, I'd think. >> >> The advantage of introducing changes gradually is that (1) the changes >> can be examined and tested one at a time, and (2) if later a change >> proves to be problematic, it can be isolated, understood, and fixed >> more easily. The strategy you are suggesting would have neither of >> those advantages. > > (To avoid confusion: by "The strategy you are suggesting" I mean > introducing dead code first and activating it later, not the path and > url object idea. The path and url object approach would be very > nice. :)) If this is all a topic branch then it doesn't matter much whether a couple lines of code is introduced at patch 8 of a branch or patch 13. Sure, it matters a little, but... https://secure.wikimedia.org/wikipedia/en/wiki/Opportunity_cost If it *isn't* going in a topic branch, if its not visible as a collected work in history, if its going to be rebased on top of master, then yeah I can see why you're so concerned. -- Alligator sandwich, and make it snappy! -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.
Jonathan Nieder wrote: > Michael G Schwern wrote: >> I would suggest that worrying whether a few lines of code are introduced now >> or 10 patches later in the same branch which is all going to be merged in one >> go (and retesting the patches after it) is not the most important thing. [...] > In that case they should be one patch, I'd think. > > The advantage of introducing changes gradually is that (1) the changes > can be examined and tested one at a time, and (2) if later a change > proves to be problematic, it can be isolated, understood, and fixed > more easily. The strategy you are suggesting would have neither of > those advantages. (To avoid confusion: by "The strategy you are suggesting" I mean introducing dead code first and activating it later, not the path and url object idea. The path and url object approach would be very nice. :)) Sorry for the lack of clarity. Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.
Michael G Schwern wrote: > On 2012.7.28 12:30 PM, Jonathan Nieder wrote: >> Since this part of the series is not tested with SVN 1.7, this is >> basically adding dead code, right? That could be avoided by >> reordering the changes to keep "canonicalize_url" as-is until later in >> the series when the switchover is safe. > > I would suggest that worrying whether a few lines of code are introduced now > or 10 patches later in the same branch which is all going to be merged in one > go (and retesting the patches after it) is not the most important thing. The > code needs humans looking over it and deciding if canonicalizations were > missed or applied inappropriately. Or hey, work on that path and url object > idea that makes a lot of real code mess go away. In that case they should be one patch, I'd think. The advantage of introducing changes gradually is that (1) the changes can be examined and tested one at a time, and (2) if later a change proves to be problematic, it can be isolated, understood, and fixed more easily. The strategy you are suggesting would have neither of those advantages. Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.
On 2012.7.28 12:30 PM, Jonathan Nieder wrote: >> I didn't know about that. I don't know what your SVN backwards compat >> requirements are, but if that behavior goes back far enough in SVN to satisfy >> you folks, then canonicalize_url() should fall back to >> SVN::_Core::svn_path_canonicalize(). > > svn_path_canonicalize() has been usable for this kind of thing since > SVN 1.1, possibly earlier. Great! Then _canonicalize_url_ourselves() can probably be replaced with that. Just take my advice and do it after 1.7 is working and the code is ready for canonicalization. >> But try it at the end of the patch >> series. The code has to be prepared for canonicalization first. Then how it >> actually does it can be improved. > > Since this part of the series is not tested with SVN 1.7, this is > basically adding dead code, right? That could be avoided by > reordering the changes to keep "canonicalize_url" as-is until later in > the series when the switchover is safe. I would suggest that worrying whether a few lines of code are introduced now or 10 patches later in the same branch which is all going to be merged in one go (and retesting the patches after it) is not the most important thing. The code needs humans looking over it and deciding if canonicalizations were missed or applied inappropriately. Or hey, work on that path and url object idea that makes a lot of real code mess go away. -- ROCKS FALL! EVERYONE DIES! http://www.somethingpositive.net/sp05032002.shtml -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.
Michael G Schwern wrote:
> On 2012.7.28 6:50 AM, Jonathan Nieder wrote:
>> If I am reading Subversion r873487 correctly, in ancient times,
>> svn_path_canonicalize() did the appropriate tweaking for URIs. Today
>> its implementation is comforting:
>>
>> const char *
>> svn_path_canonicalize(const char *path, apr_pool_t *pool)
>> {
>>if (svn_path_is_url(path))
>> return svn_uri_canonicalize(path, pool);
>>else
>> return svn_dirent_canonicalize(path, pool);
>> }
[...]
> I didn't know about that. I don't know what your SVN backwards compat
> requirements are, but if that behavior goes back far enough in SVN to satisfy
> you folks, then canonicalize_url() should fall back to
> SVN::_Core::svn_path_canonicalize().
svn_path_canonicalize() has been usable for this kind of thing since
SVN 1.1, possibly earlier.
> But try it at the end of the patch
> series. The code has to be prepared for canonicalization first. Then how it
> actually does it can be improved.
Since this part of the series is not tested with SVN 1.7, this is
basically adding dead code, right? That could be avoided by
reordering the changes to keep "canonicalize_url" as-is until later in
the series when the switchover is safe.
Thanks. Will play around with this code more.
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.
On 2012.7.28 6:50 AM, Jonathan Nieder wrote:
>> --- a/perl/Git/SVN/Utils.pm
>> +++ b/perl/Git/SVN/Utils.pm
> [...]
>> @@ -100,6 +102,20 @@ API as a URL.
>> =cut
>>
>> sub canonicalize_url {
>> +my $url = shift;
>> +
>> +# The 1.7 way to do it
>> +if ( defined &SVN::_Core::svn_uri_canonicalize ) {
>> +return SVN::_Core::svn_uri_canonicalize($url);
>> +}
>> +# There wasn't a 1.6 way to do it, so we do it ourself.
>> +else {
>> +return _canonicalize_url_ourselves($url);
>> +}
>> +}
>> +
>> +
>> +sub _canonicalize_url_ourselves {
>> my ($url) = @_;
>> $url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;
>
> Leaves me a bit nervous.
As it should, SVN dumped a mess on us.
> What effect should we expect this change to have? Is our emulation
> of svn_uri_canonicalize already perfect and this change just a little
> futureproofing in case svn_uri_canonicalize gets even better, or is
> this a trap waiting to happen when new callers of canonicalize_url
> start relying on, e.g., %-encoding of special characters?
This change is *just* about sliding in the SVN API call and seeing if git-svn
still works with SVN 1.6. It should have no effect on SVN 1.6. These patches
are a very slow and careful refactoring doing just one thing at a time. Every
time I tried to do too many things at once, tests broke and I had to tease the
patch apart.
At this point in the patch series the code is not ready for canonicalization.
Until 3/8 in the next patch series, canonicalize_url() basically does nothing
on SVN 1.6 so the code has never had to deal with the problem. 3/8 deals with
improving _canonicalize_url_ourselves() to work more like
svn_uri_canonicalize() and thus "turns on" canonicalization for SVN 1.6 and
deals with the breakage.
> If I am reading Subversion r873487 correctly, in ancient times,
> svn_path_canonicalize() did the appropriate tweaking for URIs. Today
> its implementation is comforting:
>
> const char *
> svn_path_canonicalize(const char *path, apr_pool_t *pool)
> {
> if (svn_path_is_url(path))
> return svn_uri_canonicalize(path, pool);
> else
> return svn_dirent_canonicalize(path, pool);
> }
>
> It might be easier to rely on that on pre-1.7 systems.
I didn't know about that. I don't know what your SVN backwards compat
requirements are, but if that behavior goes back far enough in SVN to satisfy
you folks, then canonicalize_url() should fall back to
SVN::_Core::svn_path_canonicalize(). But try it at the end of the patch
series. The code has to be prepared for canonicalization first. Then how it
actually does it can be improved.
--
Defender of Lexical Encapsulation
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.
Hi,
Michael G. Schwern wrote:
> --- a/perl/Git/SVN/Utils.pm
> +++ b/perl/Git/SVN/Utils.pm
[...]
> @@ -100,6 +102,20 @@ API as a URL.
> =cut
>
> sub canonicalize_url {
> + my $url = shift;
> +
> + # The 1.7 way to do it
> + if ( defined &SVN::_Core::svn_uri_canonicalize ) {
> + return SVN::_Core::svn_uri_canonicalize($url);
> + }
> + # There wasn't a 1.6 way to do it, so we do it ourself.
> + else {
> + return _canonicalize_url_ourselves($url);
> + }
> +}
> +
> +
> +sub _canonicalize_url_ourselves {
> my ($url) = @_;
> $url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;
Leaves me a bit nervous.
What effect should we expect this change to have? Is our emulation
of svn_uri_canonicalize already perfect and this change just a little
futureproofing in case svn_uri_canonicalize gets even better, or is
this a trap waiting to happen when new callers of canonicalize_url
start relying on, e.g., %-encoding of special characters?
If I am reading Subversion r873487 correctly, in ancient times,
svn_path_canonicalize() did the appropriate tweaking for URIs. Today
its implementation is comforting:
const char *
svn_path_canonicalize(const char *path, apr_pool_t *pool)
{
if (svn_path_is_url(path))
return svn_uri_canonicalize(path, pool);
else
return svn_dirent_canonicalize(path, pool);
}
It might be easier to rely on that on pre-1.7 systems.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.
From: "Michael G. Schwern"
No change on SVN 1.6. The tests all pass with SVN 1.6 if
canonicalize_url() does nothing, so tests passing doesn't have
much meaning.
The tests are so messed up right now with SVN 1.7 it isn't really
useful to check. They will be useful later.
---
perl/Git/SVN/Utils.pm | 16
1 file changed, 16 insertions(+)
diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm
index c842d00..9d5d3c5 100644
--- a/perl/Git/SVN/Utils.pm
+++ b/perl/Git/SVN/Utils.pm
@@ -3,6 +3,8 @@ package Git::SVN::Utils;
use strict;
use warnings;
+use SVN::Core;
+
use base qw(Exporter);
our @EXPORT_OK = qw(
@@ -100,6 +102,20 @@ API as a URL.
=cut
sub canonicalize_url {
+ my $url = shift;
+
+ # The 1.7 way to do it
+ if ( defined &SVN::_Core::svn_uri_canonicalize ) {
+ return SVN::_Core::svn_uri_canonicalize($url);
+ }
+ # There wasn't a 1.6 way to do it, so we do it ourself.
+ else {
+ return _canonicalize_url_ourselves($url);
+ }
+}
+
+
+sub _canonicalize_url_ourselves {
my ($url) = @_;
$url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;
return $url;
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
