Note: beyond the review below, I'm registering a general concern
around this notion of "hey! let's drop off this data into some global
variables^W^Woh, I mean the wc state database... and then some time
later, we'll perform some operation around them".
This is *so* ripe for danger, I'm depressed.
Honestly. This "temporary table" is just another name for "global
variable". "Results are collected in a table". Really? REALLY? How is
that different from a global? Functions going this way and that,
dropping off rows in the table as a *side effect*, and then along some
other code path later in the control flow, we decide to pick them back
up and run with them.
Stinky.
See my further review below on the implementation of this Badness:
On Tue, Apr 5, 2011 at 17:53, <hwright_at_apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_wc/wc-queries.sql Tue Apr 5 21:53:47 2011
> @@ -340,6 +340,30 @@ REPLACE INTO actual_node (
> wc_id, local_relpath, parent_relpath, changelist)
> VALUES (?1, ?2, ?3, ?4)
>
> +-- STMT_CREATE_CHANGELIST_LIST
> +DROP TABLE IF EXISTS changelist_list;
> +CREATE TEMPORARY TABLE changelist_list (
> + wc_id INTEGER NOT NULL,
> + local_relpath TEXT NOT NULL,
> + notify INTEGER,
> + changelist TEXT NOT NULL
> + );
> +CREATE INDEX changelist_list_index ON changelist_list(local_relpath);
Why doesn't this have wc_id?
> +
> +-- STMT_INSERT_CHANGELIST_LIST
> +INSERT INTO changelist_list(wc_id, local_relpath, notify, changelist)
> +VALUES (?1, ?2, ?3, ?4);
> +
> +-- STMT_DELETE_CHANGELIST_LIST_RECURSIVE
> +DELETE FROM changelist_list
> +WHERE local_relpath = ?1 OR local_relpath LIKE ?2 ESCAPE '#'
> +
> +-- STMT_SELECT_CHANGELIST_LIST_RECURSIVE
> +SELECT local_relpath, notify, changelist
> +FROM changelist_list
> +WHERE local_relpath = ?1 or local_relpath LIKE ?2 ESCAPE '#'
> +ORDER BY local_relpath
And these two statements should have wc_id.
>...
> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Tue Apr 5 21:53:47 2011
>...
> svn_error_t *
> +svn_wc__db_changelist_list_notify(svn_wc_notify_func2_t notify_func,
> + void *notify_baton,
> + svn_wc__db_t *db,
> + const char *local_abspath,
> + apr_pool_t *scratch_pool)
> +{
Should the contents of this function be transacted?
>...
Cheers,
-g
Received on 2011-04-06 14:58:32 CEST