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

Re: [PATCH] allow conflict descriptions with NULL repos_relpaths

From: Bert Huijben <bert_at_qqmail.nl>
Date: Tue, 12 Aug 2014 20:04:37 +0000

I don't see a problem with not allowing a repository location, but it doesn't make sense to me to have a revision in that case. You either have both, or have none.

If you want to tell that the node didn't exist in a revision it makes more sense to me to tell that it did’t exist at a specific path in that revision… which should already be possible, with the original checks.


Sent from Windows Mail

From: Stefan Sperling
Sent: ‎Tuesday‎, ‎August‎ ‎12‎, ‎2014 ‎8‎:‎12‎ ‎PM
To: dev_at_subversion.apache.org

I'd like to always make update/switch target revisions available to
the conflict resolver. We currently don't record this information
for tree conflict victims which are not present in the target revision.
For instance, 'svn info' might show:

Tree conflict: local file edit, incoming file delete or move upon update
  Source left: (file) ^/trunk/alpha_at_2
  Source right: (none)

With the patch below, 'svn info' shows the target revision where the
node has been found missing:

Tree conflict: local file edit, incoming file delete or move upon update
  Source left: (file) ^/trunk/alpha_at_2
  Source right: (none) @4

One use case I have in mind is a conflict resolver which scans the log
to detect moves heuristically. Without knowing the target revision it
is impossible to know how many revisions need to be scanned. The resolver
is forced to scan up to HEAD which can imply a huge performance hit.

The information might also be needed in other cases, and is already available
whenever the victim exists in the repository at the target revision.
I think not recording this information for missing nodes was a mistake.

There is a slight API change involved. The semantics of
svn_wc_conflict_version_create2() must change such that the conflict victim's
repos relpath is allowed to be NULL if the node kind is 'none'.
Currently the API requires a canonical path in all cases, a contract which
cannot be fulfilled for nodes which don't exist.
The revision number must still be valid in all cases.

Essentially, we're making the interface a bit more liberal in what it accepts,
so it's not a breaking change unless a caller expects to see an error in this
situation. We don't have any such code. Still, should I be cautious and rev
the API for this?

Are there any objections to this plan?

Record the update/switch target revision for missing tree conflicts
victims in the tree conflict description so the revision can always
be retrieved during conflict resolution.

* subversion/include/svn_wc.h
  (svn_wc_conflict_version_create2): Update docstring.
   A NULL 'repos_relpath' is now valid if 'kind' is svn_node_none.

* subversion/libsvn_wc/conflicts.c
  (conflict__prepend_location, conflict__read_location): Handle NULL
   repos-relpath fields in conflict description.

* subversion/libsvn_wc/update_editor.c
  (complete_conflict): Create conflict versions for paths which don't exist
   so revision number information will be recorded.

* subversion/libsvn_wc/util.c
  (svn_wc_conflict_version_create2): Accept a NULL repos_relpath if the
   node kind is svn_node_none.

* subversion/svn/util.c
  (svn_cl__node_description): Print an empty path if the victim's kind
   is svn_node_none, instead of printing the ^/... placeholder path.

Index: subversion/include/svn_wc.h
--- subversion/include/svn_wc.h (revision 1617507)
+++ subversion/include/svn_wc.h (working copy)
@@ -1711,8 +1711,9 @@ typedef struct svn_wc_conflict_version_t
  * @a revision and the @c node_kind to @a kind. Make only shallow
  * copies of the pointer arguments.
- * @a repos_root_url, @a repos_relpath and @a revision must be valid,
- * non-null values. @a repos_uuid should be a valid UUID, but can be
+ * @a repos_root_url, and @a revision must be valid, non-null values.
+ * @a repos_relpath must be a canonical fspath, but can be @c NULL if kind
+ * is @svn_node_none. @a repos_uuid should be a valid UUID, but can be
  * NULL if unknown. @a kind can be any kind, even 'none' or 'unknown'.
  * @since New in 1.8.
Index: subversion/libsvn_wc/conflicts.c
--- subversion/libsvn_wc/conflicts.c (revision 1617507)
+++ subversion/libsvn_wc/conflicts.c (working copy)
@@ -117,8 +117,11 @@ conflict__prepend_location(svn_skel_t *skel,
   svn_skel__prepend_int(location->peg_rev, loc, result_pool);
- svn_skel__prepend_str(apr_pstrdup(result_pool, location->path_in_repos), loc,
- result_pool);
+ if (!location->path_in_repos) /* can be NULL if non-existent */
+ svn_skel__prepend(svn_skel__make_empty_list(result_pool), loc);
+ else
+ svn_skel__prepend_str(apr_pstrdup(result_pool, location->path_in_repos), loc,
+ result_pool);
   if (!location->repos_uuid) /* Can theoretically be NULL */
     svn_skel__prepend(svn_skel__make_empty_list(result_pool), loc);
@@ -168,7 +171,10 @@ conflict__read_location(svn_wc_conflict_version_t
     repos_uuid = NULL;
   c = c->next;
- repos_relpath = apr_pstrmemdup(result_pool, c->data, c->len);
+ if (c->is_atom)
+ repos_relpath = apr_pstrmemdup(result_pool, c->data, c->len);
+ else
+ repos_relpath = NULL;
   c = c->next;
   SVN_ERR(svn_skel__parse_int(&v, c, scratch_pool));
Index: subversion/libsvn_wc/update_editor.c
--- subversion/libsvn_wc/update_editor.c (revision 1617507)
+++ subversion/libsvn_wc/update_editor.c (working copy)
@@ -840,7 +840,9 @@ complete_conflict(svn_skel_t *conflict,
   if (is_complete)
     return SVN_NO_ERROR; /* Already completed */
- if (old_repos_relpath)
+ if (old_repos_relpath == NULL)
+ local_kind = svn_node_none;
+ if (SVN_IS_VALID_REVNUM(old_revision))
     original_version = svn_wc_conflict_version_create2(eb->repos_root,
@@ -850,15 +852,14 @@ complete_conflict(svn_skel_t *conflict,
     original_version = NULL;
- if (new_repos_relpath)
- target_version = svn_wc_conflict_version_create2(eb->repos_root,
- eb->repos_uuid,
- new_repos_relpath,
- *eb->target_revision,
- target_kind,
- result_pool);
- else
- target_version = NULL;
+ if (new_repos_relpath == NULL)
+ target_kind = svn_node_none;
+ target_version = svn_wc_conflict_version_create2(eb->repos_root,
+ eb->repos_uuid,
+ new_repos_relpath,
+ *eb->target_revision,
+ target_kind,
+ result_pool);
   if (eb->switch_repos_relpath)
Index: subversion/libsvn_wc/util.c
--- subversion/libsvn_wc/util.c (revision 1617507)
+++ subversion/libsvn_wc/util.c (working copy)
@@ -296,11 +296,15 @@ svn_wc_conflict_version_create2(const char *repos_
   version = apr_pcalloc(result_pool, sizeof(*version));
- SVN_ERR_ASSERT_NO_RETURN(svn_uri_is_canonical(repos_url, result_pool)
- && svn_relpath_is_canonical(repos_relpath)
- && SVN_IS_VALID_REVNUM(revision)
- /* ### repos_uuid can be NULL :( */);
+ if (repos_relpath)
+ SVN_ERR_ASSERT_NO_RETURN(svn_relpath_is_canonical(repos_relpath));
+ else
+ SVN_ERR_ASSERT_NO_RETURN(kind == svn_node_none);
+ SVN_ERR_ASSERT_NO_RETURN(svn_uri_is_canonical(repos_url, result_pool)
+ && SVN_IS_VALID_REVNUM(revision)
+ /* ### repos_uuid can be NULL :( */);
   version->repos_url = repos_url;
   version->peg_rev = revision;
   version->path_in_repos = repos_relpath;
Index: subversion/svn/util.c
--- subversion/svn/util.c (revision 1617507)
+++ subversion/svn/util.c (working copy)
@@ -923,6 +923,11 @@ svn_cl__node_description(const svn_wc_conflict_ver
   if (node->path_in_repos)
     path_str = node->path_in_repos;
+ else if (node->node_kind == svn_node_none)
+ {
+ root_str = "";
+ path_str = "";
+ }
   return apr_psprintf(pool, "(%s) %s@%ld",
Received on 2014-08-12 22:07:55 CEST

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.