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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-04-03 21:31:48 CEST