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 <stsp_at_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
- application/pgp-signature attachment: stored
Received on 2008-04-04 14:30:58 CEST