On Tue, 2010-04-27, Greg Stein wrote:
> 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.
(FWIW it was my introducing the extra item in r927056 that is the
problem, rather than this r927098.)
> 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.
Thanks for pointing this out. I've just added such an assertion
(r938835) - a bit late in this case, but doing so now will help me
remember to do so again if a similar situation comes up again.
- Julian
