[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: branch tc_url_rev: almost ready to merge

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Mon, 24 Nov 2008 12:23:26 +0000

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...

I have made a lot of comments. I agree that the branch is ready (I'd say
over-due) for re-integrating, feature-wise, once we get these bugs
fixed. Please don't do any more feature development on this branch.

> Neels J Hofmeyr wrote:
> > Hi tree-conflicts folks,
> >
> > I think we're ready to merge tc_url_rev to trunk code-wise. But Julian,
> > could you maybe polish over the question/todo comments that were in your
> > initial patch (and survived), and remind us if we forgot something?
> >
> > BTW, gstein and stsp were both +1 on merging to trunk earlier to"day" on IRC
> > (under the assumption that everything that's there is good). So am I.

[...]
> 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 ...?

> +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))

> + apr_hash_set(att_hash, "revision", APR_HASH_KEY_STRING,
> + apr_itoa(pool, version->peg_rev));

Add:
if (version->node_kind != svn_node_unknown)

> + 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()".

> 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".

> + 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.

> + 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.)
 
> +/** 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.

> +/** 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.

> + * @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?

> + * @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.

> + 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. */"

> + /* 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?

[...]
> 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.)

> 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.

> 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?

> 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.

> }
> 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?

> 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.

> 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?

>
> + 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.

> + /* "Their" repos_url (repository root URL) will be the same. */

Add: "because a cross-repository switch is not possible" ?

> +

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?

> + 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?)

> + }
> + }
> +
> + older_version = apr_pcalloc(pool, sizeof(*older_version));

(We're supposed to have an alloc function.)

> + 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:

  older_version->node_kind =
    (entry->schedule == svn_wc_schedule_add) ? svn_node_none
    : (entry->schedule == svn_wc_schedule_delete) ? svn_node_unknown
      : entry->kind;

> + /* 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.)

> + 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?

> 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.

> 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.

>
> - 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.)

> 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. */

> 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.

> + 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.)

> + {
> + 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

---------------------------------------------------------------------
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 15:26:39 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.