You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Küng <to...@gmail.com> on 2012/01/12 19:55:10 UTC
[patch] applying patch can fail with dry_run
Hi,
When applying a patch in dry-run mode, svn can return an error even if
the patching would work just fine. The returned error is then:
The Node ..../newDirectory was not found
If you want to reproduce this, here's the mailing list thread on the
TSVN mailing list:
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=4061&dsMessageId=2899409
(steps to reproduce at the bottom, with the patch files attached to that
thread).
Attached here is my patch which would fix this issue: when running in
dry_run mode, the function delete_empty_dirs() isn't executed.
I'm not completely sure but I think that function is only necessary when
the patch is actually applied.
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.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [patch] applying patch can fail with dry_run
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Küng <to...@gmail.com> writes:
> I'm not sure how this is triggered exactly. If you check the mailing
> list thread on the TSVN mailing list:
> http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=4061&dsMessageId=2899409
> you can see that both the working copy and the patch to reproduce the
> issue are quite big.
I was hoping you had reduced it to a small test case.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: [patch] applying patch can fail with dry_run
Posted by Stefan Küng <to...@gmail.com>.
On 16.01.2012 11:34, Philip Martin wrote:
> Philip Martin<ph...@wandisco.com> writes:
>
>> Now I can reproduce the error.
>
> So the problem occurs when we have a patch file with one item that adds
> two levels of directories and another item that deletes a file:
>
> Index: wc/A/B/f
> ===================================================================
> --- wc/A/B/f (revision 0)
> +++ wc/A/B/f (working copy)
> @@ -0,0 +1 @@
> +foo
> Index: wc/f
> ===================================================================
> --- wc/f (revision 1)
> +++ wc/f (working copy)
> @@ -1 +0,0 @@
> -5
>
> $ svn patch --dry-run --strip 1 x.x wc
> A wc/A
> A wc/A/B
> A wc/A/B/f
> D wc/f
> svn: E155010: The node '/home/pm/sw/subversion/obj/wc/A' was not found.
>
>
> The following appears to work:
>
> Index: subversion/libsvn_client/patch.c
> ===================================================================
> --- subversion/libsvn_client/patch.c (revision 1231875)
> +++ subversion/libsvn_client/patch.c (working copy)
> @@ -2718,7 +2718,7 @@
> empty_dirs = apr_hash_make(scratch_pool);
> non_empty_dirs = apr_hash_make(scratch_pool);
> iterpool = svn_pool_create(scratch_pool);
> - for (i = 0; i< targets_info->nelts; i++)
> + for (i = 0; i< deleted_targets->nelts; i++)
> {
> svn_boolean_t parent_empty;
> patch_target_info_t *target_info;
> @@ -2729,7 +2729,7 @@
> if (ctx->cancel_func)
> SVN_ERR(ctx->cancel_func(ctx->cancel_baton));
>
> - target_info = APR_ARRAY_IDX(targets_info, i, patch_target_info_t *);
> + target_info = APR_ARRAY_IDX(deleted_targets, i, patch_target_info_t *);
>
> parent = svn_dirent_dirname(target_info->local_abspath, iterpool);
>
Confirmed. This works.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [patch] applying patch can fail with dry_run
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Now I can reproduce the error.
So the problem occurs when we have a patch file with one item that adds
two levels of directories and another item that deletes a file:
Index: wc/A/B/f
===================================================================
--- wc/A/B/f (revision 0)
+++ wc/A/B/f (working copy)
@@ -0,0 +1 @@
+foo
Index: wc/f
===================================================================
--- wc/f (revision 1)
+++ wc/f (working copy)
@@ -1 +0,0 @@
-5
$ svn patch --dry-run --strip 1 x.x wc
A wc/A
A wc/A/B
A wc/A/B/f
D wc/f
svn: E155010: The node '/home/pm/sw/subversion/obj/wc/A' was not found.
The following appears to work:
Index: subversion/libsvn_client/patch.c
===================================================================
--- subversion/libsvn_client/patch.c (revision 1231875)
+++ subversion/libsvn_client/patch.c (working copy)
@@ -2718,7 +2718,7 @@
empty_dirs = apr_hash_make(scratch_pool);
non_empty_dirs = apr_hash_make(scratch_pool);
iterpool = svn_pool_create(scratch_pool);
- for (i = 0; i < targets_info->nelts; i++)
+ for (i = 0; i < deleted_targets->nelts; i++)
{
svn_boolean_t parent_empty;
patch_target_info_t *target_info;
@@ -2729,7 +2729,7 @@
if (ctx->cancel_func)
SVN_ERR(ctx->cancel_func(ctx->cancel_baton));
- target_info = APR_ARRAY_IDX(targets_info, i, patch_target_info_t *);
+ target_info = APR_ARRAY_IDX(deleted_targets, i, patch_target_info_t *);
parent = svn_dirent_dirname(target_info->local_abspath, iterpool);
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: [patch] applying patch can fail with dry_run
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Küng <to...@gmail.com> writes:
> On 12.01.2012 22:34, Philip Martin wrote:
>> Stefan Küng<to...@gmail.com> writes:
>>
>>> $ svn co https://triplea.svn.sourceforge.net/svnroot/triplea/trunk wc -r3375
>>> $ svn patch test.patch wc --dry-run
>>
>> No error on my machine, it would simply create wc/src, and does without
>> --dry-run.
>>
>
> why would a dry run create the folder? a dry-run must not change anything.
The --dry-run merge shows that it would create a directory src. If I
remove --dry-run it does create the directory. No errors.
> when I run
> svn patch --dry-run test.patch triplea
> I get the attached output.
>
> As you can see, the last line is the returned error I mentioned before.
> Using an svn client built from the latest 1.7.x branch.
So your output starts
A triplea\src\games\strategy\engine\framework\startup\ui\editors
While mine starts
A wc/src
A wc/src/games
A wc/src/games/strategy
A wc/src/games/strategy/engine
A wc/src/games/strategy/engine/framework
A wc/src/games/strategy/engine/framework/startup
A wc/src/games/strategy/engine/framework/startup/ui
A wc/src/games/strategy/engine/framework/startup/ui/editors
>From which I deduce that the checkout/patch instructions were wrong. I
need to apply the patch to:
https://triplea.svn.sourceforge.net/svnroot/triplea/trunk/triplea
not:
https://triplea.svn.sourceforge.net/svnroot/triplea/trunk
Now I can reproduce the error.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: [patch] applying patch can fail with dry_run
Posted by Stefan Küng <to...@gmail.com>.
On 12.01.2012 22:34, Philip Martin wrote:
> Stefan Küng<to...@gmail.com> writes:
>
>> $ svn co https://triplea.svn.sourceforge.net/svnroot/triplea/trunk wc -r3375
>> $ svn patch test.patch wc --dry-run
>
> No error on my machine, it would simply create wc/src, and does without
> --dry-run.
>
why would a dry run create the folder? a dry-run must not change anything.
when I run
svn patch --dry-run test.patch triplea
I get the attached output.
As you can see, the last line is the returned error I mentioned before.
Using an svn client built from the latest 1.7.x branch.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [patch] applying patch can fail with dry_run
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Küng <to...@gmail.com> writes:
> $ svn co https://triplea.svn.sourceforge.net/svnroot/triplea/trunk wc -r3375
> $ svn patch test.patch wc --dry-run
No error on my machine, it would simply create wc/src, and does without
--dry-run.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: [patch] applying patch can fail with dry_run
Posted by Stefan Küng <to...@gmail.com>.
On 12.01.2012 21:13, Philip Martin wrote:
> Stefan Küng<to...@gmail.com> writes:
>
>> 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).
>
> I'm not clear what problem the patch is triggering. I assume the patch
> deletes one or more files but then what happens?
>
> At present a --dry-run patch that deletes a file will track the
> deletions and notify about a directory that would be empty and deleted,
> we don't want to lose that feature.
I'm not sure how this is triggered exactly. If you check the mailing
list thread on the TSVN mailing list:
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=4061&dsMessageId=2899409
you can see that both the working copy and the patch to reproduce the
issue are quite big.
From what I could find out I think the problem is when the patch
removes all versioned files in a folder, but adds new files in that same
folder.
In a real run, the file is added to the working copy and the folder is
not empty anymore. In a dry-run however, the folder is marked as
removed, but then the still unversioned file is still there and that's
when the error
The Node ..../newDirectory was not found
is thrown.
To reproduce:
download the file
http://tortoisesvn.tigris.org/ds/getDSMessageAttachment.do/Updating_PBEM_system.patch?dsForumId=4061&dsMessageId=2899409&dsAttachmentId=3443155&dsAttachmentMime=application/octet-stream
(save as test.patch)
$ svn co https://triplea.svn.sourceforge.net/svnroot/triplea/trunk wc -r3375
$ svn patch test.patch wc --dry-run
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [patch] applying patch can fail with dry_run
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Küng <to...@gmail.com> writes:
> 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).
I'm not clear what problem the patch is triggering. I assume the patch
deletes one or more files but then what happens?
At present a --dry-run patch that deletes a file will track the
deletions and notify about a directory that would be empty and deleted,
we don't want to lose that feature.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: [patch] applying patch can fail with dry_run
Posted by Stefan Küng <to...@gmail.com>.
On 12.01.2012 20:07, Philip Martin wrote:
> Stefan Küng<to...@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
Re: [patch] applying patch can fail with dry_run
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Küng <to...@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.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com