Re: [PATCH] Fix issue #4049 - SEGV on patch that deletes and skips

On Wed, Nov 9, 2011 at 10:46 AM, Philip Martin
<philip.martin_at_wandisco.com> wrote:
> Daniel Näslund <dannas_at_dannas.name> writes:
>
>> Index: subversion/libsvn_client/patch.c
>> ===================================================================
>> --- subversion/libsvn_client/patch.c  (revision 1199347)
>> +++ subversion/libsvn_client/patch.c  (arbetskopia)
>> @@ -255,6 +255,7 @@ typedef struct patch_target_t {
>>  typedef struct patch_target_info_t {
>>    const char *local_abspath;
>>    svn_boolean_t deleted;
>> +  svn_boolean_t skipped;
>>  } patch_target_info_t;
>>
>>
>> @@ -2730,6 +2731,11 @@ delete_empty_dirs(apr_array_header_t *targets_info
>>          SVN_ERR(ctx->cancel_func(ctx->cancel_baton));
>>
>>        target_info = APR_ARRAY_IDX(targets_info, i, patch_target_info_t *);
>> +
>> +      /* Skipped dirs will not have a local_abspath set. */
>> +      if (target_info->skipped)
>> +        continue;
>> +
>>        parent = svn_dirent_dirname(target_info->local_abspath, iterpool);
>>
>>        if (apr_hash_get(non_empty_dirs, parent, APR_HASH_KEY_STRING))
>> @@ -2915,6 +2921,7 @@ apply_patches(void *baton,
>>                target_info->local_abspath = apr_pstrdup(scratch_pool,
>>                                                         target->local_abspath);
>>                target_info->deleted = target->deleted;
>> +              target_info->skipped = target->skipped;
>>                APR_ARRAY_PUSH(targets_info,
>>                               patch_target_info_t *) = target_info;
>>
>
> What is not clear to me is why skipped targets get added to targets_info
> in the first place.  How about moving the APR_ARRAY_PUSH inside the
> !target->skipped section:
>
> apply_patches(...)
> {
>   ...
>     if (! target->filtered)
>       {
>          ...
>          APR_ARRAY_PUSH(targets_info, ...)
>          if (! target->skipped)
>            {
>            }
>       }
> }

Doh, looking at it from perspective that appears to be the right fix. No
changes has been made to skipped targets so no need to check if they've
been deleted.

I'll test to see that the patch works with your suggested change and
commit it later tonight (UTC+1) if no one has any other suggestions.

A side note; when I tested the patch I needed to use abspath's in the
patch file or else the target wouldn't get skipped. For prop targets
that didn't have existing parent-dirs, an error was returned. I would
have expected that such path would get skipped or that we would create
the parent dirs. Will investigate and come back with a test case.

--
Daniel