On Wed, 2008-10-08 at 05:20 +0200, Neels J Hofmeyr wrote:
>
> Julian Foad wrote:
> >>> Is this really an error or am I still missing something?
> >
> > It's really an error.
> >
> > Can you maybe add a test for it, and/or file an issue, and/or fix it, or
> > just ignore this case, while still writing the directory compare thing?
>
> Other than this error appearing in the tests, the directory compare thing is
> already working. Your suggested way of implementing diff_summarize looks
> very good in terms of avoiding to re-invent the wheel.
Great. It doesn't seem to be working quite right yet, though. A simple
test on the command line, in a Subversion WC with your patch applied,
shows:
[[[
$ svn status
M subversion/tests/cmdline/merge_tests.py
M subversion/libsvn_client/merge.c
M subversion/libsvn_client/diff.c
M subversion/libsvn_client/repos_diff_summarize.c
$ svn diff --summarize -r33519 subversion/
M subversion/subversion/libsvn_wc/update_editor.c
]]]
(My WC BASE is presently at r33519, except for "update_editor.c" which
is at r33553.)
The output of "update_editor.c" is correct (r33519 of it differs from
r33553 of it) except that the file path is printed with an extra
"subversion/" component in it.
However, the four locally modified files are not shown. It looks like it
is showing -r33519:BASE whereas it should be showing -r33519:Working.
DEBUGGING
In diff_summarize_repos_wc(), it calls
svn_client__get_diff_summarize_editor(). However, that is for getting a
(repos <> repos) editor. We need a (repos <> wc) editor. The equivalent
non-summarize code path uses svn_wc_get_diff_editor5(). A call to that
is present but commented out, and we haven't written the set of diff
callbacks that need to be provided to it.
There are two different mechanisms for (streamily) representing a tree
diff: the "delta editor" and the "diff callbacks". In the existing (svn
1.5) diff code, notice how the two different mechanisms are used:
(repos <> repos):
normal-diff:
editor = svn_client__get_diff_editor(CALLBACKS);
reporter = svn_ra_do_diff3(editor);
reporter->finish_report();
diff-summarize:
editor = svn_client__get_diff_summarize_editor(SUMMARIZE_FUNC);
reporter = svn_ra_do_diff3(editor);
reporter->finish_report();
(repos <> wc):
normal-diff:
editor = svn_wc_get_diff_editor5(CALLBACKS);
reporter = svn_ra_do_diff3(editor);
svn_wc_crawl_revisions3(reporter);
diff-summarize:
n/a
(wc <> wc):
normal-diff:
svn_wc_diff5(CALLBACKS);
diff-summarize:
n/a
So it looks like one way to fill in the (repos <> wc) summarize case
would be with a new function:
svn_wc_get_diff_summarize_editor()
which would be based on svn_client__get_diff_summarize_editor() and
svn_wc_get_diff_editor5().
However, it would make much more sense if it is possible to write a set
of summarizing diff-callbacks that could be passed to any of the three
existing (normal-diff) code paths:
(repos <> repos):
editor = svn_client__get_diff_editor(SUMMARIZE_CALLBACKS);
...
(repos <> wc):
editor = svn_wc_get_diff_editor5(SUMMARIZE_CALLBACKS);
...
(wc <> wc):
svn_wc_diff5(SUMMARIZE_CALLBACKS);
That would simplify (reduce code duplication in) the existing (repos <>
repos) summarize case as well as trivially supporting the missing cases.
> Now, if I/we could manage to fix the cases where diff is just broken, it
> would be a nice side-effect of tree-conflicts. Is it a -- as stsp would say
> -- huge can of worms, though? What other diff failures are known?
I really don't want to go into that tonight. Like you say, let's make
the use of this diff framework be the incentive for fixing the broken
cases.
> I'd still +1 on carrying on with the diff_summarize solution of dirs_same_p,
> for now ignoring the breaking freak cases. It drastically raises the general
> incentive to fix (and exposure of) broken diff cases.
>
> Julian, your original patch suggestion contains a change to files_same_p(),
> which I've dropped because I didn't understand it then. I'll still see
> whether I'll re-include it:
> * subversion/libsvn_client/merge.c
> (files_same_p): Change to an implementation more like we're going to use
> for directories - one that compares against the repository rather than
> a local temporary file - to help me get up to speed on how to do this.
> (merge_file_deleted): Use the new version of files_same_p().
There is no reason to include that change, unless you know of a reason.
> A couple of tests still fail with my patch, I'll investigate.
> merge_tests.py 3 88 89 114 115 119
> diff_tests.py 45
OK.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-10-09 02:22:29 CEST