On Wed, Mar 24, 2010 at 12:02, <julianf...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_wc/workqueue.c Wed Mar 24 16:02:26 2010
>...
> @@ -1596,9 +1590,12 @@ run_postcommit(svn_wc__db_t *db,
> SVN_ERR(svn_skel__parse_proplist(&new_dav_cache, arg5->next,
> scratch_pool));
> keep_changelist = svn_skel__parse_int(arg5->next->next, scratch_pool) != 0;
> - tmp_text_base_abspath = apr_pstrmemdup(scratch_pool,
> - arg5->next->next->next->data,
> - arg5->next->next->next->len);
> + if (arg5->next->next->next->len == 0)
> + tmp_text_base_abspath = NULL;
> + else
> + tmp_text_base_abspath = apr_pstrmemdup(scratch_pool,
> + arg5->next->next->next->data,
> + arg5->next->next->next->len);
This is a problem.
Working copies before this revision will not have this extra skel
item. Thus, stale work items will crash a libsvn_wc which incorporates
this change.
There are several options:
1) ignore the issue. svn devs can figure it out.
2) code to allow the missing item
3) format bump
This change (accidentally) fell into (1). Since it is very rare to
have stale work items, nobody ran into this. While fine, we need to be
*aware* that this can happen whenever work item layouts are changed.
For example, I recently changed OP_FILE_INSTALL to take an optional
parameter for the source of the translation. Old work items won't have
this, and will follow their original semantic: take the source from
the pristine.
Back to this particular case... the fallback of NOT providing the skel
item effectively means keeping the old code around (which you moved
over to adm_ops). Not ideal. With full hindsight, were you do write
this revision today? I'd say: put an
SVN_ERR_ASSERT(arg5->next->next->next != NULL) in there, along with a
comment on why it might get thrown. If a dev hit the assertion, then
they'd know what happened ("d'oh! stale log!!!").
The likelihood of it ever being a problem is near miniscule. But we
need to *recognize its existence*, and the assertion is a low-cost
barrier against the issue.
>...
Cheers,
-g
