Johan Corveleyn wrote on Mon, Jan 31, 2011 at 02:47:50 +0100:
> On Fri, Jan 28, 2011 at 7:58 PM, Daniel Shahaf <[email protected]>
> 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 <[email protected]>
> >> 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.
