You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2011/11/29 01:58:49 UTC
Re: svn commit: r1207663 - in /subversion/trunk/subversion: libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
stsp@apache.org writes:
> Author: stsp
> Date: Mon Nov 28 22:34:49 2011
> New Revision: 1207663
> +def patch_target_no_eol_at_eof(sbox):
> + "patch target with no eol at eof"
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + patch_file_path = make_patch_path(sbox)
> + iota_path = os.path.join(wc_dir, 'iota')
> +
> + iota_contents = [
> + "This is the file iota."
> + ]
> +
> + svntest.main.file_write(iota_path, ''.join(iota_contents))
> + expected_output = svntest.wc.State(wc_dir, {
> + 'iota' : Item(verb='Sending'),
> + })
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.tweak('iota', wc_rev=2)
> + svntest.actions.run_and_verify_commit(wc_dir, expected_output,
> + expected_status, None, wc_dir)
> + unidiff_patch = [
> + "--- iota\t(revision 1)\n",
> + "+++ iota\t(working copy)\n",
> + "@@ -1,7 +1,7 @@\n",
> + "-This is the file iota.\n"
> + "\\ No newline at end of file\n",
> + "+It is the file 'iota'.\n",
> + "\\ No newline at end of file\n",
> + ]
> +
> + svntest.main.file_write(patch_file_path, ''.join(unidiff_patch))
> +
> + iota_contents = [
> + "It is the file 'iota'."
> + ]
> + expected_output = [
> + 'U %s\n' % os.path.join(wc_dir, 'iota'),
> + ]
> +
> + expected_disk = svntest.main.greek_state.copy()
> + expected_disk.tweak('iota', contents=''.join(iota_contents))
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.tweak('iota', status='M ', wc_rev=2)
> +
> + expected_skip = wc.State('', { })
> +
> + svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
> + expected_output,
> + expected_disk,
> + expected_status,
> + expected_skip,
> + None, # expected err
> + 1, # check-props
> + 1) # dry-run
This test failed on the buildbot: 'iota' not showing status 'M'. I
believe this is because there is no sleep between the commit allowing
the patching to be fast enough that the file's timestamp doesn't change
and the file shows as unmodified. I can demonstrate that it is a
timestamp problem using:
Index: subversion/tests/cmdline/patch_tests.py
===================================================================
--- subversion/tests/cmdline/patch_tests.py (revision 1207719)
+++ subversion/tests/cmdline/patch_tests.py (working copy)
@@ -34,6 +34,7 @@
import textwrap
import zlib
import posixpath
+import time
# Our testing module
import svntest
@@ -4036,6 +4037,10 @@
expected_skip = wc.State('', { })
+ svntest.actions.run_and_verify_svn(None, None, [], 'up', '-r', '1', wc_dir)
+ svntest.actions.run_and_verify_svn(None, None, [], 'up', sbox.ospath('iota'))
+ time.sleep(0.1)
+
svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
expected_output,
expected_disk,
We need to sleep whenever a test relies on timestamp changes and in this
test there are two places: between the initial checkout and the
file_write, and between the commit and the patch.
Looking at the rest of the file I don't see do_sleep_for_timestamps or
explicit sleeps in any of the tests. I suspect some of the others are
vulnerable to similar races.
--
Philip
Re: svn commit: r1207663 - in /subversion/trunk/subversion: libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
>> > It might also be possible to write an annotation for the test that
>> > configures the environment to enable the sleep within the libraries
>> > for that particular test. It would be a bit less granular (on the
>> > order of a test, not a specific command), but would be easier to
>> > maintain once the initial plumbing is in place.
Something like this would be good. Note that we don't want to change
SVN_I_LOVE_CORRUPTED_WORKING_COPIES_SO_DISABLE_SLEEP_FOR_TIMESTAMPS for
the python process as this affects all the tests running in parallel.
What we really want is a way to control the python process to clear the
env var for a single svn invocation.
Note that at present some tests do use do_sleep_for_timestamps
explicitly and this has the side-effect of changing the environment for
all parallell tests. The tests that use do_sleep_for_timestamps don't
use no_sleep_for_timestamps because it is not possible for a test to do
that without potentially breaking a parallel test.
>> Index: subversion/tests/cmdline/svntest/actions.py
>> ==========================================================
>> =========
>> --- subversion/tests/cmdline/svntest/actions.py (revision 1207768)
>> +++ subversion/tests/cmdline/svntest/actions.py (working copy)
>> @@ -1180,7 +1180,9 @@ def run_and_verify_patch(dir, patch_path,
>>
>> # Update and make a tree of the output.
>> patch_command = patch_command + args
>> + do_sleep_for_timestamps()
>> exit_code, out, err = main.run_svn(True, *patch_command)
>> + no_sleep_for_timestamps()
No, you can't use no_sleep_for_timestamps if tests are running in
parallel.
>> Index: subversion/libsvn_client/patch.c
>> ==========================================================
>> =========
>> --- subversion/libsvn_client/patch.c (revision 1207768)
>> +++ subversion/libsvn_client/patch.c (working copy)
>> @@ -2926,6 +2926,8 @@ apply_patches(/* The path to the patch file. */
>> SVN_ERR(svn_diff_close_patch_file(patch_file, iterpool));
>> svn_pool_destroy(iterpool);
>>
>> + svn_io_sleep_for_timestamps(abs_wc_path, scratch_pool);
>> +
>
> It's not the patch code that causes the problem. The operation before the
> patch should have waited until it is safe to exit.
Exactly.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
RE: svn commit: r1207663 - in /subversion/trunk/subversion: libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: dinsdag 29 november 2011 8:04
> To: Hyrum K Wright
> Cc: Philip Martin; dev@subversion.apache.org
> Subject: Re: svn commit: r1207663 - in /subversion/trunk/subversion:
> libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
>
> On Mon, Nov 28, 2011 at 10:12:38PM -0600, Hyrum K Wright wrote:
> > On Mon, Nov 28, 2011 at 6:58 PM, Philip Martin
> > <ph...@wandisco.com> wrote:
> > > stsp@apache.org writes:
> > >
> > >> Author: stsp
> > >> Date: Mon Nov 28 22:34:49 2011
> > >> New Revision: 1207663
> > >
> > >> +def patch_target_no_eol_at_eof(sbox):
> > >> + "patch target with no eol at eof"
> > >> +
> > >> + sbox.build()
> > >> + wc_dir = sbox.wc_dir
> > >> +
> > >> + patch_file_path = make_patch_path(sbox)
> > >> + iota_path = os.path.join(wc_dir, 'iota')
> > >> +
> > >> + iota_contents = [
> > >> + "This is the file iota."
> > >> + ]
> > >> +
> > >> + svntest.main.file_write(iota_path, ''.join(iota_contents))
> > >> + expected_output = svntest.wc.State(wc_dir, {
> > >> + 'iota' : Item(verb='Sending'),
> > >> + })
> > >> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> > >> + expected_status.tweak('iota', wc_rev=2)
> > >> + svntest.actions.run_and_verify_commit(wc_dir, expected_output,
> > >> + expected_status, None,
wc_dir)
> > >> + unidiff_patch = [
> > >> + "--- iota\t(revision 1)\n",
> > >> + "+++ iota\t(working copy)\n",
> > >> + "@@ -1,7 +1,7 @@\n",
> > >> + "-This is the file iota.\n"
> > >> + "\\ No newline at end of file\n",
> > >> + "+It is the file 'iota'.\n",
> > >> + "\\ No newline at end of file\n",
> > >> + ]
> > >> +
> > >> + svntest.main.file_write(patch_file_path, ''.join(unidiff_patch))
> > >> +
> > >> + iota_contents = [
> > >> + "It is the file 'iota'."
> > >> + ]
> > >> + expected_output = [
> > >> + 'U %s\n' % os.path.join(wc_dir, 'iota'),
> > >> + ]
> > >> +
> > >> + expected_disk = svntest.main.greek_state.copy()
> > >> + expected_disk.tweak('iota', contents=''.join(iota_contents))
> > >> +
> > >> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> > >> + expected_status.tweak('iota', status='M ', wc_rev=2)
> > >> +
> > >> + expected_skip = wc.State('', { })
> > >> +
> > >> + svntest.actions.run_and_verify_patch(wc_dir,
> os.path.abspath(patch_file_path),
> > >> + expected_output,
> > >> + expected_disk,
> > >> + expected_status,
> > >> + expected_skip,
> > >> + None, # expected err
> > >> + 1, # check-props
> > >> + 1) # dry-run
> > >
> > > This test failed on the buildbot: 'iota' not showing status 'M'. I
> > > believe this is because there is no sleep between the commit allowing
> > > the patching to be fast enough that the file's timestamp doesn't
change
> > > and the file shows as unmodified. I can demonstrate that it is a
> > > timestamp problem using:
> > >
> > > Index: subversion/tests/cmdline/patch_tests.py
> > >
> ==========================================================
> =========
> > > --- subversion/tests/cmdline/patch_tests.py (revision 1207719)
> > > +++ subversion/tests/cmdline/patch_tests.py (working copy)
> > > @@ -34,6 +34,7 @@
> > > import textwrap
> > > import zlib
> > > import posixpath
> > > +import time
> > >
> > > # Our testing module
> > > import svntest
> > > @@ -4036,6 +4037,10 @@
> > >
> > > expected_skip = wc.State('', { })
> > >
> > > + svntest.actions.run_and_verify_svn(None, None, [], 'up', '-r', '1',
> wc_dir)
> > > + svntest.actions.run_and_verify_svn(None, None, [], 'up',
> sbox.ospath('iota'))
> > > + time.sleep(0.1)
> > > +
> > > svntest.actions.run_and_verify_patch(wc_dir,
> os.path.abspath(patch_file_path),
> > > expected_output,
> > > expected_disk,
> > >
> > > We need to sleep whenever a test relies on timestamp changes and in
> this
> > > test there are two places: between the initial checkout and the
> > > file_write, and between the commit and the patch.
> > >
> > > Looking at the rest of the file I don't see do_sleep_for_timestamps or
> > > explicit sleeps in any of the tests. I suspect some of the others are
> > > vulnerable to similar races.
> >
> > It might also be possible to write an annotation for the test that
> > configures the environment to enable the sleep within the libraries
> > for that particular test. It would be a bit less granular (on the
> > order of a test, not a specific command), but would be easier to
> > maintain once the initial plumbing is in place.
>
> That is a good idea.
>
> In the meantime, maybe something like below will help?
> Note that 'svn patch' doesn't sleep for timestamps at the moment
> so the alternative would be adding calls to sleep() to the tests
> which isn't all that pretty.
>
> [[[
> * subversion/libsvn_client/patch.c
> (apply_patches): Sleep for timestamps after applying the patch.
>
> * subversion/tests/cmdline/svntest/actions.py
> (run_and_verify_patch): Enable sleep for timestamps during 'svn patch'
> invocation.
> ]]]
>
> Index: subversion/tests/cmdline/svntest/actions.py
> ==========================================================
> =========
> --- subversion/tests/cmdline/svntest/actions.py (revision 1207768)
> +++ subversion/tests/cmdline/svntest/actions.py (working copy)
> @@ -1180,7 +1180,9 @@ def run_and_verify_patch(dir, patch_path,
>
> # Update and make a tree of the output.
> patch_command = patch_command + args
> + do_sleep_for_timestamps()
> exit_code, out, err = main.run_svn(True, *patch_command)
> + no_sleep_for_timestamps()
>
> if error_re_string:
> rm = re.compile(error_re_string)
> Index: subversion/libsvn_client/patch.c
> ==========================================================
> =========
> --- subversion/libsvn_client/patch.c (revision 1207768)
> +++ subversion/libsvn_client/patch.c (working copy)
> @@ -2926,6 +2926,8 @@ apply_patches(/* The path to the patch file. */
> SVN_ERR(svn_diff_close_patch_file(patch_file, iterpool));
> svn_pool_destroy(iterpool);
>
> + svn_io_sleep_for_timestamps(abs_wc_path, scratch_pool);
> +
It's not the patch code that causes the problem. The operation before the
patch should have waited until it is safe to exit.
This patch doesn't fix the problem as it should have waited earlier.
Probably at a checkout or update...
Technically it is just relevant when some code records the timestamps in the
recorded_* columns of NODES.
Most of the tests in our test suite make sure the size of the file changes
in cases like this. The size change then triggers the proper compare that in
normal use can also be triggered by just the time change.
Bert
> return SVN_NO_ERROR;
> }
>
Re: svn commit: r1207663 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Nov 28, 2011 at 10:12:38PM -0600, Hyrum K Wright wrote:
> On Mon, Nov 28, 2011 at 6:58 PM, Philip Martin
> <ph...@wandisco.com> wrote:
> > stsp@apache.org writes:
> >
> >> Author: stsp
> >> Date: Mon Nov 28 22:34:49 2011
> >> New Revision: 1207663
> >
> >> +def patch_target_no_eol_at_eof(sbox):
> >> + "patch target with no eol at eof"
> >> +
> >> + sbox.build()
> >> + wc_dir = sbox.wc_dir
> >> +
> >> + patch_file_path = make_patch_path(sbox)
> >> + iota_path = os.path.join(wc_dir, 'iota')
> >> +
> >> + iota_contents = [
> >> + "This is the file iota."
> >> + ]
> >> +
> >> + svntest.main.file_write(iota_path, ''.join(iota_contents))
> >> + expected_output = svntest.wc.State(wc_dir, {
> >> + 'iota' : Item(verb='Sending'),
> >> + })
> >> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> >> + expected_status.tweak('iota', wc_rev=2)
> >> + svntest.actions.run_and_verify_commit(wc_dir, expected_output,
> >> + expected_status, None, wc_dir)
> >> + unidiff_patch = [
> >> + "--- iota\t(revision 1)\n",
> >> + "+++ iota\t(working copy)\n",
> >> + "@@ -1,7 +1,7 @@\n",
> >> + "-This is the file iota.\n"
> >> + "\\ No newline at end of file\n",
> >> + "+It is the file 'iota'.\n",
> >> + "\\ No newline at end of file\n",
> >> + ]
> >> +
> >> + svntest.main.file_write(patch_file_path, ''.join(unidiff_patch))
> >> +
> >> + iota_contents = [
> >> + "It is the file 'iota'."
> >> + ]
> >> + expected_output = [
> >> + 'U %s\n' % os.path.join(wc_dir, 'iota'),
> >> + ]
> >> +
> >> + expected_disk = svntest.main.greek_state.copy()
> >> + expected_disk.tweak('iota', contents=''.join(iota_contents))
> >> +
> >> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> >> + expected_status.tweak('iota', status='M ', wc_rev=2)
> >> +
> >> + expected_skip = wc.State('', { })
> >> +
> >> + svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
> >> + expected_output,
> >> + expected_disk,
> >> + expected_status,
> >> + expected_skip,
> >> + None, # expected err
> >> + 1, # check-props
> >> + 1) # dry-run
> >
> > This test failed on the buildbot: 'iota' not showing status 'M'. I
> > believe this is because there is no sleep between the commit allowing
> > the patching to be fast enough that the file's timestamp doesn't change
> > and the file shows as unmodified. I can demonstrate that it is a
> > timestamp problem using:
> >
> > Index: subversion/tests/cmdline/patch_tests.py
> > ===================================================================
> > --- subversion/tests/cmdline/patch_tests.py (revision 1207719)
> > +++ subversion/tests/cmdline/patch_tests.py (working copy)
> > @@ -34,6 +34,7 @@
> > import textwrap
> > import zlib
> > import posixpath
> > +import time
> >
> > # Our testing module
> > import svntest
> > @@ -4036,6 +4037,10 @@
> >
> > expected_skip = wc.State('', { })
> >
> > + svntest.actions.run_and_verify_svn(None, None, [], 'up', '-r', '1', wc_dir)
> > + svntest.actions.run_and_verify_svn(None, None, [], 'up', sbox.ospath('iota'))
> > + time.sleep(0.1)
> > +
> > svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
> > expected_output,
> > expected_disk,
> >
> > We need to sleep whenever a test relies on timestamp changes and in this
> > test there are two places: between the initial checkout and the
> > file_write, and between the commit and the patch.
> >
> > Looking at the rest of the file I don't see do_sleep_for_timestamps or
> > explicit sleeps in any of the tests. I suspect some of the others are
> > vulnerable to similar races.
>
> It might also be possible to write an annotation for the test that
> configures the environment to enable the sleep within the libraries
> for that particular test. It would be a bit less granular (on the
> order of a test, not a specific command), but would be easier to
> maintain once the initial plumbing is in place.
That is a good idea.
In the meantime, maybe something like below will help?
Note that 'svn patch' doesn't sleep for timestamps at the moment
so the alternative would be adding calls to sleep() to the tests
which isn't all that pretty.
[[[
* subversion/libsvn_client/patch.c
(apply_patches): Sleep for timestamps after applying the patch.
* subversion/tests/cmdline/svntest/actions.py
(run_and_verify_patch): Enable sleep for timestamps during 'svn patch'
invocation.
]]]
Index: subversion/tests/cmdline/svntest/actions.py
===================================================================
--- subversion/tests/cmdline/svntest/actions.py (revision 1207768)
+++ subversion/tests/cmdline/svntest/actions.py (working copy)
@@ -1180,7 +1180,9 @@ def run_and_verify_patch(dir, patch_path,
# Update and make a tree of the output.
patch_command = patch_command + args
+ do_sleep_for_timestamps()
exit_code, out, err = main.run_svn(True, *patch_command)
+ no_sleep_for_timestamps()
if error_re_string:
rm = re.compile(error_re_string)
Index: subversion/libsvn_client/patch.c
===================================================================
--- subversion/libsvn_client/patch.c (revision 1207768)
+++ subversion/libsvn_client/patch.c (working copy)
@@ -2926,6 +2926,8 @@ apply_patches(/* The path to the patch file. */
SVN_ERR(svn_diff_close_patch_file(patch_file, iterpool));
svn_pool_destroy(iterpool);
+ svn_io_sleep_for_timestamps(abs_wc_path, scratch_pool);
+
return SVN_NO_ERROR;
}
Re: svn commit: r1207663 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Mon, Nov 28, 2011 at 6:58 PM, Philip Martin
<ph...@wandisco.com> wrote:
> stsp@apache.org writes:
>
>> Author: stsp
>> Date: Mon Nov 28 22:34:49 2011
>> New Revision: 1207663
>
>> +def patch_target_no_eol_at_eof(sbox):
>> + "patch target with no eol at eof"
>> +
>> + sbox.build()
>> + wc_dir = sbox.wc_dir
>> +
>> + patch_file_path = make_patch_path(sbox)
>> + iota_path = os.path.join(wc_dir, 'iota')
>> +
>> + iota_contents = [
>> + "This is the file iota."
>> + ]
>> +
>> + svntest.main.file_write(iota_path, ''.join(iota_contents))
>> + expected_output = svntest.wc.State(wc_dir, {
>> + 'iota' : Item(verb='Sending'),
>> + })
>> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
>> + expected_status.tweak('iota', wc_rev=2)
>> + svntest.actions.run_and_verify_commit(wc_dir, expected_output,
>> + expected_status, None, wc_dir)
>> + unidiff_patch = [
>> + "--- iota\t(revision 1)\n",
>> + "+++ iota\t(working copy)\n",
>> + "@@ -1,7 +1,7 @@\n",
>> + "-This is the file iota.\n"
>> + "\\ No newline at end of file\n",
>> + "+It is the file 'iota'.\n",
>> + "\\ No newline at end of file\n",
>> + ]
>> +
>> + svntest.main.file_write(patch_file_path, ''.join(unidiff_patch))
>> +
>> + iota_contents = [
>> + "It is the file 'iota'."
>> + ]
>> + expected_output = [
>> + 'U %s\n' % os.path.join(wc_dir, 'iota'),
>> + ]
>> +
>> + expected_disk = svntest.main.greek_state.copy()
>> + expected_disk.tweak('iota', contents=''.join(iota_contents))
>> +
>> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
>> + expected_status.tweak('iota', status='M ', wc_rev=2)
>> +
>> + expected_skip = wc.State('', { })
>> +
>> + svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
>> + expected_output,
>> + expected_disk,
>> + expected_status,
>> + expected_skip,
>> + None, # expected err
>> + 1, # check-props
>> + 1) # dry-run
>
> This test failed on the buildbot: 'iota' not showing status 'M'. I
> believe this is because there is no sleep between the commit allowing
> the patching to be fast enough that the file's timestamp doesn't change
> and the file shows as unmodified. I can demonstrate that it is a
> timestamp problem using:
>
> Index: subversion/tests/cmdline/patch_tests.py
> ===================================================================
> --- subversion/tests/cmdline/patch_tests.py (revision 1207719)
> +++ subversion/tests/cmdline/patch_tests.py (working copy)
> @@ -34,6 +34,7 @@
> import textwrap
> import zlib
> import posixpath
> +import time
>
> # Our testing module
> import svntest
> @@ -4036,6 +4037,10 @@
>
> expected_skip = wc.State('', { })
>
> + svntest.actions.run_and_verify_svn(None, None, [], 'up', '-r', '1', wc_dir)
> + svntest.actions.run_and_verify_svn(None, None, [], 'up', sbox.ospath('iota'))
> + time.sleep(0.1)
> +
> svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
> expected_output,
> expected_disk,
>
> We need to sleep whenever a test relies on timestamp changes and in this
> test there are two places: between the initial checkout and the
> file_write, and between the commit and the patch.
>
> Looking at the rest of the file I don't see do_sleep_for_timestamps or
> explicit sleeps in any of the tests. I suspect some of the others are
> vulnerable to similar races.
It might also be possible to write an annotation for the test that
configures the environment to enable the sleep within the libraries
for that particular test. It would be a bit less granular (on the
order of a test, not a specific command), but would be easier to
maintain once the initial plumbing is in place.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/