RE: svn commit: r980784 - in /subversion/trunk/subversion/libsvn_wc: adm_files.c node.c wc_db.h

> -----Original Message-----
> From: Daniel Näslund [mailto:daniel_at_longitudo.com]
> Sent: zondag 1 augustus 2010 14:46
> To: dev_at_subversion.apache.org
> Subject: Re: svn commit: r980784 - in
> /subversion/trunk/subversion/libsvn_wc: adm_files.c node.c wc_db.h
>
> On Fri, Jul 30, 2010 at 01:28:39PM -0000, philip_at_apache.org wrote:
> > Author: philip
> > Date: Fri Jul 30 13:28:39 2010
> > New Revision: 980784
> >
> > Modified: subversion/trunk/subversion/libsvn_wc/adm_files.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/adm_
> files.c?rev=980784&r1=980783&r2=980784&view=diff
> >
> =======================================================================
> =======
> > --- subversion/trunk/subversion/libsvn_wc/adm_files.c (original)
> > +++ subversion/trunk/subversion/libsvn_wc/adm_files.c Fri Jul 30
> 13:28:39 2010
> > @@ -622,40 +624,59 @@ svn_wc__internal_ensure_adm(svn_wc__db_t
> > repos_relpath, repos_root_url,
> repos_uuid,
> > revision, depth,
> scratch_pool));
> >
> > - /* Now, get the existing url and repos for PATH. */
> > - SVN_ERR(svn_wc__get_entry(&entry, db, local_abspath, FALSE,
> svn_node_unknown,
> > - FALSE, scratch_pool, scratch_pool));
> > + SVN_ERR(svn_wc__db_read_info(&status, NULL,
> > + &db_revision, &db_repos_relpath,
> > + &db_repos_root_url, &db_repos_uuid,
> > + NULL, NULL, NULL, NULL, NULL, NULL,
> NULL,
> > + NULL, NULL, NULL, NULL, NULL, NULL,
> NULL,
> > + NULL, NULL, NULL, NULL,
> > + db, local_abspath, scratch_pool,
> scratch_pool));
> >
> > - /* When the directory exists and is scheduled for deletion do not
> > - * check the revision or the URL. The revision can be any
> > + /* When the directory exists and is scheduled for deletion or is
> not-present
> > + * do not check the revision or the URL. The revision can be any
> > * arbitrary revision and the URL may differ if the add is
> > * being driven from a merge which will have a different URL. */
> > - if (entry->schedule != svn_wc_schedule_delete)
> > + if (status != svn_wc__db_status_deleted
> > + && status != svn_wc__db_status_obstructed_delete)
> > {
> > - if (entry->revision != revision)
> > + /* ### Should we match copyfrom_revision? */
> > + if (db_revision != revision)
> > return
> > svn_error_createf(SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> > _("Revision %ld doesn't match existing "
> > "revision %ld in '%s'"),
> > - revision, entry->revision,
> local_abspath);
> > + revision, db_revision, local_abspath);
> >
> > /* The caller gives us a URL which should match the entry.
> However,
> > some callers compensate for an old problem in entry->url
> and pass
> > the copyfrom_url instead. See ^/notes/api-errata/wc002.txt.
> As
> > a result, we allow the passed URL to match copyfrom_url if
> it
> > - does match the entry's primary URL. */
> > - /** ### comparing URLs, should they be canonicalized first? */
> > - if (strcmp(entry->url, url) != 0
> > - && (entry->copyfrom_url == NULL
> > - || strcmp(entry->copyfrom_url, url) != 0)
> > - && (!svn_uri_is_ancestor(repos_root_url, entry->url)
> > - || strcmp(entry->uuid, repos_uuid) != 0))
> > + does not match the entry's primary URL. */
> > + /* ### comparing URLs, should they be canonicalized first? */
>
> Previously urls were compared but now, we're comparing uuid, root_url
> and repos_relpath. Those are all already canonicalized, right? Appears
> not, since a bit higher up we're doing:
>
> repos_relpath = svn_uri_is_child(repos_root_url, url, scratch_pool);
> repos_relpath = svn_path_uri_decode(repos_relpath, scratch_pool);
>
> We probably should add a svn_relpath_canonicalize() after those and
> maybe the documentation on the top of svn_dirent_uri.h should mention
> that a relpath is supposed to not be encoded as an uri. And shouldn't
> all relpaths, uris and dirents be canonicalized when they reach
> libsvn_wc?

svn_uri_is_child guarantees that the output argument is canonical (as it's
input must be a canonical url). But it is still escaped.

svn_path_uri_decode() doesn't change the canonicalization. (The only
canonicalization rules in repos_relpaths are around using / and ./)

        Bert
>
> > + if (strcmp(db_repos_uuid, repos_uuid)
> > + || strcmp(db_repos_root_url, repos_root_url)
> > + || !svn_relpath_is_ancestor(db_repos_relpath,
> repos_relpath))
> > {
> > - return
> > - svn_error_createf(SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> > - _("URL '%s' doesn't match existing "
> > - "URL '%s' in '%s'"),
> > - url, entry->url, local_abspath);
> > + const char *copyfrom_root_url, *copyfrom_repos_relpath;
> > +
> > +
> SVN_ERR(svn_wc__internal_get_copyfrom_info(&copyfrom_root_url,
> > +
> &copyfrom_repos_relpath,
> > + NULL, NULL,
> NULL,
> > + db,
> local_abspath,
> > + scratch_pool,
> > + scratch_pool));
> > +
> > + if (copyfrom_root_url == NULL
> > + || strcmp(copyfrom_root_url, repos_root_url)
> > + || strcmp(copyfrom_repos_relpath, repos_relpath))
> > + return
> > + svn_error_createf(SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> > + _("URL '%s' doesn't match existing "
> > + "URL '%s' in '%s'"),
> > + url,
> > + svn_uri_join(db_repos_root_url,
> > + db_repos_relpath,
> scratch_pool),
> > + local_abspath);
> > }
> > }
>
> Cheers,
> Daniel (who is just trying to learn to do code reviews).
Received on 2010-08-02 00:14:36 CEST