On Wed, 11 Oct 2006, Paul Burba wrote:
...
> > > I'm not totally clear from this comment how the revision arg is
> > > supposed to be used, currently we just pass SVN_INVALID_REVNUM.
> > > The doc string for delete_entry() says:
> > >
> > > /** Remove the directory entry named @a path, a child of the
> directory
> > > * represented by @a parent_baton. If @a revision is set, it is
> used as
> > > a
> > > * sanity check to ensure that you are removing the revision of @a
> path
> > > * that you really think you are.
> > > *
> > > * All allocations should be performed in @a pool.
> > > */
> > >
> > > But I'm not sure what a "sanity check" implies exactly. I'd like to
> > > think
> > > that what I'm doing (passing the rev the path was deleted) is
> > > compatible
> > > with the doc string, but it seems *too* easy that an essentially
> > > unused arg already exists and does just what I require :-)
> >
> > The "If @a revision is set, it is used as a sanity check" part of this
> > gives me concern that returning an incorrect "deleted rev" might have
> > side-effects. What type of sanity check is performed on the revnum,
> > and by what code?
...
> I realized I needed to understand production and consumption of tree
> deltas better. I still have only a basic grasp of this, but here is how I
> understand the problem:
>
> If the following are all true...
>
> 1) An svn operation causes a tree delta
> to be produced in libsvn_repos.
>
> 2) libsvn_repos/reporter.c's delta_dirs()
> or update_entry() is used to construct
> the tree delta.
>
> 3) The delta's producer calls the
> svn_delta_editor_t's delete_entry()
> callback from delta_dirs()
> or update_dirs().
>
> 4) The delete_entry() implementation isn't
> just a wrapper around the actual
> implementation callback in the edit baton.
> e.g. commit_util.c(1483):delete_entry()
>
> 5) The implementation isn't just the network
> layer consuming the edit operation and
> passing it to it's ultimate consumer.
> e.g. libsvn_ra_svn\editor.c(157):ra_svn_delete_entry()
Yup. The wrapping editor API implementation often adds additional
logic around its delegation to the wrapped editor callback.
> ...Then with my patch there is now the possibility of unintended
> side-effects as the delta consumer's delete_entry() implementations are
> now getting a revision argument other than SVN_INVALID_REVNUM.
Right.
> So if I understand the risk correctly, there are 6 delete_entry()
> implmentations that meet the above criteria and are potential risks:
>
> libsvn_client\repos_diff.c(428):delete_entry(const char *path,
> svn_revnum_t base_revision, void *parent_baton, apr_pool_t *pool)
>
> libsvn_client\repos_diff_summarize.c(119):delete_entry(const char *path,
> svn_revnum_t base_revision, void *parent_baton, apr_pool_t *pool)
>
> libsvn_wc\diff.c(980):delete_entry(const char *path, svn_revnum_t
> base_revision, void *parent_baton, apr_pool_t *pool)
>
> libsvn_wc\update_editor.c(1037):delete_entry(const char *path,
> svn_revnum_t revision, void *parent_baton, apr_pool_t *pool)
>
> libsvn_delta\default_editor.c(75):delete_entry(const char *path,
> svn_revnum_t revision, void *parent_baton, apr_pool_t *pool)
>
> All six implmentations are all safe as none use the svn_revnum_t
> argument.
Additional logic which makes use of the REVISION argument could be
inserted by use of an externally-defined editor (e.g. by TortoiseSVN).
This is only going to be a risk if we make a "best guess" about the
revision being deleted in svn_repos_deleted_rev() (rather than
returning SVN_INVALID_REVNUM), and happen to return an incorrect
revision.
- application/pgp-signature attachment: stored
Received on Wed Oct 25 23:30:41 2006