You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2014/08/12 20:12:09 UTC
[PATCH] allow conflict descriptions with NULL repos_relpaths
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@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@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,
eb->repos_uuid,
old_repos_relpath,
@@ -850,15 +852,14 @@ complete_conflict(svn_skel_t *conflict,
else
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)
SVN_ERR(svn_wc__conflict_skel_set_op_switch(conflict,
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",
svn_cl__node_kind_str_human_readable(node->node_kind),
Re: [PATCH] allow conflict descriptions with NULL repos_relpaths
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Aug 12, 2014 at 08:04:37PM +0000, Bert Huijben wrote:
> 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
No, that's not what I want to tell.
> 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.
I already know the path is missing. What I don't know is which revision
the conflicted node was updated to when the conflict was recorded.
We don't write this revision down when the node is missing.
I need to know the update's target revision for a conflict resolution
tool I'm writing. And at some point (I hope) 'svn' will want to know
the revision as well.
Alternatively, we could record this revision seperately somewhere.
But then we would store it redundantly whenever the victim is present
in the target revision.
Re: [PATCH] allow conflict descriptions with NULL repos_relpaths
Posted by Bert Huijben <be...@qqmail.nl>.
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.
Bert
Sent from Windows Mail
From: Stefan Sperling
Sent: Tuesday, August 12, 2014 8:12 PM
To: dev@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@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@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,
eb->repos_uuid,
old_repos_relpath,
@@ -850,15 +852,14 @@ complete_conflict(svn_skel_t *conflict,
else
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)
SVN_ERR(svn_wc__conflict_skel_set_op_switch(conflict,
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",
svn_cl__node_kind_str_human_readable(node->node_kind),