On Mon, 2008-11-24 at 16:05 +0100, Stefan Sperling wrote:
> On Mon, Nov 24, 2008 at 12:23:26PM +0000, Julian Foad wrote:
> > On Mon, 2008-11-24 at 07:02 +0100, Neels J Hofmeyr wrote:
> > > For convenience, find attached the current branch diff. I can't comment on
> > > it anymore, need to go to bed...
> >
> > Reviewing in line...
>
> Thanks Julian!
>
> I've read all your comments, and I mostly agree.
> Some comments below.
[...]
> > > + /* Remember to update svn_wc__conflict_version_dup()
> > > + * in case you add fields to this struct. */
> > > +} svn_wc_conflict_version_t;
> >
> > The structure's alloc and dup functions should be here.
[...]
> > If the struct is public then its alloc and dup functions should be
> > public too. (And if one is private they all should be private.)
>
> I don't think clients will ever need to allocate this structure
> directly. They will always be getting it from libsvn_wc.
> However, they need to be able to read the fields.
>
> That's why the struct itself is public while the functions are
> private. Should we document this in the struct docstring?
>
> Something like:
> + * Clients are not expected to be allocating this structure
> + * directly.
I get a bit muddled about public/private considerations. I argued that
the add/del/get-tree-conflict functions should be private to Subversion.
Now we are adding tree conflict data to this structure that is already
public, and we are considering whether to keep its alloc and dup
functions private. I think the answer here is that that alloc and dup
functions are "part of" the proper semantic definition of a data
structure. Note that C++ requires[1] an alloc function ("constructor")
and a dup ("copy constructor") for every class. The semantics and
backward-compatibility issues of these functions are simple, so there
should be no problem with making them public. A client might want to
keep copies of these structures outside the pool that we provide them
in, so there is a need for them to be public.
Neels made them public in the branch.
[1] What C++ actually requires is a no-arguments "default constuctor".
Also the programmer can override the requirement but has to do so
explicitly in each case.
> > I lost a bit of defensive coding here: the loop can now finish cleanly
> > after a trailing separator character with no further conflict
> > description. Perhaps I should re-instate the check that another conflict
> > follows the separator.
>
> +1
Will do.
> Note that gstein is currently working on skels support, but we should
> still reinstate this check anyway.
Ah! I didn't know, though I'm not too surprised and am glad to hear it.
> > > + /* older_version */
> > > + if (conflict->older_version)
> > > + SVN_ERR(write_node_version_info(buf, conflict->older_version, pool));
> > > +
> > > + svn_stringbuf_appendbytes(buf, &field_separator, 1);
> > > +
> > > + /* their_version */
> > > + if (conflict->their_version)
> > > + SVN_ERR(write_node_version_info(buf, conflict->their_version, pool));
> >
> > Oops - we cannot just omit these from the output string. We still need
> > the required number of separators in the output string.
> >
> > (subversion/tests/libsvn_wc/tree_conflict_data_tests 4 and 5 fail
> > because of this.)
>
> Right, so let's change it to printing "unknown" or something when we
> don't know. But I'd argue that whenever we run into this situation in
> a tree conflict scenario, there's a bug (text and prop may be a
> different story). Unless there are known cases where we cannot possibly
> get at the node's repo URL.
I decided that structure-pointer-is-null means the same as
all-fields-unknown, so I made it output empty fields in this case. When
reading back in again it will allocate a structure and set all of the
fields to their default values.
This may not be perfect but I think it is OK.
- 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-11-25 12:30:02 CET