Re: Comments on draft-ietf-deltav-versioning
Geoffrey M. Clemm (geoffrey.clemm@rational.com)
Thu, 9 Dec 1999 17:03:43 -0500
Date: Thu, 9 Dec 1999 17:03:43 -0500
Message-Id: <9912092203.AA02694@tantalum>
From: "Geoffrey M. Clemm" <geoffrey.clemm@rational.com>
To: ietf-dav-versioning@w3.org
In-Reply-To: <1999Nov15.105439.1250.1384867@otismtp.ott.oti.com>
Subject: Re: Comments on draft-ietf-deltav-versioning
Great review, Tim! Comments below. (If anyone things anything in
here merits a discussion thread, please generate a separate thread per
discussion topic, with an appropriate new title).
From: Tim_Ellison@oti.com (Tim Ellison OTT)
--------------------
draft-ietf-deltav-versioning-01.1
October 22, 1999
Clemm & Kaler
Section 1.2 Terms
Revision Label
"The same revision label may be assigned to many different versioned
resources."
Being pedantic, the label is assigned to a revision of the versioned
resource.
Fixed.
Section 1.2 Terms
Default Workspace
"workspace may be a modifiable" should read "workspace may be modifiable"
Fixed.
Section 1.2 Terms
Baseline
1) "the revision of every members of" should read "the revision of every
member of"
Fixed.
2) So this implies that all reachable resources must be revisions, not
non-versioned resources, not working resources.
Yes.
3) Should state that only the top-level namespace is captured in a baseline.
Not sure what you mean by this ... please explain.
Section 2.1 Creating versioned resources
"When a resource is created in a versioned collection ...
it is created as a versioned resource."
Why?
So that a versioned collection only contains versioned resources.
Otherwise various key optimizations cannot be applied to reflecting
versioned collections in multiple workspaces.
Section 2.2 Modifying a Versioned Resource
1) I think the comparison drawn in paragraph 2 is unhelpful, since lock and
checkout semantics are so different. It doesn't help to explain anything
about checkout.
In the past, folks have asked for a compare/contrast between using locking
and using versioning.
2) para 2. last sentence. "an update to a checked out resource is only
visible to" should say that it may be visible to other clients, i.e. it is
not necessarily visible since it depends upon their RSR's.
Changed this to say "only visible to clients using that workspace".
Section 3.4 Versioned Resource Properties
1) In general it would be useful to declare the default values, where they
exist, throughout this section (i.e. single-checkout, auto-version, etc.)
2) "the following additional properties" additional to what? guess=resource
Removed the term "additional".
Section 3.4.5 DAV:single-checkout
"when the ... property ... is set" should read "when the ... property ... is
true"
Fixed.
3.4.7 DAV:baselines
Would be helpful to define the members of the collection.
Will do (not done yet).
3.5 Revision Properties
Additional properties to what? guess = versioned resource
Removed the term "additional".
3.5.2 DAV:predecessor
3.5.3 DAV:successors
Would be helpful to state that the href value is workspace independent.
Probably be worth doing a pass to make sure this is made clear in all
contexts where it is true. Perhaps define a term like "stable-href",
and use it consistently?
3.5.8 DAV:activity
"href)" should read "(href)"
Fixed.
3.6 Working Resource Properties
Additional properties to what? guess = revision
Removed the term "additional".
3.6.2 DAV:checkin-policy
1) "if the resource is identical to"
How is identical going to be defined? That there has been no
PUT/PROPPATCH's successfully applied to the resource, or that props and
content are bytewise indistinguishable ... or server defined?
Got rid of the "identical-..." checkin policies.
2) The DTD allows for ANY element as a checkin-policy. The spec should
state what a server should do if it does not support the checkin policy, and
which are mandatory.
Added "must fail if not recognized".
3.7 Workspace Properties
Additional properties to what? guess = resource
Removed the term "additional".
3..2 DAV:revision-selection-rule
1) para. 1 first sentence "versioned resource is selected" should read
"versioned resource that is selected"
Fixed.
2) para 2 "If it selects a particular revision, it may also detect a
conflict" IMHO an RSR that is in conflict should not select any revision.
Done.
3) last para. but 3 re: DAV:rsr-latest. Here latest means chronologically
latest, in section 1.2 Terms Activity latest means last in the succ/pred
lineage. It should be consistent (and IMHO it should be succ/pred lineage).
If DAV:rsr-latest referred to succ/pred lineage, it would be ambiguous
whenever there was branching, and wouldn't achieve the desired effect,
namely "the last revision anyone ever created".
3.8 Activity Properties
3.9 Baseline Properties
Additional properties to what? guess = resource
Removed the term "additional".
3.9.2 DAV:predecessor
"baseline is the baseline that selected by" ?? does not make sense.
Why are all these baseline properties maintained anyway?
So that you can determine baseline history, and branch/merge them.
3.10 Repository Properties
Additional properties to what? guess = resource
Removed the term "additional".
3.10.4 DAV:configurations
"creation of new activities" should read "creation of new configurations"
Fixed.
3.10.6 DAV:default-workspace
Should be the default for all versioned resources that do not specify their
own default workspace (ref. 3.4.2) i.e. the default default.
I was tempted to do this, but I thought it might be simpler to just say
that all versioned resources in a single repository must have the same
default workspace (i.e. the one specified by their repository). I'll go
with whatever people want here.
4.1 GET
Should define the effect of GET on new types introduced by versioning, if
only to say that it is unspecified.
Added "it MUST fail".
4.2 PUT
para. 2 "PUT MUST fail unless the versioned resource has a DAV:auto-version
property and no Target-Selector has been specified."
Why is it conditional on no Target-Selector being specified? this seems
strange.
The idea here is that if someone that is specifying a Target-Selector,
they should know about versioning and it probably is an error on the
part of the client/user if they are trying to modify something that is
not checked out.
4.2 PUT
para 3. "... whose initial revision ..." Why would the initial PUT cause a
revision to be made, and not a working resource? In principle, there is
nothing special about the first PUT that means it should be preserved in
version history.
To ensure that only versioned resources are members of versioned collections.
4.2 PUT
para 4. "... and no Workspace or Revision-Selection header ..."
1) We should agree on the terms for the header, be it 'Target-Selector' or
'Revision-Selection' (of course this will likely be split into separate
headers soon.)
Put back as "Target-Selector" for now.
2) Why must the PUT fail if the workspace and revision-selection have not
been specified?
Which sentence in 4.2 does this refer to?
4.3 PROPFIND
I found this section confusing. I didn't understand what it was trying to
say.
This is the "property resource" report functionality. We agreed to move
this into a separate REPORT, so I'll try to make it clearer when it is
rewritten there (not done yet).
4.5 COPY
para 2.
Why can't we allow COPY to succeed for all but versioned resources?
I predict clients and servers will interpret the COPY differently
no matter how hard we try to specify it, so I think we're better off
just disallowing it.
4.6 DELETE
para. 2
Should specify what happens if the DELETE does not specify 'all bindings'
--> fail?
The Adv. Col group got rid of the AllBindings header, so I just deleted this
sentence.
4.9 LOCK
"A write lock on a versioned resource checks out the target ... into the
default workspace" No -- this cannot be true.
OK, it's gone.
4.10 UNLOCK
"An UNLOCK ... checks in ..." No, please, no! Lock/unlock,
checkin/checkout should be orthogonal.
Also gone.
5.1 MKRESOURCE
1) para. 1 "... standard data containers ..." Can we give these a
friendlier name, I suggest 'ordinary resource'.
Jim Whitehead suggested this name ... Jim: is this change OK?
2) precondition #2, "a DELETE request ... MUST succeed" there is no
explicit DELETE request in this case.
Same comments for the semantics section.
It was implied by the Overwrite. The Adv. Col team decided to get rid
of the Overwrite header for MKRESOURCE, so I just did the same for us,
and this precondition goes away.
3) postconditions. The properties on the resource will be those specified
AND default values.
Are we going to give servers some flexibility as to what the default
values of the properties are?
4) response marshaling. Change 412 (precondition failed) to 405 (method not
allowed) so that it is the same as MKCOL.
No longer have this error case, so just deleted it.
5.1.1 New DAV:resourcetype Values
1) should state that the values MUST be (i.e. caps)
Done.
2) why do the values all end in '-resourcetype' this seems redundant?
We have other elements with the name "workspace", "activity", etc,
which have different DTD's.
3) what is the new type "DAV:versioned-resource-resourcetype" refer to?
Does a resource change type when it is put under version control? Can we
no longer distinguish collections and ordinary resources?
The type of a versioned resource is DAV:versioned-resource-resourcetype.
The type of a revision of a versioned resource will be collection, or whatever.
5.1.2 Example - MKRESOURCE
1) Given that the resource types are elements line 10 should read
<D:resourcetype><D:workspace-resourcetype/></D:resourcetype>
Fixed.
2) The Request-URI was /project1, the response href is bar.html ;-|
Fixed.
3) The response description is strange. The response is a conflict on the
property setting, and the description says that the existing resource could
not be deleted. This description would cause the entire response to return
412 (or 405 proposed).
Replaced this with a "200 OK". (Why make life complicated :-)
5.2.1 DAV:report-request
1) The DTD is wrong, it should be as follows:
<!ELEMENT report-request (available-reports-request | ... )>
<!ELEMENT available-reports-request EMPTY>
etc.
Fixed.
2) The document is inconsistent about the name of the element
'available-reports-request' often calling it ''...-report-...' (singular).
All references need to be fixed up.
Fixed.
5.3 Available REPORT
Heading level too high within the document, since 5.1 is MKRESOURCE and 5.2
is REPORT, this section should be 5.2.x
I cheated in a few places to avoid 4 levels of section nesting ...
this was one such place (:-).
5.3.2 DAV:available-report-response
1) The available reports are not a comma separated list
Fixed.
2) The DTD should be re-written as shown above for 5.2.1
Fixed.
3) The available reports DTD should have a '*' on the end since a server may
support multiple report types.
Fixed.
5.4.2 DAV:conflict-report-response
The DTD calls it conflict(s) plural. All references in the document need to
be fixed up.
Fixed.
5.4.3 to 5.4.5 are logical subparts of 5.4.2 so should be subordinate
heading levels.
Another 4 level avoidance result.
5.4.4 DAV:common-ancestor
Should spec what is returned if there is no common ancestor.
All revisions of a versioned resource have a common ancestor (the
root revision, if nothing else).
5.5.1 DAV:compare-request
First line ends with two periods.
Fixed.
5.5.3 DAV:added
5.5.4 DAV:deleted
5.5.5 DAV:changed
What is the ANY directive for? If it is for expansion maybe should be
multiple ANYs.
Fixed.
5.5.6 Example - Compare REPORT
The response indicates that the target 'myOtherCollection' was both added
and deleted. What does that mean (for a target)?
I just deleted the "delete" of 'myOtherCollection'.
6.1 CHECKOUT
1) para. 1 line 3. should not imply that the server may select a random
workspace, but explicitly state that the server will choose the default
workspace of the versioned resource, or if none, the default workspace of
the repository.
This is the "workspace token" usage of a workspace. If the client requests
a checkout with no workspace, the server allocates one for it.
2) Precondition #4. Why do we have this precondition?
Otherwise the working resource will disappear when it is checked in.
3) Precondition #5 How can this be true? see 3.8.3 for a contradiction.
I have no idea what I had in mind here ... just deleted it.
4) Request marshaling should mention body marshaling?
I believe we agreed in Washington to get rid of the property update
aspect of CHECKOUT, so I fixed it by removing the body.
5) Semantics -- now the server is allocating a workspace for the new working
resource ... ?
If the request did not specify one, yes.
6) Postcondition #4 There is no such property defined (DAV:checked-out).
Fixed (should be DAV:working-resources).
7) Response marshaling, should not be DAV:href (just href)
Fixed.
8) Consider changing the 201 created response to 200 ok?
I'm happy either way. It does create a new working resource, which
could imply a 201 is better?
9) Does the entire CHECKOUT fail if a property update (in the request body)
fails?
Got rid of the property update.
6.1.1 Example - CHECKOUT
1) Groovy example, with the resource on a different server to the workspace.
I thought this was out of scope for this spec. Otherwise we need new and
interesting failure conditions.
The protocol should work just fine with the workspace and versioned resource
being on different servers, but I didn't see this in the example?? In any
case, what failure conditions would you like to see for this case?
2) Response shows <d:workspace> property ... there is no such property on a
working resource. Should it be workspaces (plural) in which case that is a
collection.
Added the DAV:workspace property to working resources (it should have been
there from the start).
6.2 CHECKIN
1) Precondition #1. Why? why not allow "default", c.f. checkout or explicit
working resource URL
Replaced this with "must identify a working resource".
2) Precondition #3. should read "if DAV:identical-abort is the
DAV:checkin-policy"
3) Semantics para. 3 should read "... if the selected working resource is
not a copy ..."
Got rid of identical-abort.
4) Postconditions #3. "... a selected activity ..." should read "... a
current activity ..."
I think "selected activity" is right. A version is checked in as part
of the activity it was checked out to, not the one that is current at
checkin time.
5) Postconditions should mention current label.
Done.
6) Consider changing status code 201 created to 200 ok?
I could go either way. (It does create a new revision resource.)
Any other votes?
6.2.1 Example - CHECKIN
Maybe the response should return the revision URL or at least the revision
ID?
Fixed it to return the revision URL of the new revision.
7.1 Target-Selector
para. 3 should acknowledge that there may be no revision selected.
This is true for all the paragraphs ... why do you think it is
important to point it out in para. 3? (Not disagreeing with you,
just asking for clarification).
Again, great review!
Cheers,
Geoff