Gerco Ballintijn writes:
> On 2/20/2007 4:19 PM, Peter Lundblad wrote:
> >
> > Unfortunately, the editor is not documented to be extensible, so
> > approaches 2 and 3 will require revising that struct, which will be a lot
> > of work.
> >
>
> I'm not sure what you mean with "not documented to be extensible."
> Other structures have changed, right? I'm also not sure why it
Other structs have been extended if they either are documented to be
extensible in the public API (so, the _create/_dup functions are basically
required to be used) or our APIs doesn't consume such structs.
In this case, we have a factory function, but nothing requires a library
to use it. Adding a new field that the library depends on means bad things.
When we revise this interface, we obviously shouldn't remake this
mistake.
> would be a lot of work. By bumping the version number on struct
> svn_delta_editor_t and constructor svn_delta_default_editor you're
> done. Current edit producer and consumer will simply ignore the
> extra method, whether using the old or the new version of struct
> svn_delta_editor_t.
Yes, it is just a simple matter of API revising;)
It is just that it will require lots of revved functions as well.
If you want to do that, I'm all for it (in a separate patch, please).
>
>
> >
> > If someone wants to take that (which we will need to do at some
> > point for other things), that'd be great, but I'm not sure I think
> > this feature need to block on that, especially since it can be useful
> > without the ideal genericity.
> >
>
> Any implementation of this functionality will impact functions and/or
> structures in the public APIs and the changes thus have to stay to
> "2.0". So, avoiding an unsatisfactory solutions here seems desirable
> to me.
Yes. OTOH, no one has come up with a design for something that actually
needs the ability to set revprops at abitrary times, so we don't know if
this solution is satisfactory either. Designing an API for some needs
we don't know yet might just be wasted work.
For example, say we make it possible to set revprops at arbitrary times.
Then we design something that could benefit from having the revprops available
before any file/directory changes are made. Of course, we don't have such
a thing, so we don't know. What we know is that, for the current use cases
we are working on, we have the properties available early.
We don't even know if signed commits will be implemented using revprops
and how it will work.
>
>
> >>>>>
> >>>>> A separate issue was (is) whether to ignore, to filter, or to flag the
> >>>>> standard revision properties as errors when passed in the table.
> >>>>>
> >>>>
> >>>> I think it would make sense to disallow any changes to the svn:
> >>>> properties, since they're already set through other processes (and you
> >>>> wouldn't want, for example, to have to work out whether someone could
> >>>> override svn:author).
> >>>>
> >>>
> >>> But would such a prohibition be for the command-line client *only* or
> >>> for all (relevant) public APIs?
> >>>
> >
> > To make sense, this has to be prohibited on the server side.
> >
>
> This is certainly possible for the svn protocol. I am not so sure
> you can easily recongize the distinction between valid and invalid
> revprop changes in the DAV implementation. I was also wondering
> whether, beside the svn client, libsvn_client would also have to
> perform this check, and possible some of the other libraries.
So, what's the problem we want to solve here?
IMO, the most important thing is to disallow svn:author to be set arbitrarily,
since the author is established using authentication. As I understand
mod_dav_svn, svn:author is set when a transaction is opened if authentication
was required. Couldn't we just make sure the author prop couldn't be
reset if it is already set?
Resetting svn:log doesn't need to be enforced on the server (well,
who cares if someone changes her mind?) and svn:date will be reset anyway.
Regarding svn_client versus cmdline level, I could go either way.
Enforcing it in the library makes live slightly easier for other clients.
>
>
> On 2/24/2007 10:04 PM, Peter Lundblad wrote:
> >
> > Also, while thinking of this, I'm not sure that this commit-specific
> > function belongs in the general editor interface.
> >
>
> Well, I don't think it is commit-specific. The interface changes could
> also be used to implement the svnsync and load/dump stuff. I would say
> its more generic than that. The method should probably be called
> change_edit_prop() to make it more generic. :-)
>
I don't understand why this would be useful for dump. It has direct
access to the repository (and to be useful, it needs the props before
any other editing operations anyway).
For replay, indeed, it would be a marginal performance improvement, avoding
some round-trips per revision.
>
> >
> > An alternative is to have svn_ra_get_commit_editor() return
> > a callback/baton that can be used for this purpose.
> >
>
> Do you mean a callback and baton parallel to the existing edit
> callback and batton? Even "only" changing this method still has a
> significant ripple effect, as I discovered when writing my first
> patch...
Yeah, that was what I was talking about, and yes, we have to do some API
revving. I'm not against that at all. But we need to make sure it is
appropriate and needed before doing it.
>
>
> >
> > We should restrict the use of this callback to either before
> > any editing operations to simplify the implementation.
> >
>
> Why this restriction? This makes it impossible to do the after-
> editing changing of revprops, which people seem to like, and the
> current implementation doesn't seem to need it. Besides in that
> case you can just give the properties straightaway, like I did
> in my first patch.
I meant to say "either before or after" (sorry for the confusion).
(But see my conclusion below.)
>
>
> On 2/26/2007 10:08 AM, Peter Lundblad wrote:
> >
> > Gerco Ballintijn writes:
> >> Index: subversion/include/svn_ra_svn.h
> >> ===================================================================
> >> --- subversion/include/svn_ra_svn.h (revision 23480)
> >> +++ subversion/include/svn_ra_svn.h (working copy)
> >> @@ -43,6 +43,7 @@
> >> #define SVN_RA_SVN_CAP_EDIT_PIPELINE "edit-pipeline"
> >> #define SVN_RA_SVN_CAP_SVNDIFF1 "svndiff1"
> >> #define SVN_RA_SVN_CAP_ABSENT_ENTRIES "absent-entries"
> >> +#define SVN_RA_SVN_CAP_CHANGE_REV_PROP "change-rev-prop"
> >>
> >
> > We shouldn't need a new capability for this. The invocation of a new
> > command shouldn't abort the edit.
> >
>
> Are sure even for the pipe-lined editing case?
Good point, you're right. This is a bit sad, because every extension of
the editor will require capability negotiation. Anyway, we can live with that.
>
>
> >> Index: subversion/include/svn_client.h
> >> ===================================================================
> >> --- subversion/include/svn_client.h (revision 23480)
> >> +++ subversion/include/svn_client.h (working copy)
> >> @@ -762,6 +762,10 @@
> >> /** MIME types map.
> >> * @since New in 1.5. */
> >> apr_hash_t *mimetypes_map;
> >> +
> >> + /** Table holding the extra revision properties to be set.
> >> + * @since New in 1.5. */
> >> + apr_hash_t * revprop_table;
> > ^ Spurious space.
> >
> > More importantly, the documentation needs to be extended to say
> > what the types of the keys and avlues are. Also, what is the relationship
> > between this table and log_msg_func2?
> >
>
> I'm confused: Why would there be a relationship with the log_msg_func2?
Because they can both provide svn:log. Either prohibit svn:log
in the revprops table, document it and enforce it, or document what
happens if one of the log_msg_funcs returns a message and the table contains
the property.
>
>
> >
> >> @@ -406,14 +409,18 @@
> >> the consumer must read and discard edit operations until writing
> >> unblocks or it reads an abort-edit.
> >>
> >> -If edit pipelining is not negotiated, then the target-rev, open-root,
> >> -delete-entry, add-dir, open-dir, close-dir, and close-file operations
> >> -produce empty responses. Errors produced other operations are
> >> -reported by the enclosing close-dir or close-file operation.
> >> +If edit pipelining is not negotiated, then the target-rev,
> >> +change-rev-prop, open-root, delete-entry, add-dir, open-dir, close-dir,
> >> +and close-file operations produce empty responses. Errors produced
> >> +other operations are reported by the enclosing close-dir or close-file
> >> +operation.
> >>
> >> target-rev
> >> params: ( rev:number )
> >>
> >> + change-rev-prop
> >> + params: ( name:string [ value:string ] )
> >> +
> >
> > When is it useful to specify no value? This is normally for deleting
> > properties, but is that applicable here?
> >
>
> I chose this mainly because the lowlevel repos supported it. However,
> I thought I could also prove useful to be able to set and later clear
> a property.
Why make this inconsistent with the other property modification
functions in the editor?
OK, so to conclude:
I think we could extend the editor with a mechanism to set
revision/transaction properties for the target of the editor, but I
think it would be marginally useful. The only use case for setting
revision properties after editor->open_root is called is commits with
signatures, but no one knows how that would work AFAIK. So, I'm
inclined to say that the approach in the first patch is good for our
current purposes and that we should let the design of signed commits
take care of what it needs when that day comes. Gerco, I'm sorry to
waste your work in this way, so as I said before, let's decide on the
design before continuing on the implementation. What do others think?
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Feb 28 09:57:15 2007