> -----Original Message-----
> From: Greg Stein [mailto:gst...@gmail.com]
> Sent: maandag 9 mei 2011 22:44
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1101071 - in
> /subversion/trunk/subversion/libsvn_wc: upgrade.c wc-metadata.sql
> wc_db.c
>
> On Mon, May 9, 2011 at 11:43, <rhuij...@apache.org> wrote:
> >...
> > +++ subversion/trunk/subversion/libsvn_wc/wc-metadata.sql Mon May 9
> 15:43:56 2011
> > @@ -530,7 +530,64 @@ BEGIN
> > WHERE checksum = OLD.checksum;
> > END;
> >
> > +-- STMT_CREATE_EXTERNALS
> >
> > +CREATE TABLE EXTERNALS (
> > + /* Working copy location related fields (like NODES)*/
> > +
> > + wc_id INTEGER NOT NULL REFERENCES WCROOT (id),
> > + local_relpath TEXT NOT NULL,
> > +
> > + /* The working copy root can't be recorded as an external in itself
> > + so this will never be NULL. */
> > + parent_relpath TEXT NOT NULL,
> > +
> > + /* Repository location fields */
> > +
> > + /* Always set for file and symlink externals. NULL for directory
externals
> */
> > + repos_id INTEGER REFERENCES REPOSITORY (id),
> > + repos_path TEXT,
> > + revision INTEGER,
>
> Shouldn't that be repos_relpath?
I made it identical to NODES.
(We have some notes about a final cleanup bump before 1.7... Maybe we should
start thinking about that)
>
> > +
> > + /* Content fields */
> > +
> > + /* the kind of the external. */
> > + kind TEXT NOT NULL,
> > +
> > + /* Variouse information (NULL for directories; see NODES for
> explanation) */
> > + properties BLOB,
> > + checksum TEXT REFERENCES PRISTINE (checksum),
> > + symlink_target TEXT,
> > +
> > + /* Last-Change fields (NULL for directories; see NODES for
explanation)
> */
> > + changed_revision INTEGER,
> > + changed_date INTEGER,
> > + changed_author TEXT,
> > +
> > + /* Various cache fields (NULL for directories; see NODES for
explanation)
> */
> > + translated_size INTEGER,
> > + last_mod_time INTEGER,
> > + dav_cache BLOB,
>
> Since this is a new table, I think we can get these columns names to
> better reflect their semantic (and use in the code): recorded_size and
> recorded_mod_time.
I followed nodes (see note above). No problem to fix this now.
>
> > +
> > +
> > + /* The local relpath of the directory NODE defining this external
> > + (Defaults to the parent directory of the file external after
upgrade) */
> > + record_relpath TEXT NOT NULL,
>
> This name is confusing with the recorded_* columns. Could you use
> 'definition_relpath' here? It might even by nice to use something like
> 'def_local_relpath' to indicate the relationship to 'local_relpath'.
def_local_relpath looks better to me than the current name.
>
> > +
> > + /* The url of the external as used in the definition */
> > + recorded_url TEXT NOT NULL,
>
> Is this a full URL, or a repos_relpath? Please detail in the column
> since we mostly use <repos_id, repos_relpath> elsewhere.
I actually use this as a repos_relpath alike path in the code I just
committed. Thanks for catching this misnaming.
> > +
> > + /* The operational (peg) and node revision if this is a revision
fixed
> > + external; otherwise NULL. (Usually these will both have the same
> value) */
> > + recorded_operational_revision TEXT NOT NULL,
> > + recorded_revision TEXT NOT NULL,
>
> How about 'defined_*' for these? ... or something besides 'recorded'
> since we've been using that to imply state from elsewhere that we
> simply record into the database.
That first one should then be defined_repos_relpath.
(What about that def_local_relpath?)
> The comment says these can be NULL, yet you label them as NOT NULL.
Hmm... I even put NULL in them. Strange.
> > +
> > + PRIMARY KEY (wc_id, local_relpath)
> > +);
> > +
> > +CREATE INDEX I_EXTERNALS_PARENT ON EXTERNALS (wc_id,
> parent_relpath);
> > +CREATE UNIQUE INDEX I_EXTERNALS_RECORDED ON EXTERNALS (wc_id,
> record_relpath,
> > + local_relpath);
>
> STMT_SELECT_EXTERNALS_DEFINED uses <wc_id, record_relpath>. There is
> no query that uses the triple you define above, so this index seems
> pretty useless (and, in fact, can slow things down since it must be
> maintained).
>
> I do see that you also have queries based on <wc_id, local_relpath>.
> Is the intent to create one index for both types of WHERE clauses?
> Does the one index work for both?
For most operations on the external we use the local_abspath of the external
(wc_id, local_relpath)
If we want full entry compatibility we need to query the file externals
inside a directory. To add these in svn_wc_entry_t instances.
(Not sure if we should. I could just call adding them to svn_wc_entry_t a
bug in 1.6...)
And for svn:externals update processing (to fix the local changes to
svn:externals problem) we need to find all the externals that were defined
via svn:externals on a specific parent.
So, yes I think we will need all of these in queries. (The parent one is the
least likely variant)
Bert
