You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2008/08/28 17:59:02 UTC
[PATCH] Conflict description - factory functions and doc strings
I'm showing this UNFINISHED patch to ask: Does what I'm doing here look
like a sane improvement?
- Julian
Re: [PATCH] Conflict description - factory functions and doc
strings
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2008-08-28 at 14:04 -0400, Karl Fogel wrote:
> Julian Foad <ju...@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...
I'm finding that
(a) we implied in the doc-string that there should be a constructor so
that the structure can be extended in a backward-compatible way, but
there wasn't one;
(b) there are many rules about what fields make sense in various
combinations for different types of conflict, and those rules aren't
clearly documented and there is no place where they can be
programatically enforced or encouraged. Constructors provide a place
where the user can confidently gather all the required data in one
place. Specific constructors for different types of conflict enable the
user to ignore the fields that are not relevant.
(c) when it comes to extending it to hold tree conflicts as well, the
idea that each user should just know what fields to fill in in what
combinations becomes too fragile.
If I keep going down this route, a next phase might be to provide
accessor functions for reading the structure. Not yet sure how necessary
that is, but the sort of thing I'm thinking is that the old/theirs/mine
files might not necessarily be stored and retrieved as temporary file
names, but rather stored as references to the WC entry and later
retrieved as svn_stream_t's, or something like that. In the limit, it
might be good for the structure itself to become opaque, non-public.
Basically, this feels like a good way to go, but we haven't done much of
this from what I can see so I wanted a sanity check. I'm also not sure
how far it is worth taking the idea. Probably not too far.
I may pursue this a bit further and then show a complete patch.
Thanks.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Conflict description - factory functions and doc strings
Posted by Karl Fogel <kf...@red-bean.com>.
Julian Foad <ju...@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@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org