On Fri, Aug 22, 2008 at 04:24:51PM +0100, Julian Foad wrote:
> On Wed, 2008-08-20 at 10:58 +0100, Julian Foad wrote:
> > Here is a log message for the changes currently sitting on the
> > tree-conflicts branch.
>
> And here's the public API part of it with some review comments from
> myself.
Thanks!
Some comments:
> > +
> > + /**
> > + * For a directory only, all tree-conflicted children, stored
> > + * in an array of @c svn_wc_conflict_description_t,
> > + * @since New in 1.6.
> > + */
> > + apr_array_header_t *tree_conflicts;
>
> For backward compatibility, must move the new field to end of struct, or
> rev the struct.
OK.
> > +/** The user operation that exposed a conflict.
> > + *
> > + * @since New in 1.6.
> > + */
> > +typedef enum svn_wc_operation_t
> > +{
> > + svn_wc_operation_update,
> > + svn_wc_operation_switch,
> > + svn_wc_operation_merge,
> > +
> > +} svn_wc_operation_t;
>
> Why is 'operation' to be used only for tree conflicts? Can we make it
> univeral and a separate enhancement?
It was born on the branch because the branch had a need for it.
If it turns out to be useful for other things, it can readily
be used.
I don't see a benefit in making it a separate enhancement
vs. merging it into trunk from the tree-conflicts branch directly.
If trunk needed this today, it would already be there.
> > + /** The path to the victim of a tree conflict.
> > + *
> > + * See the notes at the top of subversion/libsvn_wc/tree_conflicts.h
> > + * for the definition of a tree conflict victim.
> > + *
> > + * @since New in 1.6.
> > + */
> > + const char *victim_path;
>
> 'victim_path' needs clearer documentation.
Most definitely.
Please see the comment I've added to that docstring in r32607.
> If this can only represent
> one victim, then why the separate field at all? How do other fields
> (esp. base_file etc.) behave if this represents a tree conflict?
As I see it, the struct is only supposed to represent a single
tree conflict victim. As such, the victim_path field is superfluous,
as the path field in the same struct already provides a place to
stick path information.
I highly suspect that we never use both the path and victim_path
fields at the same time. If that is really the case (I haven't
checked yet, can you check?), then this field can be removed and
the path field can take its place, for all I care.
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-08-22 17:46:25 CEST