RE: trunk failing tests on Windows XP (32 bit): prop-tests.py 33, stat-tests.py 5, upgrade-tests.py 11
-Original Message- From: Paul Burba [mailto:ptbu...@gmail.com] Sent: vrijdag 1 oktober 2010 15:46 To: Bert Huijben Cc: Johan Corveleyn; Subversion Development Subject: Re: trunk failing tests on Windows XP (32 bit): prop-tests.py 33, stat-tests.py 5, upgrade-tests.py 11 On Thu, Sep 30, 2010 at 8:10 PM, Bert Huijben b...@qqmail.nl wrote: -Original Message- From: Johan Corveleyn [mailto:jcor...@gmail.com] Sent: vrijdag 1 oktober 2010 1:51 To: Subversion Development Subject: trunk failing tests on Windows XP (32 bit): prop-tests.py 33, stat-tests.py 5, upgrade-tests.py 11 Hi devs, The following tests fail on my machine (Windows XP 32-bit, built with VCE 2008, ra_local): - prop-tests.py 33: test properties of obstructed subdirectories svn: Can't open directory 'C:\research\svn\client_build\svn_trunk\Release\subversion\tests\cmdlin e\svn-test-work\working_copies\prop_tests-33\A\C': The directory name is invalid. - stat-tests.py 5: status on versioned items whose type has changed svn: Can't open directory 'C:\research\svn\client_build\svn_trunk\Release\subversion\tests\cmdlin e\svn-test-work\working_copies\stat_tests-5\A': The directory name is invalid. - upgrade_tests.py 11: missing directories and obstructing files svn: Can't open directory 'C:\research\svn\client_build\svn_trunk\Release\subversion\tests\cmdlin e\svn-test-work\working_copies\upgrade_tests-11\A\D': The directory name is invalid. They all seem to try to open some path as a directory, but it isn't a directory (but an empty file or something). Am I the only one seeing this? Is this known/normal? This seems to be related to the APR version you use. (Paul Burba reported this same problem earlier this week). Hi Johan, Bert is correct, I saw these failure earlier this week while using APR 1.3.12. All the failures occur when a file unexpectedly obstructs a directory. The APR macro APR_STATUS_IS_ENOTDIR in 1.3.x does not handle the ERROR_DIRECTORY 267 The directory name is invalid that Windows raises in this case. This was fixed in APR in http://svn.apache.org/viewvc?view=revisionrevision=821306 and is in APR 1.4.2. I think we should just add this error to the SVN__APR_STATUS_IS_ENOTDIR() macro to fix compatibility with older apr versions, but I would like to know why this didn't fail a few weeks ago. (I haven't been very active for the last week, but I haven't seen any relevant commits on the commit list in the last few weeks) Bert Paul
Re: [PATCH] extend svn_subst_translate_string() to record whether re-encoding and/or line ending translation were performed
On Fri, Oct 1, 2010 at 4:14 AM, Bert Huijben b...@qqmail.nl wrote:
@@ -650,7 +651,13 @@ translate_newline(const char *eol_str,
*src_format_len = newline_len;
}
/* Translate the newline */
- return translate_write(dst, eol_str, eol_str_len);
+ svn_error_t *err = translate_write(dst, eol_str, eol_str_len);
No declarations mixed in with statements - we stick to C'89 rules. But
I don't think there is any need to insert this new code *after* the
write - it can just as well go before the write, leaving the 'return'
how it was.
The code can just use SVN_ERR() here, as you can't be sure the output is
available in error conditions anyway, so the extra check can be avoided on
errors.
+ if (eol_translated) {
+ if (newline_len != eol_str_len ||
+ strncmp(newline_buf, eol_str, newline_len))
+ *eol_translated = TRUE;
+ }
+ return err;
And this can be a return SVN_NO_ERROR;
I am not sure what the extra check is.
Is this preferred?:
/* Translate the newline */
SVN_ERR(translate_write(dst, eol_str, eol_str_len));
if (translated_eol) {
if (newline_len != eol_str_len ||
strncmp(newline_buf, eol_str, newline_len))
*translated_eol = TRUE;
}
return SVN_NO_ERROR;
[PATCH] -cM-N command line syntax
So, in 1.3 or 1.4 we gained '-c' as a convenience for specifying
changesets as opposed to revision endpoints. Then in 1.5 we got the
svn:mergeinfo property, which expresses ranges of changsets. I don't
know who decided to use - instead of : as the range operator, but I
do note that the 'svn diff' pretty-printer for svn:mergeinfo follows
the same - convention.
The following patch allows -c on the command line to use the same
range syntax as svn:mergeinfo. Thus you can cut and paste those ranges
from 'svn diff' output, into commands such as 'svn log' and 'svn merge'.
For example, if I look at a pending merge in my wc:
$ svn diff -N
Property changes on: .
___
Modified: svn:mergeinfo
Merged /dc/trunk:r13053-13055,13069-13071,13342,13389
if I want to look at the log messages, I can now cut and paste the
revision ranges:
$ svn log -v ^/dc/trunk -c13053-13055,13069-13071,13342,13389
whereas in the past I'd have to convert to -r syntax:
$ svn log -v ^/dc/trunk -r13053:13055 -r13069:13071 -r13342 -r13389
The same is true if I want to redo the merge elsewhere, but even more
so, as the -r syntax would have entailed subtracting one from the
starting point of each range.
Is this, then, a worthwhile feature addition? I don't want to add
syntax that nobody else wants. In particular, this patch highlights
the existing inconsistency of : vs. - as range operators.
Peter
[[[
Parse ranges in '-c' in the 'svn' client.
* subversion/svn/main.c
(main): Parse - in -c arguments. Also move three variables one
scope inward.
* subversion/svn/log-cmd.c
(svn_cl__log): Don't assume -c ranges comprise exactly one revision.
]]]
--- subversion/svn/main.c 2010-10-01 10:54:52.0 -0500
+++ subversion/svn/main.c 2010-10-02 17:55:45.0 -0500
@@ -1211,9 +1211,6 @@ main(int argc, const char *argv[])
break;
case 'c':
{
- char *end;
- svn_revnum_t changeno;
- svn_opt_revision_range_t *range;
apr_array_header_t *change_revs =
svn_cstring_split(opt_arg, , \n\r\t\v, TRUE, pool);
@@ -1227,6 +1224,9 @@ main(int argc, const char *argv[])
for (i = 0; i change_revs-nelts; i++)
{
+ char *end;
+ svn_revnum_t changeno, changeno_end;
+ svn_opt_revision_range_t *range;
const char *change_str =
APR_ARRAY_IDX(change_revs, i, const char *);
@@ -1236,7 +1236,14 @@ main(int argc, const char *argv[])
### {DATE} and the special words. */
while (*change_str == 'r')
change_str++;
- changeno = strtol(change_str, end, 10);
+ changeno = changeno_end = strtol(change_str, end, 10);
+ if (end != change_str *end == '-')
+{
+ change_str = end+1;
+ while (change_str == 'r')
+change_str++;
+ changeno_end = strtol(change_str, end, 10);
+}
if (end == change_str || *end != '\0')
{
err = svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
@@ -1259,12 +1266,14 @@ main(int argc, const char *argv[])
if (changeno 0)
{
range-start.value.number = changeno - 1;
- range-end.value.number = changeno;
+ range-end.value.number = changeno_end;
}
else
{
changeno = -changeno;
- range-start.value.number = changeno;
+ if (changeno_end 0)
+changeno_end = -changeno_end;
+ range-start.value.number = changeno_end;
range-end.value.number = changeno - 1;
}
opt_state.used_change_arg = TRUE;
--- subversion/svn/log-cmd.c2010-10-01 10:54:52.0 -0500
+++ subversion/svn/log-cmd.c2010-10-02 17:55:45.0 -0500
@@ -481,9 +481,9 @@ svn_cl__log(apr_getopt_t *os,
range = APR_ARRAY_IDX(opt_state-revision_ranges, i,
svn_opt_revision_range_t *);
if (range-start.value.number range-end.value.number)
-range-start = range-end;
+range-start.value.number++;
else
-range-end = range-start;
+range-end.value.number++;
}
}
Re: [WIP PATCH] Make svn_diff_diff skip identical prefix and suffix to make diff and blame faster
Hi, Here is a second iteration of the patch. It now passes make check. Differences from the previous version are: - Support for \r eol-style (\n and \r\n was already ok). - The number of prefix_lines is now passed to svn_diff__lcs, so it can use that value to set the position offset of the EOF marker correctly, in case one of both files has become empty after skipping the prefix. This fixes the crashes in blame_tests.py 2 and 7. The patch is pretty big, so please let me know if I should split it up to make it more reviewable (I could easily split it up between the prefix-finding and the suffix-finding, at the cost of having overview over the entire algorithm). Still to do: - Think about why results are sometimes different (because of eliminated suffix, the LCS can sometimes be slightly different), and what can be done about it. - Generalize for more than 2 datasources (for diff3 and diff4). - revv svn_diff_fns_t and maybe other stuff I've changed in public API. - Add support for -x-b, -x-w, and -x--ignore-eol-style options. But I'd like to do those things in follow-up patches, after this one has been reviewed and digested a little bit. So at this point: review, feedback, ... very welcome :-). Log message: [[[ Make svn_diff_diff skip identical prefix and suffix to make diff and blame faster. * subversion/include/svn_diff.h (svn_diff_fns_t): Added new function types datasources_open and get_prefix_lines to the vtable. * subversion/libsvn_diff/diff_memory.c (datasources_open): New function (does nothing). (get_prefix_lines): New function (does nothing). (svn_diff__mem_vtable): Added new functions datasources_open and get_prefix_lines. * subversion/libsvn_diff/diff_file.c (svn_diff__file_baton_t): Added members prefix_lines, suffix_start_chunk[4] and suffix_offset_in_chunk. (increment_pointer_or_chunk, decrement_pointer_or_chunk): New functions. (find_identical_prefix, find_identical_suffix): New functions. (datasources_open): New function, to open both datasources and find their identical prefix and suffix. (get_prefix_lines): New function. (datasource_get_next_token): Stop at start of identical suffix. (svn_diff__file_vtable): Added new functions datasources_open and get_prefix_lines. * subversion/libsvn_diff/diff.h (svn_diff__get_tokens): Added argument datasource_opened, to indicate that the datasource was already opened. * subversion/libsvn_diff/token.c (svn_diff__get_tokens): Added argument datasource_opened. Only open the datasource if datasource_opened is FALSE. Set the starting offset of the position list to the number of prefix lines. * subversion/libsvn_diff/lcs.c (svn_diff__lcs): Added argument prefix_lines. Use this to correctly set the offset of the sentinel position for EOF, even if one of the files became empty after eliminating the identical prefix. * subversion/libsvn_diff/diff.c (svn_diff__diff): Add a chunk of common diff for identical prefix. (svn_diff_diff): Use new function datasources_open, to open original and modified at once, and find their identical prefix and suffix. Pass prefix_lines to svn_diff__lcs and to svn_diff__diff. * subversion/libsvn_diff/diff3.c (svn_diff_diff3): Pass datasource_opened = FALSE to svn_diff__get_tokens. Pass prefix_lines = 0 to svn_diff__lcs. * subversion/libsvn_diff/diff4.c (svn_diff_diff4): Pass datasource_opened = FALSE to svn_diff__get_tokens. Pass prefix_lines = 0 to svn_diff__lcs. ]]] Cheers, -- Johan Index: subversion/include/svn_diff.h === --- subversion/include/svn_diff.h (revision 1003326) +++ subversion/include/svn_diff.h (working copy) @@ -112,6 +112,11 @@ typedef struct svn_diff_fns_t svn_error_t *(*datasource_open)(void *diff_baton, svn_diff_datasource_e datasource); + /** Open the datasources of type @a datasources. */ + svn_error_t *(*datasources_open)(void *diff_baton, apr_off_t *prefix_lines, + svn_diff_datasource_e datasource0, + svn_diff_datasource_e datasource1); + /** Close the datasource of type @a datasource. */ svn_error_t *(*datasource_close)(void *diff_baton, svn_diff_datasource_e datasource); @@ -124,6 +129,9 @@ typedef struct svn_diff_fns_t void *diff_baton, svn_diff_datasource_e datasource); + /** Get the number of identical prefix lines from the @a diff_baton. */ + apr_off_t (*get_prefix_lines)(void *diff_baton); + /** A function for ordering the tokens, resembling 'strcmp' in functionality. * @a compare should contain the return value of the comparison: * If @a ltoken and @a rtoken are equal, return 0. If @a ltoken is Index: subversion/libsvn_diff/diff_file.c
Re: [PATCH] -cM-N command line syntax
[Peter Samuelson] Is this, then, a worthwhile feature addition? I don't want to add syntax that nobody else wants. In particular, this patch highlights the existing inconsistency of : vs. - as range operators. Hmmm, in tests/cmdline/log_tests.py XFail(log_chanage_range), I see Lieven thought -c might someday take ranges with : instead of -. But since the output uses -, I'd argue the cut-and-paste utility of sticking with that outweighs the consistency of :. Unless perhaps we should support both, which would be trivial. Peter
