You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by da...@apache.org on 2010/05/01 23:06:37 UTC
svn commit: r940111 - in /subversion/trunk/subversion:
include/private/svn_wc_private.h include/svn_wc.h libsvn_wc/node.c
libsvn_wc/status.c svn/cl.h svn/status-cmd.c svn/status.c
Author: dannas
Date: Sat May 1 21:06:36 2010
New Revision: 940111
URL: http://svn.apache.org/viewvc?rev=940111&view=rev
Log:
A first step towards avoiding expensive computations involving conflicts
in the status code.
The idea is to have one single flag indicating if a path has any
conflicts. If the API users wants more information, they can get it
themselves. Two pros: The code will (hopefully) be faster and we get a
more unified grip on the conflict information. Earlier it has been
spread across status->text_status, status->prop_status and
status->tree_conflict.
* subversion/svn/cl.h
(svn_cl__print_status,
(svn_cl__print_status_xml): Add ctx parameter.
* subversion/svn/status.c
(print_status): Add ctx parameter. Check status->conflicted and use wc
funcs for fetching further information.
svn_cl__print_status_xml,
(svn_cl__print_status): Do an explicit check for tree conflicts
instead of relying on fields in svn_wc_status3_t.
* subversion/svn/status-cmd.c
(print_status_normal_or_xml): Update callers to pass a ctx parameter.
* subversion/include/svn_wc.h
(svn_wc_status3_t): Add conflicted field.
* subversion/include/private/svn_wc_private.h
(svn_wc__node_check_conflicts): New.
* subversion/libsvn_wc/status.c
(assemble_status): Initialize status->conflicted.
* subversion/libsvn_wc/node.c
(svn_wc__node_check_conflicts): New.
Modified:
subversion/trunk/subversion/include/private/svn_wc_private.h
subversion/trunk/subversion/include/svn_wc.h
subversion/trunk/subversion/libsvn_wc/node.c
subversion/trunk/subversion/libsvn_wc/status.c
subversion/trunk/subversion/svn/cl.h
subversion/trunk/subversion/svn/status-cmd.c
subversion/trunk/subversion/svn/status.c
Modified: subversion/trunk/subversion/include/private/svn_wc_private.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_wc_private.h?rev=940111&r1=940110&r2=940111&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_wc_private.h (original)
+++ subversion/trunk/subversion/include/private/svn_wc_private.h Sat May 1 21:06:36 2010
@@ -598,6 +598,25 @@ svn_wc__node_is_file_external(svn_boolea
const char *local_abspath,
apr_pool_t *scratch_pool);
+/**
+ * Check what kinds of conflicts we have on @a local_abspath.
+ *
+ * We could have returned the conflicts at once if it wasn't for the fact
+ * that there can be multiple prop conflicts.
+ *
+ * One or two of @a prop_conflicted, @a text_conflicted and @a
+ * tree_conflicted can be NULL if we're not interrested in that particular
+ * value.
+ */
+svn_error_t *
+svn_wc__node_check_conflicts(svn_boolean_t *prop_conflicted,
+ svn_boolean_t *text_conflicted,
+ svn_boolean_t *tree_conflicted,
+ svn_wc_context_t *wc_ctx,
+ const char *local_abspath,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
/**
* Recursively acquire write locks for @a local_abspath if
Modified: subversion/trunk/subversion/include/svn_wc.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_wc.h?rev=940111&r1=940110&r2=940111&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_wc.h (original)
+++ subversion/trunk/subversion/include/svn_wc.h Sat May 1 21:06:36 2010
@@ -3645,6 +3645,8 @@ typedef struct svn_wc_status3_t
/** The locally present lock creation date.
*/
apr_time_t lock_creation_date;
+
+ svn_boolean_t conflicted;
/* NOTE! Please update svn_wc_dup_status3() when adding new fields here. */
} svn_wc_status3_t;
Modified: subversion/trunk/subversion/libsvn_wc/node.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/node.c?rev=940111&r1=940110&r2=940111&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/node.c (original)
+++ subversion/trunk/subversion/libsvn_wc/node.c Sat May 1 21:06:36 2010
@@ -1049,3 +1049,32 @@ svn_wc__node_is_file_external(svn_boolea
local_abspath,
scratch_pool));
}
+
+svn_error_t *
+svn_wc__node_check_conflicts(svn_boolean_t *prop_conflicted,
+ svn_boolean_t *text_conflicted,
+ svn_boolean_t *tree_conflicted,
+ svn_wc_context_t *wc_ctx,
+ const char *local_abspath,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ const apr_array_header_t *conflicts;
+ int i;
+
+ SVN_ERR(svn_wc__db_read_conflicts(&conflicts, wc_ctx->db, local_abspath,
+ result_pool, scratch_pool));
+
+ for (i = 0; i < conflicts->nelts; i++)
+ {
+ svn_wc_conflict_description2_t *cd;
+ cd = APR_ARRAY_IDX(conflicts, i, svn_wc_conflict_description2_t *);
+ if (prop_conflicted && cd->kind == svn_wc_conflict_kind_property)
+ *prop_conflicted = TRUE;
+ else if (text_conflicted && cd->kind == svn_wc_conflict_kind_text)
+ *text_conflicted = TRUE;
+ else if (tree_conflicted && cd->kind == svn_wc_conflict_kind_tree)
+ *tree_conflicted = TRUE;
+ }
+ return SVN_NO_ERROR;
+}
Modified: subversion/trunk/subversion/libsvn_wc/status.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/status.c?rev=940111&r1=940110&r2=940111&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/status.c (original)
+++ subversion/trunk/subversion/libsvn_wc/status.c Sat May 1 21:06:36 2010
@@ -302,6 +302,7 @@ assemble_status(svn_wc_status3_t **statu
svn_revnum_t changed_rev;
const char *changed_author;
apr_time_t changed_date;
+ svn_boolean_t conflicted;
#ifdef HAVE_SYMLINK
svn_boolean_t wc_special;
#endif /* HAVE_SYMLINK */
@@ -393,6 +394,7 @@ assemble_status(svn_wc_status3_t **statu
stat->lock_owner = NULL;
stat->lock_comment = NULL;
stat->lock_creation_date = 0;
+ stat->conflicted = (tree_conflict != NULL);
*status = stat;
return SVN_NO_ERROR;
@@ -401,7 +403,7 @@ assemble_status(svn_wc_status3_t **statu
SVN_ERR(svn_wc__db_read_info(NULL, NULL, &revision, NULL, NULL, NULL,
&changed_rev, &changed_date, &changed_author,
NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL, NULL, &conflicted,
&lock, db, local_abspath, result_pool,
scratch_pool));
@@ -641,6 +643,7 @@ assemble_status(svn_wc_status3_t **statu
stat->lock_owner = lock ? lock->owner : NULL;
stat->lock_comment = lock ? lock->comment : NULL;
stat->lock_creation_date = lock ? lock->date : 0;
+ stat->conflicted = conflicted;
*status = stat;
Modified: subversion/trunk/subversion/svn/cl.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/cl.h?rev=940111&r1=940110&r2=940111&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/cl.h (original)
+++ subversion/trunk/subversion/svn/cl.h Sat May 1 21:06:36 2010
@@ -398,6 +398,7 @@ svn_cl__print_status(const char *path,
unsigned int *text_conflicts,
unsigned int *prop_conflicts,
unsigned int *tree_conflicts,
+ svn_client_ctx_t *ctx,
apr_pool_t *pool);
@@ -406,6 +407,7 @@ svn_cl__print_status(const char *path,
svn_error_t *
svn_cl__print_status_xml(const char *path,
const svn_wc_status3_t *status,
+ svn_client_ctx_t *ctx,
apr_pool_t *pool);
Modified: subversion/trunk/subversion/svn/status-cmd.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/status-cmd.c?rev=940111&r1=940110&r2=940111&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/status-cmd.c (original)
+++ subversion/trunk/subversion/svn/status-cmd.c Sat May 1 21:06:36 2010
@@ -149,7 +149,7 @@ print_status_normal_or_xml(void *baton,
struct status_baton *sb = baton;
if (sb->xml_mode)
- return svn_cl__print_status_xml(path, status, pool);
+ return svn_cl__print_status_xml(path, status, sb->ctx, pool);
else
return svn_cl__print_status(path, status, sb->detailed,
sb->show_last_committed,
@@ -158,6 +158,7 @@ print_status_normal_or_xml(void *baton,
&sb->text_conflicts,
&sb->prop_conflicts,
&sb->tree_conflicts,
+ sb->ctx,
pool);
}
Modified: subversion/trunk/subversion/svn/status.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/status.c?rev=940111&r1=940110&r2=940111&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/status.c (original)
+++ subversion/trunk/subversion/svn/status.c Sat May 1 21:06:36 2010
@@ -34,6 +34,7 @@
#include "cl.h"
#include "svn_private_config.h"
#include "tree-conflicts.h"
+#include "private/svn_wc_private.h"
/* Return the single character representation of STATUS */
static char
@@ -108,6 +109,7 @@ print_status(const char *path,
unsigned int *text_conflicts,
unsigned int *prop_conflicts,
unsigned int *tree_conflicts,
+ svn_client_ctx_t *ctx,
apr_pool_t *pool)
{
enum svn_wc_status_kind text_status = status->text_status;
@@ -118,20 +120,39 @@ print_status(const char *path,
'C' in the tree-conflict column, overriding any other status.
We also print a separate line describing the nature of the tree
conflict. */
- if (status->tree_conflict)
+ if (status->conflicted)
{
const char *desc;
- tree_status_code = 'C';
- svn_cl__get_human_readable_tree_conflict_description(
- &desc, status->tree_conflict, pool);
- tree_desc_line = apr_psprintf(pool, "\n > %s", desc);
- (*tree_conflicts)++;
- }
- else
- {
- if (text_status == svn_wc_status_conflicted)
+ const char *local_abspath;
+ svn_boolean_t text_conflicted = FALSE;
+ svn_boolean_t prop_conflicted = FALSE;
+ svn_boolean_t tree_conflicted = FALSE;
+
+ SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
+
+ SVN_ERR(svn_wc__node_check_conflicts(&prop_conflicted,
+ &text_conflicted,
+ &tree_conflicted, ctx->wc_ctx,
+ local_abspath, pool, pool));
+
+
+ if (tree_conflicted)
+ {
+ const svn_wc_conflict_description2_t *tree_conflict;
+ svn_wc_conflict_description_t *old_tree_conflict;
+ SVN_ERR(svn_wc__get_tree_conflict(&tree_conflict, ctx->wc_ctx,
+ local_abspath, pool, pool));
+ old_tree_conflict = svn_wc__cd2_to_cd(tree_conflict, pool);
+
+ tree_status_code = 'C';
+ svn_cl__get_human_readable_tree_conflict_description(
+ &desc, old_tree_conflict, pool);
+ tree_desc_line = apr_psprintf(pool, "\n > %s", desc);
+ (*tree_conflicts)++;
+ }
+ else if (text_conflicted)
(*text_conflicts)++;
- if (status->prop_status == svn_wc_status_conflicted)
+ else if (prop_conflicted)
(*prop_conflicts)++;
}
@@ -263,15 +284,23 @@ print_status(const char *path,
svn_error_t *
svn_cl__print_status_xml(const char *path,
const svn_wc_status3_t *status,
+ svn_client_ctx_t *ctx,
apr_pool_t *pool)
{
svn_stringbuf_t *sb = svn_stringbuf_create("", pool);
apr_hash_t *att_hash;
+ const char *local_abspath;
+ svn_boolean_t tree_conflicted = FALSE;
if (status->text_status == svn_wc_status_none
&& status->repos_text_status == svn_wc_status_none)
return SVN_NO_ERROR;
+ SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
+ SVN_ERR(svn_wc__node_check_conflicts(NULL, NULL, &tree_conflicted,
+ ctx->wc_ctx, local_abspath, pool,
+ pool));
+
svn_xml_make_open_tag(&sb, pool, svn_xml_normal, "entry",
"path", svn_dirent_local_style(path, pool), NULL);
@@ -291,7 +320,7 @@ svn_cl__print_status_xml(const char *pat
if (status->entry && ! status->entry->copied)
apr_hash_set(att_hash, "revision", APR_HASH_KEY_STRING,
apr_psprintf(pool, "%ld", status->revision));
- if (status->tree_conflict)
+ if (tree_conflicted)
apr_hash_set(att_hash, "tree-conflicted", APR_HASH_KEY_STRING,
"true");
svn_xml_make_open_tag_hash(&sb, pool, svn_xml_normal, "wc-status",
@@ -390,15 +419,24 @@ svn_cl__print_status(const char *path,
unsigned int *text_conflicts,
unsigned int *prop_conflicts,
unsigned int *tree_conflicts,
+ svn_client_ctx_t *ctx,
apr_pool_t *pool)
{
+ const char *local_abspath;
+ svn_boolean_t tree_conflicted = FALSE;
+
+ SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
+ SVN_ERR(svn_wc__node_check_conflicts(NULL, NULL, &tree_conflicted,
+ ctx->wc_ctx, local_abspath, pool,
+ pool));
if (! status
- || (skip_unrecognized && !(status->entry || status->tree_conflict))
+ || (skip_unrecognized && !(status->entry || tree_conflicted))
|| (status->text_status == svn_wc_status_none
&& status->repos_text_status == svn_wc_status_none))
return SVN_NO_ERROR;
return print_status(svn_dirent_local_style(path, pool),
detailed, show_last_committed, repos_locks, status,
- text_conflicts, prop_conflicts, tree_conflicts, pool);
+ text_conflicts, prop_conflicts, tree_conflicts,
+ ctx, pool);
}
RE: another hole in tree-conflicts! -- was: Re: svn commit: r940111 - in /subversion/trunk/subversion: include/private/svn_wc_private.h include/svn_wc.h libsvn_wc/node.c libsvn_wc/status.c svn/cl.h svn/status-cmd.c svn/status.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Neels J Hofmeyr [mailto:neels@elego.de]
> Sent: woensdag 5 mei 2010 11:37
> To: dev@subversion.apache.org; Julian Foad; Stefan Sperling; Stephen
> Butler
> Subject: another hole in tree-conflicts! -- was: Re: svn commit:
> r940111 - in /subversion/trunk/subversion:
> include/private/svn_wc_private.h include/svn_wc.h libsvn_wc/node.c
> libsvn_wc/status.c svn/cl.h svn/status-cmd.c svn/status.c
>
> On 05/03/2010 10:25 PM, Daniel Näslund wrote:
> > On Mon, May 03, 2010 at 03:07:49PM +0200, Neels J Hofmeyr wrote:
> [...]
> >>> stat->lock_creation_date = 0;
> >>> + stat->conflicted = (tree_conflict != NULL);
> >>
> >> I don't understand -- what about prop and text conflicts? Below,
> >> ->conflicted stands for all three of them.
> >> (If this is correct, I guess it needs a comment)
> >
> > Added a comment in r940600. AFAIK, the only scenario where we can get
> a
> > conflict on an unversioned node is for an incoming delete to a
> locally
> > deleted path. (It will be troublesome to fit that case into the idea
> of
> > storing tree conflict info on the node instead of its parent).
>
> ah, ok, that makes sense. Shouldn't be trouble to store such tree-
> conflict
> in the ng-ACTUAL tree? ...but, thinking aloud a little:
>
> The entry can't be NULL for a schedule-deleted node (asked for with
> show_hidden == TRUE). Analogously, the wc-ng tables do have both ng-
> BASE and
> ng-WORKING nodes for a locally deleted path.
This is the case where a node was deleted locally (schedule delete) and
deleted remotely.
After the update we couldn't leave it schedule delete as the entry was
removed from the repository, so the complete node was removed in 1.6 and a
tree conflict recorded in the parent *entry*.
(All cases where we have a node would fall through a different codepath in
the status walk)
We discussed this scenario a few weeks ago on irc, and there are two ways to
recording this in WC-NG:
1. Allow an ACTUAL_NODE record for nodes without BASE_NODE and WORKING_NODE
2. Keep a NOT-PRESENT BASE_NODE record for this case
I recommended option 2, as it doesn't introduce another status just for
recording this conflict type which we would have to check everywhere.
(Adding another status would require every loop over the result of
svn_wc__db_read_children() to check for this status).
> So, if there is no metadata in the WC on the node, there can't be *any*
> (tree-)conflict on it! Right? Let's see:
>
> Another case: obstructed; there is no metadata in the WC, but a
> file/folder
> sits at a path. Say update wants to put something at that path: it'll
> create
> a base node (above theory holds -- TC needs to have metadata).
In WC-1.0 we record this on the parent, without an entry.
In WC-NG we can create the node, but with an 'obstructed' conflict on it.
(So we can allow the 'svn update to continue')
The rest of your mail talks about how merge handles this conflict, which is
a less interesting case for the wc-ng work where I'm looking at as it is a
client feature. (But it might require recording a conflict on a not-present
node anyway)
Bert
Re: another hole in tree-conflicts!
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-05-05 at 11:37 +0200, Neels J Hofmeyr wrote:
> On 05/03/2010 10:25 PM, Daniel Näslund wrote:
> > On Mon, May 03, 2010 at 03:07:49PM +0200, Neels J Hofmeyr wrote:
> [...]
> >>> stat->lock_creation_date = 0;
> >>> + stat->conflicted = (tree_conflict != NULL);
> >>
> >> I don't understand -- what about prop and text conflicts? Below,
> >> ->conflicted stands for all three of them.
> >> (If this is correct, I guess it needs a comment)
> >
> > Added a comment in r940600. AFAIK, the only scenario where we can get a
> > conflict on an unversioned node is for an incoming delete to a locally
> > deleted path.
You must be talking about during an update (or switch) that results in a
tree conflict, because if it was a *merge* then the node wouldn't become
unversioned yet, it would still be scheduled for deletion, so it would
still have an "entry" and there would be no problem in storing the
metadata. That's what I'm assuming in the rest of this email.
So what about an incoming delete to a locally *edited* path, during
up/sw? That is another combination that would also leave the node
unversioned and with a tree conflict.
> (It will be troublesome to fit that case into the idea of
> > storing tree conflict info on the node instead of its parent).
>
> ah, ok, that makes sense. Shouldn't be trouble to store such tree-conflict
> in the ng-ACTUAL tree? ...but, thinking aloud a little:
>
> The entry can't be NULL for a schedule-deleted node (asked for with
> show_hidden == TRUE). Analogously, the wc-ng tables do have both ng-BASE and
> ng-WORKING nodes for a locally deleted path.
>
> So, if there is no metadata in the WC on the node, there can't be *any*
> (tree-)conflict on it! Right?
I'm not sure if you're making a statement about how we should design
WC-NG tree conflict handling, or what.
> Let's see:
>
> Another case: obstructed; there is no metadata in the WC, but a file/folder
> sits at a path. Say update wants to put something at that path: it'll create
> a base node (above theory holds -- TC needs to have metadata).
That's good.
> Ok, furthermore, say merge wants to put something there, at the obstructed
> path. It'll create a working node that is added (obstructed_add). Again,
> there is a WC metadata entry.
That's good.
> Ok, even furthermore, say there is nothing, absolutely nothing at WC path
> foo.c, and I do a two-URL merge. The URL I merge from has foo.c on the left,
> but no foo.c on the right. So technically, the merge should want to delete
> foo.c in my WC, where it never existed in any way. (I never thought about
> this before!)
That's the case commonly known as "incoming delete, locally deleted on
my branch (and committed)".
> How does merge handle that?
>
> * neels writes a test script
>
> [[[
> [foo.c never existed on trunk, and r3 deletes foo.c on unrelated branch]
> + svn merge -c3 '^/branch' trunk/
> --- Merging r3 into 'trunk':
> C trunk/foo.c
> --- Recording mergeinfo for merge of r3 into 'trunk':
> U trunk
> Summary of conflicts:
> Tree conflicts: 1
> + svn info trunk/foo.c
> Path: trunk/foo.c
> Name: foo.c
> Node Kind: none
> Tree conflict: local delete, incoming delete upon merge
> Source left: (file) file:///tmp/trunk.EKH/repos/branch/foo.c@2
> Source right: (file) file:///tmp/trunk.EKH/repos/branch/foo.c@3
>
> + svn status trunk
> M trunk
> ! C trunk/foo.c
> > local delete, incoming delete upon merge
> Summary of conflicts:
> Tree conflicts: 1
> ]]]
>
> Good grief, another hole in tree-conflicts! It says it was locally deleted
That's all good IF we understand "deleted" to mean "not here" or
"deleted, possibly a long time ago on this branch".
> and status '!',
Ah, that status doesn't look right. It should have a space instead of
"!", I think.
> but it never existed locally. It should rather say something
> like... er... 'missing' doesn't really hit home either. 'local void,
> incoming delete' ?? Do we need another conflict_reason to flag this?
It would be better if we recorded a different reason. My suggestion was
always "not here".
> Resolving this automatically would definitely need different behaviour than
> a truly locally deleted node or truly missing node.
>
> * neels attaches the test script
>
> Is this the real justification why there could possibly be a tree-conflict
> on an entry==NULL node?
Yes, I believe so.
Stefan Sperling wrote:
> What it should really be doing is realise that the node never existed
> in the branch and so deleting it is not a conflict (I know svn is not
> that smart yet and won't be for a while...)
>
> But I think the missing status notification is just fine in the
> meantime.
> I agree the text should say "local missing" instead of "local delete",
> but IIRC we don't know the difference since we're not searching branch
> history [yet].
About the reporting of the "local" state
----------------------------------------
I recommend the following states for "not present":
* "not here" if the node is not on the tip of the branch
* "deleted" if the node is scheduled for deletion
* "missing" if the node is supposed to be present but is not found on
the disk
"Missing" is true in a general English sense for any of these states,
but it might be better to reserve it to describe the case where the
metadata says the node is present but it's not there on disk.
Similarly, "local delete" could, in a general English sense, mean "it's
not here and so was presumably deleted by somebody, some time,
somewhere", but instead we should reserve it for its normal Subversion
meaning of "scheduled for deletion on the next commit".
We can determine the difference between these three states by looking at
the local WC; that doesn't require searching the history of the branch.
(If we later want to indicate more precisely what happened to this file
in the history of the branch, it's not as simple as saying "was deleted
on branch" because that doesn't necessarily mean "was present at branch
creation and was deleted exactly once in its history" - there are
several other variants possible.)
- Julian
Re: another hole in tree-conflicts! -- was: Re: svn commit:
r940111 - in /subversion/trunk/subversion:
include/private/svn_wc_private.h include/svn_wc.h libsvn_wc/node.c
libsvn_wc/status.c svn/cl.h svn/status-cmd.c svn/status.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, May 05, 2010 at 11:37:09AM +0200, Neels J Hofmeyr wrote:
> Ok, even furthermore, say there is nothing, absolutely nothing at WC path
> foo.c, and I do a two-URL merge. The URL I merge from has foo.c on the left,
> but no foo.c on the right. So technically, the merge should want to delete
> foo.c in my WC, where it never existed in any way. (I never thought about
> this before!) How does merge handle that?
>
> * neels writes a test script
>
> [[[
> [foo.c never existed on trunk, and r3 deletes foo.c on unrelated branch]
> + svn merge -c3 '^/branch' trunk/
> --- Merging r3 into 'trunk':
> C trunk/foo.c
> --- Recording mergeinfo for merge of r3 into 'trunk':
> U trunk
> Summary of conflicts:
> Tree conflicts: 1
> + svn info trunk/foo.c
> Path: trunk/foo.c
> Name: foo.c
> Node Kind: none
> Tree conflict: local delete, incoming delete upon merge
> Source left: (file) file:///tmp/trunk.EKH/repos/branch/foo.c@2
> Source right: (file) file:///tmp/trunk.EKH/repos/branch/foo.c@3
>
> + svn status trunk
> M trunk
> ! C trunk/foo.c
> > local delete, incoming delete upon merge
> Summary of conflicts:
> Tree conflicts: 1
> ]]]
>
> Good grief, another hole in tree-conflicts! It says it was locally deleted
> and status '!', but it never existed locally. It should rather say something
> like... er... 'missing' doesn't really hit home either. 'local void,
> incoming delete' ?? Do we need another conflict_reason to flag this?
> Resolving this automatically would definitely need different behaviour than
> a truly locally deleted node or truly missing node.
What it should really be doing is realise that the node never existed
in the branch and so deleting it is not a conflict (I know svn is not
that smart yet and won't be for a while...)
But I think the missing status notification is just fine in the meantime.
I agree the text should say "local missing" instead of "local delete",
but IIRC we don't know the difference since we're not searching branch
history [yet].
Stefan
another hole in tree-conflicts! -- was: Re: svn commit: r940111
- in /subversion/trunk/subversion: include/private/svn_wc_private.h include/svn_wc.h
libsvn_wc/node.c libsvn_wc/status.c svn/cl.h svn/status-cmd.c svn/status.c
Posted by Neels J Hofmeyr <ne...@elego.de>.
On 05/03/2010 10:25 PM, Daniel Näslund wrote:
> On Mon, May 03, 2010 at 03:07:49PM +0200, Neels J Hofmeyr wrote:
[...]
>>> stat->lock_creation_date = 0;
>>> + stat->conflicted = (tree_conflict != NULL);
>>
>> I don't understand -- what about prop and text conflicts? Below,
>> ->conflicted stands for all three of them.
>> (If this is correct, I guess it needs a comment)
>
> Added a comment in r940600. AFAIK, the only scenario where we can get a
> conflict on an unversioned node is for an incoming delete to a locally
> deleted path. (It will be troublesome to fit that case into the idea of
> storing tree conflict info on the node instead of its parent).
ah, ok, that makes sense. Shouldn't be trouble to store such tree-conflict
in the ng-ACTUAL tree? ...but, thinking aloud a little:
The entry can't be NULL for a schedule-deleted node (asked for with
show_hidden == TRUE). Analogously, the wc-ng tables do have both ng-BASE and
ng-WORKING nodes for a locally deleted path.
So, if there is no metadata in the WC on the node, there can't be *any*
(tree-)conflict on it! Right? Let's see:
Another case: obstructed; there is no metadata in the WC, but a file/folder
sits at a path. Say update wants to put something at that path: it'll create
a base node (above theory holds -- TC needs to have metadata).
Ok, furthermore, say merge wants to put something there, at the obstructed
path. It'll create a working node that is added (obstructed_add). Again,
there is a WC metadata entry.
Ok, even furthermore, say there is nothing, absolutely nothing at WC path
foo.c, and I do a two-URL merge. The URL I merge from has foo.c on the left,
but no foo.c on the right. So technically, the merge should want to delete
foo.c in my WC, where it never existed in any way. (I never thought about
this before!) How does merge handle that?
* neels writes a test script
[[[
[foo.c never existed on trunk, and r3 deletes foo.c on unrelated branch]
+ svn merge -c3 '^/branch' trunk/
--- Merging r3 into 'trunk':
C trunk/foo.c
--- Recording mergeinfo for merge of r3 into 'trunk':
U trunk
Summary of conflicts:
Tree conflicts: 1
+ svn info trunk/foo.c
Path: trunk/foo.c
Name: foo.c
Node Kind: none
Tree conflict: local delete, incoming delete upon merge
Source left: (file) file:///tmp/trunk.EKH/repos/branch/foo.c@2
Source right: (file) file:///tmp/trunk.EKH/repos/branch/foo.c@3
+ svn status trunk
M trunk
! C trunk/foo.c
> local delete, incoming delete upon merge
Summary of conflicts:
Tree conflicts: 1
]]]
Good grief, another hole in tree-conflicts! It says it was locally deleted
and status '!', but it never existed locally. It should rather say something
like... er... 'missing' doesn't really hit home either. 'local void,
incoming delete' ?? Do we need another conflict_reason to flag this?
Resolving this automatically would definitely need different behaviour than
a truly locally deleted node or truly missing node.
* neels attaches the test script
Is this the real justification why there could possibly be a tree-conflict
on an entry==NULL node?
> Thanks for your review,
heh, I'm glad I can say some intelligent stuff, too. Usually it was Julian
telling me about my patches in much the same way. :)
~Neels
Re: svn commit: r940111 - in /subversion/trunk/subversion:
include/private/svn_wc_private.h include/svn_wc.h libsvn_wc/node.c
libsvn_wc/status.c svn/cl.h svn/status-cmd.c svn/status.c
Posted by Daniel Näslund <da...@longitudo.com>.
On Mon, May 03, 2010 at 03:07:49PM +0200, Neels J Hofmeyr wrote:
>
> dannas@apache.org wrote:
> > Author: dannas
> > Date: Sat May 1 21:06:36 2010
> > New Revision: 940111
> >
> [...]
>
> > (svn_cl__print_status_xml): Add ctx parameter.
> >
> > * subversion/svn/status.c
> > (print_status): Add ctx parameter. Check status->conflicted and use wc
> > funcs for fetching further information.
> > svn_cl__print_status_xml,
>
> log message error ^
Fixed.
[...]
> > ==============================================================================
> > --- subversion/trunk/subversion/include/svn_wc.h (original)
> > +++ subversion/trunk/subversion/include/svn_wc.h Sat May 1 21:06:36 2010
> > @@ -3645,6 +3645,8 @@ typedef struct svn_wc_status3_t
> > /** The locally present lock creation date.
> > */
> > apr_time_t lock_creation_date;
> > +
> > + svn_boolean_t conflicted;
>
> How about a short comment? ^
> something like "TRUE if tree-, prop- and/or text-conflicted"
Done in a follow-up. Redone in yet another follow-up after Bert pointed
out that we will have obstructed and patch conflicts in the future.
> >
> > /* NOTE! Please update svn_wc_dup_status3() when adding new fields here. */
>
> I think you did not heed this note. ^
I just pasted the line from svn_wc_status2_t and changed the number 2 to
3. Converting between status2_t and status_t was done with a cast and
that worked straight off, except for the ones that needed to be
reallocated with dup_statusX(). I don't know, maybe it's easy to forget
that? I thought that it made no harm and that I had seen similar notes
about updating a dup() func in other structs (though I can't find any
such usages when I looked today).
> > ==============================================================================
> > --- subversion/trunk/subversion/libsvn_wc/status.c (original)
> > +++ subversion/trunk/subversion/libsvn_wc/status.c Sat May 1 21:06:36 2010
> > @@ -302,6 +302,7 @@ assemble_status(svn_wc_status3_t **statu
> > svn_revnum_t changed_rev;
> > const char *changed_author;
> > apr_time_t changed_date;
> > + svn_boolean_t conflicted;
> > #ifdef HAVE_SYMLINK
> > svn_boolean_t wc_special;
> > #endif /* HAVE_SYMLINK */
> > @@ -393,6 +394,7 @@ assemble_status(svn_wc_status3_t **statu
> > stat->lock_owner = NULL;
> > stat->lock_comment = NULL;
> > stat->lock_creation_date = 0;
> > + stat->conflicted = (tree_conflict != NULL);
>
> I don't understand -- what about prop and text conflicts? Below,
> ->conflicted stand for all three of them.
> (If this is correct, I guess it needs a comment)
Added a comment in r940600. AFAIK, the only scenario where we can get a
conflict on an unversioned node is for an incoming delete to a locally
deleted path. (It will be troublesome to fit that case into the idea of
storing tree conflict info on the node instead of its parent).
Thanks for your review,
Daniel
Re: svn commit: r940111 - in /subversion/trunk/subversion: include/private/svn_wc_private.h
include/svn_wc.h libsvn_wc/node.c libsvn_wc/status.c svn/cl.h svn/status-cmd.c
svn/status.c
Posted by Neels J Hofmeyr <ne...@elego.de>.
dannas@apache.org wrote:
> Author: dannas
> Date: Sat May 1 21:06:36 2010
> New Revision: 940111
>
[...]
> (svn_cl__print_status_xml): Add ctx parameter.
>
> * subversion/svn/status.c
> (print_status): Add ctx parameter. Check status->conflicted and use wc
> funcs for fetching further information.
> svn_cl__print_status_xml,
log message error ^
> (svn_cl__print_status): Do an explicit check for tree conflicts
> instead of relying on fields in svn_wc_status3_t.
>
> * subversion/svn/status-cmd.c
> (print_status_normal_or_xml): Update callers to pass a ctx parameter.
>
> * subversion/include/svn_wc.h
> (svn_wc_status3_t): Add conflicted field.
>
> * subversion/include/private/svn_wc_private.h
> (svn_wc__node_check_conflicts): New.
>
> * subversion/libsvn_wc/status.c
> (assemble_status): Initialize status->conflicted.
>
> * subversion/libsvn_wc/node.c
> (svn_wc__node_check_conflicts): New.
>
> Modified:
> subversion/trunk/subversion/include/private/svn_wc_private.h
> subversion/trunk/subversion/include/svn_wc.h
> subversion/trunk/subversion/libsvn_wc/node.c
> subversion/trunk/subversion/libsvn_wc/status.c
> subversion/trunk/subversion/svn/cl.h
> subversion/trunk/subversion/svn/status-cmd.c
> subversion/trunk/subversion/svn/status.c
>
> Modified: subversion/trunk/subversion/include/private/svn_wc_private.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_wc_private.h?rev=940111&r1=940110&r2=940111&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/private/svn_wc_private.h (original)
> +++ subversion/trunk/subversion/include/private/svn_wc_private.h Sat May 1 21:06:36 2010
> @@ -598,6 +598,25 @@ svn_wc__node_is_file_external(svn_boolea
> const char *local_abspath,
> apr_pool_t *scratch_pool);
>
> +/**
> + * Check what kinds of conflicts we have on @a local_abspath.
> + *
> + * We could have returned the conflicts at once if it wasn't for the fact
> + * that there can be multiple prop conflicts.
> + *
> + * One or two of @a prop_conflicted, @a text_conflicted and @a
> + * tree_conflicted can be NULL if we're not interrested in that particular
> + * value.
> + */
> +svn_error_t *
> +svn_wc__node_check_conflicts(svn_boolean_t *prop_conflicted,
> + svn_boolean_t *text_conflicted,
> + svn_boolean_t *tree_conflicted,
> + svn_wc_context_t *wc_ctx,
> + const char *local_abspath,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
> +
>
> /**
> * Recursively acquire write locks for @a local_abspath if
>
> Modified: subversion/trunk/subversion/include/svn_wc.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_wc.h?rev=940111&r1=940110&r2=940111&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_wc.h (original)
> +++ subversion/trunk/subversion/include/svn_wc.h Sat May 1 21:06:36 2010
> @@ -3645,6 +3645,8 @@ typedef struct svn_wc_status3_t
> /** The locally present lock creation date.
> */
> apr_time_t lock_creation_date;
> +
> + svn_boolean_t conflicted;
How about a short comment? ^
something like "TRUE if tree-, prop- and/or text-conflicted"
>
> /* NOTE! Please update svn_wc_dup_status3() when adding new fields here. */
I think you did not heed this note. ^
> } svn_wc_status3_t;
>
> Modified: subversion/trunk/subversion/libsvn_wc/node.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/node.c?rev=940111&r1=940110&r2=940111&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/node.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/node.c Sat May 1 21:06:36 2010
> @@ -1049,3 +1049,32 @@ svn_wc__node_is_file_external(svn_boolea
> local_abspath,
> scratch_pool));
> }
> +
> +svn_error_t *
> +svn_wc__node_check_conflicts(svn_boolean_t *prop_conflicted,
> + svn_boolean_t *text_conflicted,
> + svn_boolean_t *tree_conflicted,
> + svn_wc_context_t *wc_ctx,
> + const char *local_abspath,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + const apr_array_header_t *conflicts;
> + int i;
> +
> + SVN_ERR(svn_wc__db_read_conflicts(&conflicts, wc_ctx->db, local_abspath,
> + result_pool, scratch_pool));
> +
> + for (i = 0; i < conflicts->nelts; i++)
> + {
> + svn_wc_conflict_description2_t *cd;
> + cd = APR_ARRAY_IDX(conflicts, i, svn_wc_conflict_description2_t *);
> + if (prop_conflicted && cd->kind == svn_wc_conflict_kind_property)
> + *prop_conflicted = TRUE;
> + else if (text_conflicted && cd->kind == svn_wc_conflict_kind_text)
> + *text_conflicted = TRUE;
> + else if (tree_conflicted && cd->kind == svn_wc_conflict_kind_tree)
> + *tree_conflicted = TRUE;
> + }
> + return SVN_NO_ERROR;
> +}
nice :)
>
> Modified: subversion/trunk/subversion/libsvn_wc/status.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/status.c?rev=940111&r1=940110&r2=940111&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/status.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/status.c Sat May 1 21:06:36 2010
> @@ -302,6 +302,7 @@ assemble_status(svn_wc_status3_t **statu
> svn_revnum_t changed_rev;
> const char *changed_author;
> apr_time_t changed_date;
> + svn_boolean_t conflicted;
> #ifdef HAVE_SYMLINK
> svn_boolean_t wc_special;
> #endif /* HAVE_SYMLINK */
> @@ -393,6 +394,7 @@ assemble_status(svn_wc_status3_t **statu
> stat->lock_owner = NULL;
> stat->lock_comment = NULL;
> stat->lock_creation_date = 0;
> + stat->conflicted = (tree_conflict != NULL);
I don't understand -- what about prop and text conflicts? Below,
->conflicted stand for all three of them.
(If this is correct, I guess it needs a comment)
[...diff truncated]
Nice cleanup!
~Neels