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.
> > + * @since New in 1.6.
> > +*/
> > +typedef struct svn_wc_conflict_version_t
> > +{
> > + /* Where to find this node version in a repository */
> > + const char *repos_url; /* URL of repository root */
> > + /* ### Also? repos_uuid; */
>
> That doesn't need to be a "###" attention-grabber. It would be better as
> "/* These fields should be whatever is necessary to identify the source.
> This could include the repository UUID, if we feel that handling
> conflicts properly during a repository move might be an issue. */" Or it
> can just go away.
It might come in handy during strange inter-repository merges where
the URLs aren't sufficient to distinguish the items involved,
e.g. because repositories have switched locations at some point
in the past, or something...
Well, that sounds pretty much like PEBKAC, but whatever, let's just
leave it in.
> > + * @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.
>
> Clients should use what to allocate?
[...]
> > + /* 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.
>
> Consider: Would it make sense for this struct be a private definition,
> even though it appears within a public structure?
[...]
> 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.
+1 on a private allocation function anyway so we only need
to tweak one place when changing the struct to fix allocations.
> > + while (start < end)
> > {
> > + svn_wc_conflict_description_t *conflict;
> > +
> > SVN_ERR(read_one_tree_conflict(&conflict, &start, end, dir_path, pool));
> > if (conflict != NULL)
> > APR_ARRAY_PUSH(*conflicts, svn_wc_conflict_description_t *) = conflict;
> > +
> > + /* *START should now point to a description separator
> > + * if there are any descriptions left. */
> > + if (start < end)
> > + SVN_ERR(read_desc_separator(&start, end));
> > }
>
> 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
Note that gstein is currently working on skels support, but we should
still reinstate this check anyway.
> > + /* 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.
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-11-24 16:05:36 CET