This is all looking good now. Some responses in line.
On Tue, 2008-11-25 at 06:52 +0100, Neels J Hofmeyr wrote:
> >> Index: subversion/svn/tree-conflicts.c
> >> ===================================================================
[...]
> >> + if (conflict->older_version)
> >> + SVN_ERR(add_conflict_version_xml(&str,
> >> + (conflict->operation
> >> + == svn_wc_operation_merge)
> >> + ? "left"
> >> + : "older",
> >> + conflict->older_version,
> >> + pool));
> >> +
> >> + if (conflict->their_version)
> >> + SVN_ERR(add_conflict_version_xml(&str,
> >> + (conflict->operation
> >> + == svn_wc_operation_merge)
> >> + ? "right"
> >> + : "their",
> >> + conflict->their_version,
> >> + pool));
> >
> > I don't like the XML to use different names for the left and right sides
> > depending on what operation caused the conflict. Since the names "older"
> > and "their" are very human-centric and too loaded with meaning (e.g. the
> > "older" version might not actually be older), let's use "left" and
> > "right" or "source-left" and "source-right".
>
> How about on other info? We currently do the same thing in "svn info"
> output. I don't like blowing up the information like that, either.
>
> changed all of it,
> output to "Source left"/"Source right",
> XML to source-left/source-right,
> field names and arguments to src_left_version/src_right_version.
>
> Hope you agree, r34396.
OK.
I'm happy with the XML output and the field names being changed like
that.
About the human-readable strings, I don't have a strong opinion. There
is definitely an argument for having them more "tailored" to match the
user's concept of the operation, but "Source left" and "Source right"
are also OK for me.
> >> +svn_wc__conflict_version_dup(const svn_wc_conflict_version_t *version,
> >> + apr_pool_t *pool);
> >> +
> >
> > 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.)
>
> So, now they're public. :)
> Making these two private means:
> make svn_wc_conflict_description_create_tree() private
> (the _text and _prop sisters of this one are public)
> or make it not take pointer arguments for the ..._version_t's.
> (but they should still be optional)
>
> Not having these public is awkward.
OK.
> >> Index: subversion/libsvn_wc/wc.h
> >> ===================================================================
> >> svn_error_t *
> >> svn_wc__merge_internal(svn_stringbuf_t **log_accum,
> >> enum svn_wc_merge_outcome_t *merge_outcome,
> >> const char *left,
> >> + svn_wc_conflict_version_t *left_version,
> >> const char *right,
> >> + svn_wc_conflict_version_t *right_version,
> >
> > I note that in this context "conflict_version_t" is not a very
> > appropriate name for the info, as there is no conflict (at least not a
> > known conflict, and usually no conflict at all) at this point.
>
> That's probably true. But we can't call it merge_version_t or
> update_version_t, and oh no, please not update_switch_merge_version_t.
> How about change_version_t?
Let's just leave it.
> >> @@ -610,7 +611,11 @@ complete_directory(struct edit_baton *eb,
> >> if (!target_access && entry->kind == svn_node_dir)
> >> {
> >> int log_number = 0;
> >> - SVN_ERR(do_entry_deletion(eb, eb->anchor, eb->target,
> >> + /* I can't find a tree-conflict activated from this call,
> >> + * so we don't *need* THEIR_PATH at all and can skip
> >> + * composing it. Just pass NULL. */
> >> + /* ### TODO make really really sure. */
> >> + SVN_ERR(do_entry_deletion(eb, eb->anchor, eb->target, NULL,
> >> &log_number, pool));
> >
> > What? If the called function is documented as requiring THEIR_PATH, pass
> > it. It's none of our business whether it actually uses it, and to make
> > such shortcuts based on inside knowledge of the callee is asking for
> > bugs later when the callee is modified.
(Sorry to sound so harsh.)
> I can't figure out what to pass there because I can't get this to be
> relevant. It's never printed and stuff.
> ...ok, could fabricate a temporary printf and try then. But it's slightly
> brain-damaged to obtain this kind of information at these points. I tried to
> do it but it became so confusing. So I left it like that for now.
OK, thanks for having a go. (I've had a look for a few minutes and I
can't work it out. It's not worth spending more time on now.)
> >> + else
> >> + {
> >> + /* Oh no! We have no complete URL!
> >> + * At the time of writing this, there is no known case that
> >> + * would cause this to happen. Shout if you've found one. */
> >
> > This comment and block of code is hard to understand.
> >
> > I think it should be more like this:
> >
> > {
> > /* "Their" URL is not readily available, although it is sub-path
> > * of eb->switch_url. For now, just go without it.
> > * ### TODO: Construct "their" path_in_repos. */
> > *their* path_in_repos = NULL;
> > }
> >
> >> + path_in_repos = svn_path_is_child(repos_url, eb->switch_url,
> >> + pool);
> >> + path_in_repos = apr_pstrcat(
> >> + pool, path_in_repos,
> >> + "_THIS_IS_INCOMPLETE",
> >> + NULL);
> >
> > (Why not just set *their* path_in_repos to NULL?)
>
> For example, if I switch from foo to bar, SWITCH_URL will still say
> ".../bar", so, like this, it becomes humanly possible to figure out what is
> going on.
If "there is no known case..." it doesn't seem worth it... but OK,
equally it's doing no harm.
> >> + older_version->repos_url = repos_url;
> >> + older_version->peg_rev = entry->revision;
> >> + older_version->path_in_repos = path_in_repos;
> >> + older_version->node_kind =
> >> + (entry->schedule == svn_wc_schedule_delete) ? svn_node_none
> >> + : entry->kind;
> >
> > No! "Older" means the incoming left-side, i.e. the pristine base. So:
>
> I was wondering about it but thought someone might have had a reason for that.
I'm not sure we understand each other. If the schedule is delete, that
means the WC version is "none" but we want to know the "base" version
which is not "none". So, if sched==delete, then base kind = entry->kind.
And if sched == normal or replace, kind = entry->kind. But if
sched==add, then base kind is 'none' but working kind is entry->kind.
So, if sched==add, base kind=none. In any other case, kind != none, but
in the "delete" case we don't know... (###Or do we?)
> >
> > older_version->node_kind =
> > (entry->schedule == svn_wc_schedule_add) ? svn_node_none
> > : (entry->schedule == svn_wc_schedule_delete) ? svn_node_unknown
> > : entry->kind;
>
> done. But I'm not sure that we've had svn_node_unknown in there before.
> Might have to add some handling code... Well, I guess empty strings are fine.
The idea is that any of the fields in "svn_wc_conflict_version_t" may
have their "null" or "unknown" value. For the "node kind" field "none"
is a valid meaningful value (in a tree conflict situation) whereas
"unknown" is the "unknown" value.
> >> @@ -4004,7 +4070,13 @@ close_edit(void *edit_baton,
> >> pretend that the editor deleted the entry. The helper function
> >> do_entry_deletion() will take care of the necessary steps. */
> >> if ((*eb->target) && (svn_wc__adm_missing(eb->adm_access, target_path)))
> >> - SVN_ERR(do_entry_deletion(eb, eb->anchor, eb->target, &log_number, pool));
> >> + /* I can't find a tree-conflict activated from this call
> >> + * (tried e.g. in update_tests.py 14 update_deleted_missing_dir),
> >> + * so we don't *need* THEIR_PATH at all and can skip composing it.
> >> + * Just pass NULL. */
> >> + /* ### TODO make really really sure. */
> >> + SVN_ERR(do_entry_deletion(eb, eb->anchor, eb->target, NULL,
> >> + &log_number, pool));
> >
> > (As with the similar place above) Don't pass null if the argument is
> > required.
>
> tweaked comments.
OK.
> >> Index: subversion/libsvn_wc/tree_conflicts.c
> >> ===================================================================
> >> + 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.
>
> I'm not following... not doing anything.
I re-wrote the parsing code a few days ago, and in doing so I made it
not notice if the string ended with a description delimiter and then no
description. Previously the code detected that condition as an error.
Because of my change, tree_conflict_data_tests 3 (testing for error
conditions) fails.
I should re-instate the check.
> >> + /* 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.)
>
> Some bells ringing. Did you fix it already?
Ah. It may have been misleading to say the tests were failing because of
this. MY versions of the tests were failing because of this... because I
had already tweaked them to expect the extra fields. But the checked-in
versions were expecting no extra fields, so were probably passing.
Sorry for the confusion there.
Yes, I did fix this yesterday.
> Wow, what a productive review.
> Phew, I'm exhausted :)
Thanks ever so much for fixing (nearly) all of this overnight.
- 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 13:28:10 CET