On Tue, 2008-10-07 at 18:40 +0200, Daniel Shahaf wrote:
> 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.
No, you're probably more right than I am. I've just had a more thorough
look, and it seems these are fields that would result in an undesirable
loss of something if we discard them, so they are described as "fields
that we really want to retain".
- Julian
> > > + 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:57:39 CEST