Re: svn commit: r32804 - in trunk/subversion: include libsvn_wc

On Tue, 2008-09-02 at 12:52 -0400, Karl Fogel wrote:
> julianfoad_at_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.)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org