You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2008/09/02 16:52:48 UTC
Re: svn commit: r32804 - in trunk/subversion: include libsvn_wc
julianfoad@tigris.org writes:
> --- subversion/include/svn_wc.h (revision 32795)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -1176,6 +1187,47 @@ typedef struct svn_wc_conflict_descripti
>
> } svn_wc_conflict_description_t;
>
> +/**
> + * Allocate an @c svn_wc_conflict_description_t structure in @a pool,
> + * initialize to represent a text conflict, and return it.
> + *
> + * Set the @c path field of the created struct to @a path, the @c access
> + * field to @a adm_access, the @c kind field to @c
> + * svn_wc_conflict_kind_text, the @c node_kind to @c svn_node_file, the @c
> + * action to @c svn_wc_conflict_action_edit, and the @c reason to @c
> + * svn_wc_conflict_reason_edited.
> + *
> + * @note: It is the caller's responsibility to set the other required fields
> + * (such as the four file names and @c mime_type and @c is_binary).
> + *
> + * @since New in 1.6.
> + */
> +svn_wc_conflict_description_t *
> +svn_wc_conflict_description_create_text(const char *path,
> + svn_wc_adm_access_t *adm_access,
> + apr_pool_t *pool);
Is @a path stored by reference, or is its value copied into @a pool?
Doc string should say.
> +/**
> + * Allocate an @c svn_wc_conflict_description_t structure in @a pool,
> + * initialize to represent a property conflict, and return it.
> + *
> + * Set the @c path field of the created struct to @a path, the @c access
> + * field to @a adm_access, the @c kind field to @c
> + * svn_wc_conflict_kind_prop, the @c node_kind to @a node_kind, and the @c
> + * property_name to @a property_name.
> + *
> + * @note: It is the caller's responsibility to set the other required fields
> + * (such as the four file names and @c action and @c reason).
> + *
> + * @since New in 1.6.
> + */
> +svn_wc_conflict_description_t *
> +svn_wc_conflict_description_create_prop(const char *path,
> + svn_wc_adm_access_t *adm_access,
> + svn_node_kind_t node_kind,
> + const char *property_name,
> + apr_pool_t *pool);
Same here, of course, and the same question applies to the @a adm_access
and @a property_name fields.
(I can see in the implementations that it's by reference, so I guess
that makes my questioning form rhetorical only.)
This next point is unrelated to your commit, I just wanted to make a
note of it:
Below, it seems we are including the *old* commit email output too, even
though the log message and diffs are above. See the 'Modified' field
followed by old-style diffs complete with dates in their headers, etc:
> Modified:
> trunk/subversion/include/svn_wc.h
> trunk/subversion/libsvn_wc/merge.c
> trunk/subversion/libsvn_wc/props.c
> trunk/subversion/libsvn_wc/util.c
>
> Modified: trunk/subversion/include/svn_wc.h
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/include/svn_wc.h?pathrev=32804&r1=32803&r2=32804
> ==============================================================================
> --- trunk/subversion/include/svn_wc.h Fri Aug 29 04:12:29 2008 (r32803)
> +++ trunk/subversion/include/svn_wc.h Fri Aug 29 04:13:14 2008 (r32804)
> @@ -1115,7 +1115,9 @@ typedef enum svn_wc_conflict_kind_t
> *
> * @note Fields may be added to the end of this structure in future
> * versions. Therefore, to preserve binary compatibility, users
> - * should not directly allocate structures of this type.
> + * should not directly allocate structures of this type but should use
> + * svn_wc_create_conflict_description_text() or
> + * svn_wc_create_conflict_description_prop() instead.
> *
> * @since New in 1.5.
> */
> @@ -1185,6 +1187,47 @@ typedef struct svn_wc_conflict_descripti
>
> } svn_wc_conflict_description_t;
>
> +/**
> + * Allocate an @c svn_wc_conflict_description_t structure in @a pool,
> + * initialize to represent a text conflict, and return it.
> + *
> + * Set the @c path field of the created struct to @a path, the @c access
> + * field to @a adm_access, the @c kind field to @c
> + * svn_wc_conflict_kind_text, the @c node_kind to @c svn_node_file, the @c
> + * action to @c svn_wc_conflict_action_edit, and the @c reason to @c
> + * svn_wc_conflict_reason_edited.
> + *
> + * @note: It is the caller's responsibility to set the other required fields
> + * (such as the four file names and @c mime_type and @c is_binary).
> + *
> + * @since New in 1.6.
> + */
> +svn_wc_conflict_description_t *
> +svn_wc_conflict_description_create_text(const char *path,
> + svn_wc_adm_access_t *adm_access,
> + apr_pool_t *pool);
> +
> +/**
> + * Allocate an @c svn_wc_conflict_description_t structure in @a pool,
> + * initialize to represent a property conflict, and return it.
> + *
> + * Set the @c path field of the created struct to @a path, the @c access
> + * field to @a adm_access, the @c kind field to @c
> + * svn_wc_conflict_kind_prop, the @c node_kind to @a node_kind, and the @c
> + * property_name to @a property_name.
> + *
> + * @note: It is the caller's responsibility to set the other required fields
> + * (such as the four file names and @c action and @c reason).
> + *
> + * @since New in 1.6.
> + */
> +svn_wc_conflict_description_t *
> +svn_wc_conflict_description_create_prop(const char *path,
> + svn_wc_adm_access_t *adm_access,
> + svn_node_kind_t node_kind,
> + const char *property_name,
> + apr_pool_t *pool);
> +
>
> /** The way in which the conflict callback chooses a course of action.
> *
>
> Modified: trunk/subversion/libsvn_wc/merge.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/merge.c?pathrev=32804&r1=32803&r2=32804
> ==============================================================================
> --- trunk/subversion/libsvn_wc/merge.c Fri Aug 29 04:12:29 2008 (r32803)
> +++ trunk/subversion/libsvn_wc/merge.c Fri Aug 29 04:13:14 2008 (r32804)
> @@ -398,24 +398,20 @@ svn_wc__merge_internal(svn_stringbuf_t *
> if (conflict_func)
> {
> svn_wc_conflict_result_t *result = NULL;
> - svn_wc_conflict_description_t cdesc;
> + svn_wc_conflict_description_t *cdesc;
>
> - cdesc.path = merge_target;
> - cdesc.node_kind = svn_node_file;
> - cdesc.kind = svn_wc_conflict_kind_text;
> - cdesc.is_binary = FALSE;
> - cdesc.mime_type = (mimeprop && mimeprop->value)
> - ? mimeprop->value->data : NULL;
> - cdesc.access = adm_access;
> - cdesc.action = svn_wc_conflict_action_edit;
> - cdesc.reason = svn_wc_conflict_reason_edited;
> - cdesc.base_file = left;
> - cdesc.their_file = right;
> - cdesc.my_file = tmp_target;
> - cdesc.merged_file = result_target;
> - cdesc.property_name = NULL;
> + cdesc = svn_wc_conflict_description_create_text(merge_target,
> + adm_access,
> + pool);
> + cdesc->is_binary = FALSE;
> + cdesc->mime_type = (mimeprop && mimeprop->value)
> + ? mimeprop->value->data : NULL,
> + cdesc->base_file = left;
> + cdesc->their_file = right;
> + cdesc->my_file = tmp_target;
> + cdesc->merged_file = result_target;
>
> - SVN_ERR(conflict_func(&result, &cdesc, conflict_baton, pool));
> + SVN_ERR(conflict_func(&result, cdesc, conflict_baton, pool));
> if (result == NULL)
> return svn_error_create(SVN_ERR_WC_CONFLICT_RESOLVER_FAILURE,
> NULL, _("Conflict callback violated API:"
> @@ -714,24 +710,19 @@ svn_wc__merge_internal(svn_stringbuf_t *
> if (conflict_func)
> {
> svn_wc_conflict_result_t *result = NULL;
> - svn_wc_conflict_description_t cdesc;
> + svn_wc_conflict_description_t *cdesc;
>
> - cdesc.path = merge_target;
> - cdesc.node_kind = svn_node_file;
> - cdesc.kind = svn_wc_conflict_kind_text;
> - cdesc.is_binary = TRUE;
> - cdesc.mime_type = (mimeprop && mimeprop->value)
> - ? mimeprop->value->data : NULL;
> - cdesc.access = adm_access;
> - cdesc.action = svn_wc_conflict_action_edit;
> - cdesc.reason = svn_wc_conflict_reason_edited;
> - cdesc.base_file = left;
> - cdesc.their_file = right;
> - cdesc.my_file = tmp_target;
> - cdesc.merged_file = NULL; /* notice there is NO merged file! */
> - cdesc.property_name = NULL;
> + cdesc = svn_wc_conflict_description_create_text(merge_target,
> + adm_access, pool);
> + cdesc->is_binary = TRUE;
> + cdesc->mime_type = (mimeprop && mimeprop->value)
> + ? mimeprop->value->data : NULL,
> + cdesc->base_file = left;
> + cdesc->their_file = right;
> + cdesc->my_file = tmp_target;
> + cdesc->merged_file = NULL; /* notice there is NO merged file! */
>
> - SVN_ERR(conflict_func(&result, &cdesc, conflict_baton, pool));
> + SVN_ERR(conflict_func(&result, cdesc, conflict_baton, pool));
> if (result == NULL)
> return svn_error_create(SVN_ERR_WC_CONFLICT_RESOLVER_FAILURE,
> NULL, _("Conflict callback violated API:"
>
> Modified: trunk/subversion/libsvn_wc/props.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/props.c?pathrev=32804&r1=32803&r2=32804
> ==============================================================================
> --- trunk/subversion/libsvn_wc/props.c Fri Aug 29 04:12:29 2008 (r32803)
> +++ trunk/subversion/libsvn_wc/props.c Fri Aug 29 04:13:14 2008 (r32804)
> @@ -1274,7 +1274,8 @@ maybe_generate_propconflict(svn_boolean_
> return SVN_NO_ERROR;
> }
>
> - cdesc = apr_pcalloc(pool, sizeof(*cdesc));
> + cdesc = svn_wc_conflict_description_create_prop(
> + path, adm_access, is_dir ? svn_node_dir : svn_node_file, propname, pool);
>
> /* Create a tmpfile for each of the string_t's we've got. */
> if (working_val)
> @@ -1361,12 +1362,6 @@ maybe_generate_propconflict(svn_boolean_
> }
>
> /* Build the rest of the description object: */
> - cdesc->path = path;
> - cdesc->node_kind = is_dir ? svn_node_dir : svn_node_file;
> - cdesc->kind = svn_wc_conflict_kind_property;
> - cdesc->property_name = propname;
> - cdesc->access = adm_access;
> -
> if (!is_dir && working_props)
> mime_propval = apr_hash_get(working_props, SVN_PROP_MIME_TYPE,
> APR_HASH_KEY_STRING);
>
> Modified: trunk/subversion/libsvn_wc/util.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/util.c?pathrev=32804&r1=32803&r2=32804
> ==============================================================================
> --- trunk/subversion/libsvn_wc/util.c Fri Aug 29 04:12:29 2008 (r32803)
> +++ trunk/subversion/libsvn_wc/util.c Fri Aug 29 04:13:14 2008 (r32804)
> @@ -291,3 +291,38 @@ svn_wc__path_switched(const char *wc_pat
>
> return SVN_NO_ERROR;
> }
> +
> +svn_wc_conflict_description_t *
> +svn_wc_conflict_description_create_text(const char *path,
> + svn_wc_adm_access_t *adm_access,
> + apr_pool_t *pool)
> +{
> + svn_wc_conflict_description_t *conflict;
> +
> + conflict = apr_palloc(pool, sizeof(*conflict));
> + conflict->path = path;
> + conflict->node_kind = svn_node_file;
> + conflict->kind = svn_wc_conflict_kind_text;
> + conflict->access = adm_access;
> + conflict->action = svn_wc_conflict_action_edit;
> + conflict->reason = svn_wc_conflict_reason_edited;
> + return conflict;
> +}
> +
> +svn_wc_conflict_description_t *
> +svn_wc_conflict_description_create_prop(const char *path,
> + svn_wc_adm_access_t *adm_access,
> + svn_node_kind_t node_kind,
> + const char *property_name,
> + apr_pool_t *pool)
> +{
> + svn_wc_conflict_description_t *conflict;
> +
> + conflict = apr_palloc(pool, sizeof(*conflict));
> + conflict->path = path;
> + conflict->node_kind = node_kind;
> + conflict->kind = svn_wc_conflict_kind_property;
> + conflict->access = adm_access;
> + conflict->property_name = property_name;
> + return conflict;
> +}
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r32804 - in trunk/subversion: include libsvn_wc
Posted by Karl Fogel <kf...@red-bean.com>.
Karl Fogel <kf...@red-bean.com> writes:
> This next point is unrelated to your commit, I just wanted to make a
> note of it:
>
> Below, it seems we are including the *old* commit email output too, even
> though the log message and diffs are above. See the 'Modified' field
> followed by old-style diffs complete with dates in their headers, etc:
Never mind, I saw that this was a log message error that you corrected
later.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r32804 - in trunk/subversion: include libsvn_wc
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2008-09-02 at 19:45 +0100, Julian Foad wrote:
> On Tue, 2008-09-02 at 12:52 -0400, Karl Fogel wrote:
> > julianfoad@tigris.org writes:
> > > --- subversion/include/svn_wc.h (revision 32795)
> > > +++ subversion/include/svn_wc.h (working copy)
> > > @@ -1176,6 +1187,47 @@ typedef struct svn_wc_conflict_descripti
> > >
> > > } svn_wc_conflict_description_t;
> > >
> > > +/**
> > > + * Allocate an @c svn_wc_conflict_description_t structure in @a pool,
> > > + * initialize to represent a text conflict, and return it.
> > > + *
> > > + * Set the @c path field of the created struct to @a path, the @c access
> > > + * field to @a adm_access, the @c kind field to @c
> > > + * svn_wc_conflict_kind_text, the @c node_kind to @c svn_node_file, the @c
> > > + * action to @c svn_wc_conflict_action_edit, and the @c reason to @c
> > > + * svn_wc_conflict_reason_edited.
> > > + *
> > > + * @note: It is the caller's responsibility to set the other required fields
> > > + * (such as the four file names and @c mime_type and @c is_binary).
> > > + *
> > > + * @since New in 1.6.
> > > + */
> > > +svn_wc_conflict_description_t *
> > > +svn_wc_conflict_description_create_text(const char *path,
> > > + svn_wc_adm_access_t *adm_access,
> > > + apr_pool_t *pool);
> >
> > Is @a path stored by reference, or is its value copied into @a pool?
> > Doc string should say.
>
> Thanks for catching this. Same applies to some other constructors that
> we have.
>
> Is the attached patch "shallow-copy-1.patch" OK?
It seemed safe enough to me. Committed in r32905.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r32804 - in trunk/subversion: include libsvn_wc
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2008-09-02 at 12:52 -0400, Karl Fogel wrote:
> julianfoad@tigris.org writes:
> > --- subversion/include/svn_wc.h (revision 32795)
> > +++ subversion/include/svn_wc.h (working copy)
> > @@ -1176,6 +1187,47 @@ typedef struct svn_wc_conflict_descripti
> >
> > } svn_wc_conflict_description_t;
> >
> > +/**
> > + * Allocate an @c svn_wc_conflict_description_t structure in @a pool,
> > + * initialize to represent a text conflict, and return it.
> > + *
> > + * Set the @c path field of the created struct to @a path, the @c access
> > + * field to @a adm_access, the @c kind field to @c
> > + * svn_wc_conflict_kind_text, the @c node_kind to @c svn_node_file, the @c
> > + * action to @c svn_wc_conflict_action_edit, and the @c reason to @c
> > + * svn_wc_conflict_reason_edited.
> > + *
> > + * @note: It is the caller's responsibility to set the other required fields
> > + * (such as the four file names and @c mime_type and @c is_binary).
> > + *
> > + * @since New in 1.6.
> > + */
> > +svn_wc_conflict_description_t *
> > +svn_wc_conflict_description_create_text(const char *path,
> > + svn_wc_adm_access_t *adm_access,
> > + apr_pool_t *pool);
>
> Is @a path stored by reference, or is its value copied into @a pool?
> Doc string should say.
Thanks for catching this. Same applies to some other constructors that
we have.
Is the attached patch "shallow-copy-1.patch" OK?
Now that you raise the question, shallow copying doesn't seem like the
ideal (safe and intuitive) behaviour for a constructor. It might be
better to change this (and other constructors) to make a deep copy of
pointer arguments such as "path". But I'm not so sure it makes sense to
duplicate an adm_access baton for use at a later time.
I'll not attempt that now.
- Julian
> > +/**
> > + * Allocate an @c svn_wc_conflict_description_t structure in @a pool,
> > + * initialize to represent a property conflict, and return it.
> > + *
> > + * Set the @c path field of the created struct to @a path, the @c access
> > + * field to @a adm_access, the @c kind field to @c
> > + * svn_wc_conflict_kind_prop, the @c node_kind to @a node_kind, and the @c
> > + * property_name to @a property_name.
> > + *
> > + * @note: It is the caller's responsibility to set the other required fields
> > + * (such as the four file names and @c action and @c reason).
> > + *
> > + * @since New in 1.6.
> > + */
> > +svn_wc_conflict_description_t *
> > +svn_wc_conflict_description_create_prop(const char *path,
> > + svn_wc_adm_access_t *adm_access,
> > + svn_node_kind_t node_kind,
> > + const char *property_name,
> > + apr_pool_t *pool);
>
> Same here, of course, and the same question applies to the @a adm_access
> and @a property_name fields.
>
> (I can see in the implementations that it's by reference, so I guess
> that makes my questioning form rhetorical only.)