You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by cm...@collab.net on 2002/01/21 14:27:25 UTC
[Vladimir Prus ] Re: svn diff segfault
Sheesh, I almost lost this mail. Vladimir, please be sure in the
future to send patches to the whole dev-list.
------- Start of forwarded message -------
Content-Type: text/plain;
charset="koi8-r"
From: Vladimir Prus <gh...@cs.msu.su>
To: cmpilato@collab.net
Subject: Re: svn diff segfault
Date: Fri, 18 Jan 2002 13:13:41 +0300
References: <E1...@zigzag.cs.msu.su> <x7...@pascal.ch.collab.net>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Message-Id: <E1...@zigzag.cs.msu.su>
cmpilato@collab.net wrote:
> Technically, we don't
> require tests to accompany a patch, but I can't think of a better way
> to impress a development community than to say, "Here's a bug fix...and
> here's my proof!"
I'm of the same mind.
Updated patch follows:
Log message:
The 'diff' command no longer crashes when two revisions of a file differ in
properties but not in text.
* subversion/libsvn_client/repos_diff.c
(close_file): Check for presense of any change to report was added.
* subversion/tests/clients/cmdline/diff_tests.py
(diff_only_property_change): New function.
Patch:
Index: ./subversion/libsvn_client/repos_diff.c
===================================================================
--- ./subversion/libsvn_client/repos_diff.c
+++ ./subversion/libsvn_client/repos_diff.c Fri Jan 18 13:00:08 2002
@@ -568,7 +568,10 @@
{
struct file_baton *b = file_baton;
- SVN_ERR (run_diff_cmd (b));
+ /* It is possible to have only change to properties, without texdelta
+ In this case no difference should be reported and no need to run diff.
*/
+ if (b->path_end_revision)
+ SVN_ERR (run_diff_cmd (b));
svn_pool_destroy (b->pool);
Index: ./subversion/tests/clients/cmdline/diff_tests.py
===================================================================
--- ./subversion/tests/clients/cmdline/diff_tests.py
+++ ./subversion/tests/clients/cmdline/diff_tests.py Fri Jan 18 12:55:46 2002
@@ -547,6 +547,32 @@
return 0
+def diff_only_property_change(sbox):
+ "diff when property was changed but text was not"
+
+ if sbox.build():
+ return 1
+
+ wc_dir = sbox.wc_dir
+
+ current_dir = os.getcwd();
+ os.chdir(sbox.wc_dir);
+
+ svntest.main.run_svn(None, 'propset', 'svn:eol-style', 'none', "iota")
+ svntest.main.run_svn(None, 'ci')
+
+ result = 0
+
+ if os.system(svntest.main.svn_binary + " diff -r1:2"):
+ result = 1
+
+ if not result and os.system(svntest.main.svn_binary + " diff -r2:1"):
+ result = 1
+
+ os.chdir(current_dir)
+ return result
+
+
########################################################################
# Run the tests
@@ -562,6 +588,7 @@
diff_non_recursive,
diff_repo_subset,
diff_non_version_controlled_file,
+ diff_only_property_change,
]
if __name__ == '__main__':
Note: svntest.main.run_svn provides to way to detect segfault. I've found
nothing appropriate, so have used plain os.system.
Regards,
Vladimir
------- End of forwarded message -------
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: POpen3/POpen4 on Win32
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Jan 22, 2002 at 10:56:02AM -0600, cmpilato@collab.net wrote:
> "Bill Tutt" <ra...@lyra.org> writes:
>...
> > Do our test cases need poll() on POpen3/POpen4, or is just retrieving
> > the exit code good enough? If we need something more flexible like
> > POpen3/POpen4, maybe I can code up a Python version of the code that
> > does the required footwork on Win32.
>
> My only real concern is SEGFAULT detection. To that extent, whatever
> works. Subversion doesn't make a habit of silently failing about much
> of anything, so we can always check stderr for non-fatal failures.
Read the docco for waitpid(). You can get back information on whether the
program exited normally or not. That should be what you're looking for.
Python has the function for this, such as os.WIFEXITED(status)
Regarding pipes and popen2.py and whatnot... we can definitely fix up the
run_svn() command to Do The Right Thing.
On Windows, there is a "win32popenmodule" (or somesuch name). It provides
much of the same pipe semantics, so we could use that to spawn/run an SVN
client.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: POpen3/POpen4 on Win32
Posted by cm...@collab.net.
"Bill Tutt" <ra...@lyra.org> writes:
> Well, yes that's true with the popen2 module and the Popen3 and Popen4
> classes, but close() on the file object returned from the Win32
> os.popen*() variants should always return the exit code of the child
> process.
Hmm...that's a good thought.
> Do our test cases need poll() on POpen3/POpen4, or is just retrieving
> the exit code good enough? If we need something more flexible like
> POpen3/POpen4, maybe I can code up a Python version of the code that
> does the required footwork on Win32.
My only real concern is SEGFAULT detection. To that extent, whatever
works. Subversion doesn't make a habit of silently failing about much
of anything, so we can always check stderr for non-fatal failures.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: POpen3/POpen4 on Win32
Posted by Bill Tutt <ra...@lyra.org>.
> From: cmpilato@collab.net [mailto:cmpilato@collab.net]
>
> Hrm...bother. For some reason I seem to recall that our Python tests
> won't run under Windows right now anyway, but certainly we should do
> all we can to enforce that brokenness.
>
> And here's a quote from the Python online docs to support your claim:
>
> The only way to control the input and output streams and also
> retrieve the return codes is to use the Popen3 and Popen4 classes
> from the popen2 module; there are only available on Unix.
>
Well, yes that's true with the popen2 module and the Popen3 and Popen4
classes, but close() on the file object returned from the Win32
os.popen*() variants should always return the exit code of the child
process.
The docs appear to be incorrect.
(http://cvs.sourceforge.net/cgi-bin/viewcvs.cgi/python/python/dist/src/M
odules/posixmodule.c?rev=HEAD&content-type=text/vnd.viewcvs-markup)
Additionally, I'm not quite sure why nobody tried to get the Popen3 and
Popen4 classes working with Win32, it's not like it's all that hard to
modify the Win32 popen code in Python in order to provide a suitable C
extension interface to provide the appropriate behavior. (and that's not
just because I wrote the code it's derived from)
Do our test cases need poll() on POpen3/POpen4, or is just retrieving
the exit code good enough? If we need something more flexible like
POpen3/POpen4, maybe I can code up a Python version of the code that
does the required footwork on Win32.
Bill
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn diff segfault
Posted by cm...@collab.net.
Vladimir Prus <gh...@cs.msu.su> writes:
> cmpilato@collab.net wrote:
>
> > Well, I was about to freak out reading this portion of the patch until
> > I saw you comment here. Whew! So, you've hit on something that has
> > been bothering me for some time about our Python tests -- we don't
> > detect segfaults. How hard would it be to adjust svntest.main.run_svn()
> > so that it *could* return an error if a segfault occured (perhaps a -1
> > instead of a 1, something) ? My feeling is that the Right Way to do
> > this is to first fix the broken run_svn() function, then use it. That
> > way you essentially get credit for fixing the entire Python test suite,
> > and we don't have to revisit this again. :-)
>
> I don't think we can do it in a completely portable fashion. AFAIK,
> os.popen2/os.popen3 give no way to find program's return status, and the
> Popen3 class is not available on windows. Would it be ok to use Popen3 on
> Unix, and retain the current behaviour on win?
Hrm...bother. For some reason I seem to recall that our Python tests
won't run under Windows right now anyway, but certainly we should do
all we can to enforce that brokenness.
And here's a quote from the Python online docs to support your claim:
The only way to control the input and output streams and also
retrieve the return codes is to use the Popen3 and Popen4 classes
from the popen2 module; there are only available on Unix.
And let me guess... os.system will only give you the return value,
right? (reading, reading, reading) Yep. Oh, and the return value for
that is system-dependent. How...portable. Thanks, Python.
Well, I dunno what to do, then. We can't just look for 'core' files
because your environment variables can toggle the creation (and name,
right?) of those core files, if memory serves correctly.
Any suggestions, list-folk?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn diff segfault
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On Wed, Jan 23, 2002 at 06:28:29PM +0300, Vladimir Prus wrote:
> cmpilato@collab.net wrote:
>
> > Well, I was about to freak out reading this portion of the patch until
> > I saw you comment here. Whew! So, you've hit on something that has
> > been bothering me for some time about our Python tests -- we don't
> > detect segfaults. How hard would it be to adjust svntest.main.run_svn()
> > so that it *could* return an error if a segfault occured (perhaps a -1
> > instead of a 1, something) ? My feeling is that the Right Way to do
> > this is to first fix the broken run_svn() function, then use it. That
> > way you essentially get credit for fixing the entire Python test suite,
> > and we don't have to revisit this again. :-)
>
> I don't think we can do it in a completely portable fashion. AFAIK,
> os.popen2/os.popen3 give no way to find program's return status, and the
> Popen3 class is not available on windows. Would it be ok to use Popen3 on
> Unix, and retain the current behaviour on win?
i looked at this a little while back when writing some test cases, and
came to the same conclusion regarding os.popen2 and os.popen3. didn't
find the Popen3 class though, so that might be a reasonable solution.
i wonder (not having looked at it yet), if it would be possible to
provide the same functionality on windows and submit it back to the
python people. any win32 people out there willing to take a look?
-garrett
--
garrett rooney Unix was not designed to stop you from
rooneg@electricjellyfish.net doing stupid things, because that would
http://electricjellyfish.net/ stop you from doing clever things.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn diff segfault
Posted by Vladimir Prus <gh...@cs.msu.su>.
cmpilato@collab.net wrote:
> Well, I was about to freak out reading this portion of the patch until
> I saw you comment here. Whew! So, you've hit on something that has
> been bothering me for some time about our Python tests -- we don't
> detect segfaults. How hard would it be to adjust svntest.main.run_svn()
> so that it *could* return an error if a segfault occured (perhaps a -1
> instead of a 1, something) ? My feeling is that the Right Way to do
> this is to first fix the broken run_svn() function, then use it. That
> way you essentially get credit for fixing the entire Python test suite,
> and we don't have to revisit this again. :-)
I don't think we can do it in a completely portable fashion. AFAIK,
os.popen2/os.popen3 give no way to find program's return status, and the
Popen3 class is not available on windows. Would it be ok to use Popen3 on
Unix, and retain the current behaviour on win?
- Volodya
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn diff segfault [PATCH]
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Applied the patch, Vladimir; looked good, & the regression test
properly failed before the fix and passed afterwards :-). Thanks!
Is `msu.ru' Moscow State University? I stayed at your campus in early
September, it was beautiful...
-Karl
Vladimir Prus <gh...@cs.msu.su> writes:
> Changed.
>
> Log message:
> The 'diff' command no longer crashes when two revisions of a file differ in
> properties but not in text.
>
> * subversion/libsvn_client/repos_diff.c
>
> (close_file): Check for presence of any change to report was added.
>
> * subversion/tests/clients/cmdline/diff_tests.py
>
> (diff_only_property_change): New function.
>
> Patch:
> Index: ./subversion/libsvn_client/repos_diff.c
> ===================================================================
> --- ./subversion/libsvn_client/repos_diff.c
> +++ ./subversion/libsvn_client/repos_diff.c Mon Jan 28 18:36:42 2002
> @@ -99,7 +99,8 @@
> apr_file_t *file_start_revision;
>
> /* The path and APR file handle to the temporary file that contains the
> - second repository version */
> + second repository version. These fields are set when processing textdelta and
> + file deletion, and will be NULL if there's no textual difference to the file. */
> svn_stringbuf_t *path_end_revision;
> apr_file_t *file_end_revision;
>
> @@ -571,7 +572,10 @@
> {
> struct file_baton *b = file_baton;
>
> - SVN_ERR (run_diff_cmd (b));
> + /* It is possible to have only change to properties, without textdelta
> + In this case no difference should be reported and no need to run diff. */
> + if (b->path_end_revision)
> + SVN_ERR (run_diff_cmd (b));
>
> svn_pool_destroy (b->pool);
>
> Index: ./subversion/tests/clients/cmdline/diff_tests.py
> ===================================================================
> --- ./subversion/tests/clients/cmdline/diff_tests.py
> +++ ./subversion/tests/clients/cmdline/diff_tests.py Mon Jan 28 18:38:43 2002
> @@ -621,6 +621,33 @@
>
> return 0
>
> +# test 10
> +def diff_only_property_change(sbox):
> + "diff when property was changed but text was not"
> +
> + if sbox.build():
> + return 1
> +
> + wc_dir = sbox.wc_dir
> +
> + current_dir = os.getcwd();
> + os.chdir(sbox.wc_dir);
> +
> + svntest.main.run_svn(None, 'propset', 'svn:eol-style', 'none', "iota")
> + svntest.main.run_svn(None, 'ci')
> +
> + result = 0
> +
> + if os.system(svntest.main.svn_binary + " diff -r1:2"):
> + result = 1
> +
> + if not result and os.system(svntest.main.svn_binary + " diff -r2:1"):
> + result = 1
> +
> + os.chdir(current_dir)
> + return result
> +
> +
>
> ########################################################################
> # Run the tests
> @@ -636,8 +663,9 @@
> diff_non_recursive,
> diff_repo_subset,
> diff_non_version_controlled_file,
> - diff_pure_repository_update_a_file
> - ]
> + diff_pure_repository_update_a_file,
> + diff_only_property_change
> + ]
>
> if __name__ == '__main__':
> svntest.main.run_tests(test_list)
>
>
> Regards,
> Vladimir
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn diff segfault [PATCH]
Posted by Vladimir Prus <gh...@cs.msu.su>.
cmpilato@collab.net wrote:
> > Patch:
> > ...
> This looks basically okay (some grammatical/spelling nits, but
> whatever). What's missing is some comment in the definition of the
> file baton's path_end_revision member that explains that
> path_end_revision is expected to be NULL for all files whose textual
> contents did not change over the span of the diff'ed revisions.
Changed.
Log message:
The 'diff' command no longer crashes when two revisions of a file differ in
properties but not in text.
* subversion/libsvn_client/repos_diff.c
(close_file): Check for presence of any change to report was added.
* subversion/tests/clients/cmdline/diff_tests.py
(diff_only_property_change): New function.
Patch:
Index: ./subversion/libsvn_client/repos_diff.c
===================================================================
--- ./subversion/libsvn_client/repos_diff.c
+++ ./subversion/libsvn_client/repos_diff.c Mon Jan 28 18:36:42 2002
@@ -99,7 +99,8 @@
apr_file_t *file_start_revision;
/* The path and APR file handle to the temporary file that contains the
- second repository version */
+ second repository version. These fields are set when processing textdelta and
+ file deletion, and will be NULL if there's no textual difference to the file. */
svn_stringbuf_t *path_end_revision;
apr_file_t *file_end_revision;
@@ -571,7 +572,10 @@
{
struct file_baton *b = file_baton;
- SVN_ERR (run_diff_cmd (b));
+ /* It is possible to have only change to properties, without textdelta
+ In this case no difference should be reported and no need to run diff. */
+ if (b->path_end_revision)
+ SVN_ERR (run_diff_cmd (b));
svn_pool_destroy (b->pool);
Index: ./subversion/tests/clients/cmdline/diff_tests.py
===================================================================
--- ./subversion/tests/clients/cmdline/diff_tests.py
+++ ./subversion/tests/clients/cmdline/diff_tests.py Mon Jan 28 18:38:43 2002
@@ -621,6 +621,33 @@
return 0
+# test 10
+def diff_only_property_change(sbox):
+ "diff when property was changed but text was not"
+
+ if sbox.build():
+ return 1
+
+ wc_dir = sbox.wc_dir
+
+ current_dir = os.getcwd();
+ os.chdir(sbox.wc_dir);
+
+ svntest.main.run_svn(None, 'propset', 'svn:eol-style', 'none', "iota")
+ svntest.main.run_svn(None, 'ci')
+
+ result = 0
+
+ if os.system(svntest.main.svn_binary + " diff -r1:2"):
+ result = 1
+
+ if not result and os.system(svntest.main.svn_binary + " diff -r2:1"):
+ result = 1
+
+ os.chdir(current_dir)
+ return result
+
+
########################################################################
# Run the tests
@@ -636,8 +663,9 @@
diff_non_recursive,
diff_repo_subset,
diff_non_version_controlled_file,
- diff_pure_repository_update_a_file
- ]
+ diff_pure_repository_update_a_file,
+ diff_only_property_change
+ ]
if __name__ == '__main__':
svntest.main.run_tests(test_list)
Regards,
Vladimir
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Vladimir Prus ] Re: svn diff segfault
Posted by cm...@collab.net.
Vladimir Prus <gh...@cs.msu.su> writes:
> Updated patch follows:
>
> Log message:
> The 'diff' command no longer crashes when two revisions of a file differ in
> properties but not in text.
>
> * subversion/libsvn_client/repos_diff.c
>
> (close_file): Check for presense of any change to report was added.
>
> * subversion/tests/clients/cmdline/diff_tests.py
>
> (diff_only_property_change): New function.
Excellent. Fix AND a test!
> Patch:
> Index: ./subversion/libsvn_client/repos_diff.c
> ===================================================================
> --- ./subversion/libsvn_client/repos_diff.c
> +++ ./subversion/libsvn_client/repos_diff.c Fri Jan 18 13:00:08 2002
> @@ -568,7 +568,10 @@
> {
> struct file_baton *b = file_baton;
>
> - SVN_ERR (run_diff_cmd (b));
> + /* It is possible to have only change to properties, without texdelta
> + In this case no difference should be reported and no need to run diff.
> */
> + if (b->path_end_revision)
> + SVN_ERR (run_diff_cmd (b));
>
> svn_pool_destroy (b->pool);
>
This looks basically okay (some grammatical/spelling nits, but
whatever). What's missing is some comment in the definition of the
file baton's path_end_revision member that explains that
path_end_revision is expected to be NULL for all files whose textual
contents did not change over the span of the diff'ed revisions. That
way anyone who need to change that behavior can be reasonably expected
to determine the implications of their change, and come up with some
other method for avoiding diffs on same-text files.
> Index: ./subversion/tests/clients/cmdline/diff_tests.py
> ===================================================================
> --- ./subversion/tests/clients/cmdline/diff_tests.py
> +++ ./subversion/tests/clients/cmdline/diff_tests.py Fri Jan 18 12:55:46 2002
> @@ -547,6 +547,32 @@
>
> return 0
>
> +def diff_only_property_change(sbox):
> + "diff when property was changed but text was not"
> +
> + if sbox.build():
> + return 1
> +
> + wc_dir = sbox.wc_dir
> +
> + current_dir = os.getcwd();
> + os.chdir(sbox.wc_dir);
> +
> + svntest.main.run_svn(None, 'propset', 'svn:eol-style', 'none', "iota")
> + svntest.main.run_svn(None, 'ci')
> +
> + result = 0
> +
> + if os.system(svntest.main.svn_binary + " diff -r1:2"):
> + result = 1
> +
> + if not result and os.system(svntest.main.svn_binary + " diff -r2:1"):
> + result = 1
> +
> + os.chdir(current_dir)
> + return result
> +
> +
>
> ########################################################################
> # Run the tests
> @@ -562,6 +588,7 @@
> diff_non_recursive,
> diff_repo_subset,
> diff_non_version_controlled_file,
> + diff_only_property_change,
> ]
>
> if __name__ == '__main__':
>
>
> Note: svntest.main.run_svn provides to way to detect segfault. I've found
> nothing appropriate, so have used plain os.system.
Well, I was about to freak out reading this portion of the patch until
I saw you comment here. Whew! So, you've hit on something that has
been bothering me for some time about our Python tests -- we don't
detect segfaults. How hard would it be to adjust svntest.main.run_svn()
so that it *could* return an error if a segfault occured (perhaps a -1
instead of a 1, something) ? My feeling is that the Right Way to do
this is to first fix the broken run_svn() function, then use it. That
way you essentially get credit for fixing the entire Python test suite,
and we don't have to revisit this again. :-)
At any rate, the spirit of this patch is wonderful, and I look forward
to the finished patch (which needs only contain the actual Subversion
code changes, I wouldn't expect you to fix the Python tests if that's
not your aim).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org