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 2008/04/02 11:15:43 UTC
[PATCH] tree conflicts and obstructions
Hi,
here's my first shot at tree conflicts and obstructions.
I'm posting this here mainly for review and testing by
Julian and Senthil, but if anyone else wants to take a
peek, go right ahead :)
[[[
On the tree-conflicts branch, flag tree conflicts upon obstructed
files during merge.
* subversion/libsvn_wc/tree_conflicts.c
(tree_conflict_phrases): New member obstructed, which holds
a phrase describing an obstructed tree conflict victim.
(new_tree_conflict_phrases): Initialise new member of tree_conflict_phrases.
(select_our_phrase, read_reason,
svn_wc__write_tree_conflicts_to_entry,
svn_wc_append_tree_conflict_info_xml): Handle new member of
tree_conflict_phrases.
* subversion/libsvn_wc/tree_conflicts.h
(SVN_WC__CONFLICT_REASON_OBSTRUCTED): New constant.
* subversion/libsvn_client/merge.c
(persist_tree_conflict): New function.
(merge_file_changed): Use persist_tree_conflict instead of inline code.
(merge_file_added, merge_file_deleted): Use persist_tree_conflict instead
of inline code. Detect and persist tree conflicts caused by obstructions.
]]]
Thanks,
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: [PATCH] tree conflicts and obstructions
Posted by Senthil Kumaran S <se...@collab.net>.
Hi Stefan,
Stefan Sperling wrote:
> it will probably require adjusting a lot of regression tests.
Yes it does.
> We should concentrate on getting those changes done before doing any
> further UI adjustments, IMHO.
I completely agree your point here (when it comes to UI we need to think about
more bikesheds!). But I just wanted to document it before we forget :)
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] tree conflicts and obstructions
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Apr 03, 2008 at 01:11:29PM +0530, Senthil Kumaran S wrote:
> Hi Stefan,
>
> Stefan Sperling wrote:
>> here's my first shot at tree conflicts and obstructions.
>>
>> I'm posting this here mainly for review and testing by
>> Julian and Senthil, but if anyone else wants to take a
>> peek, go right ahead :)
>
> I am ok with this patch.
Great :)
> Just for the record am summarizing our IRC conversation here:
>
> As we (stsp, julianfoad and myself) discussed yesterday in IRC, we need to
> have a notification to the user about the tree conflict victim, since
> otherwise it goes unnoticed except for the 'C' notification in the
> (tree)conflicted directory, but the exact files ie., the tree conflict
> victim is not known to the user at a first look, unless he does a 'svn
> info'. Currently it is 'Skipped' in your patch. It can be 'Obstructed'
> during the notification. If that is the case the merge_test 92 should be
> changed accordingly to handle this notification. But there is another
> problem of 'what to show in 'svn status' for these tree conflict victims
> via obstruction.
Thanks for the summary!
I think we should leave all output as it is for now, because changing
it will probably require adjusting a lot of regression tests.
For now I'd like to focus on getting something done that works,
even if it does not look nice. We can always polish up the interface
later. There are still changes in the pipeline for the tree conflicts
branch that will have a major effect on the conflict detection code,
e.g. directories support, 'svn resolved' changes, and possibly
honouring of properties. See the tree conflicts issue list, which
can be retrieved by putting "tree-conflicts" (without the quotes!)
into the "URL:" field of the issue tracker query form.
http://subversion.tigris.org/issues/query.cgi
We should concentrate on getting those changes done before doing any
further UI adjustments, IMHO.
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: [PATCH] tree conflicts and obstructions
Posted by Senthil Kumaran S <se...@collab.net>.
Hi Stefan,
Stefan Sperling wrote:
> here's my first shot at tree conflicts and obstructions.
>
> I'm posting this here mainly for review and testing by
> Julian and Senthil, but if anyone else wants to take a
> peek, go right ahead :)
I am ok with this patch.
Just for the record am summarizing our IRC conversation here:
As we (stsp, julianfoad and myself) discussed yesterday in IRC, we need to have
a notification to the user about the tree conflict victim, since otherwise it
goes unnoticed except for the 'C' notification in the (tree)conflicted
directory, but the exact files ie., the tree conflict victim is not known to
the user at a first look, unless he does a 'svn info'. Currently it is
'Skipped' in your patch. It can be 'Obstructed' during the notification. If
that is the case the merge_test 92 should be changed accordingly to handle this
notification. But there is another problem of 'what to show in 'svn status' for
these tree conflict victims via obstruction.
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] tree conflicts and obstructions
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
>>>@@ -729,22 +766,15 @@ merge_file_changed(svn_wc_adm_access_t *
[...]
>>>+ SVN_ERR(persist_tree_conflict(adm_access, victim_path,
>>>+ svn_node_file,
>>>+ svn_wc_conflict_action_edit,
>>>+ svn_wc_conflict_reason_missing,
>>>+ merge_b->pool));
>>
>>In this case, (though not affected by your current patch,) the reason is not
>>necessarily "missing": the reason depends on the 'kind' of item that we found
>>on disk. We might want to be a bit more precise there.
>
> This is still a TODO, I didn't change the reason yet.
> It is out of scope for obstructions, but I agree with your concern.
> Should we create an issue for this?
I think this will come up naturally as part of refining the UI so no need for
an issue. Or maybe just one issue that says "the notifications in the UI could
be more precise" and then it could mention this particular code path so we
don't miss it.
>>>+ SVN_ERR(persist_tree_conflict(adm_access, victim_path,
>>>+ svn_node_file,
>>>+ svn_wc_conflict_action_delete,
>>
>>The action of this function is "add", not "delete".
>
> Doh! Thanks, corrected in all cases.
You missed one :-)
>>BUT... after this point we reach an "else":
>>... which means "we know this is a versioned file here where we're trying to
>>add one". This condition should also be treated as a tree conflict, perhaps the
>>very same type of conflict. (So, why was there an "if" statement? It seems to
>>be something to do with...)
>>
>>Again, this isn't a problem with your change, it was already that way.
>
> Also flagged as a tree conflict for now.
See comment in line below.
>>>@@ -1144,9 +1212,25 @@ merge_file_deleted(svn_wc_adm_access_t *
>>
>>Now we're in the merge_file_deleted() function, case svn_node_file, and we've
>>ensured the file on disk has the same contents as the one we're wanting to delete.
>>
>>However, we haven't yet determined the WC schedule state of this file. In order
>>to be able to delete it without conflict, we require that the file is versioned
>>and its schedule state is anything except schedule_delete.
>
> This is a separate bug which has nothing to do with obstructions.
> Can we file an issue for this one, too?
You're right that it's not specifically to do with converting obstructions into
tree conflicts, so yes please file a separate issue.
It is an issue to do with tree conflicts and obstructions, though. Previously,
the code was written to not care about tree conflicts. As long as it ended up
with no file present, it didn't care how it achieved that. (I think previously
the "svn_client__wc_delete" silently did nothing in this case. I haven't
verified this.)
Now, in implementing tree conflicts, we do have to care. If there is a
versioned file, we can delete it. If not, we can't and so we must raise a conflict.
>>> err = svn_client__wc_delete(mine, parent_access, merge_b->force,
>>> merge_b->dry_run, keep_local, NULL, NULL,
>>> merge_b->ctx, subpool);
>>>- if (err && state)
>>>+ if (err)
>>
>>(OK, this change corrects a bug whereby "err" was previously neither cleared
>>nor returned when "state" was NULL. I might correct this on trunk.)
>
> Yikes! I didn't even notice. Yes, please fix it on trunk!
I have done.
>>> {
>>>- *state = svn_wc_notify_state_obstructed;
>>>+ /* The file is obstructed, so it is a tree conflict victim.
>>>+ * See notes about obstructions in notes/tree-conflicts/detection.txt.
>>>+ */
>>
>>So, we're saying: if this function failed to delete the file, then we must flag
>>a conflict because there's some sort of obstruction.
>>
>>That's probably right, but I'm not sure exactly how svn_client__wc_delete()
>>behaves. We need to update its doc-string to specify the conditions under which
>>it fails, so that we can rely on it here.
>
> I've update the comment:
>
> /* The file deletion the merge wanted to do could not be carried
> * out for some reason, so the file deletion was obstructed.
> * Hence the file merge wants to delete is a tree conflict victim.
> * See notes about obstructions in notes/tree-conflicts/detection.txt.
> */
Great.
> I agree that we should verify in which cases svn_client__wc_delete
> can fail, so that we don't end up flagging tree conflicts that
> aren't any. Issue? :)
Yes, please file an issue. The more (or equally) important thing that could go
wrong is that the function could quietly do nothing in lots of cases without
raising an error, and then we'd be failing to detect the conflict.
> Attached is an updated patch based on your review.
>
> [[[
>
> On the tree-conflicts branch, flag tree conflicts upon obstructed
> files during merge.
>
> * subversion/libsvn_wc/tree_conflicts.c
> (tree_conflict_phrases): New members merge_added and obstructed.
> (new_tree_conflict_phrases): Initialise new members of
> tree_conflict_phrases.
> (select_our_phrase, read_reason,
> svn_wc__write_tree_conflicts_to_entry,
> svn_wc_append_tree_conflict_info_xml): Handle new members of
> tree_conflict_phrases.
>
> * subversion/libsvn_wc/tree_conflicts.h
> (SVN_WC__CONFLICT_REASON_OBSTRUCTED,
> SVN_WC__CONFLICT_ACTION_ADDED): New constants.
>
> * subversion/libsvn_client/merge.c
> (tree_conflict): New function.
> (merge_file_changed): Use new function tree_conflict instead of inline code.
> (merge_file_added, merge_file_deleted): Use new function tree_conflict
> instead of inline code. Detect and persist tree conflicts caused by
> obstructed merge operations.
>
> ]]]
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 30255)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -313,6 +313,51 @@ add_parent_to_tree_conflicted_dirs(merge
> APR_ARRAY_PUSH(merge_b->tree_conflicted_dirs, const char *) = dir_path;
> }
>
> +/* Cause a tree conflict notification, and if the merge is not
> + * a dry run, also make the tree conflict persistent. Do nothing
> + * if the merge is record-only.
> + *
> + * The tree conflict is assumed to have happened during a merge using
"...happened to the file VICTIM_PATH..."
just to get that argument mentioned.
> + * merge baton MERGE_B.
> + *
> + * ADM_ACCESS corresponds to the tree-conflicted directory
> + * This directory must be the victim's parent directory.
> + *
> + * NODE_KIND, ACTION, and REASON correspond to the fields
> + * of the same names in svn_wc_conflict_description_t.
> + *
> + * This function exists mainly to increase the legibility
> + * of the tree conflict detection code below, because we
> + * do this quite often with only few parameters being tweaked.
This last paragraph seems to be just explaining the purpose of every local
function in the world!
BTW, despite those two comments, you write a very good doc-string.
> + */
> +static svn_error_t*
> +tree_conflict(merge_cmd_baton_t *merge_b,
> + svn_wc_adm_access_t *adm_access,
> + const char *victim_path,
> + svn_node_kind_t node_kind,
> + svn_wc_conflict_action_t action,
> + svn_wc_conflict_reason_t reason)
> +{
[...]
> @@ -968,6 +1008,15 @@ merge_file_added(svn_wc_adm_access_t *ad
> }
> break;
> case svn_node_dir:
> + /* The file add the merge wants to carry out is obstructed by
> + * a directory, so the file the merge wants to add is a tree
> + * conflict victim.
> + * See notes about obstructions in notes/tree-conflicts/detection.txt.
> + */
> + SVN_ERR(tree_conflict(merge_b, adm_access, mine,
> + svn_node_file,
> + svn_wc_conflict_action_delete,
That's the one you missed. "action_add"
> @@ -992,12 +1041,31 @@ merge_file_added(svn_wc_adm_access_t *ad
> user must have recreated it, don't touch it */
> if (!entry || entry->schedule == svn_wc_schedule_delete)
> {
> + /* The file add the merge wants to carry out is obstructed by
> + * an unversioned file, so the file the merge wants to add
> + * is a tree conflict victim. See notes about obstructions in
> + * notes/tree-conflicts/detection.txt.
> + */
> + SVN_ERR(tree_conflict(merge_b, adm_access, mine,
> + svn_node_file,
> + svn_wc_conflict_action_add,
> + svn_wc_conflict_reason_obstructed));
> +
> /* this will make the repos_editor send a 'skipped' message */
> if (content_state)
> *content_state = svn_wc_notify_state_obstructed;
> }
> else
> {
> + /* The file add the merge wants to carry out is obstructed by
> + * a versioned file, so the file the merge wants to add is a
> + * tree conflict victim. See notes about obstructions in
> + * notes/tree-conflicts/detection.txt.
> + */
> + SVN_ERR(tree_conflict(merge_b, adm_access, mine,
> + svn_node_file,
> + svn_wc_conflict_action_add,
> + svn_wc_conflict_reason_obstructed));
This should go further down, in the "else" after the "if
(dry_run_deleted_p(...))", because the dry_run_deleted_p case seems to be
correctly handled as a non-conflict.
And let's add a "###" comment here, warning that "This doesn't seem to
correspond to the following code which seems to be handling this as a
non-conflict."
> if (dry_run_deleted_p(merge_b, mine))
> {
> if (content_state)
[...]
The rest is fine. Please commit it with these tweaks.
Thanks.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] tree conflicts and obstructions
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Apr 03, 2008 at 08:31:30PM +0100, Julian Foad wrote:
> Stefan Sperling wrote:
>> here's my first shot at tree conflicts and obstructions.
>
> OK.
Julian,
>> +static svn_error_t*
>> +persist_tree_conflict(svn_wc_adm_access_t *adm_access,
>> + const char *victim_path,
>> + svn_node_kind_t node_kind,
>> + svn_wc_conflict_action_t action,
>> + svn_wc_conflict_reason_t reason,
>> + apr_pool_t *pool)
>
> Improvement suggestion:
>
> Every time you use this function it's in the same context of:
>
> add_parent_to_tree_conflicted_dirs(merge_b, mine);
> if (! merge_b->dry_run)
> {
> char *victim_path = svn_path_basename(mine, merge_b->pool);
> SVN_ERR(persist_tree_conflict(adm_access, victim_path, ...));
> }
>
> so it would be good to expand it (or create another wrapper function around it)
> to do all of that.
Done. In fact, the condition (! merge_b->record_only) should also
guard all tree conflict handling altogether, so refactoring this
code also caught the bug that I totally forgot about merge_b->record_only.
Thanks for pointing this one out!
>> @@ -729,22 +766,15 @@ merge_file_changed(svn_wc_adm_access_t *
>> /* This is use case 4 described in the paper attached to issue
>> * #2282. See also notes/tree-conflicts/detection.txt
>> */
>> - char *victim_path = svn_path_basename(mine, merge_b->pool);
>> -
>> add_parent_to_tree_conflicted_dirs(merge_b, mine);
>> if (! merge_b->dry_run)
>> {
>> - svn_wc_conflict_description_t *conflict;
>> -
>> - conflict = apr_pcalloc(merge_b->pool, -
>> sizeof(svn_wc_conflict_description_t));
>> - conflict->victim_path = victim_path;
>> - conflict->node_kind = svn_node_file;
>> - conflict->operation = svn_wc_operation_merge;
>> - conflict->action = svn_wc_conflict_action_edit;
>> - conflict->reason = svn_wc_conflict_reason_missing;
>> - SVN_ERR(svn_wc_add_tree_conflict_data(conflict, adm_access,
>> - merge_b->pool));
>> + char *victim_path = svn_path_basename(mine, merge_b->pool);
>> + SVN_ERR(persist_tree_conflict(adm_access, victim_path,
>> + svn_node_file,
>> + svn_wc_conflict_action_edit,
>> + svn_wc_conflict_reason_missing,
>> + merge_b->pool));
>> }
>> }
>>
>
> In this case, (though not affected by your current patch,) the reason is not
> necessarily "missing": the reason depends on the 'kind' of item that we found
> on disk. We might want to be a bit more precise there.
This is still a TODO, I didn't change the reason yet.
It is out of scope for obstructions, but I agree with your concern.
Should we create an issue for this?
>> + *
>> + * The file is obstructed, so it is a tree conflict victim.
>> + * See notes about obstructions in
>> + * notes/tree-conflicts/detection.txt.
>> + */
>> + add_parent_to_tree_conflicted_dirs(merge_b, mine);
>> + if (! merge_b->dry_run)
>> + {
>> + char *victim_path = svn_path_basename(mine, merge_b->pool);
>> + SVN_ERR(persist_tree_conflict(adm_access, victim_path,
>> + svn_node_file,
>> + svn_wc_conflict_action_delete,
>
> The action of this function is "add", not "delete".
>
Doh! Thanks, corrected in all cases.
>> + svn_wc_conflict_reason_obstructed,
>
> I would say the reason is this new reason:
>
> svn_wc_conflict_reason_added, /* object is already added */
>
> because I think we use the term "obstruction" for objects which are unexpected
> (of the wrong type, or unversioned), not for objects which we know are there
> and match their metadata.
>
> But, the exact "reason" is not very important at this stage.
Also still a TODO. Should we file an issue?
>> + svn_wc_conflict_reason_obstructed,
>> + merge_b->pool));
>> + }
>> +
>> /* this will make the repos_editor send a 'skipped' message */
>> if (content_state)
>> *content_state = svn_wc_notify_state_obstructed;
>
> BUT... after this point we reach an "else":
>
>> }
>> else
>> {
>
> ... which means "we know this is a versioned file here where we're trying to
> add one". This condition should also be treated as a tree conflict, perhaps the
> very same type of conflict. (So, why was there an "if" statement? It seems to
> be something to do with...)
>
> Again, this isn't a problem with your change, it was already that way.
Also flagged as a tree conflict for now.
>> if (dry_run_deleted_p(merge_b, mine))
>> {
>> if (content_state)
>> *content_state = svn_wc_notify_state_changed;
>> }
>
> OK. That says, "if this is a dry run and the file wouldn't otherwise be here,
> then report that the add would go ahead".
>
> There's a minor bug (inconsistency) here: in the normal case, the "prop_state"
> output is also updated according to whether the new file has any properties, so
> we should do this in the dry-run case too. I'll make that fix on trunk if you
> agree.
I don't know whether prop_state should be updated during dry runs,
but if it usually is, then this is a bug.
>> else
>> {
>> /* Indicate that we merge because of an add to handle a
>> special case for binary files with no local mods. */
>> merge_b->add_necessitated_merge = TRUE;
>>
>> SVN_ERR(merge_file_changed(adm_access, content_state,
>> prop_state, mine, older, yours,
>> rev1, rev2,
>> mimetype1, mimetype2,
>> prop_changes, original_props,
>> baton));
>>
>> /* Reset the state so that the baton can safely be reused
>> in subsequent ops occurring during this merge. */
>> merge_b->add_necessitated_merge = FALSE;
>> }
>> }
>
> What's this special case doing? Why isn't this case a conflict?
As discussed on IRC, we're waiting for feedback by Paul Burba on this one.
>> @@ -1144,9 +1212,25 @@ merge_file_deleted(svn_wc_adm_access_t *
>
> Now we're in the merge_file_deleted() function, case svn_node_file, and we've
> ensured the file on disk has the same contents as the one we're wanting to delete.
>
> However, we haven't yet determined the WC schedule state of this file. In order
> to be able to delete it without conflict, we require that the file is versioned
> and its schedule state is anything except schedule_delete.
This is a separate bug which has nothing to do with obstructions.
Can we file an issue for this one, too?
>> err = svn_client__wc_delete(mine, parent_access, merge_b->force,
>> merge_b->dry_run, keep_local, NULL, NULL,
>> merge_b->ctx, subpool);
>> - if (err && state)
>> + if (err)
>
> (OK, this change corrects a bug whereby "err" was previously neither cleared
> nor returned when "state" was NULL. I might correct this on trunk.)
Yikes! I didn't even notice. Yes, please fix it on trunk!
>
>> {
>> - *state = svn_wc_notify_state_obstructed;
>> + /* The file is obstructed, so it is a tree conflict victim.
>> + * See notes about obstructions in notes/tree-conflicts/detection.txt.
>> + */
>
> So, we're saying: if this function failed to delete the file, then we must flag
> a conflict because there's some sort of obstruction.
>
> That's probably right, but I'm not sure exactly how svn_client__wc_delete()
> behaves. We need to update its doc-string to specify the conditions under which
> it fails, so that we can rely on it here.
I've update the comment:
/* The file deletion the merge wanted to do could not be carried
* out for some reason, so the file deletion was obstructed.
* Hence the file merge wants to delete is a tree conflict victim.
* See notes about obstructions in notes/tree-conflicts/detection.txt.
*/
I agree that we should verify in which cases svn_client__wc_delete
can fail, so that we don't end up flagging tree conflicts that
aren't any. Issue? :)
> Thanks.
> - Julian
Attached is an updated patch based on your review.
[[[
On the tree-conflicts branch, flag tree conflicts upon obstructed
files during merge.
* subversion/libsvn_wc/tree_conflicts.c
(tree_conflict_phrases): New members merge_added and obstructed.
(new_tree_conflict_phrases): Initialise new members of
tree_conflict_phrases.
(select_our_phrase, read_reason,
svn_wc__write_tree_conflicts_to_entry,
svn_wc_append_tree_conflict_info_xml): Handle new members of
tree_conflict_phrases.
* subversion/libsvn_wc/tree_conflicts.h
(SVN_WC__CONFLICT_REASON_OBSTRUCTED,
SVN_WC__CONFLICT_ACTION_ADDED): New constants.
* subversion/libsvn_client/merge.c
(tree_conflict): New function.
(merge_file_changed): Use new function tree_conflict instead of inline code.
(merge_file_added, merge_file_deleted): Use new function tree_conflict
instead of inline code. Detect and persist tree conflicts caused by
obstructed merge operations.
]]]
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: [PATCH] tree conflicts and obstructions
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> here's my first shot at tree conflicts and obstructions.
OK. My questions in IRC yesterday about when an incoming "add file" meets a
versioned entry in the WC whose disk file is missing... have gone away.
> [[[
>
> On the tree-conflicts branch, flag tree conflicts upon obstructed
> files during merge.
>
> * subversion/libsvn_wc/tree_conflicts.c
> (tree_conflict_phrases): New member obstructed, which holds
> a phrase describing an obstructed tree conflict victim.
> (new_tree_conflict_phrases): Initialise new member of tree_conflict_phrases.
> (select_our_phrase, read_reason,
> svn_wc__write_tree_conflicts_to_entry,
> svn_wc_append_tree_conflict_info_xml): Handle new member of
> tree_conflict_phrases.
>
> * subversion/libsvn_wc/tree_conflicts.h
> (SVN_WC__CONFLICT_REASON_OBSTRUCTED): New constant.
>
> * subversion/libsvn_client/merge.c
> (persist_tree_conflict): New function.
> (merge_file_changed): Use persist_tree_conflict instead of inline code.
> (merge_file_added, merge_file_deleted): Use persist_tree_conflict instead
> of inline code. Detect and persist tree conflicts caused by obstructions.
>
> ]]]
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 30178)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -330,6 +330,43 @@ add_parent_to_tree_conflicted_dirs(merge
> APR_ARRAY_PUSH(merge_b->tree_conflicted_dirs, const char *) = dir_path;
> }
>
> +/* Creates a conflict descriptor for the tree conflict victim
> + * at VICTIM_PATH and uses it to mark a tree conflict that happened
> + * during a merge.
> + *
> + * ADM_ACCESS corresponds to the tree-conflicted directory
> + * This directory must be the victim's parent directory.
> + *
> + * NODE_KIND, ACTION, and REASON correspond to the fields
> + * of the same names in svn_wc_conflict_description_t.
> + *
> + * Do all allocations in POOL.
> + *
> + * This function exists mainly to increase the legibility
> + * of the tree conflict detection code below, because we
> + * do this quite often with only few parameters being tweaked.
> + */
> +static svn_error_t*
> +persist_tree_conflict(svn_wc_adm_access_t *adm_access,
> + const char *victim_path,
> + svn_node_kind_t node_kind,
> + svn_wc_conflict_action_t action,
> + svn_wc_conflict_reason_t reason,
> + apr_pool_t *pool)
Improvement suggestion:
Every time you use this function it's in the same context of:
add_parent_to_tree_conflicted_dirs(merge_b, mine);
if (! merge_b->dry_run)
{
char *victim_path = svn_path_basename(mine, merge_b->pool);
SVN_ERR(persist_tree_conflict(adm_access, victim_path, ...));
}
so it would be good to expand it (or create another wrapper function around it)
to do all of that.
> @@ -729,22 +766,15 @@ merge_file_changed(svn_wc_adm_access_t *
> /* This is use case 4 described in the paper attached to issue
> * #2282. See also notes/tree-conflicts/detection.txt
> */
> - char *victim_path = svn_path_basename(mine, merge_b->pool);
> -
> add_parent_to_tree_conflicted_dirs(merge_b, mine);
> if (! merge_b->dry_run)
> {
> - svn_wc_conflict_description_t *conflict;
> -
> - conflict = apr_pcalloc(merge_b->pool,
> - sizeof(svn_wc_conflict_description_t));
> - conflict->victim_path = victim_path;
> - conflict->node_kind = svn_node_file;
> - conflict->operation = svn_wc_operation_merge;
> - conflict->action = svn_wc_conflict_action_edit;
> - conflict->reason = svn_wc_conflict_reason_missing;
> - SVN_ERR(svn_wc_add_tree_conflict_data(conflict, adm_access,
> - merge_b->pool));
> + char *victim_path = svn_path_basename(mine, merge_b->pool);
> + SVN_ERR(persist_tree_conflict(adm_access, victim_path,
> + svn_node_file,
> + svn_wc_conflict_action_edit,
> + svn_wc_conflict_reason_missing,
> + merge_b->pool));
> }
> }
>
In this case, (though not affected by your current patch,) the reason is not
necessarily "missing": the reason depends on the 'kind' of item that we found
on disk. We might want to be a bit more precise there.
> @@ -939,7 +969,23 @@ merge_file_added(svn_wc_adm_access_t *ad
> SVN_ERR(svn_wc_entry(&entry, mine, adm_access, FALSE, subpool));
> if (entry && entry->schedule != svn_wc_schedule_delete)
> {
> - /* It's versioned but missing. */
> + /* It's versioned but missing.
(Now we're in merge_file_added(), case svn_node_none.)
OK.
> + *
> + * The file is obstructed, so it is a tree conflict victim.
> + * See notes about obstructions in
> + * notes/tree-conflicts/detection.txt.
> + */
> + add_parent_to_tree_conflicted_dirs(merge_b, mine);
> + if (! merge_b->dry_run)
> + {
> + char *victim_path = svn_path_basename(mine, merge_b->pool);
> + SVN_ERR(persist_tree_conflict(adm_access, victim_path,
> + svn_node_file,
> + svn_wc_conflict_action_delete,
The action of this function is "add", not "delete".
> + svn_wc_conflict_reason_obstructed,
I would say the reason is this new reason:
svn_wc_conflict_reason_added, /* object is already added */
because I think we use the term "obstruction" for objects which are unexpected
(of the wrong type, or unversioned), not for objects which we know are there
and match their metadata.
But, the exact "reason" is not very important at this stage.
> + merge_b->pool));
> + }
> +
> if (content_state)
> *content_state = svn_wc_notify_state_obstructed;
> svn_pool_destroy(subpool);
> @@ -983,6 +1029,20 @@ merge_file_added(svn_wc_adm_access_t *ad
> }
> break;
> case svn_node_dir:
> + /* The file is obstructed by a directory, so it is a tree conflict victim.
> + * See notes about obstructions in notes/tree-conflicts/detection.txt.
> + */
> + add_parent_to_tree_conflicted_dirs(merge_b, mine);
> + if (! merge_b->dry_run)
> + {
> + char *victim_path = svn_path_basename(mine, merge_b->pool);
> + SVN_ERR(persist_tree_conflict(adm_access, victim_path,
> + svn_node_file,
> + svn_wc_conflict_action_delete,
Again, it's "add".
> + svn_wc_conflict_reason_obstructed,
Yes.
> + merge_b->pool));
> + }
> +
> if (content_state)
> {
> /* directory already exists, is it under version control? */
> @@ -1007,6 +1067,21 @@ merge_file_added(svn_wc_adm_access_t *ad
Now, this is the case where there is an svn_node_file on disk...
> user must have recreated it, don't touch it */
> if (!entry || entry->schedule == svn_wc_schedule_delete)
... and this says, "but there isn't a versioned file on disk here"...
> {
> + /* The file is obstructed by an unversioned file, so it is
> + * a tree conflict victim. See notes about obstructions in
> + * notes/tree-conflicts/detection.txt.
... so, yes, certainly this add is obstructed by an unversioned item.
> + */
> + add_parent_to_tree_conflicted_dirs(merge_b, mine);
> + if (! merge_b->dry_run)
> + {
> + char *victim_path = svn_path_basename(mine, merge_b->pool);
> + SVN_ERR(persist_tree_conflict(adm_access, victim_path,
> + svn_node_file,
> + svn_wc_conflict_action_delete,
(It's "add" again.)
> + svn_wc_conflict_reason_obstructed,
> + merge_b->pool));
> + }
> +
> /* this will make the repos_editor send a 'skipped' message */
> if (content_state)
> *content_state = svn_wc_notify_state_obstructed;
BUT... after this point we reach an "else":
> }
> else
> {
... which means "we know this is a versioned file here where we're trying to
add one". This condition should also be treated as a tree conflict, perhaps the
very same type of conflict. (So, why was there an "if" statement? It seems to
be something to do with...)
Again, this isn't a problem with your change, it was already that way.
> if (dry_run_deleted_p(merge_b, mine))
> {
> if (content_state)
> *content_state = svn_wc_notify_state_changed;
> }
OK. That says, "if this is a dry run and the file wouldn't otherwise be here,
then report that the add would go ahead".
There's a minor bug (inconsistency) here: in the normal case, the "prop_state"
output is also updated according to whether the new file has any properties, so
we should do this in the dry-run case too. I'll make that fix on trunk if you
agree.
> else
> {
> /* Indicate that we merge because of an add to handle a
> special case for binary files with no local mods. */
> merge_b->add_necessitated_merge = TRUE;
>
> SVN_ERR(merge_file_changed(adm_access, content_state,
> prop_state, mine, older, yours,
> rev1, rev2,
> mimetype1, mimetype2,
> prop_changes, original_props,
> baton));
>
> /* Reset the state so that the baton can safely be reused
> in subsequent ops occurring during this merge. */
> merge_b->add_necessitated_merge = FALSE;
> }
> }
What's this special case doing? Why isn't this case a conflict?
> @@ -1144,9 +1212,25 @@ merge_file_deleted(svn_wc_adm_access_t *
Now we're in the merge_file_deleted() function, case svn_node_file, and we've
ensured the file on disk has the same contents as the one we're wanting to delete.
However, we haven't yet determined the WC schedule state of this file. In order
to be able to delete it without conflict, we require that the file is versioned
and its schedule state is anything except schedule_delete.
> err = svn_client__wc_delete(mine, parent_access, merge_b->force,
> merge_b->dry_run, keep_local, NULL, NULL,
> merge_b->ctx, subpool);
> - if (err && state)
> + if (err)
(OK, this change corrects a bug whereby "err" was previously neither cleared
nor returned when "state" was NULL. I might correct this on trunk.)
> {
> - *state = svn_wc_notify_state_obstructed;
> + /* The file is obstructed, so it is a tree conflict victim.
> + * See notes about obstructions in notes/tree-conflicts/detection.txt.
> + */
So, we're saying: if this function failed to delete the file, then we must flag
a conflict because there's some sort of obstruction.
That's probably right, but I'm not sure exactly how svn_client__wc_delete()
behaves. We need to update its doc-string to specify the conditions under which
it fails, so that we can rely on it here.
> + add_parent_to_tree_conflicted_dirs(merge_b, mine);
> + if (! merge_b->dry_run)
> + {
> + char *victim_path = svn_path_basename(mine, merge_b->pool);
> + SVN_ERR(persist_tree_conflict(adm_access, victim_path,
> + svn_node_file,
> + svn_wc_conflict_action_delete,
> + svn_wc_conflict_reason_obstructed,
> + merge_b->pool));
> + }
> +
> + if (state)
> + *state = svn_wc_notify_state_obstructed;
> +
> svn_error_clear(err);
> }
> else if (state)
> @@ -1155,6 +1239,20 @@ merge_file_deleted(svn_wc_adm_access_t *
> }
> break;
> case svn_node_dir:
> + /* The file is obstructed by a directory, it is a tree conflict victim.
> + * See notes about obstructions in notes/tree-conflicts/detection.txt.
> + */
> + add_parent_to_tree_conflicted_dirs(merge_b, mine);
> + if (! merge_b->dry_run)
> + {
> + char *victim_path = svn_path_basename(mine, merge_b->pool);
> + SVN_ERR(persist_tree_conflict(adm_access, victim_path,
> + svn_node_file,
> + svn_wc_conflict_action_delete,
> + svn_wc_conflict_reason_obstructed,
> + merge_b->pool));
> + }
> +
Yes, this case is fine.
> if (state)
> *state = svn_wc_notify_state_obstructed;
> break;
Thanks.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org