Greg Hudson <ghudson@MIT.EDU> writes:
> I didn't notice that double standard when change #1 was proposed. Now
> that I see the collateral damage change #1 did (by making it
> impossible for an editor implementation to clean up resources after
> the edit without extra bookkeeping), I see the double standard and I
> question the wisdom of change #1.
I agree with you that we made a decision that the editor interface
would not be the enforcer all the editor's semantic constraints. But
the real motivation for change #1, perhaps never very clearly
expressed, was that the relationship between get_editor() and
replace_root() was very confusing -- more confusing than merely
failing to enforce certain constraints.
Here's why: the call to get an editor was *always* followed by a call
to replace_root(). No other editor call could come between them. In
other words, there was no point creating an editor unless one was then
going to call replace_root() immediately ["immediately" in the sense
that any calls inserted between them are irrelevant to the editor,
because they can't affect its data structures].
Well, if we can only do A and B together, and always in the same
order, then what's the difference between A and B? Why are they
separate? They might as well be combined, as they're effectively one
operation anyway.
But I also think getting rid of replace_root() works out well for some
other aspects of the editor interface. Given our organizing principle
that the editor is primarily about tree transformations, and that
other stuff should be done by calling functions on batons (functions
which generally just do something to that baton and don't return a new
baton), then the presence of an externally visible edit_baton didn't
quite fit. If the editor is really about tree transformations, then
having the edit finish when the root directory is closed makes a lot
of sense.
Now, I can't deny that in the case of the XML outputter, close_edit()
was a nice place to handle postfix txdeltas and print out
"</delta-pkg>", and that doing so in close_directory(root) is somewhat
less elegant. However, it wasn't that big a deal to implement for the
XML output editor (uh, I hope), and in general, I think the *real*
issue here remains unaffected by change #1 -- namely, that we have a
special exception to our otherwise perfect depth-first nesting:
postfix textdeltas.
But there's always going to be some sort of special handling necessary
for postfix txdeltas, and in some cases, that special handling will
have to include ref-counting on directories, even now.
Let me offer the update editor as an example:
When close_directory() is called, the update editor can't actually do
anything to the directory (such as bump its revision number) until all
the outstanding file_batons have been closed with close_file(). When
all the file_batons for directory D have been closed, then the
postponed work involved in closing D gets done, *even though* there
may be many other file_batons still open for various other
directories.
The only reason you're able to avoid this in the XML output editor is
that you don't have to go back and do anything to a directory when
that directory's files are finished. You've already output the
"</dir>" tag, and because the <text-delta-ref>'s are just lightweight
metadata, it's acceptable to keep them all in one high-level pool and
ref-count just on that pool, instead of per-directory.
This exception to perfect depth-firstness in the editor constraints is
like a bump in a rug: if we try to push it down in one place, it will
just pop up in another. I think we just have to deal with it -- it's
the editor interface's nod to efficiency, to early detection of
conflicts.
It's my gut feeling that by removing replace_root() and close_edit(),
we've removed a source of confusion, without forcing too much extra
complexity into editor implementations. Plus, some editors'
replace_root() and close_edit() functions didn't do much anyway, and
those editors have effectively gotten simpler, because the two
extraneous functions no longer need to be implemented at all.
I don't think we're going to find some beautiful way around the fact
that we postpone some work on files, and that the postponement affects
what we do with their parent directories. But my experience with the
update editor is that dealing with this slight oddness is not really
so hard, and (correct me if I'm wrong) I think you didn't have so much
trouble dealing with it in the XML output editor either.
Is there some particular case that you're worried will get a lot
harder to handle without a close_edit() function? I think we've
already taken care of the two most-affected editors, and they're none
the worse for it.
-K
> I grant that if almost every editor needs to track when a directory is
> finished, then tracking when the root directory is finished isn't
> extra bookkeeping. But I tend to think that the wc update editor is
> fairly unique in this regard; and in fact, I don't think there's even
> an instrinsic need in the wc update editor. All that stuff could be
> done in one fell swoop at the end of the edit without consuming any
> additional resources, if postfix text deltas are being used.
>
> I'll restate my opinion on the editor interface:
>
> * I still think we should get rid of directory and file
> batons, just as I did back in October. Relative to a single
> edit baton (plus explicit one-time-use batons for deferred
> text deltas), they invite more type-safety issues, create
> all sorts of invalid uses of the editor, and add complexity.
> All they give you in return is the ability (which we don't
> actually take advantage of) to notice quickly when a caller
> forgets to close a directory. Plus a hedge in case we ever
> decide to allow non-hierarchial editing, but I don't think
> that kind of hedge is valuable.
>
> * In the presence of the above change, I think the editor
> interface should look more like the byte stream interface,
> with the editor baton stored in the structure and callers
> using wrapper functions; for example:
>
> err = svn_edit_replace_directory(editor, "foodir");
> ...
> err = svn_edit_close_directory(editor);
>
> instead of edit_fns->replace_directory(baton, "foodir") like
> we have now.
>
> * In the absence of the above changes, I think there needs to
> be an explicit call when the edit is over. If that means
> restoring the editor baton and making get-editor return
> three pieces of information (edit_fns, editor baton, root
> directory baton), so be it.
Received on Sat Oct 21 14:36:20 2006