Johan Corveleyn wrote on Mon, Jan 31, 2011 at 02:47:50 +0100: > On Fri, Jan 28, 2011 at 7:58 PM, Daniel Shahaf <d...@daniel.shahaf.name> > wrote: > > Johan Corveleyn wrote on Fri, Jan 28, 2011 at 14:04:07 +0100: > >> On Fri, Jan 28, 2011 at 9:29 AM, Daniel Shahaf <d...@daniel.shahaf.name> > >> wrote: > >> > May I suggest that, if this code is to be released, then you validate > >> > its correctnss? For example, a minimal regression test that is written > >> > to record trunk's pre-branch behaviour might be sufficient. > >> > >> ... I'll accept your suggestion. I'll try to take some time for that > >> this weekend. If anyone beats me to it, that would be fine as well of > >> course :). > >> > > Sorry, didn't get around to it yet. Maybe sometime during next week. > > > Thanks :-). I've looked into it, but it seems the output functions in > > svn_diff.h are oriented to diff2/diff3 only (they don't have an > > 'ancestor' enum or parameters); and in a quick test, I couldn't get > > tools/diff/diff4 to output anything sensible: > > > > % for i in 1 2 3 4; do echo $i>$i; done > > % ./tools/diff/diff4 1 2 3 4 > > 3 > > % > > I think it's more or less sensible, although I'm not completely sure. > If you look at the "usage" of diff4, you see: > > Usage: /path/to/diff4 <mine> <older> <yours> <ancestor> > > I think what you did is: take the difference between 2 and 3 (older > and yours), and apply that to 1 (mine), using 4 as the common > ancestor. Apparently that yields "3" :-). > > Compare that with the use of diff3, with the files 1, 2 and 3: > > Usage: /path/to/diff3 <mine> <older> <yours> > > $ diff3 1 2 3 > <<<<<<< 1 > 1 > ======= > 3 > >>>>>>> 3 > > Hmmm, that looks like a merge conflict, which seems logical (trying to > apply the diff between 2 and 3, to the file 1 which doesn't contain a > line '2'). >
Agreed. > So now I also don't really understand why diff4 successfully merges > it, given the ancestor 4. Maybe the reasoning is as follows, given > that 4 is the common ancestor: > > - 1 has evolved out of 4 (so '1' is the mine's replacement for '4') > > - 2 has evolved out of 4 (so in the merge source, '2' is the > replacement for '4') > > - So with "variance-adjusted patching", the diff between 2 and 3 > translates into: "replace whatever '4' was transformed into (in this > case '1') into '3'". That would yield '3'. > > Not sure though. > > > How can we test diff4 then? Do we have to add public diff4 APIs in > > order to be able to test svn_diff_diff4_2()? > > Maybe we should come up with a better example. In the meantime, I'm > trying to understand diff4 (reading kfogel's article at [1], as > referenced in diff4.c). > That article was very helpful --- thanks for the pointer! > > Daniel > > (on the one hand I'd much prefer to test the API changes before > > releasing them; on the other hand, adding API's related to an API > > that no one (to our knowledge) uses seems odd) > > Yes. I think we should give it some effort to come up with some > minimal testing. But if it takes too long, we should probably not let > this be blocking.... > I've went ahead and made a patch out of the second example in that article. However, I admit that I got it to work by fiddling with the order of the four parameters until the test passed :-) Could you have a look? (attached) Thanks, Daniel > Cheers, > -- > Johan > > [1] > http://svn.apache.org/repos/asf/subversion/trunk/notes/variance-adjusted-patching.html
Index: subversion/libsvn_subr/stream.c =================================================================== --- subversion/libsvn_subr/stream.c (revision 1065108) +++ subversion/libsvn_subr/stream.c (working copy) @@ -1373,6 +1373,22 @@ svn_stream_for_stdout(svn_stream_t **out, apr_pool svn_error_t * +svn_stream_for_stderr(svn_stream_t **err, apr_pool_t *pool) +{ + apr_file_t *stderr_file; + apr_status_t apr_err; + + apr_err = apr_file_open_stderr(&stderr_file, pool); + if (apr_err) + return svn_error_wrap_apr(apr_err, "Can't open stderr"); + + *err = svn_stream_from_aprfile2(stderr_file, TRUE, pool); + + return SVN_NO_ERROR; +} + + +svn_error_t * svn_string_from_stream(svn_string_t **result, svn_stream_t *stream, apr_pool_t *result_pool, Index: subversion/tests/libsvn_diff/diff-diff3-test.c =================================================================== --- subversion/tests/libsvn_diff/diff-diff3-test.c (revision 1065108) +++ subversion/tests/libsvn_diff/diff-diff3-test.c (working copy) @@ -2050,6 +2050,41 @@ test_three_way_merge_conflict_styles(apr_pool_t *p static svn_error_t * +test_diff4(apr_pool_t *pool) +{ + svn_diff_t *diff; + svn_stream_t *actual, *expected; + svn_boolean_t same; + svn_stream_t *err; + + /* Usage: tools/diff/diff4 <mine> <older> <yours> <ancestor> */ + /* tools/diff/diff4 B2 T2 T3 T1 > B2new */ + SVN_ERR(svn_diff_file_diff4(&diff, "T2", "B2", "T3", "T1", pool)); + + /* Sanity. */ + SVN_TEST_ASSERT(! svn_diff_contains_conflicts(diff)); + SVN_TEST_ASSERT(svn_diff_contains_diffs(diff)); + + /* Comparison. */ + SVN_ERR(svn_stream_open_readonly(&expected, "B2new", pool, pool)); + actual = svn_stream_from_stringbuf( + svn_stringbuf_create_ensure(417, pool), /* 417 == wc -c < B2new */ + pool); + SVN_ERR(svn_stream_for_stderr(&err, pool)); + SVN_ERR(svn_diff_file_output_merge(svn_stream_tee(actual, err, pool), diff, + "T2", "B2", "T3", + NULL, NULL, NULL, NULL, + FALSE, + FALSE, + pool)); + SVN_ERR(svn_stream_contents_same2(&same, actual, expected, pool)); + SVN_TEST_ASSERT(same); + + return SVN_NO_ERROR; +} + + +static svn_error_t * random_trivial_merge(apr_pool_t *pool) { int i; @@ -2300,5 +2335,7 @@ struct svn_test_descriptor_t test_funcs[] = "3-way merge, adjacent changes"), SVN_TEST_PASS2(test_three_way_merge_conflict_styles, "3-way merge with conflict styles"), + SVN_TEST_PASS2(test_diff4, + "4-way merge; see variance-adjusted-patching.html"), SVN_TEST_NULL }; Index: subversion/tests/libsvn_diff/B2 =================================================================== --- subversion/tests/libsvn_diff/B2 (revision 0) +++ subversion/tests/libsvn_diff/B2 (working copy) @@ -0,0 +1,14 @@ +int main (int argc, char **argv) +{ + /* line minus-five of context */ + /* line minus-four of context */ + /* line minus-three of context */ + /* line -1 of context */ + printf ("Hello, world!\n"); + /* newly inserted line of context */ + /* line plus-one of context */ + /* line plus-two of context */ + /* line plus-three of context */ + /* line plus-four of context */ + /* line plus-five of context */ +} Index: subversion/tests/libsvn_diff/T1 =================================================================== --- subversion/tests/libsvn_diff/T1 (revision 0) +++ subversion/tests/libsvn_diff/T1 (working copy) @@ -0,0 +1,14 @@ +int main (int argc, char **argv) +{ + /* line minus-five of context */ + /* line minus-four of context */ + /* line minus-three of context */ + /* line minus-two of context */ + /* line minus-one of context */ + printf ("Hello, world!\n"); + /* line plus-one of context */ + /* line plus-two of context */ + /* line plus-three of context */ + /* line plus-four of context */ + /* line plus-five of context */ +} Index: subversion/tests/libsvn_diff/T2 =================================================================== --- subversion/tests/libsvn_diff/T2 (revision 0) +++ subversion/tests/libsvn_diff/T2 (working copy) @@ -0,0 +1,16 @@ +#include <stdio.h> + +int main (int argc, char **argv) +{ + /* line minus-five of context */ + /* line minus-four of context */ + /* line minus-three of context */ + /* line minus-two of context */ + /* line minus-one of context */ + printf ("Hello, world!\n"); + /* line plus-one of context */ + /* line plus-two of context */ + /* line plus-three of context */ + /* line plus-four of context */ + /* line plus-five of context */ +} Index: subversion/tests/libsvn_diff/T3 =================================================================== --- subversion/tests/libsvn_diff/T3 (revision 0) +++ subversion/tests/libsvn_diff/T3 (working copy) @@ -0,0 +1,16 @@ +#include <stdio.h> + +int main (int argc, char **argv) +{ + /* line minus-five of context */ + /* line minus-four of context */ + /* line minus-three of context */ + /* line minus-two of context */ + /* line minus-one of context */ + printf ("Good-bye, cruel world!\n"); + /* line plus-one of context */ + /* line plus-two of context */ + /* line plus-three of context */ + /* line plus-four of context */ + /* line plus-five of context */ +} Index: subversion/tests/libsvn_diff/B2new =================================================================== --- subversion/tests/libsvn_diff/B2new (revision 0) +++ subversion/tests/libsvn_diff/B2new (working copy) @@ -0,0 +1,14 @@ +int main (int argc, char **argv) +{ + /* line minus-five of context */ + /* line minus-four of context */ + /* line minus-three of context */ + /* line -1 of context */ + printf ("Good-bye, cruel world!\n"); + /* newly inserted line of context */ + /* line plus-one of context */ + /* line plus-two of context */ + /* line plus-three of context */ + /* line plus-four of context */ + /* line plus-five of context */ +} Index: subversion/include/svn_io.h =================================================================== --- subversion/include/svn_io.h (revision 1065108) +++ subversion/include/svn_io.h (working copy) @@ -930,6 +930,16 @@ svn_stream_t * svn_stream_from_aprfile(apr_file_t *file, apr_pool_t *pool); +/** Set @a *err to a generic stream connected to stderr, allocated in + * @a pool. The stream and its underlying APR handle will be closed + * when @a pool is cleared or destroyed. + * + * @since New in 1.7. + */ +svn_error_t * +svn_stream_for_stderr(svn_stream_t **err, + apr_pool_t *pool); + /** Set @a *out to a generic stream connected to stdout, allocated in * @a pool. The stream and its underlying APR handle will be closed * when @a pool is cleared or destroyed.