Okay, I'm ready to admit that Change #1 is bad, Greg Hudson is right.
Change #1 got rid of the replace_root(), close_edit() and the global
edit_baton, meaning that
1. Now getting an editor is equivalent to the old sequence
get_editor(), replace_root().
2. Closing the root directory baton now often does some extra work,
namely, the work that close_edit() used to do.
The argument in favor went roughly like this: since getting an editor
was always followed by exactly one call to replace_root(), and no
other editor call could come between them, they are logically one
operation and should be unified.
What we failed to realize is that this assumption doesn't always hold.
Getting an editor was *not* always followed immediately by
replace_root() -- sometimes it was followed by close_edit() instead,
such as when there are no changes to report.
Why is this important? Here are two examples:
1. svn_wc_crawl_local_mods() takes an editor and its baton, then
crawls over the working copy reporting changes for commit. It
doesn't know if it's committing to an xml file or to a
repository, it just calls into the editor it was handed. One
important feature of the crawler was that if it found no local
changes at all, it never even called replace_root(). Thus, in
an XML commit, no XML output would be produced at all, and in a
real commit, it would be easy for the editor to avoid generating
any network traffic.
But now, since creating the editor includes the effect of
replace_root(), the caller who passes the editor and baton to
svn_wc_crawl_local_mods() has already done too much. Although
only the crawler has enough information to know whether or not
to do anything, its caller has already forced the issue. Bad.
The only solution I can see is to pass a factory function into
the crawler and having it dynamically generate the editor at the
time when it formerly called replace_root(). I tried writing a
prototype and doc string for the new crawler, and while the
resulting interface wasn't particularly awful on their face, the
implications for existing callers really weren't pretty. The
commit editor is usually composed with a trace_editor or two;
now instead of doing the composition up front and passing the
result as a ready-to-use tool to whoever needs it, we'd have to
defer it into the callee, and any context required for the
creation of the editor would have to be wrapped up in a baton
and passed along as part of a factory closure to the editor
driver. Haven't we got enough of that going on already?
2. The update editor is acquired from the working copy library and
handed to some other layer (say, ra_dav), which drives the
editor if there are any changes. If there are no changes, it
doesn't do anything -- in the old way, replace_root() would
never be called.
Now the update editor can't even be created until its driver
knows whether or not there are changes to drive. Transplant all
the objections from above and edit appropriately, and you'll see
the situation isn't pleasant.
Here's another way to think of the situation:
There are two interested parties for every editor: the editor's
creator, and its driver. They are not usually the same code; instead,
the creator creates the editor of its choice and passes it to a driver
of its choice. Thus, the creator is usually code which orchestrates
different libraries, and the editor is one of the tools it uses to get
those libraries to talk to each other.
Now, the editor may need some context that is best supplied from the
creator side, and other context best supplied by the driver side. By
fully creating the editor before passing it to the driver, the creator
can use whatever context it has handy (without going through the
trouble of wrapping it up in a factory baton). The driver transmits
its context through the editor calls we all know and love (plus
proposed new ones such as set_revision).
By deferring editor creation, we'd be arranging things such that the
editor can be given context in only *one* of the two places where it
might need it. So any context from the other place has to be wrapped
up in brown paper and passed along to the other side. I think that
results in more work, more complexity, and less comprehensibility.
(Sure, it might mean we could get rid of editor functions like
set_revision(), since now the editor creation would be happening at a
time when the revision information is available, but that benefit
seems pretty minor when compared with the complexity added to callers
of drivers.)
I now agree with Greg Hudson that Change #1 is just going to be a pain
in the long run, and would like to revert it (we'd still keep Change
#8, of course, that one totally rocks).
But, we'd do two important things differently this time:
1. Document that merely getting an editor doesn't actually "do"
anything (with the definition of "do" made appropriately
explicit), and
2. Call it start_edit() instead of replace_root(), and document
that it can only be called once per edit. Maybe change our
editors to enforce this, too. (As Greg H pointed out, there are
a zillion wrong ways to use an editor, so it's no big deal if
there are a zillion + 1 instead.)
Thoughts?
-Karl
Received on Sat Oct 21 14:36:21 2006