> -----Original Message-----
> From: Stefan Sperling [mailto:stsp_at_elego.de]
> Sent: dinsdag 29 november 2011 8:04
> To: Hyrum K Wright
> Cc: Philip Martin; dev_at_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
> > <philip.martin_at_wandisco.com> wrote:
> > > stsp_at_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;
> }
>
Received on 2011-11-29 09:25:21 CET