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

On Mon, Nov 30, 2009 at 9:42 AM, Philip Martin
<philip.martin_at_wandisco.com> 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 <dev_at_subversion.apache.org> mailing list.
To subscribe to the new list, send an empty e-mail to <dev-subscribe_at_subversion.apache.org>.