>> Index: subversion/svn/tree-conflicts.c
>> ===================================================================
>> --- subversion/svn/tree-conflicts.c (.../trunk) (revision 34373)
>> +++ subversion/svn/tree-conflicts.c (.../branches/tc_url_rev) (revision 34373)
>
>> +/* Helper for svn_cl__append_tree_conflict_info_xml().
>> + * Appends the attributes of the given VERSION to ATT_HASH. */
>
> SIDE is ...?
done.
>
>> +static svn_error_t *
>> +add_conflict_version_xml(svn_stringbuf_t **pstr,
>> + const char *side,
>> + svn_wc_conflict_version_t *version,
>> + apr_pool_t *pool)
>> +{
>> + apr_hash_t *att_hash = apr_hash_make(pool);
>> +
>> +
>> + apr_hash_set(att_hash, "side", APR_HASH_KEY_STRING, side);
>> +
>> + if (version->repos_url)
>> + apr_hash_set(att_hash, "repos-url", APR_HASH_KEY_STRING,
>> + version->repos_url);
>> +
>> + if (version->path_in_repos)
>> + apr_hash_set(att_hash, "path-in-repos", APR_HASH_KEY_STRING,
>> + version->path_in_repos);
>> +
>
> Add:
> if (! SVN_IS_VALID_REVNUM(version->peg_rev))
done.
>
>> + apr_hash_set(att_hash, "revision", APR_HASH_KEY_STRING,
>> + apr_itoa(pool, version->peg_rev));
>
> Add:
> if (version->node_kind != svn_node_unknown)
done.
>
>> + apr_hash_set(att_hash, "kind", APR_HASH_KEY_STRING,
>> + svn_cl__node_kind_str_xml(version->node_kind));
>> +
>> + svn_xml_make_open_tag_hash(pstr, pool, svn_xml_self_closing,
>> + "version", att_hash);
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +
>> svn_error_t *
>> svn_cl__append_tree_conflict_info_xml(
>> svn_stringbuf_t *str,
>> @@ -88,34 +125,11 @@ svn_cl__append_tree_conflict_info_xml(
> [...]
>> - apr_hash_set(att_hash, "operation", APR_HASH_KEY_STRING, tmp);
>> + apr_hash_set(att_hash, "operation", APR_HASH_KEY_STRING,
>> + svn_wc_operation_str_xml(conflict->operation, pool));
>
> It's not right for a WC function to know about client-layer XML output.
> The function should be "svn_cl__operation_str_xml()".
done.
moving svn_wc_operation_str_human_readable() along.
>
>> switch (conflict->action)
>> {
>> @@ -160,8 +174,32 @@ svn_cl__append_tree_conflict_info_xml(
>> }
>> apr_hash_set(att_hash, "reason", APR_HASH_KEY_STRING, tmp);
>>
>> - svn_xml_make_open_tag_hash(&str, pool, svn_xml_self_closing,
>> + /* Open the tree-conflict tag. */
>> + svn_xml_make_open_tag_hash(&str, pool, svn_xml_normal,
>> "tree-conflict", att_hash);
>>
>> + /* Add child tags for OLDER_VERSION and THEIR_VERSION. */
>> +
>> + 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.
>
>> + svn_xml_make_close_tag(&str, pool, "tree-conflict");
>> +
>> return SVN_NO_ERROR;
>> }
>> +
>> Index: subversion/svn/info-cmd.c
>> ===================================================================
>> --- subversion/svn/info-cmd.c (.../trunk) (revision 34373)
>> +++ subversion/svn/info-cmd.c (.../branches/tc_url_rev) (revision 34373)
>> @@ -431,10 +431,36 @@ print_info(void *baton,
>>
>> if (info->tree_conflict)
>> {
>> - const char *desc;
>> + const char *desc, *older_version, *their_version;
>> +
>> SVN_ERR(svn_cl__get_human_readable_tree_conflict_description(
>> &desc, info->tree_conflict, pool));
>> - svn_cmdline_printf(pool, _("Tree conflict: %s"), desc);
>> + older_version =
>> + svn_cl__node_description(info->tree_conflict->older_version, pool);
>> + their_version =
>> + svn_cl__node_description(info->tree_conflict->their_version, pool);
>> +
>> + SVN_ERR_ASSERT(older_version && their_version);
>
> Whoa! Description struct doc string says they're optional.
Oh yeah, forgot about this one!
done.
>
>> + svn_cmdline_printf(pool,
>> + "%s: %s\n"
>> + " %s: %s\n"
>> + " %s: %s",
>> + _("Tree conflict"),
>> + desc,
>> + (info->tree_conflict->operation
>> + == svn_wc_operation_merge)
>> + ? _("Merge left") /* (1) */
>> + : _("Older version"),
>> + older_version,
>> + (info->tree_conflict->operation
>> + == svn_wc_operation_merge)
>> + ? _("Merge right")
>> + : _("Their version"),
>> + their_version);
>> + /* (1): Sneaking in a space in "Merge left" so that it is the
>> + * same length as "Merge right" while it still starts in the same
>> + * column. That's just a tiny tweak in the English `svn'. */
>> }
>>
>> /* Print extra newline separator. */
>> Index: subversion/include/svn_wc.h
>> ===================================================================
>> --- subversion/include/svn_wc.h (.../trunk) (revision 34373)
>> +++ subversion/include/svn_wc.h (.../branches/tc_url_rev) (revision 34373)
>> @@ -1167,7 +1167,54 @@ typedef enum svn_wc_operation_t
>>
>> } svn_wc_operation_t;
>>
>> +/** Provides a possibly localized human readable string for
>> + * a given @a operation.
>> + * @note @a pool is currently not used.
>> + * @since New in 1.6.
>> + */
>> +const char *
>> +svn_wc_operation_str_human_readable(svn_wc_operation_t operation,
>> + apr_pool_t *pool);
>
> Should be "svn_cl__..." instead, because presentation is a high-level
> task. (Imagine a client that uses a different word for "update" in its
> UI.)
done (mentioned it already).
>
>> +/** Provides an XML name for a given @a operation.
>> + * @note @a pool is currently not used.
>> + * @since New in 1.6.
>> + */
>> +const char *
>> +svn_wc_operation_str_xml(svn_wc_operation_t operation, apr_pool_t *pool);
>
> Should be "svn_cl__..." instead.
done (mentioned it already).
Up to here in r34395.
>
>> +/** Info about one of the conflicting versions of a node. Each field may
>> + * have its respective null/invalid/unknown value if the corresponding
>> + * information is not relevant or not available.
>
> Add:
> ### Consider making some or all of the info mandatory, to reduce
> complexity.
done in 34397
>
>> + * @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?
Yeah, this makes no sense with having this public function, which takes
these structs as arguments:
svn_wc_conflict_description_t *
svn_wc_conflict_description_create_tree(const char *path,
svn_wc_adm_access_t *adm_access,
svn_node_kind_t node_kind,
svn_wc_operation_t operation,
svn_wc_conflict_version_t
*src_left_version,
svn_wc_conflict_version_t
*src_right_version,
apr_pool_t *pool);
I see that rhujiben as created constructor functions. Making them public is
necessary with above use of svn_wc_conflict_description_create_tree(),
r34400.
>
>> + * @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.
applied a tweaked version of it.
>
>> + svn_revnum_t peg_rev; /* revision at which to look up path_in_repos */
>> + const char *path_in_repos; /* path within repos; must not start with '/' */
>> +
>> + /* Info about this node */
>> + svn_node_kind_t node_kind; /* note that 'none' is a legitimate value */
>> +
>> + /* ### Also? ... */
>> + /* Where to find a local copy of the node */
>> + /* const char *content_cache_path; */
>> + /* const char *props_cache_path; */
>
> This comment block doesn't need to be a "###" attention-grabber. It
> could just say "/* TODO: Add metadata about a local copy of the node, if
> and when we store one. */"
done.
>
>> + /* 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?
I think not. It's just making things very annoying. We'd have to change use
of svn_wc_conflict_description_create_tree() to not take pointer arguments
of this struct. Making them public in r34400.
>
> [...]
>> Index: subversion/include/private/svn_wc_private.h
>> ===================================================================
>> --- subversion/include/private/svn_wc_private.h (.../trunk) (revision 34373)
>> +++ subversion/include/private/svn_wc_private.h (.../branches/tc_url_rev) (revision 34373)
>> @@ -227,6 +227,15 @@ svn_wc_conflict_description_t *
>> svn_wc__conflict_description_dup(const svn_wc_conflict_description_t *conflict,
>> apr_pool_t *pool);
>>
>> +/** Return a duplicate of @a version, allocated in @a pool.
>> + * No part of the new version will be shared with @a version.
>> + *
>> + * @since New in 1.6.
>> + */
>> +svn_wc_conflict_version_t *
>> +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.
>
>> Index: subversion/libsvn_wc/util.c
>> ===================================================================
>> --- subversion/libsvn_wc/util.c (.../trunk) (revision 34373)
>> +++ subversion/libsvn_wc/util.c (.../branches/tc_url_rev) (revision 34373)
> [...]
>> Index: subversion/libsvn_wc/wc.h
>> ===================================================================
>> --- subversion/libsvn_wc/wc.h (.../trunk) (revision 34373)
>> +++ subversion/libsvn_wc/wc.h (.../branches/tc_url_rev) (revision 34373)
>> @@ -255,16 +255,24 @@ svn_wc__text_modified_internal_p(svn_boolean_t *mo
>> conflict is encountered, giving the callback a chance to resolve
>> the conflict (before marking the file 'conflicted').
>>
>> + When LEFT_VERSION and RIGHT_VERSION are non-NULL, pass them to the
>> + conflict resolver as older_version and their_version.
>> +
>> + ## TODO: We should store the information in LEFT_VERSION and RIGHT_VERSION
>> + in the workingcopy for future retrieval via svn info.
>> +
>> Property changes sent by the update are provided in PROP_DIFF.
>>
>> - For a complete description, see svn_wc_merge2() for which this is
>> + For a complete description, see svn_wc_merge3() for which this is
>> the (loggy) implementation.
>> */
>> 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?
>
>> const char *merge_target,
>> const char *copyfrom_text,
>> svn_wc_adm_access_t *adm_access,
>> Index: subversion/libsvn_wc/update_editor.c
>> ===================================================================
>> --- subversion/libsvn_wc/update_editor.c (.../trunk) (revision 34373)
>> +++ subversion/libsvn_wc/update_editor.c (.../branches/tc_url_rev) (revision 34373)
>> @@ -556,6 +556,7 @@ static svn_error_t *
>> do_entry_deletion(struct edit_baton *eb,
>> const char *parent_path,
>> const char *path,
>> + const char *their_url,
>
> Unused? Undocumented?
Used!
Thanks for noticing.
>
>> int *log_number,
>> apr_pool_t *pool);
>>
>> @@ -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.
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.
>
>> }
>> else
>> @@ -1389,6 +1394,8 @@ check_tree_conflict(svn_wc_conflict_description_t
>> const svn_wc_entry_t *entry,
>> svn_wc_adm_access_t *parent_adm_access,
>> svn_wc_conflict_action_t action,
>> + svn_node_kind_t their_node_kind,
>> + const char *their_url,
>
> Undocumented?
thanks. done.
>
>> apr_pool_t *pool)
>> {
>> svn_wc_conflict_reason_t reason = (svn_wc_conflict_reason_t)(-1);
>> @@ -1415,6 +1422,7 @@ check_tree_conflict(svn_wc_conflict_description_t
>>
>> case svn_wc_conflict_action_delete:
>> /* Use case 3: Deleting a locally-deleted item. */
>> + their_node_kind = svn_node_none;
>
> Why override the caller's value? The caller should correctly specify this.
oh! removed that.
>
>> if (entry->schedule == svn_wc_schedule_delete
>> || entry->schedule == svn_wc_schedule_replace)
>> reason = svn_wc_conflict_reason_deleted;
>> @@ -1460,11 +1468,61 @@ check_tree_conflict(svn_wc_conflict_description_t
>> if (reason != (svn_wc_conflict_reason_t)(-1))
>> {
>> svn_wc_conflict_description_t *conflict;
>> + svn_wc_conflict_version_t *older_version;
>> + svn_wc_conflict_version_t *their_version;
>> + const char *repos_url = NULL;
>> + const char *path_in_repos = NULL;
>
> Path of what in repos?
It's first used for src-left and then reused for src-right.
Added comments.
>
>>
>> + repos_url = entry->repos;
>> + path_in_repos = svn_path_is_child(repos_url, entry->url, pool);
>> + if (path_in_repos == NULL)
>> + path_in_repos = ".";
>
> I don't think path "." ("the current working directory") makes sense for
> a repository path. I guess it should be "/" if entry->url is the same as
> entry->repos.
Hm, that makes sense. done.
>
>> + /* "Their" repos_url (repository root URL) will be the same. */
>
> Add: "because a cross-repository switch is not possible" ?
done.
>
>> +
>
> A comment telling the purpose of this if-block would be useful.
>
>> + if (eb->switch_url != NULL)
>> + {
>> + /* do_entry_deletion() still passes NULL for their_url
>> + * sometimes. However, I could not find any case where
>> + * it was needed during those calls. This is just paranoia: */
>> + if (their_url != NULL)
>> + path_in_repos = svn_path_is_child(repos_url, their_url, pool);
>
> Shouldn't this be *their* path_in_repos, not equal to *older*
> path_in_repos?
Ha! Thanks, that's actually the source-right side composed in the wrong
place. Gee, so many bugs. You're really good, Julian :)
>
>> + 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.
>
>> + }
>> + }
>> +
>> + older_version = apr_pcalloc(pool, sizeof(*older_version));
>
> (We're supposed to have an alloc function.)
fixed by someone else.
>
>> + 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.
>
> 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.
>
>> + /* entry->kind is both base kind and working kind, because schedule
>> + * replace-by-different-kind is not supported. */
>> + /* ### TODO: but in case the entry is locally removed, entry->kind
>> + * is svn_node_none and doesn't reflect the older kind. Then we
>> + * need to find out the older kind in a different way! */
>> +
>> + their_version = apr_pcalloc(pool, sizeof(*their_version));
>> + their_version->repos_url = repos_url;
>> + their_version->peg_rev = *eb->target_revision;
>> + their_version->path_in_repos = path_in_repos;
>
> (*Their* path_in_repos needs to be different from the *older*
> path_in_repos.)
yup, already did that as mentioned above.
>
>> + their_version->node_kind = their_node_kind;
>> +
>> conflict = svn_wc_conflict_description_create_tree(
>> full_path, parent_adm_access, entry->kind,
>> eb->switch_url ? svn_wc_operation_switch : svn_wc_operation_update,
>> - pool);
>> + older_version, their_version, pool);
>> conflict->action = action;
>> conflict->reason = reason;
>>
>> @@ -1577,6 +1635,7 @@ static svn_error_t *
>> do_entry_deletion(struct edit_baton *eb,
>> const char *parent_path,
>> const char *path,
>> + const char *their_url,
>
> Undocumented?
done. (already mentioned)
>
>> int *log_number,
>> apr_pool_t *pool)
>> {
> [...]
>> @@ -3756,8 +3822,8 @@ merge_file(svn_wc_notify_state_t *content_state,
>> Remember that this function wants full paths! */
>> SVN_ERR(svn_wc__merge_internal
>> (&log_accum, &merge_outcome,
>> - merge_left,
>> - new_text_base_path,
>> + merge_left, NULL,
>> + new_text_base_path, NULL,
>> fb->path,
>> fb->copied_working_text,
>> adm_access,
>> @@ -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.
>
>> Index: subversion/libsvn_wc/merge.c
>> ===================================================================
>> --- subversion/libsvn_wc/merge.c (.../trunk) (revision 34373)
>> +++ subversion/libsvn_wc/merge.c (.../branches/tc_url_rev) (revision 34373)
> [...]
>> @@ -910,7 +918,9 @@ svn_wc_merge3(enum svn_wc_merge_outcome_t *merge_o
>> svn_stringbuf_t *log_accum = svn_stringbuf_create("", pool);
>>
>> SVN_ERR(svn_wc__merge_internal(&log_accum, merge_outcome,
>> - left, right, merge_target,
>> + left, NULL,
>> + right, NULL,
>
> Add:
> /* ### TODO: Pass version info here. */
>
>> + merge_target,
>> NULL,
>> adm_access,
>> left_label, right_label, target_label,
>> Index: subversion/libsvn_wc/tree_conflicts.c
>> ===================================================================
>> --- subversion/libsvn_wc/tree_conflicts.c (.../trunk) (revision 34373)
>> +++ subversion/libsvn_wc/tree_conflicts.c (.../branches/tc_url_rev) (revision 34373)
> [...]
>> @@ -332,34 +335,29 @@ svn_wc__read_tree_conflicts(apr_array_header_t **c
>> apr_pool_t *pool)
>> {
>> const char *start, *end;
>> - svn_wc_conflict_description_t *conflict = NULL;
>>
>> if (conflict_data == NULL)
>> - {
>> - return SVN_NO_ERROR;
>> - }
>> + return SVN_NO_ERROR;
>>
>> SVN_ERR_ASSERT(*conflicts);
>>
>> start = conflict_data;
>> end = start + strlen(start);
>>
>> - while (start != NULL && start <= end) /* Yes, '<=', because 'start == end'
>> - is a special case that is dealt
>> - with further down the call chain. */
>> + 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.
>
>>
>> - if (start != NULL)
>> - /* Not all conflicts have been read from the entry, but no error
>> - * has been thrown yet. We should not even be here! */
>> - return svn_error_create(SVN_ERR_WC_CORRUPT, NULL,
>> - _("Invalid tree conflict data in 'entries' file, "
>> - "but no idea what went wrong"));
>> -
>> return SVN_NO_ERROR;
>> }
>>
>> @@ -384,6 +382,58 @@ write_string_field(svn_stringbuf_t *buf,
>> }
>> }
>>
>> +/* Append to BUF the string corresponding to enumeration value N, as found
>> + * in MAP. */
>> +static svn_error_t *
>> +write_enum_field(svn_stringbuf_t *buf,
>> + const enum_mapping_t *map,
>> + int n)
>> +{
>> + int i;
>> +
>> + for (i = 0; ; i++)
>> + {
>> + SVN_ERR_ASSERT(map[i].str != NULL);
>> + if (map[i].val == n)
>> + break;
>> + }
>> + svn_stringbuf_appendcstr(buf, map[i].str);
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +/* Append to BUF the denary form of the number N. */
>> +static void
>> +write_integer_field(svn_stringbuf_t *buf,
>> + int n,
>> + apr_pool_t *pool)
>> +{
>> + const char *str = apr_psprintf(pool, "%d", n);
>> +
>> + svn_stringbuf_appendcstr(buf, str);
>> +}
>> +
>> +/* Append to BUF the several fields that represent VERSION_INFO, */
>> +static svn_error_t *
>> +write_node_version_info(svn_stringbuf_t *buf,
>> + const svn_wc_conflict_version_t *version_info,
>> + apr_pool_t *pool)
>> +{
>> + if (version_info->repos_url)
>> + write_string_field(buf, version_info->repos_url);
>> + svn_stringbuf_appendbytes(buf, &field_separator, 1);
>> +
>> + if (SVN_IS_VALID_REVNUM(version_info->peg_rev))
>> + write_integer_field(buf, version_info->peg_rev, pool);
>> + svn_stringbuf_appendbytes(buf, &field_separator, 1);
>> +
>> + if (version_info->path_in_repos)
>> + write_string_field(buf, version_info->path_in_repos);
>> + svn_stringbuf_appendbytes(buf, &field_separator, 1);
>> +
>> + SVN_ERR(write_enum_field(buf, node_kind_map, version_info->node_kind));
>> + return SVN_NO_ERROR;
>> +}
>> +
>> /*
>> * This function could be static, but we need to link to it
>> * in a unit test in tests/libsvn_wc/, so it isn't.
>> @@ -402,82 +452,45 @@ svn_wc__write_tree_conflicts(char **conflict_data,
>> const svn_wc_conflict_description_t *conflict =
>> APR_ARRAY_IDX(conflicts, i, svn_wc_conflict_description_t *);
>>
>> - /* Escape separator chars while writing victim path. */
>> + /* Victim path (escaping separator chars). */
>> path = svn_path_basename(conflict->path, pool);
>> SVN_ERR_ASSERT(strlen(path) > 0);
>> write_string_field(buf, path);
>>
>> svn_stringbuf_appendbytes(buf, &field_separator, 1);
>>
>> - switch (conflict->node_kind)
>> - {
>> - case svn_node_dir:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__NODE_DIR);
>> - break;
>> - case svn_node_file:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__NODE_FILE);
>> - break;
>> - default:
>> - SVN_ERR_MALFUNCTION();
>> - }
>> + /* node_kind */
>> + SVN_ERR_ASSERT(conflict->node_kind == svn_node_dir
>> + || conflict->node_kind == svn_node_file);
>> + SVN_ERR(write_enum_field(buf, node_kind_map, conflict->node_kind));
>>
>> svn_stringbuf_appendbytes(buf, &field_separator, 1);
>>
>> - switch (conflict->operation)
>> - {
>> - case svn_wc_operation_update:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__OPERATION_UPDATE);
>> - break;
>> - case svn_wc_operation_switch:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__OPERATION_SWITCH);
>> - break;
>> - case svn_wc_operation_merge:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__OPERATION_MERGE);
>> - break;
>> - default:
>> - SVN_ERR_MALFUNCTION();
>> - }
>> + /* operation */
>> + SVN_ERR(write_enum_field(buf, operation_map, conflict->operation));
>>
>> svn_stringbuf_appendbytes(buf, &field_separator, 1);
>>
>> - switch (conflict->action)
>> - {
>> - case svn_wc_conflict_action_edit:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__CONFLICT_ACTION_EDITED);
>> - break;
>> - case svn_wc_conflict_action_delete:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__CONFLICT_ACTION_DELETED);
>> - break;
>> - case svn_wc_conflict_action_add:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__CONFLICT_ACTION_ADDED);
>> - break;
>> - default:
>> - SVN_ERR_MALFUNCTION();
>> - }
>> + /* action */
>> + SVN_ERR(write_enum_field(buf, action_map, conflict->action));
>>
>> svn_stringbuf_appendbytes(buf, &field_separator, 1);
>>
>> - switch (conflict->reason)
>> - {
>> - case svn_wc_conflict_reason_edited:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__CONFLICT_REASON_EDITED);
>> - break;
>> - case svn_wc_conflict_reason_deleted:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__CONFLICT_REASON_DELETED);
>> - break;
>> - case svn_wc_conflict_reason_added:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__CONFLICT_REASON_ADDED);
>> - break;
>> - case svn_wc_conflict_reason_missing:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__CONFLICT_REASON_MISSING);
>> - break;
>> - case svn_wc_conflict_reason_obstructed:
>> - svn_stringbuf_appendcstr(buf, SVN_WC__CONFLICT_REASON_OBSTRUCTED);
>> - break;
>> - default:
>> - SVN_ERR_MALFUNCTION();
>> - }
>> + /* reason */
>> + SVN_ERR(write_enum_field(buf, reason_map, conflict->reason));
>>
>> + svn_stringbuf_appendbytes(buf, &field_separator, 1);
>> +
>> + /* 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?
>
>> if (i < (conflicts->nelts - 1))
>> svn_stringbuf_appendbytes(buf, &desc_separator, 1);
>> }
>> Index: subversion/libsvn_wc/log.c
>> ===================================================================
>> --- subversion/libsvn_wc/log.c (.../trunk) (revision 34373)
>> +++ subversion/libsvn_wc/log.c (.../branches/tc_url_rev) (revision 34373)
>> @@ -571,10 +571,10 @@ log_do_merge(struct log_runner *loggy,
>>
>> /* Now do the merge with our full paths. */
>> err = svn_wc__merge_internal(&log_accum, &merge_outcome,
>> - left, right, name, NULL, loggy->adm_access,
>> - left_label, right_label, target_label,
>> - FALSE, loggy->diff3_cmd, NULL, NULL,
>> - NULL, NULL, loggy->pool);
>> + left, NULL, right, NULL, name, NULL,
>> + loggy->adm_access, left_label, right_label,
>> + target_label, FALSE, loggy->diff3_cmd, NULL,
>> + NULL, NULL, NULL, loggy->pool);
>
> Add:
> /* ### TODO: Fill in the left_version and right_version args. */
done.
>
>> if (err && loggy->rerun && APR_STATUS_IS_ENOENT(err->apr_err))
>> {
>> svn_error_clear(err);
>> Index: subversion/libsvn_client/merge.c
>> ===================================================================
>> --- subversion/libsvn_client/merge.c (.../trunk) (revision 34373)
>> +++ subversion/libsvn_client/merge.c (.../branches/tc_url_rev) (revision 34373)
>> @@ -327,11 +327,13 @@ is_path_conflicted_by_merge(merge_cmd_baton_t *mer
>> * The tree conflict, with its victim specified by VICTIM_PATH, is
>> * assumed to have happened during a merge using merge baton MERGE_B.
>> *
>> - * ADM_ACCESS corresponds to the tree-conflicted directory
>> - * This directory must be the victim's parent directory.
>> + * ADM_ACCESS must correspond to the victim's parent directory (even if
>> + * the victim is a directory).
>> *
>> - * NODE_KIND, ACTION, and REASON correspond to the fields
>> - * of the same names in svn_wc_conflict_description_t.
>> + * NODE_KIND must be the node kind of "old" and "theirs" and "mine";
>> + * this function cannot cope with node kind clashes.
>> + * ACTION and REASON correspond to the fields
>> + * of the same names in svn_wc_tree_conflict_description_t.
>> */
>> static svn_error_t*
>> tree_conflict(merge_cmd_baton_t *merge_b,
>> @@ -342,14 +344,42 @@ tree_conflict(merge_cmd_baton_t *merge_b,
>> svn_wc_conflict_reason_t reason)
>> {
>> svn_wc_conflict_description_t *conflict;
>> + const char *src_repos_url; /* root URL of source repository */
>>
>> if (merge_b->record_only || merge_b->dry_run)
>> return SVN_NO_ERROR;
>>
>> conflict = svn_wc_conflict_description_create_tree(
>> - victim_path, adm_access, node_kind, svn_wc_operation_merge, merge_b->pool);
>> + victim_path, adm_access, node_kind, svn_wc_operation_merge,
>> + apr_palloc(merge_b->pool, sizeof(svn_wc_conflict_version_t)),
>> + apr_palloc(merge_b->pool, sizeof(svn_wc_conflict_version_t)),
>
> We're supposed to have and use an allocator function.
got one now, applied it. Thanks.
>
>> + merge_b->pool);
>> conflict->action = action;
>> conflict->reason = reason;
>> +
>> + SVN_ERR(svn_ra_get_repos_root2(merge_b->ra_session1, &src_repos_url,
>> + merge_b->pool));
>> +
>> + if (conflict->older_version)
>
> Condition is always true because we allocated a struct. (Same below.)
Thanks, tweaked the whole thing anyway.
>
>> + {
>> + conflict->older_version->repos_url = src_repos_url;
>> + conflict->older_version->peg_rev = merge_b->merge_source.rev1;
>> + conflict->older_version->path_in_repos
>> + = svn_path_is_child(src_repos_url, merge_b->merge_source.url1,
>> + merge_b->pool);
>> + conflict->older_version->node_kind = node_kind;
>> + }
>> +
>> + if (conflict->their_version)
>> + {
>> + conflict->their_version->repos_url = src_repos_url;
>> + conflict->their_version->peg_rev = merge_b->merge_source.rev2;
>> + conflict->their_version->path_in_repos
>> + = svn_path_is_child(src_repos_url, merge_b->merge_source.url2,
>> + merge_b->pool);
>> + conflict->their_version->node_kind = node_kind;
>> + }
>> +
>> SVN_ERR(svn_wc__add_tree_conflict(conflict, adm_access, merge_b->pool));
>> return SVN_NO_ERROR;
>> }
>
>
> - Julian
Above in r34401.
Wow, what a productive review.
Phew, I'm exhausted :)
~Neels
Received on 2008-11-25 06:53:27 CET