On Sun, Nov 23, 2008 at 03:44:21AM +0100, Neels J Hofmeyr wrote:
> Hey Stefan,
>
> you broke use of your own change in
> svn_wc_conflict_description_create_tree() with this commit, causing a
> segfault. See inline...
Doh!
> > @@ -350,26 +350,27 @@ tree_conflict(merge_cmd_baton_t *merge_b
> > return SVN_NO_ERROR;
> >
> > conflict = svn_wc_conflict_description_create_tree(
> > - victim_path, adm_access, node_kind, svn_wc_operation_merge, merge_b->pool);
> > + victim_path, adm_access, node_kind, svn_wc_operation_merge,
> > + NULL, NULL, merge_b->pool);
>
> See, here you pass NULL for both OLDER_VERSION and THEIR_VERSION...
>
> > conflict->action = action;
> > conflict->reason = reason;
> >
> > SVN_ERR(svn_ra_get_repos_root2(merge_b->ra_session1, &src_repos_url,
> > merge_b->pool));
> >
> > - conflict->older_version.repos_url = src_repos_url;
> > - conflict->older_version.peg_rev = merge_b->merge_source.rev1;
> > - conflict->older_version.path_in_repos
> > + conflict->older_version->repos_url = src_repos_url;
> > + conflict->older_version->peg_rev = merge_b->merge_source.rev1;
> > + conflict->older_version->path_in_repos
> > = svn_path_is_child(src_repos_url, merge_b->merge_source.url1,
> > merge_b->pool);
> > - conflict->older_version.node_kind = node_kind;
> > + conflict->older_version->node_kind = node_kind;
> >
> > - conflict->their_version.repos_url = src_repos_url;
> > - conflict->their_version.peg_rev = merge_b->merge_source.rev2;
> > - conflict->their_version.path_in_repos
> > + conflict->their_version->repos_url = src_repos_url;
> > + conflict->their_version->peg_rev = merge_b->merge_source.rev2;
> > + conflict->their_version->path_in_repos
> > = svn_path_is_child(src_repos_url, merge_b->merge_source.url2,
> > merge_b->pool);
> > - conflict->their_version.node_kind = node_kind;
> > + conflict->their_version->node_kind = node_kind;
>
> ...and here you're setting values in them without creating the struct memory.
Ouch, missing NULL check.
> Say, why is it necessary to
>
> 1. be able to set OLDER_ and THEIR_VERSION during
> svn_wc_conflict_description_create_tree()?
That's not a strict requirement. What I found nice about this
change is that it enforced a certain order on initialising
things inside the check_tree_conflict() function.
It's now initialising all data needed to create a conflict
description, and *then* creates it. Previously, it created
the object and then initialised fields in some arbitrary order.
I know it's documented that description_create_tree does not
initialise all members, but I'd prefer as much constructor-like
behaviour here as possible, hiding member initialisation behind
function arguments.
If you disagree, that's OK, I'm not too much in love with
either solution. Both work, you just have to know what's
going on.
>
> 2. have them as pointers in the conflict description? I think this
> introduces new errors that aren't necessary, like this one :P
Take a look below at svn_wc__conflict_description_dup() from libsvn_wc/util.c
('//'-style comments added by me in this mail):
svn_wc_conflict_description_t *
svn_wc__conflict_description_dup(const svn_wc_conflict_description_t *conflict,
apr_pool_t *pool)
{
svn_wc_conflict_description_t *new_conflict;
new_conflict = apr_pcalloc(pool, sizeof(*new_conflict));
/* Shallow copy all members. */
*new_conflict = *conflict;
if (conflict->path)
new_conflict->path = apr_pstrdup(pool, conflict->path);
if (conflict->property_name)
new_conflict->property_name = apr_pstrdup(pool, conflict->property_name);
if (conflict->mime_type)
new_conflict->mime_type = apr_pstrdup(pool, conflict->mime_type);
/* NOTE: We cannot make a deep copy of adm_access. */
if (conflict->base_file)
new_conflict->base_file = apr_pstrdup(pool, conflict->base_file);
if (conflict->their_file)
new_conflict->their_file = apr_pstrdup(pool, conflict->their_file);
if (conflict->my_file)
new_conflict->my_file = apr_pstrdup(pool, conflict->my_file);
if (conflict->merged_file)
new_conflict->merged_file = apr_pstrdup(pool, conflict->merged_file);
// this is new on the branch:
if (conflict->older_version)
new_conflict->older_version =
svn_wc__conflict_version_dup(conflict->older_version, pool);
// this is new on the branch:
if (conflict->their_version)
new_conflict->their_version =
svn_wc__conflict_version_dup(conflict->their_version, pool);
return new_conflict;
}
If these members weren't pointers, we couldn't just replace them
entirely with a dup()'d version. We'd have to manually make a
deep copy of the pointer members of svn_wc_conflict_version_t
^^^^^^^^^^^^^^^
inside the svn_wc__conflict_description_dup() function instead.
^^^^^^^^^^^^^^^^
i.e.:
...
// this is new on the branch:
new_conflict.older_version.repos_url =
apr_pstrdup(pool, conflict.older_version.repos_url);
new_conflict.older_version.path_in_repos =
apr_pstrdup(pool, conflict.older_version.path_in_repos);
// this is new on the branch:
new_conflict.their_version.repos_url =
apr_pstrdup(pool, conflict.their_version.repos_url);
new_conflict.their_version.path_in_repos =
apr_pstrdup(pool, conflict.their_version.path_in_repos);
...
There are more dup() functions in our code that call other dup()
functions to deep-copy 'pointer-to-struct' type members.
See for example svn_wc_dup_notify() and svn_wc_dup_status2().
So I added a svn_wc__conflict_version_dup() function and
changed the member to pointers so I could use it.
I just thought not doing it this way was inconsistent with existing
code and therefore ugly, because we'd have to different idioms in
the code. That's all.
>
> Bert Hujiben already sets these to NULL in a new patch, when none of the
> handling code can cope with them being NULL. If you want to keep it this
> way, we need to go around and add checking for NULL everywhere. :(
Yes. I can do that if you want me to.
> I think having this as a pointer adds loads of ugly checking to the code
> while yielding only tiny benefits. +1 for changing back to inlining the
> svn_wc_version_t into the svn_wc_conflict_t struct.
If you think the above inconsistency in the dup() function is the
better alternative, that's fine with me, too.
> I'll try fix this segfault for now so I can use the branch.
Thanks, and sorry about breaking it.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-11-23 13:50:52 CET