Re: svn commit: r30542 - in branches/tree-conflicts/subversion: include libsvn_client libsvn_wc tests/cmdline tests/cmdline/svntest

On Fri, Apr 11, 2008 at 09:08:12AM -0700, julianfoad_at_tigris.org wrote:
> Author: julianfoad
> Date: Fri Apr 11 09:08:11 2008
> New Revision: 30542
>
> Log:
> Start detecting some tree conflicts on directories: in particular, the cases
> of merging a delete with a delete, and an add with an add.

> Modified: branches/tree-conflicts/subversion/libsvn_wc/tree_conflicts.c
> URL: http://svn.collab.net/viewvc/svn/branches/tree-conflicts/subversion/libsvn_wc/tree_conflicts.c?pathrev=30542&r1=30541&r2=30542
> ==============================================================================
> --- branches/tree-conflicts/subversion/libsvn_wc/tree_conflicts.c Fri Apr 11 06:20:03 2008 (r30541)
> +++ branches/tree-conflicts/subversion/libsvn_wc/tree_conflicts.c Fri Apr 11 09:08:11 2008 (r30542)
> @@ -611,6 +611,9 @@ svn_wc__write_tree_conflicts_to_entry(ap
> case svn_wc_conflict_reason_deleted:
> svn_stringbuf_appendcstr(buf, SVN_WC__CONFLICT_REASON_DELETED);
> break;
> + case svn_wc_conflict_reason_added:
> + svn_stringbuf_appendcstr(buf, SVN_WC__CONFLICT_REASON_ADDED);
> + break;
> case svn_wc_conflict_reason_missing:
> svn_stringbuf_appendcstr(buf, SVN_WC__CONFLICT_REASON_MISSING);
> break;
>

The above looks incomplete.

Shouldn't we add code to read this reason from the this_dir entry as well?
Just writing it is only half the job -- svn will throw errors upon
reading the data if it does not expect it.

I guess something like the diff below wouldn't hurt.
Does it look right? Am I correct assuming that the new reason
is only relevant during merge?

Otherwise I think your commit looks very good!
Good work, Julian! :)

I like the dedicated tree conflicts tests script.
Do you plan on migrating the existing tree conflict tests (in the update,
merge and possibly other test scripts) to the dedicated tree conflicts
tests script?

[[[
On the tree-conflicts branch, handle reading of the new
conflict reason svn_wc_conflict_reason_added.

* subversion/libsvn_wc/tree_conflicts.c:
  (tree_conflict_phrases): Add new member we_added_merge.
  (new_tree_conflict_phrases): Handle new member. While here, stop
    implying that operations/actions occur on files only, i.e. remove
    "the file" from all phrases. Also, initialise phrases in the same
    order as they are declared in the struct.
  (select_our_phrase, read_reason, svn_wc_append_tree_conflict_info_xml):

]]]

Index: subversion/libsvn_wc/tree_conflicts.c
===================================================================
--- subversion/libsvn_wc/tree_conflicts.c (revision 30573)
+++ subversion/libsvn_wc/tree_conflicts.c (working copy)
@@ -40,6 +40,7 @@ struct tree_conflict_phrases
   
   /* Used during merge. */
   const char *we_edited_merge;
+ const char *we_added_merge;
   const char *does_not_exist_merge;
   const char *obstructed;
 };
@@ -52,23 +53,26 @@ new_tree_conflict_phrases(apr_pool_t *pool)
   struct tree_conflict_phrases *phrases =
     apr_pcalloc(pool, sizeof(struct tree_conflict_phrases));
 
- phrases->update_deleted = _("The update attempted to delete the file '%s'\n"
+ phrases->update_deleted = _("The update attempted to delete '%s'\n"
                               "(possibly as part of a rename operation).\n");
 
- phrases->update_edited = _("The update attempted to edit the file '%s'.\n");
+ phrases->update_edited = _("The update attempted to edit '%s'.\n");
 
- phrases->merge_deleted = _("The merge attempted to delete the file '%s'\n"
+ phrases->merge_deleted = _("The merge attempted to delete '%s'\n"
                              "(possibly as part of a rename operation).\n");
 
- phrases->merge_edited = _("The merge attempted to edit the file '%s'.\n");
+ phrases->merge_edited = _("The merge attempted to edit '%s'.\n");
 
- phrases->merge_added = _("The merge attempted to add the file '%s'.\n");
+ phrases->merge_added = _("The merge attempted to add '%s'.\n");
 
   phrases->we_deleted = _("You have deleted '%s' locally.\n"
                           "Maybe you renamed it?\n");
 
   phrases->we_edited_update = _("You have edited '%s' locally.\n");
 
+ phrases->does_not_exist_update = _("'%s' does not exist locally.\n"
+ "Maybe you renamed it?\n");
+
   /* This one only comes up together with merge_deleted, never with
    * merge_edited. Otherwise we would have a text conflict. So we can
    * provide a more detailed hint here as to what might have happened. */
@@ -78,10 +82,12 @@ new_tree_conflict_phrases(apr_pool_t *pool)
                                "but those edits are not present on the"
                                " branch you are merging from.\n");
 
- phrases->does_not_exist_update = _("The file '%s' does not exist locally.\n"
- "Maybe you renamed it?\n");
+ phrases->we_added_merge = _("Either you have added '%s' locally,\n"
+ "or it has been added in the history of"
+ " the branch you are merging into.\n");
 
- phrases->does_not_exist_merge = _("The file '%s' does not exist locally.\n"
+
+ phrases->does_not_exist_merge = _("'%s' does not exist locally.\n"
                                     "Maybe you renamed it? Or has it been"
                                     " renamed in the history of the branch\n"
                                     "you are merging into?\n");
@@ -170,6 +176,18 @@ select_our_phrase(svn_wc_conflict_description_t *c
           }
         return NULL; /* Should never happen! */
 
+ case svn_wc_conflict_reason_added:
+ if (conflict->operation == svn_wc_operation_update
+ || conflict->operation == svn_wc_operation_switch)
+ {
+ return NULL; /* Should never happen! */
+ }
+ else if (conflict->operation == svn_wc_operation_merge)
+ {
+ return phrases->merge_added;
+ }
+ return NULL; /* Should never happen! */
+
      default:
         return NULL; /* Should never happen! */
     }
@@ -410,6 +428,8 @@ read_reason(svn_wc_conflict_description_t *conflic
     conflict->reason = svn_wc_conflict_reason_missing;
   else if (advance_on_match(start, SVN_WC__CONFLICT_REASON_OBSTRUCTED))
     conflict->reason = svn_wc_conflict_reason_obstructed;
+ else if (advance_on_match(start, SVN_WC__CONFLICT_REASON_ADDED))
+ conflict->reason = svn_wc_conflict_reason_added;
   else
     return svn_error_create(SVN_ERR_WC_CORRUPT, NULL,
              _("Invalid 'reason' field in tree conflict description"));
@@ -784,6 +804,8 @@ svn_wc_append_tree_conflict_info_xml(svn_stringbuf
       case svn_wc_conflict_reason_obstructed:
         tmp = SVN_WC__CONFLICT_REASON_OBSTRUCTED;
         break;
+ case svn_wc_conflict_reason_added:
+ tmp = SVN_WC__CONFLICT_REASON_ADDED;
       default:
         return svn_error_create(SVN_ERR_WC_CORRUPT, NULL,
             _("Bad reason in tree conflict description"));

-- 
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