On Fri, 2009-08-28 at 15:41 +0100, Stefan Sperling wrote:
> On Wed, Aug 26, 2009 at 05:42:30PM +0200, Bert Huijben wrote:
> > > Here is what diffstat has to say about this diff:
> > > include/svn_client.h | 67 -
> > > include/svn_wc.h | 39
> > > libsvn_client/client.h | 11
> > > libsvn_client/deprecated.c | 50 -
> > > libsvn_client/diff.c | 107 --
> > > libsvn_client/merge.c | 3
> > > libsvn_client/patch.c | 1744 ----------------------------------
> > > ---------
> > > libsvn_client/repos_diff.c | 479 -----------
> > > libsvn_wc/deprecated.c | 3
> > > libsvn_wc/diff.c | 1064 --------------------------
> > > svn/diff-cmd.c | 10
> > > svn/main.c | 28
> > > svn/patch-cmd.c | 4
> > > tests/cmdline/patch_tests.py | 267 ------
> > > 14 files changed, 50 insertions(+), 3826 deletions(-)
> > >
> > > Thoughts? Comments? Should we remove this?
> > > If not, ideas about how to reduce the bloat?
> >
> > As noted earlier, we could use skels instead of a reimplementation of the
> > svn protocol. Other suggestions include implementing an editor driver
> > instead of direct calls into libsvn_wc.
> >
> > In its current state I think it is a big code bloat that will be hard to
> > maintain and I would be +1 on disabling until it is better maintainable.
>
> Any more opinions on this?
>
> Are some people planning to review, or still busy reviewing,
> the implementation to make their judgement, and just need some time?
>
> Please post a short note if you are at all interested in this discussion,
> otherwise I will assume that people don't care, and apply the patch in
> a few days.
I am interested. My main observation is: It seems wrong that a whole lot
of new code should be needed in libsvn_wc and libsvn_repos. These should
already be producing diffs in a standard "Subversion-internal" form (the
delta_editor API, I presume) for both "diff" and "merge" purposes. The
translation of this internal form to/from "svnpatch" format should
happen in libsvn_client or thereabouts. If the WC and repos diff
layering is not already structured so this is possible it should become
so. I'm hand-waving on details, of course: there are some kinds of info
that need to be propagated for svnpatch that were not needed for
human-readable diff or for merge, and vice versa, but overall the
principle holds that a single Subversion-internal API should suffice.
Unfortunately, "diff" and "merge" currently use rather different code
paths. We really ought to be able to unify them to a large extent. Neels
looked in to this and found it's not easy. (And the current "diff" code
has some serious omissions, which was the reason for wanting to re-work
it in the first place.) So really the exercise may have to start in
re-working the current "diff" and making it share code and patterns with
the "merge" code path before we can cleanly add "svnpatch" to it.
The suggestion to use GIT's format instead of the current svnpatch
format sounds +1, but my comment above applies in that case too.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2389715
Received on 2009-09-01 12:25:43 CEST