Julian Foad <julianfoad_at_btopenworld.com> writes:
> I'm showing this UNFINISHED patch to ask: Does what I'm doing here look
> like a sane improvement?
>
> [...]
>
> Provide factory functions for creating svn_wc_conflict_description_t objects,
> because we require the user to use them. Make two different functions, one
> for creating a text conflict description and one for creating a property
> conflict description, because the required information differs considerably.
Are you finding in practice that you want constructors, or are you
anticipating the need? This route doesn't look insane, but I don't have
enough experience to know if it's really necessary or not...
-Karl
> Improve documentation of svn_wc_conflict_description_t.
>
> * subversion/include/svn_wc.h
> * subversion/libsvn_wc/merge.c
> (svn_wc__merge_internal):
> * subversion/libsvn_wc/util.c
> (svn_wc__path_switched):
>
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 32795)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -1109,9 +1109,14 @@ typedef enum svn_wc_conflict_kind_t
> /** A struct that describes a conflict that has occurred in the
> * working copy. Passed to @c svn_wc_conflict_resolver_func_t.
> *
> + * The conflict described by this structure is one of:
> + * - a conflict on the content of the file node @a path
> + * - a conflict on the property @a property_name of @a path
> + *
> * @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() instead.
> *
> * @since New in 1.5.
> */
> @@ -1126,26 +1131,32 @@ typedef struct svn_wc_conflict_descripti
> /** What sort of conflict are we describing? */
> svn_wc_conflict_kind_t kind;
>
> - /** Only set if this is a property conflict. */
> + /** The name of the property whose conflict is being described.
> + (Only if @a kind is 'property'; else NULL.) */
> const char *property_name;
>
> - /** The following only apply to file objects:
> - * - Whether svn thinks the object is a binary file.
> - * - If available (non-NULL), the svn:mime-type property of the path */
> + /** Whether svn thinks the object is a binary file.
> + (Only if @a kind is 'text', else unspecified.) */
> svn_boolean_t is_binary;
>
> - /** mime-type of the object */
> + /** The svn:mime-type property of the path, if available, else NULL.
> + (Only if @a kind is 'text', else unspecified.) */
> const char *mime_type;
>
> /** If not NULL, an open working copy access baton to either the
> * path itself (if @c path is a directory), or to the parent
> * directory (if @c path is a file.) */
> + /* ### Is this entirely optional, then? */
> svn_wc_adm_access_t *access;
>
> - /** The action being attempted on @c path. */
> + /** The action being attempted on @c path.
> + (When @a kind is 'text', this action must be 'edit'.)
> + (When @a kind is 'property', this action must be 'edit'.) */
> svn_wc_conflict_action_t action;
>
> - /** The reason for the conflict. */
> + /** The relative state of the target that is the reason for the conflict.
> + (When @a kind is 'text', this reason must be 'edited'.)
> + (When @a kind is 'property', this reason must be 'edited'.) */
> svn_wc_conflict_reason_t reason;
>
> /** If this is text-conflict and involves the merging of two files
> @@ -1176,6 +1187,28 @@ typedef struct svn_wc_conflict_descripti
>
> } svn_wc_conflict_description_t;
>
> +/**
> + * Allocate an @c svn_wc_conflict_description_t structure in @a pool,
> + * initialize and return it.
> + *
> + * Set the @c path field of the created struct to @a path. Set all other
> + * fields to their unknown, null or invalid value, respectively.
> + *
> + * @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,
> + const char *mime_type,
> + apr_pool_t *pool);
> +
> +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.
> *
> Index: subversion/libsvn_wc/merge.c
> ===================================================================
> --- subversion/libsvn_wc/merge.c (revision 32795)
> +++ subversion/libsvn_wc/merge.c (working copy)
> @@ -398,24 +398,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 = 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,
> + (mimeprop && mimeprop->value) ? mimeprop->value->data : NULL,
> + pool);
> + cdesc->is_binary = FALSE; /* ### ? */
> + 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:"
> Index: subversion/libsvn_wc/util.c
> ===================================================================
> --- subversion/libsvn_wc/util.c (revision 32795)
> +++ subversion/libsvn_wc/util.c (working copy)
> @@ -291,3 +291,43 @@ 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,
> + const char *mime_type,
> + apr_pool_t *pool)
> +{
> + svn_wc_conflict_description_t *conflict;
> +
> + conflict = apr_pcalloc(pool, sizeof(*conflict));
> + conflict->path = path;
> + conflict->node_kind = svn_node_file;
> + conflict->kind = svn_wc_conflict_kind_text;
> + conflict->access = adm_access;
> + conflict->is_binary = svn_mime_type_is_binary(mime_type);
> + conflict->mime_type = mime_type;
> + 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_pcalloc(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;
> + conflict->action = svn_wc_conflict_action_edit;
> + conflict->reason = svn_wc_conflict_reason_edited;
> + return conflict;
> +}
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: dev-help_at_subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-28 20:04:55 CEST