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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-04-04 15:45:44 CEST