Re: svn commit: r927098 - in /subversion/trunk/subversion/libsvn_wc: adm_ops.c workqueue.c

[Julian Foad <ju...@wandisco.com>]

On Tue, 2010-04-27, Greg Stein wrote:
> On Wed, Mar 24, 2010 at 12:02,  <ju...@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