You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ramkumar Ramachandra <ar...@gmail.com> on 2010/09/19 12:23:12 UTC
[PATCH] Infrastructure to exclude comparing lines
Hi,
I thought this patch might need a little bit of special attention. Let
me know quickly if something is obviously wrong with it- as far as I
know, all tests pass and this patch works as expected. I'm going ahead
and adding lots of tests and fixing whatever bugs there may be shortly.
[[[
Add some testing infrastructure to exclude comparison of certain lines
specified by a regular expression so that svnrdump dump tests can
report actual failures. Exercise this infrastructure in
svnrdump_tests.py. This is necessary because there is some mismatch
between the headers that `svnadmin dump` and `svnrdump dump` are able
to provide.
* subversion/tests/cmdline/svntest/verify.py
(ExpectedOutput.matches_except): Write new function that resembling
ExpectedOutput.display_lines with an extra except_re argument. It
compares EXPECTED and ACTUAL on an exact line-by-line basis,
excluding lines that match the regular expression specified in
except_re.
(ExceptedOutput.matches): Modify function to accept one more
optional argument: except_re. Call matches_except if except_re is
not None.
(compare_and_display_lines): Take an extra argument except_re that
defaults to None, and call ExcpectedOutput.matches with this
argument.
* subversion/tests/cmdline/svnrdump_tests.py
(mismatched_headers_re): Declare a new global string that specifies
the headers in which a mismatch is expected during a dumping
operation.
(run_dump_test): Pass the above regular expression string to
svntest.verify.compare_and_display_lines).
(test_list): Mark copy_and_modify_dump as a passing test.
]]]
Index: subversion/tests/cmdline/svnrdump_tests.py
===================================================================
--- subversion/tests/cmdline/svnrdump_tests.py (revision 998490)
+++ subversion/tests/cmdline/svnrdump_tests.py (working copy)
@@ -41,6 +41,11 @@ XFail = svntest.testcase.XFail
Item = svntest.wc.StateItem
Wimp = svntest.testcase.Wimp
+# Mismatched headers during dumping operation
+mismatched_headers_re = \
+ "Prop-delta: |Text-content-sha1: |Text-copy-source-md5: |" \
+ "Text-copy-source-sha1: |Text-delta-base-sha1: .*"
+
######################################################################
# Helper routines
@@ -80,7 +85,8 @@ def run_dump_test(sbox, dumpfile_name):
# Compare the output from stdout
svntest.verify.compare_and_display_lines(
- "Dump files", "DUMP", svnadmin_dumpfile, svnrdump_dumpfile)
+ "Dump files", "DUMP", svnadmin_dumpfile, svnrdump_dumpfile,
+ None, mismatched_headers_re)
def run_load_test(sbox, dumpfile_name):
"""Load a dumpfile using 'svnrdump load', dump it with 'svnadmin
@@ -177,7 +183,7 @@ test_list = [ None,
revision_0_load,
skeleton_load,
copy_and_modify_load,
- Wimp("Need to fix headers in RA layer", copy_and_modify_dump),
+ copy_and_modify_dump,
]
if __name__ == '__main__':
Index: subversion/tests/cmdline/svntest/verify.py
===================================================================
--- subversion/tests/cmdline/svntest/verify.py (revision 998490)
+++ subversion/tests/cmdline/svntest/verify.py (working copy)
@@ -108,7 +108,7 @@ class ExpectedOutput:
def __cmp__(self, other):
raise 'badness'
- def matches(self, other):
+ def matches(self, other, except_re=None):
"""Return whether SELF.output matches OTHER (which may be a list
of newline-terminated lines, or a single string). Either value
may be None."""
@@ -126,8 +126,32 @@ class ExpectedOutput:
if not isinstance(expected, list):
expected = [expected]
- return self.is_equivalent_list(expected, actual)
+ if except_re:
+ return self.matches_except(expected, actual, except_re)
+ else:
+ return self.is_equivalent_list(expected, actual)
+ def matches_except(self, expected, actual, except_re):
+ "Return whether EXPECTED and ACTUAL match except for except_re."
+ if not self.is_regex:
+ i_expected = 0
+ i_actual = 0
+ while i_expected < len(expected) and i_actual < len(actual):
+ if re.match(except_re, actual[i_actual]):
+ i_actual += 1
+ elif re.match(except_re, expected[i_expected]):
+ i_expected += 1
+ elif expected[i_expected] == actual[i_actual]:
+ i_expected += 1
+ i_actual += 1
+ else:
+ return False
+ if i_expected == len(expected) and i_actual == len(actual):
+ return True
+ return False
+ else:
+ raise "self.is_regex and except_re are mutually exclusive"
+
def is_equivalent_list(self, expected, actual):
"Return whether EXPECTED and ACTUAL are equivalent."
if not self.is_regex:
@@ -308,7 +332,7 @@ def display_lines(message, label, expected, actual
sys.stdout.write(x)
def compare_and_display_lines(message, label, expected, actual,
- raisable=None):
+ raisable=None, except_re=None):
"""Compare two sets of output lines, and print them if they differ,
preceded by MESSAGE iff not None. EXPECTED may be an instance of
ExpectedOutput (and if not, it is wrapped as such). RAISABLE is an
@@ -325,7 +349,7 @@ def compare_and_display_lines(message, label, expe
actual = [actual]
actual = [line for line in actual if not line.startswith('DBG:')]
- if not expected.matches(actual):
+ if not expected.matches(actual, except_re):
expected.display_differences(message, label, actual)
raise raisable
Re: [PATCH] Infrastructure to exclude comparing lines
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi,
I've committed this to r998661, but the Subversion bot reports some
breakages. I'm not sure what to make of them though.
-- Ram
Re: [PATCH] Infrastructure to exclude comparing lines
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Thanks for the explanation. Given that svnrdump's output, as is, is
semantically correct, and the only issue here is consistency with
svnadmin (which produces a different output with the same semantics),
and that it's more than a trivial bit of work to get the consistency,
+1 for leaving svnrdump as is.
Daniel
Ramkumar Ramachandra wrote on Thu, Sep 23, 2010 at 01:02:01 +0530:
> Daniel Shahaf writes:
> > In other words, the only case where svnrdump and svnadmin disagree on
> > the presence of the "Prop-delta:" header is in the case of
> > added-without-history files; and in that case, svnrdump prints it and
> > svnadmin doesn't, and this doesn't matter since interpreting the dumped
> > hash as a delta or as an absolute full properties list gives the same
> > result...
> >
> > Right?
>
> Exactly!
>
> > P.S. Doesn't the "Node-action:" header allow you to distinguish whether
> > it's a newly-added file or not, and thus be able to generate Prop-delta:
> > only in the cases that svnadmin generates it?
>
> Yes. Unfortunately, the actual dumping is separated from determining
> *what* to dump which is why dump_editor needs ugly hacks like
> dump_props and dump_newlines.
>
> To solve this problem, we have to reverse-engineer what `svnadmin
> dump` dumps. It should then be possible to determine what happens in
> the edge cases: is_copy replace, is_copy add, is_copy change etc. Then
> we have to set another flag, extend dump_props to accept another
> argument and conditionally write that Prop-delta header. To put it
> tersely, it's non-trivial, inelegant and time consuming- plus, it's
> not priority, considering that many other headers mismatch anyway.
Re: [PATCH] Infrastructure to exclude comparing lines
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Daniel,
Daniel Shahaf writes:
> Ramkumar Ramachandra wrote on Wed, Sep 22, 2010 at 22:24:10 +0530:
> > Ok, I think you've misunderstood this. Both of them DO dump the exact
> > same properties. Think of the Prop-delta header as just an extra
> > cosmetic line. In dumpfile v3, everything is a delta against the
> > previous revision including props.
>
> Did you mean: s/In dumpfile v3/In 'svnadmin dump --deltas'/ ?
>
> IOW, the dumpfile format version doesn't enforce that, but svnadmin
> never produces non-delta'd props when it dumps as v3 --- right?
Right. Technically, I meant dumpfile v3 with all the v3 features
exercised :)
> (I think/assume that you can take any v2 dumpfile and just change the
> header from "format: 2" to "format: 3" --- without making any other
> changes --- and still have the result a valid dumpfile...)
That's right. A dumpfile v3 just adds some extra features to v2.
> > When the Prop-delta header is
> > absent, it doesn't mean that the full props are being listed- it just
> > means that the file or directory didn't exist in a previous
> > revision. For a file or directory that didn't exist in a previous
> > revision-
> >
> > * [Prop-delta missing] is a cosmetic way of saying: "Here's a new file
> > or directory along with it's properties."
> > * Prop-delta: true is a cosmetic way of saying: "Here's a file or
> > directory. If it's new, the delta is against nothing. If it's old,
> > the delta is against a previous version. Check it for yourself-
> > information about whether the file or directory is new is already
> > present in the dumpfile."
> >
>
> In other words, the only case where svnrdump and svnadmin disagree on
> the presence of the "Prop-delta:" header is in the case of
> added-without-history files; and in that case, svnrdump prints it and
> svnadmin doesn't, and this doesn't matter since interpreting the dumped
> hash as a delta or as an absolute full properties list gives the same
> result...
>
> Right?
Exactly!
> P.S. Doesn't the "Node-action:" header allow you to distinguish whether
> it's a newly-added file or not, and thus be able to generate Prop-delta:
> only in the cases that svnadmin generates it?
Yes. Unfortunately, the actual dumping is separated from determining
*what* to dump which is why dump_editor needs ugly hacks like
dump_props and dump_newlines.
To solve this problem, we have to reverse-engineer what `svnadmin
dump` dumps. It should then be possible to determine what happens in
the edge cases: is_copy replace, is_copy add, is_copy change etc. Then
we have to set another flag, extend dump_props to accept another
argument and conditionally write that Prop-delta header. To put it
tersely, it's non-trivial, inelegant and time consuming- plus, it's
not priority, considering that many other headers mismatch anyway.
-- Ram
Re: [PATCH] Infrastructure to exclude comparing lines
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ramkumar Ramachandra wrote on Wed, Sep 22, 2010 at 22:24:10 +0530:
> Hi Daniel,
>
> Daniel Shahaf writes:
> > Ramkumar Ramachandra wrote on Mon, Sep 20, 2010 at 09:39:57 +0530:
> > > Daniel Shahaf writes:
> > > > * ignore Prop-delta... why?
> > >
> > > This is because svnrdump outputs "Prop-delta: true" everytime, whether
> > > it's really a delta or a delta from /dev/null. This is quite harmless,
> > > and the logic for omitting it when it's a delta from /dev/null can be
> > > fairly non-trivial and unnecessary. Again, svnadmin dump does it
> > > "right".
> >
> > Okay, but if the Prop-delta: header is generated by one but not by the
> > other, then the rest of the dump'd props hash should be different too.
>
> Huh? Why?
>
> > i.e., if the tests ignore difference in the Prop-delta: header, they
> > should also be finding more differences later in the props hash --- and
> > should "somehow" be ignoring those correctly...
>
> Ok, I think you've misunderstood this. Both of them DO dump the exact
> same properties. Think of the Prop-delta header as just an extra
> cosmetic line. In dumpfile v3, everything is a delta against the
> previous revision including props.
Did you mean: s/In dumpfile v3/In 'svnadmin dump --deltas'/ ?
IOW, the dumpfile format version doesn't enforce that, but svnadmin
never produces non-delta'd props when it dumps as v3 --- right?
(I think/assume that you can take any v2 dumpfile and just change the
header from "format: 2" to "format: 3" --- without making any other
changes --- and still have the result a valid dumpfile...)
> When the Prop-delta header is
> absent, it doesn't mean that the full props are being listed- it just
> means that the file or directory didn't exist in a previous
> revision. For a file or directory that didn't exist in a previous
> revision-
>
> * [Prop-delta missing] is a cosmetic way of saying: "Here's a new file
> or directory along with it's properties."
> * Prop-delta: true is a cosmetic way of saying: "Here's a file or
> directory. If it's new, the delta is against nothing. If it's old,
> the delta is against a previous version. Check it for yourself-
> information about whether the file or directory is new is already
> present in the dumpfile."
>
In other words, the only case where svnrdump and svnadmin disagree on
the presence of the "Prop-delta:" header is in the case of
added-without-history files; and in that case, svnrdump prints it and
svnadmin doesn't, and this doesn't matter since interpreting the dumped
hash as a delta or as an absolute full properties list gives the same
result...
Right?
Thanks for clarifying,
Daniel
P.S. Doesn't the "Node-action:" header allow you to distinguish whether
it's a newly-added file or not, and thus be able to generate Prop-delta:
only in the cases that svnadmin generates it?
Re: [PATCH] Infrastructure to exclude comparing lines
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Daniel,
Daniel Shahaf writes:
> Ramkumar Ramachandra wrote on Mon, Sep 20, 2010 at 09:39:57 +0530:
> > Daniel Shahaf writes:
> > > * ignore Prop-delta... why?
> >
> > This is because svnrdump outputs "Prop-delta: true" everytime, whether
> > it's really a delta or a delta from /dev/null. This is quite harmless,
> > and the logic for omitting it when it's a delta from /dev/null can be
> > fairly non-trivial and unnecessary. Again, svnadmin dump does it
> > "right".
>
> Okay, but if the Prop-delta: header is generated by one but not by the
> other, then the rest of the dump'd props hash should be different too.
Huh? Why?
> i.e., if the tests ignore difference in the Prop-delta: header, they
> should also be finding more differences later in the props hash --- and
> should "somehow" be ignoring those correctly...
Ok, I think you've misunderstood this. Both of them DO dump the exact
same properties. Think of the Prop-delta header as just an extra
cosmetic line. In dumpfile v3, everything is a delta against the
previous revision including props. When the Prop-delta header is
absent, it doesn't mean that the full props are being listed- it just
means that the file or directory didn't exist in a previous
revision. For a file or directory that didn't exist in a previous
revision-
* [Prop-delta missing] is a cosmetic way of saying: "Here's a new file
or directory along with it's properties."
* Prop-delta: true is a cosmetic way of saying: "Here's a file or
directory. If it's new, the delta is against nothing. If it's old,
the delta is against a previous version. Check it for yourself-
information about whether the file or directory is new is already
present in the dumpfile."
I hope that clears it up?
-- Ram
Re: [PATCH] Infrastructure to exclude comparing lines
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ramkumar Ramachandra wrote on Mon, Sep 20, 2010 at 09:39:57 +0530:
> Daniel Shahaf writes:
> > * ignore Prop-delta... why?
>
> This is because svnrdump outputs "Prop-delta: true" everytime, whether
> it's really a delta or a delta from /dev/null. This is quite harmless,
> and the logic for omitting it when it's a delta from /dev/null can be
> fairly non-trivial and unnecessary. Again, svnadmin dump does it
> "right".
Okay, but if the Prop-delta: header is generated by one but not by the
other, then the rest of the dump'd props hash should be different too.
i.e., if the tests ignore difference in the Prop-delta: header, they
should also be finding more differences later in the props hash --- and
should "somehow" be ignoring those correctly...
???
Re: [PATCH] Infrastructure to exclude comparing lines
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Daniel,
Daniel Shahaf writes:
> > +# Mismatched headers during dumping operation
> > +mismatched_headers_re = \
> > + "Prop-delta: |Text-content-sha1: |Text-copy-source-md5: |" \
> > + "Text-copy-source-sha1: |Text-delta-base-sha1: .*"
> > +
>
> So:
>
> * ignore sha1 because RA doesn't provide it yet
Yes, that's correct.
> * ignore Text-copy-source-md5... why? for the same reason?
Yes.
> * ignore Prop-delta... why?
This is because svnrdump outputs "Prop-delta: true" everytime, whether
it's really a delta or a delta from /dev/null. This is quite harmless,
and the logic for omitting it when it's a delta from /dev/null can be
fairly non-trivial and unnecessary. Again, svnadmin dump does it
"right".
> (It would be good to have the answer in a comment, I think.)
Ok.
> Couldn't you redefine one of these two functions (matches_except() and
> is_equivalent_list()) such that they share more code?
>
> For example:
>
> def matches_except(self, expected, actual, except_re):
> # TODO: possibly use a slightly different error message
> # if an exception is thrown
> return self.is_equivalent_list(
> filter(lambda x: not re.match(except_re, x), expected),
> filter(lambda x: not re.match(except_re, x), actual))
>
> ... or add some sort of optional argument to is_equivalent_list(), such
> that the functionalities of both functions can be achieved by varying
> the value of the optional argument.
>
> In second thought: you could make except_re a lambda expression
> (taking a single argument, the line) instead of a regex; i.e.,
>
> s/foo('Text-sha1:')/foo(lambda line: 'Text-sha1' not in line)/
>
> ?
Actually, this was sort of intentional on my part. I don't know if
this except_re (the way I've written it) is such a great idea in the
long run. We know that svnrdump needs it, but I'd like to wait until
we have more usecases before restructuring it. We might figure out
that some more general way of including/ excluding lines to match in
actual/ expected: the lambda function example you've provided is quite
illustrative. For now, let this except_re feature serve just the
svnrdump case.
-- Ram
Re: [PATCH] Infrastructure to exclude comparing lines
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ramkumar Ramachandra wrote on Sun, Sep 19, 2010 at 17:53:12 +0530:
> Hi,
>
> I thought this patch might need a little bit of special attention. Let
> me know quickly if something is obviously wrong with it- as far as I
> know, all tests pass and this patch works as expected. I'm going ahead
> and adding lots of tests and fixing whatever bugs there may be shortly.
>
> [[[
> Add some testing infrastructure to exclude comparison of certain lines
> specified by a regular expression so that svnrdump dump tests can
> report actual failures. Exercise this infrastructure in
> svnrdump_tests.py. This is necessary because there is some mismatch
> between the headers that `svnadmin dump` and `svnrdump dump` are able
> to provide.
>
> * subversion/tests/cmdline/svntest/verify.py
>
> (ExpectedOutput.matches_except): Write new function that resembling
> ExpectedOutput.display_lines with an extra except_re argument. It
> compares EXPECTED and ACTUAL on an exact line-by-line basis,
> excluding lines that match the regular expression specified in
> except_re.
>
> (ExceptedOutput.matches): Modify function to accept one more
> optional argument: except_re. Call matches_except if except_re is
> not None.
>
> (compare_and_display_lines): Take an extra argument except_re that
> defaults to None, and call ExcpectedOutput.matches with this
> argument.
>
> * subversion/tests/cmdline/svnrdump_tests.py
>
> (mismatched_headers_re): Declare a new global string that specifies
> the headers in which a mismatch is expected during a dumping
> operation.
>
> (run_dump_test): Pass the above regular expression string to
> svntest.verify.compare_and_display_lines).
>
> (test_list): Mark copy_and_modify_dump as a passing test.
> ]]]
>
> Index: subversion/tests/cmdline/svnrdump_tests.py
> ===================================================================
> --- subversion/tests/cmdline/svnrdump_tests.py (revision 998490)
> +++ subversion/tests/cmdline/svnrdump_tests.py (working copy)
> @@ -41,6 +41,11 @@ XFail = svntest.testcase.XFail
> Item = svntest.wc.StateItem
> Wimp = svntest.testcase.Wimp
>
> +# Mismatched headers during dumping operation
> +mismatched_headers_re = \
> + "Prop-delta: |Text-content-sha1: |Text-copy-source-md5: |" \
> + "Text-copy-source-sha1: |Text-delta-base-sha1: .*"
> +
So:
* ignore sha1 because RA doesn't provide it yet
* ignore Text-copy-source-md5... why? for the same reason?
* ignore Prop-delta... why?
(It would be good to have the answer in a comment, I think.)
> ######################################################################
> # Helper routines
>
> @@ -80,7 +85,8 @@ def run_dump_test(sbox, dumpfile_name):
>
> # Compare the output from stdout
> svntest.verify.compare_and_display_lines(
> - "Dump files", "DUMP", svnadmin_dumpfile, svnrdump_dumpfile)
> + "Dump files", "DUMP", svnadmin_dumpfile, svnrdump_dumpfile,
> + None, mismatched_headers_re)
>
> def run_load_test(sbox, dumpfile_name):
> """Load a dumpfile using 'svnrdump load', dump it with 'svnadmin
> @@ -177,7 +183,7 @@ test_list = [ None,
> revision_0_load,
> skeleton_load,
> copy_and_modify_load,
> - Wimp("Need to fix headers in RA layer", copy_and_modify_dump),
> + copy_and_modify_dump,
> ]
>
> if __name__ == '__main__':
> Index: subversion/tests/cmdline/svntest/verify.py
> ===================================================================
> --- subversion/tests/cmdline/svntest/verify.py (revision 998490)
> +++ subversion/tests/cmdline/svntest/verify.py (working copy)
> @@ -108,7 +108,7 @@ class ExpectedOutput:
> def __cmp__(self, other):
> raise 'badness'
>
> - def matches(self, other):
> + def matches(self, other, except_re=None):
> """Return whether SELF.output matches OTHER (which may be a list
> of newline-terminated lines, or a single string). Either value
> may be None."""
> @@ -126,8 +126,32 @@ class ExpectedOutput:
> if not isinstance(expected, list):
> expected = [expected]
>
> - return self.is_equivalent_list(expected, actual)
> + if except_re:
> + return self.matches_except(expected, actual, except_re)
> + else:
> + return self.is_equivalent_list(expected, actual)
>
Couldn't you redefine one of these two functions (matches_except() and
is_equivalent_list()) such that they share more code?
For example:
def matches_except(self, expected, actual, except_re):
# TODO: possibly use a slightly different error message
# if an exception is thrown
return self.is_equivalent_list(
filter(lambda x: not re.match(except_re, x), expected),
filter(lambda x: not re.match(except_re, x), actual))
... or add some sort of optional argument to is_equivalent_list(), such
that the functionalities of both functions can be achieved by varying
the value of the optional argument.
In second thought: you could make except_re a lambda expression
(taking a single argument, the line) instead of a regex; i.e.,
s/foo('Text-sha1:')/foo(lambda line: 'Text-sha1' not in line)/
?
> + def matches_except(self, expected, actual, except_re):
> + "Return whether EXPECTED and ACTUAL match except for except_re."
> + if not self.is_regex:
> + i_expected = 0
> + i_actual = 0
> + while i_expected < len(expected) and i_actual < len(actual):
> + if re.match(except_re, actual[i_actual]):
> + i_actual += 1
> + elif re.match(except_re, expected[i_expected]):
> + i_expected += 1
> + elif expected[i_expected] == actual[i_actual]:
> + i_expected += 1
> + i_actual += 1
> + else:
> + return False
> + if i_expected == len(expected) and i_actual == len(actual):
> + return True
> + return False
> + else:
> + raise "self.is_regex and except_re are mutually exclusive"
> +
> def is_equivalent_list(self, expected, actual):
> "Return whether EXPECTED and ACTUAL are equivalent."
> if not self.is_regex:
> @@ -308,7 +332,7 @@ def display_lines(message, label, expected, actual
> sys.stdout.write(x)
>
> def compare_and_display_lines(message, label, expected, actual,
> - raisable=None):
> + raisable=None, except_re=None):
> """Compare two sets of output lines, and print them if they differ,
> preceded by MESSAGE iff not None. EXPECTED may be an instance of
> ExpectedOutput (and if not, it is wrapped as such). RAISABLE is an
> @@ -325,7 +349,7 @@ def compare_and_display_lines(message, label, expe
> actual = [actual]
> actual = [line for line in actual if not line.startswith('DBG:')]
>
> - if not expected.matches(actual):
> + if not expected.matches(actual, except_re):
> expected.display_differences(message, label, actual)
> raise raisable
>