On Fri, Feb 4, 2011 at 7:56 AM, Daniel Shahaf <d...@daniel.shahaf.name> wrote:
> 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)
Nice. It looks good to me (haven't tested it, just looked at the code;
I assume it passes with trunk?)
The order of the parameters is correct: tools/diff/diff4.c does
exactly the same thing (switch the first and second argument), before
it passes them to svn_diff_file_diff4.
tools/diff/diff4.c, lines 75-77:
[[[
svn_err = do_diff4(ostream,
argv[2], argv[1], argv[3], argv[4],
pool);
]]]
And do_diff4 look thusly, mirroring exactly what you're doing in the test:
[[[
static svn_error_t *
do_diff4(svn_stream_t *ostream,
const char *original,
const char *modified,
const char *latest,
const char *ancestor,
apr_pool_t *pool)
{
svn_diff_t *diff;
SVN_ERR(svn_diff_file_diff4(&diff,
original, modified, latest, ancestor,
pool));
SVN_ERR(svn_diff_file_output_merge(ostream, diff,
original, modified, latest,
NULL, NULL, NULL, NULL,
FALSE,
FALSE,
pool));
return NULL;
}
]]]
So apparently, if we compare the arguments from diff4's usage message
with the names of the parameters, we have:
mine == modified
older == original
yours == latest
ancestor == ancestor
Cheers,
Johan
> Thanks,
>
> Daniel
>
>> Cheers,
>> --
>> Johan
>>
>> [1]
>> http://svn.apache.org/repos/asf/subversion/trunk/notes/variance-adjusted-patching.html
>
