Re: [Issue 1904] New - seg fault on 'svn propdel --revprop ...'

I'm tempted to close this one as "invalid" because it uses the issue
tracker for discussion that belongs on the dev@list. :-)

Anyway: You described two bugs and a broken docstring. IMHO they should
all be fixed in the "obvious" way. The post-revprop-change handler
should pipe the old value to the hook. I'm not sure what the new value
is for a deleted property, though; I suppose "empty" is the best we can
do, except that the hook would have no way of knowing that the property
was deleted. Same goes for the old value of an added prop in the
post-change hook.

I suppose adding another parameter to the hook (action: added, modified,
deleted?) wouldn't be too bad for a 1.1? I'd tend to view script
parameters the same way as structure fields, since trailing ones can be
ignored. We should put that in the versioning doc, though.

Yes, and modify mailer.py and propchange-email.pl to handle the new
parameter and show diffs as expected.

    Brane

kfogel@tigris.org wrote:

>http://subversion.tigris.org/issues/show_bug.cgi?id=1904
> Issue #:|1904
> Summary:|seg fault on 'svn propdel --revprop ...'
> Component:|subversion
> Version:|current
> Platform:|All
> URL:|
> OS/Version:|All
> Status:|NEW
> Status whiteboard:|
> Keywords:|
> Resolution:|
> Issue type:|DEFECT
> Priority:|P3
> Subcomponent:|libsvn_repos
> Assigned to:|issues@subversion
> Reported by:|kfogel
>
>
>
>
>
>
>------- Additional comments from kfogel@tigris.org Wed Jun 2 15:02:54 -0700 2004 -------
>This is with r9926 of Subversion trunk:
>
> $ svnadmin create foo
> $ svn mkdir -m "Make a directory." file://`pwd`/foo/x
> $ svn propdel --revprop -r1 svn:log file://`pwd`/foo/
> Segmentation fault
> $
>
>Tracing it down in GDB, we see in subversion/libsvn_repos/hooks.c, in the
>function create_temp_file(), that the parameter 'value' is NULL. Since the
>function accesses value->len and value->data, that's a problem.
>
>The picture gets a little bit more complicated as we trace up the stack.
>create_temp_file() is called from svn_repos__hooks_pre_revprop_change(), which
>claims in its doc string that the VALUE parameter is the old, unchanged value of
>the prop:
>
> /* Run the pre-revprop-change hook for REPOS. Use POOL for any
> temporary allocations. If the hook fails, return
> SVN_ERR_REPOS_HOOK_FAILURE.
>
> REV is the revision whose property is being changed.
> AUTHOR is the authenticated name of the user changing the prop.
> NAME is the name of the property being changed.
> VALUE is the current (unchanged) value of the property. */
> svn_error_t *
> svn_repos__hooks_pre_revprop_change (svn_repos_t *repos, ...
>
>But go up one more stack frame and we're in svn_repos_fs_change_rev_prop(),
>which does some very strange stuff. It takes the *new* property value as a
>parameter, and fetches the old value from the filesystem directly. So now it
>has both in its hands... And what does it do?
>
>It passes the *new* value to svn_repos__hooks_pre_revprop_change(), and the
>*old* value to svn_repos__hooks_post_revprop_change(). Of course, we never get
>to the latter call, because we fail in the former.
>
>Clearly this defies svn_repos__hooks_pre_revprop_change()'s documentation, but I
>think it's the doc string that's wrong. The way this system works, IIRC, is
>that each hook needs to be passed the propval that it can't obtain simply by
>asking the filesystem. For the pre-revprop-change hook, that's the new value,
>and for the post-revprop-change, it's the old value.
>
>(Note that svn_repos__hooks_post_revprop_change()'s doc string is consistent
>with this design.)
>
>I think a little more investigation and thought is needed here than I can give
>right now, that's why I'm filing an issue. Here's the problem:
>
>Our whole revprop change hook system appears to be in a half-finished state...
>Yet I can't find any of the open issues about its completion that I remember
>filing N years ago. This all started with a simple problem, namely that 'svn
>propedit --revprop -rXXX svn:log' doesn't show a diff, but instead just shows
>the new property value. We'd like for it to show diffs instead, and there were
>two ways to do it:
>
> 1. The pre-revprop-change hook and post-revprop-change hook could
> cooperate in complex and hard-to-maintain ways
>
> 2. The post-revprop-change hook could somehow get the old value, to
> diff against.
>
>We much preferred (2), and symmetrically, it seemed like a good idea for the
>pre-revprop-change to get the *new* value, as well as for post- to get the old
>value. That way you could compare old and new both before and after the change.
>
>It appears now that pre-revprop-change is getting, or trying to get, the new
>value on stdin nowadaways (I say "trying to" because there's a bug in the
>implementation of that feature, hence this issue).
>
>But there's no corresponding change to the post-revprop-change system!
>
>True, svn_repos__hooks_post_revprop_change() does take a parameter OLD_VALUE.
>But it is never used; there's just a "###" comment in the code about how we
>should pass it to the hook. Is there some reason we're not doing so, in the
>same way that we do in svn_repos__hooks_pre_revprop_change()?
>
>

-- 
Brane Čibej   <brane_at_xbc.nu>   http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org