Julian Foad wrote on Tue, 7 Oct 2008 at 17:22 +0100:
> On Tue, 2008-10-07 at 17:46 +0200, Daniel Shahaf wrote:
> > The attached patch adds support for wc format 10 (for Subversion 1.6) to
> > change-svn-wc-format.py. Review from someone more familiar with this
> > area of the code would be welcome.
>
> I'm not familiar but will review anyway.
>
Thanks.
>
> > [[[
> > Support wc format 10 in 'change-svn-wc-format.py':
> >
> > * tools/client-side/change-svn-wc-format.py
> > (re, string.lower, string.upper): Import.
>
> I don't mention imports and includes, only symbols that I'm creating or
> modifying or deleting.
It *does* introduce a new symbol -- the symbol 'lower' or the symbol 're'.
> But no big deal.
>
+1
> > class Entry:
> > "Describes an entry in a WC."
> >
> > - # The list of field indices within an entry's record which must be
> > - # retained for 1.5 -> 1.4 migration (changelist, keep-local, and depth).
> > - must_retain_fields = (30, 31, 33)
> > + # Maps format numbers to fields indices within an entry's record that must be
> > + # retained when downgrading to that format.
>
> I know you didn't change this, but it looks like these are the indices
> that must be _discarded_ rather than those that must be retained.
>
I assumed the thinking was 'retained (as empty)', but I can make the change.
> > + must_retain_fields = {
> > + # Not in 1.4: changelist, keep-local, depth, tree-conflicts, file-externals
> > + 8 : (30, 31, 33, 34, 35),
> > + # Not in 1.5: tree-conflicts, file-externals
> > + 9 : (34, 35),
> > + }
> >
> > def __init__(self):
> > self.fields = []
> > @@ -254,9 +276,25 @@ class Entry:
> >
> > # Check whether lossy conversion is being attempted.
> > lossy_fields = []
> > - for field_index in self.must_retain_fields:
> > + for field_index in self.must_retain_fields[format_nbr]:
> > if len(self.fields) - 1 >= field_index and self.fields[field_index]:
> > lossy_fields.append(Entries.entry_fields[field_index])
> > +
> > + # Special case: format 10 changed the canonicalisation of URLs (r33236).
> > + if format_nbr < 10:
>
> Shouldn't this conversion happen only when format_nbr < 10 and the old
> format number is >= 10? Not, for example, when downgrading from 9 to 8.
>
Actually, this check isn't necessary at all. It is the *new* format
that doesn't support lowercase schemes and hostnames; the old formats
allow any case. I'll drop it.
> > + url_fields = { 3 : 'url', 4 : 'repos', 20 : 'copyfrom-url'}
> > + for field_index in url_fields.keys():
> > + if len(self.fields) - 1 >= field_index and self.fields[field_index]:
> > + # Check that scheme and hostname are in lowercase.
> > + # See also change_case_of_hostname() in copy_tests.py.
> > + value = self.fields[field_index]
> > + m = re.match(r"^(.*)://([^/@]*@)?([^/]*)(/|$)", value)
> > + if m:
> > + scheme = m.group(1)
> > + host = m.group(3)
> > + if host != lower(host) or scheme != lower(scheme):
> > + lossy_fields.append(url_fields[field_index])
> > +
> > if lossy_fields:
> > raise LossyConversionException(lossy_fields,
> > "Lossy WC format conversion requested for entry '%s'\n"
Thanks for the review.
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-10-07 18:41:15 CEST