Re: [patch] applying patch can fail with dry_run

On 12.01.2012 20:07, Philip Martin wrote:
> Stefan Küng<tortoisesvn_at_gmail.com> writes:
>
>> Could someone please review my patch? I'm sure you guys can see
>> immediately whether this is save or not. If it's ok, then I'll commit
>> it later.
>
> Index: subversion/libsvn_client/patch.c
> ===================================================================
> --- subversion/libsvn_client/patch.c (Revision 1230694)
> +++ subversion/libsvn_client/patch.c (Arbeitskopie)
> @@ -2920,8 +2920,9 @@
> }
> while (patch);
>
> - /* Delete directories which are empty after patching, if any. */
> - SVN_ERR(delete_empty_dirs(targets_info, ctx, dry_run, scratch_pool));
> + if (!dry_run)
> + /* Delete directories which are empty after patching, if any. */
> + SVN_ERR(delete_empty_dirs(targets_info, ctx, dry_run, scratch_pool));
>
> SVN_ERR(svn_diff_close_patch_file(patch_file, iterpool));
> svn_pool_destroy(iterpool);
>
> It doesn't look right to me. Use dry_run to in two places, to determine
> whether to call delete_empty_dirs and as a parameter to
> delete_empty_dirs isn't sensible.

Question is: do we need the notifications for deleting empty dirs in a
dry-run? If yes, then this get's complicated:
the error is thrown from svn_wc_walk_status() called in check_dir_empty.
I could just not call that function in case of a dry-run, or catch that
specific error and go on. But then the notifications wouldn't be
completely accurate anymore.

So assuming we don't want to extend svn_wc_walk_status to take a dry-run
param, the notifications for deleting empty dirs wouldn't be accurate.

Should I just change delete_empty_dirs() to not use the dry_run param
and not get the notifications for those in a dry-run?
I think no notifications is better than inaccurate ones (at least in a
dry-run).

Stefan

-- 
        ___
   oo  // \\      "De Chelonian Mobile"
  (_,\/ \_/ \     TortoiseSVN
    \ \_/_\_/>    The coolest Interface to (Sub)Version Control
    /_/   \_\     http://tortoisesvn.net