Re: valgrind UMRs in "svn merge --record-only"

On Mon, Nov 30, 2009 at 9:42 AM, Philip Martin
<[email protected]> wrote:
> I'm seeing an unitialised memory read during "svn merge --record-only"
> in places like blame_tests.py 10.  This happens when
> libsvn_client/repos_diff.c:add_directory calls
> libsvn_client/merge.c:merge_dir_added which does:
>
>  /* Easy out: We are only applying mergeinfo changes to existing paths. */
>  if (merge_b->record_only)
>    {
>      svn_pool_destroy(subpool);
>      return SVN_NO_ERROR;
>    }
>
> without setting *state so when add_directory then goes on the check
> state it triggers the UMR.  One fix would be soemthing like:
>
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c    (revision 885260)
> +++ subversion/libsvn_client/merge.c    (working copy)
> @@ -2003,6 +2003,8 @@
>   /* Easy out: We are only applying mergeinfo changes to existing paths. */
>   if (merge_b->record_only)
>     {
> +      if (*state)
> +        *state = svn_wc_notify_state_unknown;
>       svn_pool_destroy(subpool);
>       return SVN_NO_ERROR;
>     }
>
>
> but I'm not sure that svn_wc_notify_state_unknown is correct.  Does
> the comment about only applying to existing paths mean that because
> this is an add the directory does not exist in the working copy?  Does
> that mean that svn_wc_notify_state_missing would be better than
> svn_wc_notify_state_unknown?
>
> The same code occurs in other places such as merge_dir_deleted,
> merge_file_added and while these don't trigger UMRs I suspect that is
> down to limited test coverage.  Take merge_dir_deleted for example: it
> doesn't set either *state or *tree_conflicted.  It's even less clear
> what the comment about only applying to existing paths means here.

Hi (and welcome back!) Philip,

Those 'if (merge_b->record_only)' early exits were added in r880264
which implemented the change that allows --record-only merges to apply
only svn:mergeinfo diffs, see
http://svn.haxx.se/dev/archive-2009-09/0520.shtml.

You are correct about the the comment 'We are only applying mergeinfo
changes to existing paths', as regards merge_dir_added (and
merge_file_added); since we are applying only mergeinfo diffs we do
not want to actually add any files/dirs.  Unfortunately I used the
same comment for merge_file_changed, merge_file_deleted, and
merge_dir_deleted, which is quite wrong.  The comment should
universally be changed to:

  /* Easy out: We are only applying mergeinfo differences. */

Is that any clearer?

Re what svn_wc_notify_state_t to return, I think that
svn_wc_notify_state_unchanged makes the most sense for
merge_file_changed, merge_file_deleted, merge_dir_deleted.  I'm a bit
less certain about merge_file_added and merge_dir_added, but
svn_wc_notify_state_unchanged still seems to fit, since we haven't
added anything.  I'm fairly certain that svn_wc_notify_state_missing
is wrong because how can we say a path was missing when we are asking
to add it?

The attached patch has the comment fix and sets any uninitialized
output parameters to svn_wc_notify_state_unchanged, I'm running the
tests right now to see if anything falls out...

[[[
Follow-up to r880264, be sure to set state and tree conflict output
parameters during --record-only merges.

Found by: philip

* subversion/libsvn_client/merge.c
  (merge_file_changed,
   merge_file_added,
   merge_file_deleted,
   merge_dir_added,
   merge_dir_deleted):
  Improve comments regarding early return during
  --record-only merges.  Set any svn_wc_notify_state_t * arguments to
  svn_wc_notify_state_unchanged and any tree conflicted output argument to
  false, if any of these are not already set.
]]]

Paul

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2425700

Please start new threads on the <[email protected]> mailing list.
To subscribe to the new list, send an empty e-mail to <[email protected]>.

Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c	(revision 885596)
+++ subversion/libsvn_client/merge.c	(working copy)
@@ -1426,10 +1426,12 @@
     if (prop_state)
       *prop_state = svn_wc_notify_state_unchanged;
 
-  /* Easy out: We are only applying mergeinfo changes to existing paths. */
+  /* Easy out: We are only applying mergeinfo differences. */
   if (merge_b->record_only)
     {
       svn_pool_destroy(subpool);
+      if (content_state);
+        *content_state = svn_wc_notify_state_unchanged;
       return SVN_NO_ERROR;
     }
 
@@ -1556,10 +1558,16 @@
   apr_hash_t *new_props;
   const char *mine_abspath;
 
-  /* Easy out: We are only applying mergeinfo changes to existing paths. */
+  /* Easy out: We are only applying mergeinfo differences. */
   if (merge_b->record_only)
     {
       svn_pool_destroy(subpool);
+      if (content_state)
+        *content_state = svn_wc_notify_state_unchanged;
+      if (prop_state)
+        *prop_state = svn_wc_notify_state_unchanged;
+      if (tree_conflicted)
+        *tree_conflicted = FALSE;
       return SVN_NO_ERROR;
     }
 
@@ -1859,10 +1867,14 @@
   svn_node_kind_t kind;
   const char *mine_abspath;
 
-  /* Easy out: We are only applying mergeinfo changes to existing paths. */
+  /* Easy out: We are only applying mergeinfo differences. */
   if (merge_b->record_only)
     {
       svn_pool_destroy(subpool);
+      if (state)
+        *state = svn_wc_notify_state_unchanged;
+      if (tree_conflicted)
+        *tree_conflicted = FALSE;
       return SVN_NO_ERROR;
     }
 
@@ -2000,10 +2012,14 @@
   svn_boolean_t is_deleted;
   svn_error_t *err;
 
-  /* Easy out: We are only applying mergeinfo changes to existing paths. */
+  /* Easy out: We are only applying mergeinfo differences. */
   if (merge_b->record_only)
     {
       svn_pool_destroy(subpool);
+      if (state)
+        *state = svn_wc_notify_state_unchanged;
+      if (tree_conflicted)
+        *tree_conflicted = FALSE;
       return SVN_NO_ERROR;
     }
 
@@ -2213,10 +2229,14 @@
   svn_boolean_t is_versioned;
   svn_boolean_t is_deleted;
 
-  /* Easy out: We are only applying mergeinfo changes to existing paths. */
+  /* Easy out: We are only applying mergeinfo differences. */
   if (merge_b->record_only)
     {
       svn_pool_destroy(subpool);
+      if (state)
+        *state = svn_wc_notify_state_unchanged;
+      if (tree_conflicted)
+        *tree_conflicted = FALSE;
       return SVN_NO_ERROR;
     }