You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Glasser <gl...@mit.edu> on 2006/08/10 03:14:55 UTC
atomic revprop commits at the RA level
[I couldn't find anything in the archives about this when I searched;
I suspect this says more about my searching skills than anything else,
so sorry if this has been discussed a lot before.]
Currently the repos/fs layers allow for revprops to be set on new
revisions atomically when they are committed, by setting "transaction
properties"[1]. However, the ra layer doesn't allow that:
svn_ra_commit_editor2 just returns a standard svn_delta_editor_t and
doesn't give any hooks to say "and set this revprop on this revision".
This would be a useful feature for several reasons; for example, it
would make repository mirroring tools like svnsync and SVN::Mirror
much simpler, and it would allow the enforcement of policies like "can
only commit revisions that have been cryptographically signed with the
signature in some revprop". It would also help with Issue 1976.
I'm interested in giving this a shot. I'm not quite sure how to make
its API, though.
The ra layer specifies that between calling svn_ra_get_commit_editorN
and finishing the edit on its associated editor, the caller may not
perform any other RA operations. Thus, one way of adding this feature
would to make an API like:
svn_error_t *svn_ra_set_rev_prop_for_commit(svn_ra_session_t *session,
const char *name, const
svn_string_t *value,
apr_pool_t *pool)
and specify that it's *only* legal to call this in the time between
svn_ra_get_commit_editorN and its close_edit (or abort_edit). This is
nice, because it's minimally invasive --- it doesn't even invalidate
any old APIs, just relaxes one of the constraints on use.
On the other hand, it seems a little odd to have a "get_commit_editor"
call whose main feature is to return a commit editor, but to then have
something affecting the commit not go through what it hands back at
all but rather go straight to the session. While as far as I can tell
it wouldn't be too hard to implement what I've described (well, I've
only looked at ra_local and ra_svn so far), it does smell a little of
"action at a distance".
The other alternative would be to deprecate svn_ra_get_commit_editor2
for an svn_ra_get_commit_editor3 which returns a "transaction baton"
or something instead of an svn_delta_editor_t; every normal delta
editor call would need to have an extra indirection to get at the
editor, but this object would also support the "set revprop"
operation. This seems like a pretty huge change for a relatively
minor feature; while it's certainly backwards compatible, it seems
like it would annoying for users of svn_ra who don't care about this
feature to upgrade.
So is the former API clean enough to be worth implementing, or is the
latter not too invasive, or is there some other option that I'm
completely missing?
--dave
[1] Whose purpose is as far as I can tell not actually documented in
svn_fs.h; I'll send a patch separately.
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by David Glasser <gl...@mit.edu>.
On 8/11/06, David Glasser <gl...@mit.edu> wrote:
> * No support in any of the WebDAV-related modules. I am not
> particularly familiar with any of them; I'd be happy if somebody who
> was wanted to write their implementations, but also could certainly
> dive in myself.
I looked into the WebDAV side of things. Interestingly, it appears to
me that mod_dav_svn already supports setting transaction properties:
the request that ra_dav and ra_serf use to set a new revision's log
message appears (though I haven't tested this) to also work for
arbitrary properties. It does not look like it would be incredibly
hard to make ra_dav and ra_serf use this feature.
The one bit that looks tricky here is that it looks like for ra_serf,
a txn won't actually be started during a call to ra_get_commit_editor;
it takes until the first open_root for the MKACTIVITY to go. (In
fact, ra_serf's get_commit_editor does no network activity at all.)
However, you really should be able to run set_revprop_for_commit
between get_commit_editor and open_root; you wouldn't be able to
combine it with ra_replay otherwise.
What downsides would there be to making ra_serf initialize its commit
in get_commit_editor instead of in open_root (like all of the other ra
modules)? I suppose it saves network resources if a client makes an
editor but never drives it, but that seems unlikely. (Note, by the
way, that my patch is already moving txn creation into
svn_*repos*_get_commit_editorN from its open_root.)
Anyway, more food for thought until folks get a chance to think about
this approach/patch.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by David Glasser <gl...@mit.edu>.
On 8/11/06, David Glasser <gl...@mit.edu> wrote:
> * Tested this using the ra_local C tests and svnsync. Currently passes
> "make check".
Er, um, sent a moment too soon. One other note:
svn:date just plain can't be set through txn properties: it'll always
get overwritten by the current time at the end. (This isn't really
documented.) This means that my patch to svnsync makes it fail all of
its tests.
This issue is kind of independent of the actual patch here, though;
for example, it would also prevent you from making a repos-level
svnsync-like tool which tries to copy all revprops atomically.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by David Glasser <gl...@mit.edu>.
On 8/9/06, David Glasser <gl...@mit.edu> wrote:
> So is the former API clean enough to be worth implementing, or is the
> latter not too invasive, or is there some other option that I'm
> completely missing?
I decided to give this a shot. Attached is a patch which begins to
implement it; the big obvious lack right now is that it only works
with ra_local and ra_svnserve. This is by far the biggest subversion
patch I've worked on, so I figure it's best to stick it out for review
as soon as possible.
Notes:
* I recognize that the thread about this has not really settled on a
choice of API, and some folks support adding this capability
to the svn_delta_editor_t type. Arguably neither way of doing it is
really clean: either you pollute the delta description with extra
properties, or you allow one (and only one) RA API to be used in the
middle of the editor stream. I currently prefer the latter, since
the other way would require upgrading about 50 APIs that use
svn_delta_editor_t, and this one just requires one new API and one
API documentation change (relaxing a "don't do this" constraint).
(Well, and indirectly two other API bumps.)
However, I seriously do not have much ego invested in this
particular way of doing things, and if committers want to say
"thanks for the work, but we don't like the API", then, well, that's
how it is. It's been pretty educational anyway.
* Do I need to put anything in libsvn_ra/wrapper_template.h? That is,
do I need to support the use of a brand new RA API in the old
ra_plugin conventions?
* Similarly, do I have to support the use of a brand new svnserve command
under non-pipelined editing?
* With ra_svn, the only way that you can find out that the remote
repository doesn't
support set-rev-prop is by trying it and getting an error back. As
far as I can tell this
is how the locking commands work too. Presumably users of this API who want
to be able to work against older servers will need to explicitly
trap the "command
not recognized" error.
* An earlier version of this patch didn't create
svn_repos_get_commit_editor5 and instead had each of its relevant
invokers manually create and deal with aborting the txn. This was a
huge pain, since I basically ended up needing to copy-and-paste code from
libsvn_repos to create the txns and then create wrapper editors
to deal with abortion; this effort was duplicated in both RA implementations.
I think the svn_repos_get_commit_editor5 API is saner than trying to simulate
it without it existing.
It does create a slight behavior change, though it's still
consistent with the API's documentation: the managed txn is now made
in svn_repos_get_commit_editorN itself instead of waiting until
open_root is called. Is this a problem?
* I changed svnsync to use the new API, mostly for testing
purposes. The version in this patch does *not* fall back to
non-atomic revprop copying if the server doesn't support it, since
for debugging purposes it's helpful to know that the server isn't
supporting it yet. I will certainly add that to a final patch.
* No support in any of the WebDAV-related modules. I am not
particularly familiar with any of them; I'd be happy if somebody who
was wanted to write their implementations, but also could certainly
dive in myself.
* Tested this using the ra_local C tests and svnsync. Currently passes
"make check".
[[[
New svn_ra_set_revprop_for_commit API, which allows atomically setting
revprops at initial commit time over the RA layer.
* subversion/include/svn_ra.h
(svn_ra_set_revprop_for_commit): New RA API.
(svn_ra_get_commit_editor2): Document that it's OK to call the
new RA API during an editor session.
* subversion/libsvn_ra/ra_loader.c
(svn_ra_set_revprop_for_commit): Implement vtable dispatch for new
RA API.
* subversion/libsvn_ra/ra_load.h
(svn_ra__vtable_t): Add slot for new RA API.
* subversion/include/svn_error_codes.h
(SVN_ERR_RA_SVN_INVALID_SET_REVPROP): New error for use of
set-rev-prop command in a non-committing editor session.
* subversion/include/svn_repos.h
(svn_repos_get_commit_editor5): API upgrade to allow the caller
to request access to a repos-managed txn.
* subversion/include/svn_ra_svn.h
(svn_ra_svn_drive_editor2): API upgrade to give driver access to
txn.
* subversion/libsvn_ra_local/ra_plugin.c
(svn_ra_local__get_commit_editor): Get txn from repos commit editor.
(svn_ra_local__set_revprop_for_commit): Implement new RA API.
(ra_local_vtable): Add implementation of new RA API to vtable.
* subversion/libsvn_ra_local/ra_local.h
(svn_ra_local__session_baton_t): Add slot for txn.
* subversion/tests/libsvn_ra_local/ra-local-test.c
(set_revprops_atomically, commit_callback): Add new test
which tests new RA API on ra_local (and for that matter
tests commits).
(test_funcs): Call new test.
* subversion/svnsync/main.c
(copy_revprops_atomically): Like copy_revprops, but uses new RA API;
used when copying a revision for the first time.
(do_synchronize): Use copy_revprops_atomically instead of copy_revprops
when you can.
* subversion/libsvn_repos/commit.c
(open_root): Move managed txn creation from here to ...
(svn_repos_get_commit_editor5): ... here, which is an upgraded
API which can return the managed txn.
* subversion/libsvn_ra_svn/client.c
(ra_svn_set_revprop_for_commit): Implement new RA API.
(ra_svn_vtable): Add implementation of new RA API to vtable.
* subversion/libsvn_ra_svn/protocol:
Document new set-rev-prop editor command.
* subversion/libsvn_ra_svn/editor.c
(svn_ra_svn_drive_editor2): Upgrade API to accept txn.
* subversion/libsvn_ra_svn/editorp.c
(ra_svn_driver_state_t): Add slot for txn.
(ra_svn_handle_set_revprop_for_commit): Handle new RA API.
(ra_svn_edit_cmds): Add handler for new RA API.
(svn_ra_svn__drive_editorp): Store the given txn in the
driver state.
* subversion/libsvn_ra_svn/ra_svn.h
(svn_ra_svn__drive_editorp): Take an extra txn argument.
* subversion/svnserve/serve.c
(commit): Get txn from repos_get_commit_editor5 and pass it
through to svn_ra_svn_drive_editor2.
]]]
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
Re: atomic revprop commits at the RA level
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/10/06, C. Michael Pilato <cm...@collab.net> wrote:
> Sorry, but I oppose this. The editor is a mechanism for transforming
> versioned trees, not transactions, not revisions. There isn't anything to
> date in the editor that ties it to the idea that it is modifying a
> transaction instead of just "some tree", and I see no reason to change that.
>
> Extending the editor is *not* the "only sane design". For example, look at
> what svn_ra_get_commit_editor does today with respect to the commit log
> message -- it accepts it as direct input and passes that data over the wire
> out-of-band with respect to the editor drive. Why not just drop the
> 'log_msg' parameter from this function, and replace it with a generic
> 'txn_props' hash?
One potential issue here is that you have to know the props before you
start the editor drive. I know at least one person has asked about
the possibility of determining the log message later on in the commit
process (although I can't for the life of me recall why), so it might
be desireable to provide a means that allows for the props to be
specified later on.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by David Glasser <gl...@mit.edu>.
On 8/14/06, C. Michael Pilato <cm...@collab.net> wrote:
> Yes, change_edit_props should be allowed before open_root(). This doesn't
> mean, of course, that the commit editors which currently delay this action
> until open_root() must now create the transaction at editor-creation time --
> they can just delay it until either of open_root() or change_edit_props()
> occurs.
>
> One additional thing to watch for is that we don't make it super-easy for
> folks to commit revisions that have no tree changes, just property mods.
> Not sure what the best mechanism for doing so is, though.
Aha, actually doing what you say in the first paragraph will make what
you say in the second paragraph happen. (Looks like the delay of
transaction-creation until open_root in the repos committer is exactly
what prevents "empty" commits from happening.)
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by "C. Michael Pilato" <cm...@collab.net>.
Garrett Rooney wrote:
> On 8/14/06, David Glasser <gl...@mit.edu> wrote:
>> * Should change_edit_props be allowed before open_root? If not, then
>> we definitely need to make the change to r(epos|a)_replay. If so, some
>> code in repos and ra_serf will have to be changed to do some
>> initialization in get_commit_editor instead of in open_root (or to
>> cache edit prop changes until open_root); I'm not sure why those
>> editors choose to wait until open_root to really start things, and if
>> it's important behavior to preserve.
>
> ENOCLUE here.
Yes, change_edit_props should be allowed before open_root(). This doesn't
mean, of course, that the commit editors which currently delay this action
until open_root() must now create the transaction at editor-creation time --
they can just delay it until either of open_root() or change_edit_props()
occurs.
One additional thing to watch for is that we don't make it super-easy for
folks to commit revisions that have no tree changes, just property mods.
Not sure what the best mechanism for doing so is, though.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: atomic revprop commits at the RA level
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/14/06, David Glasser <gl...@mit.edu> wrote:
> OK. So let's say we're going to do this. Does this seems like a
> reasonable plan:
>
> * Revise svn_delta_editor_t to have a change_edit_prop whose default
> implementation returns an error, and upgrade all ~50 APIs that use it.
> * Make the change_edit_prop returned by svn_repos_get_commit_editorN
> actually set the txn property.
> * Modify the ra backends and frontends so that the editors returned
> from svn_ra_get_commit_editorN so that they support the property.
> * (Maybe) Add a boolean to r(epos|a)_replay which asks it to issue
> change_edit_prop calls.
This is basically the approach to take, yeah.
> Two questions:
> * While we're going to the trouble of changing svn_delta_editor_t and
> thus adding compat versions of ~50 APIs, does it make sense to make
> the new editor type opaque (so that future additions of
> default-erroring methods wouldn't require changing all the APIs)? Or
> would that lead to other problems (like breaking bindings or
> something)?
FWIW, there's a half finished attempt at reving the editor interface
available in the fs-atomic-renames branch. We talked about making it
opaque, but you_end_up_with_these_really_long_api_names_from_hell so
we decided to simply document that users are required to use the
constructor to create an editor, so we'll always have the ability to
add new functions (as long as we can give them sane default
implementations). If we had documented the requirement to use
svn_delta_default_editor() for creating all editors when we wrote it
(pre-1.0) in the first place we'd be in much less pain now, since we
could still add stuff at the end of the structure, just like we do
with svn_client_ctx_t now.
> * Should change_edit_props be allowed before open_root? If not, then
> we definitely need to make the change to r(epos|a)_replay. If so, some
> code in repos and ra_serf will have to be changed to do some
> initialization in get_commit_editor instead of in open_root (or to
> cache edit prop changes until open_root); I'm not sure why those
> editors choose to wait until open_root to really start things, and if
> it's important behavior to preserve.
ENOCLUE here.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by David Glasser <gl...@mit.edu>.
On 8/14/06, C. Michael Pilato <cm...@collab.net> wrote:
> > I know one interesting idea that floated by (in IRC) was to create a
> > *new* vtable. This vtable would contain metadata funcs, and also an
> > editor...
>
> This is why I got over it. I started to propose this very thing last week,
> but when I realized just how awful it was, I folded.
Yeah; right now it looks to me that making r(epos|a)_get_commit_editor
return a new vtable containing a set_prop and an editor might not be
too bad, but trying to move set_target_revision out to the "meta"
vtable looks a little crazy, since as far as I can tell the majority
of the APIs that use editors end up using it anyway (for better or for
worse).
> That's why I proposed 'editor->change_edit_prop()'. It wasn't really that I
> was opposed to the editor carrying metadata about the edit, it was that
> editors shouldn't know anything about txns as revisions-in-progress (though
> I might not have expressed that well). But as long as we talk about "edit
> metadata" (which *we* know is a mechanism that can be used for, among other
> things, carrying txn props), it doesn't rub me quite as wrongly.
OK. So let's say we're going to do this. Does this seems like a
reasonable plan:
* Revise svn_delta_editor_t to have a change_edit_prop whose default
implementation returns an error, and upgrade all ~50 APIs that use it.
* Make the change_edit_prop returned by svn_repos_get_commit_editorN
actually set the txn property.
* Modify the ra backends and frontends so that the editors returned
from svn_ra_get_commit_editorN so that they support the property.
* (Maybe) Add a boolean to r(epos|a)_replay which asks it to issue
change_edit_prop calls.
Two questions:
* While we're going to the trouble of changing svn_delta_editor_t and
thus adding compat versions of ~50 APIs, does it make sense to make
the new editor type opaque (so that future additions of
default-erroring methods wouldn't require changing all the APIs)? Or
would that lead to other problems (like breaking bindings or
something)?
* Should change_edit_props be allowed before open_root? If not, then
we definitely need to make the change to r(epos|a)_replay. If so, some
code in repos and ra_serf will have to be changed to do some
initialization in get_commit_editor instead of in open_root (or to
cache edit prop changes until open_root); I'm not sure why those
editors choose to wait until open_root to really start things, and if
it's important behavior to preserve.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by "C. Michael Pilato" <cm...@collab.net>.
Ben Collins-Sussman wrote:
> In IRC and email, I know that cmpilato was really opposed to adding a
> new 'metadata' function to the editor API. I say metadata, because
> the editor is supposed to be about "how to change a versioned tree',
> and not about information surrounding the tree itself. Revprops are
> metadata. One could also argue that editor->set_target_revision() is
> also metadata about the tree, and should never have been put in.
I was opposed, yes. I got over it.
> I know one interesting idea that floated by (in IRC) was to create a
> *new* vtable. This vtable would contain metadata funcs, and also an
> editor...
This is why I got over it. I started to propose this very thing last week,
but when I realized just how awful it was, I folded.
That's why I proposed 'editor->change_edit_prop()'. It wasn't really that I
was opposed to the editor carrying metadata about the edit, it was that
editors shouldn't know anything about txns as revisions-in-progress (though
I might not have expressed that well). But as long as we talk about "edit
metadata" (which *we* know is a mechanism that can be used for, among other
things, carrying txn props), it doesn't rub me quite as wrongly.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: atomic revprop commits at the RA level
Posted by Ben Collins-Sussman <su...@red-bean.com>.
In IRC and email, I know that cmpilato was really opposed to adding a
new 'metadata' function to the editor API. I say metadata, because
the editor is supposed to be about "how to change a versioned tree',
and not about information surrounding the tree itself. Revprops are
metadata. One could also argue that editor->set_target_revision() is
also metadata about the tree, and should never have been put in.
I know one interesting idea that floated by (in IRC) was to create a
*new* vtable. This vtable would contain metadata funcs, and also an
editor...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by David Glasser <gl...@mit.edu>.
On 8/10/06, C. Michael Pilato <cm...@collab.net> wrote:
> Suggest:
>
> svn_error_t *(*change_edit_prop)(void *edit_baton,
> const char *name,
> const svn_string_t *value,
> apr_pool_t *pool);
>
> The idea being we're modifying a property of the edit itself, not the tree
> that's being edited.
After trying out the other way (making this a new ra API that works
kind of in parallel with the editor), writing half a patch, and
putting it aside for a couple of days, I think I agree that going
through the editor is cleaner.
If we want to add a function to the svn_delta_editor_t struct, that
means we have to make a new svn_delta_editor2_t struct and bump the
~50 APIs that use it, right?
Would it be reasonable to, while we're at it, make svn_delta_editor2_t
an opaque type accessed through svn_delta_open_root(editor, ...)
functions and so on? This would allow future additions to the editor
API without needing to bump 50 APIs. On the other hand, maybe adding
methods to the delta editor API should be painful :)
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/10/06, David Glasser <gl...@mit.edu> wrote:
> On 8/10/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > I'd say that the default implementation should return an error, for
> > just this reason.
>
> How do "default implementations" of editor calls work?
I explained this ot David on IRC, but for the record, we have a
function that returns a "default" version of the editor, with no-op
functions in place for each operation. I was suggesting that the
default implementation of this function would return an error, and
you'd replace it with one that does something for editors that can
actually do this.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by David Glasser <gl...@mit.edu>.
On 8/10/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> I'd say that the default implementation should return an error, for
> just this reason.
How do "default implementations" of editor calls work?
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/10/06, David Glasser <gl...@mit.edu> wrote:
> On 8/10/06, C. Michael Pilato <cm...@collab.net> wrote:
>
> > Alright. I can choke down the idea of putting this into the editor if we
> > can spin it like something generic, *not* as a "txn" or "revision" prop.
> > (Read: other ideas I've come up with lack either flexibility or finesse, or
> > both.)
> >
> > Suggest:
> >
> > svn_error_t *(*change_edit_prop)(void *edit_baton,
>
> OK. And it would be documented as "for many editors, this does
> absolutely nothing"? (That is, for an editor that isn't trying to
> create a txn/revision, like a diff or something.)
I'd say that the default implementation should return an error, for
just this reason.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by David Glasser <gl...@mit.edu>.
On 8/10/06, C. Michael Pilato <cm...@collab.net> wrote:
> Alright. I can choke down the idea of putting this into the editor if we
> can spin it like something generic, *not* as a "txn" or "revision" prop.
> (Read: other ideas I've come up with lack either flexibility or finesse, or
> both.)
>
> Suggest:
>
> svn_error_t *(*change_edit_prop)(void *edit_baton,
OK. And it would be documented as "for many editors, this does
absolutely nothing"? (That is, for an editor that isn't trying to
create a txn/revision, like a diff or something.)
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by "C. Michael Pilato" <cm...@collab.net>.
Eric Gillespie wrote:
>>Suggest:
>
>
>> svn_error_t *(*change_edit_prop)(void *edit_baton,
>> const char *name,
>> const svn_string_t *value,
>> apr_pool_t *pool);
>
> Why not make it a hash, as you suggested when you suggested
> modifying svn_ra_get_commit_editor? No sense in adding
> unnecessary round trips...
There's nothing in this API that necessitates roundtrips. If the editor
implementor wishes to cache all the edit-prop changes and send them at one
time (maybe at close_edit() time), it can choose to do so. Using a hash,
however, *forces* the editor driver to harvest all those properties into
memory at once. I don't see the value in that.
Plus, the interface as provided is consistent with the change_dir_prop() and
change_file_prop() interfaces, and consistency r00lz.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: atomic revprop commits at the RA level
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"C. Michael Pilato" <cm...@collab.net> writes:
> Alright. I can choke down the idea of putting this into the
> editor if we can spin it like something generic, *not* as a
> "txn" or "revision" prop. (Read: other ideas I've come up with
> lack either flexibility or finesse, or both.)
> Suggest:
> svn_error_t *(*change_edit_prop)(void *edit_baton,
> const char *name,
> const svn_string_t *value,
> apr_pool_t *pool);
Why not make it a hash, as you suggested when you suggested
modifying svn_ra_get_commit_editor? No sense in adding
unnecessary round trips...
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by "C. Michael Pilato" <cm...@collab.net>.
David Glasser wrote:
> On 8/10/06, C. Michael Pilato <cm...@collab.net> wrote:
>
>> Extending the editor is *not* the "only sane design". For example,
>> look at
>> what svn_ra_get_commit_editor does today with respect to the commit log
>> message -- it accepts it as direct input and passes that data over the
>> wire
>> out-of-band with respect to the editor drive. Why not just drop the
>> 'log_msg' parameter from this function, and replace it with a generic
>> 'txn_props' hash?
>
>
> I'd think you'd still want to explicitly specify log message, since it
> wouldn't be good to end up with logless transactions, and making the
> client specifically know about "svn:log" (or SVN_PROP_REVISION_PROP).
>
> And I agree with Garrett that you might want to set the property later
> --- that would be good for revision signing, for example.
Alright. I can choke down the idea of putting this into the editor if we
can spin it like something generic, *not* as a "txn" or "revision" prop.
(Read: other ideas I've come up with lack either flexibility or finesse, or
both.)
Suggest:
svn_error_t *(*change_edit_prop)(void *edit_baton,
const char *name,
const svn_string_t *value,
apr_pool_t *pool);
The idea being we're modifying a property of the edit itself, not the tree
that's being edited.
Thoughts?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: atomic revprop commits at the RA level
Posted by David Glasser <gl...@mit.edu>.
On 8/10/06, C. Michael Pilato <cm...@collab.net> wrote:
> Extending the editor is *not* the "only sane design". For example, look at
> what svn_ra_get_commit_editor does today with respect to the commit log
> message -- it accepts it as direct input and passes that data over the wire
> out-of-band with respect to the editor drive. Why not just drop the
> 'log_msg' parameter from this function, and replace it with a generic
> 'txn_props' hash?
I'd think you'd still want to explicitly specify log message, since it
wouldn't be good to end up with logless transactions, and making the
client specifically know about "svn:log" (or SVN_PROP_REVISION_PROP).
And I agree with Garrett that you might want to set the property later
--- that would be good for revision signing, for example.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: atomic revprop commits at the RA level
Posted by "C. Michael Pilato" <cm...@collab.net>.
Ben Collins-Sussman wrote:
> On 8/9/06, David Glasser <gl...@mit.edu> wrote:
>
>> The other alternative would be to deprecate svn_ra_get_commit_editor2
>> for an svn_ra_get_commit_editor3
>
>
> This is a feature we've wanted for a very long time. I think the only
> sane design is to extend our editor vtable to include a function for
> attaching properties to the transaction.
>
> Once upon a time, we envisioned the editor vtable as being perfectly
> symmetric... being used to commit from client to server, and to update
> from server to client. But it's already got a few asymmetries (like
> editor->set_target_revision, for example, which only goes from
> server->client.) A txn_propset function would be a new asymmetry, so
> it's ugly in a small sense, but overall I think it fits our design of
> 'how commits work'. If you want to do a commit, you get *one* vtable.
> It's a clean and simple.
>
> So I'd very much like to see a rev'd editor. As long as we're going
> to go through such pain... weren't there other things we wanted to add
> to the editor?
Sorry, but I oppose this. The editor is a mechanism for transforming
versioned trees, not transactions, not revisions. There isn't anything to
date in the editor that ties it to the idea that it is modifying a
transaction instead of just "some tree", and I see no reason to change that.
Extending the editor is *not* the "only sane design". For example, look at
what svn_ra_get_commit_editor does today with respect to the commit log
message -- it accepts it as direct input and passes that data over the wire
out-of-band with respect to the editor drive. Why not just drop the
'log_msg' parameter from this function, and replace it with a generic
'txn_props' hash?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: atomic revprop commits at the RA level
Posted by Ben Collins-Sussman <su...@red-bean.com>.
On 8/9/06, David Glasser <gl...@mit.edu> wrote:
> The other alternative would be to deprecate svn_ra_get_commit_editor2
> for an svn_ra_get_commit_editor3
This is a feature we've wanted for a very long time. I think the only
sane design is to extend our editor vtable to include a function for
attaching properties to the transaction.
Once upon a time, we envisioned the editor vtable as being perfectly
symmetric... being used to commit from client to server, and to update
from server to client. But it's already got a few asymmetries (like
editor->set_target_revision, for example, which only goes from
server->client.) A txn_propset function would be a new asymmetry, so
it's ugly in a small sense, but overall I think it fits our design of
'how commits work'. If you want to do a commit, you get *one* vtable.
It's a clean and simple.
So I'd very much like to see a rev'd editor. As long as we're going
to go through such pain... weren't there other things we wanted to add
to the editor?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org