On Fri, 2008-08-22 at 17:46 +0200, Stefan Sperling wrote:
> 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.
Sure. I'm looking for ways to keep the tree conflict handling as similar
as possible to non-tree-conflict handling.
> > > + /** 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.
Thanks.
> > 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.
OK, I'm glad you agree. I'll check and maybe make that change.
- Julian
---------------------------------------------------------------------
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-26 16:20:40 CEST