I just wanted to point out that SVK already has signed commits and uses
revprops to store the signatures. So it could potentially benefit from
these changes.
Michael
On 2/28/07, Peter Lundblad <plundblad@google.com> wrote:
>
> 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 16:21:41 2007