[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1330444 - in /subversion/trunk/subversion: libsvn_client/merge.c tests/cmdline/merge_tests.py

From: Paul Burba <ptburba_at_gmail.com>
Date: Wed, 25 Apr 2012 15:58:12 -0400

On Wed, Apr 25, 2012 at 3:43 PM, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>> -----Original Message-----
>> From: pburba_at_apache.org [mailto:pburba_at_apache.org]
>> Sent: woensdag 25 april 2012 19:58
>> To: commits_at_subversion.apache.org
>> Subject: svn commit: r1330444 - in /subversion/trunk/subversion:
>> libsvn_client/merge.c tests/cmdline/merge_tests.py
>>
>> Author: pburba
>> Date: Wed Apr 25 17:57:30 2012
>> New Revision: 1330444
>>
>> URL: http://svn.apache.org/viewvc?rev=1330444&view=rev
>> Log:
>> Fix issue #4169 'added subtrees with non-inheritable mergeinfo cause
>> spurious subtree mergeinfo'.
>>
>> This fixes a bug julianf noted in r1205867.
>>
>> * subversion/libsvn_client/merge.c
>>
>>   (notification_receiver_baton_t): Clarify what is and isn't stored in the
>>    added_abspaths member.
>>
>>   (notification_receiver): Only stash the roots of added subtrees rather
>>    than tracking every added subtree excepting the immediate children of
>>    added roots (which is what happened previously).  There is no need to
>>    track those other added subtrees (and as the test below demonstrates,
>>    there are edge cases where spurious subtree mergeinfo can be created).
>>
>> * subversion/tests/cmdline/merge_tests.py
>>
>>   (merge_with_added_subtrees_with_mergeinfo): New.
>>
>>   (test_list): Add merge_with_added_subtrees_with_mergeinfo.
>>
>> Modified:
>>     subversion/trunk/subversion/libsvn_client/merge.c
>>     subversion/trunk/subversion/tests/cmdline/merge_tests.py
>>
>> Modified: subversion/trunk/subversion/libsvn_client/merge.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/merge
>> .c?rev=1330444&r1=1330443&r2=1330444&view=diff
>> =================================================================
>> =============
>> --- subversion/trunk/subversion/libsvn_client/merge.c (original)
>> +++ subversion/trunk/subversion/libsvn_client/merge.c Wed Apr 25 17:57:30
>> 2012
>> @@ -2941,30 +2941,47 @@ notification_receiver(void *baton, const
>>
>>        if (notify->action == svn_wc_notify_update_add)
>>          {
>> -          svn_boolean_t is_root_of_added_subtree = FALSE;
>> -          const char *added_path = apr_pstrdup(notify_b->pool,
>> -                                               notify_abspath);
>> -          const char *added_path_parent = NULL;
>> +          svn_boolean_t root_of_added_subtree = TRUE;
>>
>>            /* Stash the root path of any added subtrees. */
>>            if (notify_b->added_abspaths == NULL)
>>              {
>> +              /* The first added path is always a root. */
>>                notify_b->added_abspaths = apr_hash_make(notify_b->pool);
>> -              is_root_of_added_subtree = TRUE;
>>              }
>>            else
>>              {
>> -              added_path_parent = svn_dirent_dirname(added_path, pool);
>> -              /* ### Bug. Testing whether its immediate parent is in the
>> -               * hash isn't enough: this is letting every other level of
>> -               * the added subtree hierarchy into the hash. */
>> -              if (!apr_hash_get(notify_b->added_abspaths, added_path_parent,
>> -                                APR_HASH_KEY_STRING))
>> -                is_root_of_added_subtree = TRUE;
>> -            }
>> -          if (is_root_of_added_subtree)
>> -            apr_hash_set(notify_b->added_abspaths, added_path,
>> -                         APR_HASH_KEY_STRING, added_path);
>> +              const char *added_path_parent =
>> +                svn_dirent_dirname(notify_abspath, pool);
>> +              apr_pool_t *iterpool = svn_pool_create(pool);
>> +
>> +              /* Is NOTIFY->PATH the root of an added subtree? */
>> +              while (strcmp(notify_b->merge_b->target->abspath,
>> +                            added_path_parent))
>> +                {
>> +                  if (apr_hash_get(notify_b->added_abspaths,
>> +                                   added_path_parent,
>> +                                   APR_HASH_KEY_STRING))
>> +                    {
>> +                      root_of_added_subtree = FALSE;
>> +                      break;
>> +                    }
>> +
>> +                  svn_pool_clear(iterpool);
>> +                  added_path_parent = svn_dirent_dirname(
>> +                    added_path_parent, iterpool);
>
> This will use unallocated memory when iterating.
>
> The first loop is safe, but after the argument to svn_dirent_dirname will be pointing to memory in the just cleared pool.
>
> Assuming the path is of a normal length the fix is to just not clear the pool.
>> +                }
>> +
>> +              svn_pool_destroy(iterpool);
>
> Because the pool is destroyed here anyway.

Thanks Bert, I switched that iterpool to a subpool (with no clear) in r1330520.

Paul

>> +            }
>> +
>> +          if (root_of_added_subtree)
>> +            {
>> +              const char *added_root_path = apr_pstrdup(notify_b->pool,
>> +                                                        notify_abspath);
>> +              apr_hash_set(notify_b->added_abspaths, added_root_path,
>> +                           APR_HASH_KEY_STRING, added_root_path);
>> +            }
>>          }
>>
>>        if (notify->action == svn_wc_notify_update_delete
>>
>
>        Bert
>
Received on 2012-04-25 21:58:45 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.