- From: Geoffrey M. Clemm <geoffrey.clemm@rational.com>
- Date: Thu, 18 Jan 2001 14:58:00 -0500 (EST)
- To: ietf-dav-versioning@w3.org
Tim: What an outstandingly thorough and insightful review!!!
The only criticism I'd make is that it kept me up all night making the
suggested corrections, so please forgive any incoherence in this
message (:-).
--------------------------
From: "Tim Ellison" <tim@peir.com>
General beefs:
There are a number of places where the text says a property must contain
a version etc. when it means that the property must "contain the URL of" a
version etc.
Fixed. (investigated all uses of the term "contain")
In the 'OPTIONS Additional Marshalling' sections there are DTDs for the
request and the response without any indication of which is which. Either
some text, or a subheading would be useful.
Done.
Trivia:
(Nothing of any significance here, just some things that I thought would add
a little more clarity or questions that came to mind as I was reading
through the document.)
Although these changes do not change any semantics (good thing, given how
close we are to last call), the improved clarity resulting from the changes
you suggest are of *great* significance.
Authors list
Check Jim Whitehead's affiliation
OK, maybe this one is only of significance to Jim, but *most* of your
changes significantly improve the clarity (:-).
1.3 Terms
'This draft' --> 'This document'
Done.
'A "version history" is a resource that contains all the versions ...'
consider '... that references all the versions ...'
We've defined the deletion of a version history to result in the
deletion of all versions in that version history, so I think "contains"
is better here.
'Root Version Resource'
consider renaming the term 'Initial Version Resource' as the term root
is used elsewhere in the document, as in a root collection.
The trouble with "initial" is that it doesn't capture the key semantic
of this version, namely, that you can reach every other version from
it. Since we only use "root" as an adjective (i.e. "root version",
"root collection"), I don't think any ambiguity arises.
1.5.2 Property Value Locking
This section is too general as it can be read to mean that requests
whose side-effects are a modification of another resource's (computed)
property MUST include the lock token of any resources that they affect. I
do not believe that this is the intent.
Fixed (computed properties are now explicitly excluded).
2.1.1 Creating a Version-Controlled Resource
para. 1 '... and creates a version in this version history...'
consider '...of this version history...' (that 'contains' thing again)
Fixed.
para. 3 '... the resource that is now under version control.'
consider '... under version control (the 'version-controlled resource).'
for clarification and definition of the term.
This ended up making the sentence read clumsily to me, so I switched it
back.
2.1.2 Modifying a Version-Controlled Resource
para. 2 '... with the DAV:auto-version property set'
I'm assuming that this has been rewritten based on the value of the
property.
Yes.
2.2.1 DAV:checked-in
'This URL can be used to retrieve this particular state of the
version-controlled resource after the version-controlled resource itself has
been modified.'
Given that this is a property of a checked-in version-controlled
resource, I fail to see how the version-controlled resource itself can be
modified.
?? By MERGE, by UPDATE, and by CHECKIN/PUT, for example. (The last
changes it into a checked-out resource, but it is still the same resource).
2.2.3 DAV:predecessor-set
Should state that the predecessors MUST be part of the same history.
This restriction is not made until checkin time.
2.2.4 DAV:precursor-set
Should state that the predecessors MUST NOT be part of the same history.
I assume you meant "precursor"? As above, not checked until checkin time.
Why are we required to make this distinction between predecessors and
precursors?
Because it makes a big difference to the client whether a version is
in the same history as another version, in terms of what you can do
(for example, you cannot UPDATE a vcr to be a precursor of the checked-in
version, but you can UPDATE it to a predecessor).
2.2.5 DAV:auto-version
Assuming that the vallue of the property is being defined rather than
saying it is set.
Yes.
As it reads now, having it set to 'false' would cause auto-versioning to
take place.
Maybe clarify what happens when a write lock expires, presumably the
checkin attempt occurs, and errors are never reported to any client.
I believe this is clear in the revised text.
2.3.5 DAV:checkin-date
'This property contains the date on the server when the version was
checked in.'
Consider '..when the version was created.' since the initial version is
not checked in.
Good point! This is just the DAV:creationdate of the version!
So why do we want a copy of this property with another name?
So my response to this point is to nuke the DAV:checkin-date
property (clients should just look at the DAV:creationdate if
they want this information). Any objections?
2.5 DAV:version-tree REPORT
Marshalling:
'...with at most one a DAV:prop...' -> '..with at most one DAV:prop...'
<!ELEMENT version-tree (href, prop, version-tree*)
Why DAV:prop and not DAV:propstat? -- what about requests for
non-existant properties?
Good point.
Why are the DAV:version-tree elements nested? It does not convey "true"
structure, especially since 'A server MAY omit the DAV:prop and the
successor DAV:version-tree elements ...' I don't see that the nesting is
helpful.
Another good point (and one that Lisa made as well).
Currently, we've defined to format of the DAV:version-tree-report
to match just one of the many ways a client might want to
display this information. A flat list is simpler and
more consistent - we can just use a DAV:multistatus response
so clients can even re-use their multi-status parsing code.
Any objections?
2.5.1 Example - DAV:version-tree-report
>>REQUEST
Last line of the XML has a gratuitous xmlns definition.
Fixed.
2.7 Additional PUT semantics
(DAV:cannot-modify-version-controlled-content) &
(DAV:auto-checkout)
Assume that the DAV:auto-version consideration is being re-written.
Yes.
(DAV:auto-checkout-checkin)
Uses the obsolete term 'revision' twice.
Fixed.
2.8 Additional PROPPATCH semantics
(DAV:cannot-modify-computed-property)
Consider removing this precondition and stating (earlier) that all
computed properties are protected.
Done.
(DAV:auto-checkout) &
(DAV:auto-checkout-checkin)
Assume that the DAV:auto-version consideration is being re-written.
Yes.
(DAV:auto-checkout-checkin)
Uses the obsolete term 'revision' twice.
Fixed.
2.9 Additional DELETE semantics
(DAV:cannot-delete-root-version)
Why? Consider that the value of DAV:root-version becomes undefined on
its deletion.
Then you lose a key semantic of a version tree, namely it is connected
and every version is reachable from the DAV:root-version. I don't see
that the benefit of allowing deletion of the root version outweighs the
benefit of have the version tree be connected.
(DAV:no-version-delete)
Reads more like a precondition.
Yup. Moved to precondition section.
(DAV:update-predecessor-set)
I presume this is to prevent "holes" in the history, but this won't work
in the face of merges.
Why not?
2.10 Additional COPY semantics
(DAV:auto-checkout)
Assume that the DAV:auto-version consideration is being re-written.
Yes.
Uses the obsolete term 'revision' twice.
Fixed.
3.1 CHECKOUT Method
What happened to the text that stated a lock token is required if the
resource is write locked?
This is required by the "locked property" semantics in the introduction.
3.2 CHECKIN Method
(DAV:initialize-version-content-and-properties)
'The DAV:checkin-date of the new version MUST be set...'
Should state where possible to be consistent with definition of property
(2.3.5).
Not needed now that this is just the DAV:creationdate property.
4.1 UPDATE Method
Marshalling:
'...with at most one DAV:version element.'
So what would zero elements mean?
Zero DAV:version elements probably means that your server supports the
label option, and you have a DAV:label-name element instead.
(DAV:must-select-version)
Consider renaming (DAV:must-select-version-in-same-version-history)
Done.
4.1.1 Example - UPDATE
'In this example, the content and dead properties of http://... are
copied to the vesion-controlled resource /foo.html, ...'
Consider 'In this example, the version-controlled resource /foo.html is
updated with the content and dead properties of http://...,'
I try not to use the term being defined (i.e. UPDATE) in its definition.
5.1.2 DAV:root-version
Consider renaming to DAV:initial-version
See earlier comments.
5.2.1 DAV:version-history
Consider changing from (computed) to (protected).
Done.
Consider defining in terms of DAV:version-set (5.1.1).
Done.
5.4 Additional OPTIONS Semantics
Is it required that the history for the resource reporting the OPTIONS
MUST be in the version-history-collection-set? Probably unnecessary since
clients can get to the history via properties.
No, there is no such requirement, and yes, it would be unncessary to make
this requirement.
Consider an example.
I think I'll leave that for the scenarios document.
6.1 Working Resource Properties
'The server allocates a distinct new URL for each new working resource.'
What does this imply? That the server cannot reuse working resource
URLs once the working resource has been checked in or deleted?
Good catch! A server is allowed to reuse working resource URL's, so
this sentence is misleading. I'll change this to just say that it
has a "server allocated" URL.
6.3 Additional COPY semantics
'...a new non-version-controlled resource at the destination...'
consider adding 'with the same content and dead properties...' as
written elsewhere in the document.
I try to avoid repeating the base semantics in the "additional semantics"
sections, and this is defined by the base semantics of the COPY operation.
6.5 Additional CHECKOUT Semantics
Additional Marshalling:
'The response MAY include a Location header.'
How else would you find a working resource URL? Consider MUST.
6.6 Additional CHECKIN Semantics
'...new version whose contents and dead properties are those of the
working resource.'
consider '...and dead properties are the same as those of the..'
Done.
(DAV:delete-working-resource)
'...if DAV:keep-checked-out is not specified, the'
consider '...is not specified in the request body'
(there are a number of places where it is confusing as to whether the
element name given in text is a property name or an element in the request
body.
Done (scanned all occurrences of "specified").
7 Server-Workspace Option
para. 4
Unless the URLs are relative, the workspace name will get in the way of
testing.
This is probably more detail than is needed in that section.
7.2 Additional Resource Properties
'...for an HTTP resource.' --> 'for a WebDAV resource.' (it has to
support properties<g>).
Fixed.
7.2.1 DAV:workspace
'The DAV:workspace property of any other type of resource MUST be the
same as the DAV:workspace of its parent collection.'
But only when the immediate parent collection defines a workspace.
Actually, this is defined to work transitively (i.e. all members,
not just immediate members) of that workspace that are not in
another workspace have the DAV:workspace property.
7.3 DAV:version-controlled-resource-url REPORT
Consider moving this section after 7.4 MKWORKSPACE Method.
Done.
Marshalling:
'The DAV:version-controlled-resource REPORT response body MUST be a
DAV:version-controlled-resource XML element' --> 'The
DAV:version-controlled-resource-url ...be a
DAV:version-controlled-resource-url ...' (two corrections)
Fixed.
What happens if the version-controlled resouce was deleted?
The report will return a 4xx (it doesn't know it was deleted,
it just knows it can't find one).
7.3.1 Example - DAV:version-controlled-resource-url REPORT
>> RESPONSE
xmlns definition should be moved out a level.
Fixed.
7.4 MKWORKSPACE Method
(DAV:workspace-location-ok)
Consider renaming to (DAV:workspace-lcoation-not-OK)
[All other OK's have been caps. ;-]
Done. (Actually, went the other way, and made them all consistently
lower case.)
(DAV:initialize-workspace-properties)
'The DAV:resource type...' --> 'The DAV:resourcetype...'
Fixed.
(DAV:initialize-workspace-properties)
'The DAV:workspace of the workspace MUST contain the request-URL.'
consider 'The DAV:workspace property of the workspace MUST identify the
new workspace.' i.e., let servers put some alias in there if they choose.
Done.
7.5 Additional OPTIONS Semantics
para.2
'If a server support the ...' --> 'If a server supports the ...'
Fixed.
Additional Marshalling:
How would a server indicate that workspaces can be created anywhere? or
here and below?
"Here and Below" is what it always means. In particular, it could just
return "/" to mean "anywhere".
7.7 Additional VERSION-CONTROL Semantics
(DAV:one-version-controlled-resource-per-history-per-workspace)
'...there MUST NOT already be a version-controlled member of that
workspace ... identifies a different version from the version history...'
consider ' ... identifies any version from the version history...'
Done.
(DAV:version-controlled-resource)
'...whose content and dead properties are initialized by those of...'
consider '...are the same as those of...'
I wanted to emphasize that the content and dead properties can later
be changed, and was afraid that "same" might be incorrectly read as
"always are the same".
7.7.1 Example VERSION-CONTROL ...
'...the content and deap properties ... are those of the version...'
consider '...are the samae as those of...'
Fixed (used "initialized to be" here as well).
8.2 MERGE Method
Marshalling:
'<!ELEMENT merge ANY>'
'..., and at most one of any element that can occur in a DAV:checkout
element.'
This is too restrictive for future extensions, consider '..., and any
legal set of elements that can occur...'
Done.
(DAV:update-merge-set)
'...and if DAV:checked-out version...' --> '..and if the DAV:checked-out
version...'
Fixed.
Final two postconditions of this method do not have any advanced status
element defined.
Fixed.
8.3 DAV:merge-preview REPORT
'<!ELEMENT merge-preview (update-preview-set, merge-preview-set,
ignore-preview-set)>'
Consider adding a '*' so that servers can generate these sets of XML on
the fly rather than having to compute the entire set of each before
responding.
Done.
9 Label Option
para. 2
consider a forward reference to 9.3 Label Header
Done.
para. 3
consider striking this paragraph -- although an interesting footnote it
doesn't add anything and may even imply permission to create duplicate
labels.
Fixed so that it is clear that duplicate labels can never be assigned.
9.2.1 Example - Setting a label
>>Request
Why the change of XML layout?
What was it before? What don't you like?
9.8 Additional CHECKOUT Semantics
postconditions
'If a version-controlled resource was checked out, ... the
version-controlled resource MUST remain checked in.'
Huh??
The "..." (that you omitted :-) stated that this was the creation of
a working resource. This leaves the vcr checked-in.
9.9 Additional UPDATE Semantics
Presumably the response is Bad Request if a client attempts to specify a
contradictory Label and <DAV:apply-to-version/>.
Yes. Added this precondition.
10 Baseline Option
The ascii art added nothing for me, consider striking it.
Done.
10.1.1 DAV:baseline-controlled-collection
Consider (computed) -> (protected)
Why? It is the inverse of the DAV:baseline-selector property.
10.2.2 DAV:subbaseline-set
'...a set of other baseline.' --> '...a set of other baselines.'
Fixed.
10.4 BASELINE-CONTROL Method
Consider adding some descriptive text regarding creating a new baseline
selector on an existing baseline.
Done.
(DAV:must-have-no-version-controlled-members)
Hmm, don't get why this one is required.
This would effectively "wipe out" the current members, which could
lose information if the member was a checked out vcr.
postconditions:
Consider each occurrence of 'the collection' --> 'the target collection'
Currently using "target" for "merge target". Since there is only
one collection, isn't the term "the collection" unambiguous?
(DAV:select-existing-baseline)
'...each version in the baseline, where the version-controlled
member...'
consider '...the baseline, and the ...'
I tried this replacement and the sentence no longer made sense to me.
Can you elaborate?
10.5 DAV:baseline-comparison REPORT
Marshalling:
Why are the results not reported as sets in keeping with the other
reports that respond with multiple URLs.
Actually, I "ungrouped" the other results, so this is now consistent.
Postconditions:
This section still seems to be referring to marshalling rather than
postconditions (of the repository) of the method as in other postcondition
sections.
Of course! I always felt those "postconditions" didn't feel right.
Thanks catching that.
10.7 Additional OPTIONS Semantics
Additional Preconditions:
'... the response body MUST contain a
DAV:baseline-controlled-collection-set element identifying which collections
are under baseline control.'
Eek, why would a client want this "global" information? I think this is
too difficult to require all servers to supply, and will potentially make
the OPTIONS request very expensive (IMHO clients presently consider OPTIONS
to be cheap and send them frequently.)
Fixed (moved it back to be a property on a workspace, where it makes
sense).
10.10 Additional CHECKIN Semantics
(DAV:create-baseline-version-set)
'...the DAV:baseline-version-set of the new baseline contains the ...'
consider '... of the new baseline version contains the ...'
A baseline *is* a version, so "baseline version" is redundant.
10.11 Additional UPDATE Semantics
(DAV:set-baseline-controlled-collection-members) last bullet point
'An UPDATE request MUST have been applied to each version-controlled
member whose DAV:checkin-set does not identify a version in the
DAV:baseline-version-set of the baseline.'
consider '... whose DAV:checkin-set does not identify the same version
...'
Done.
10.12 Additional MERGE Semantics
'If the merge version is a baseline, the merge target is a baseline
selector for the baseline history of that baseline, where the
baseline-controlled collection of that baseline selector is a member of the
merge destination of the request.'
That deserves an award for the greatest use of the word 'baseline' in
one sentence. Just read it very slowly.
Thanks for the award ... we strive for greatness (:-).
11 Activity option
para. 3
'Activity semantics then ensures that ...' --> 'Activity semantics then
ensure that ...'
Fixed.
11.2.1 DAV:activity-set
'...indicate to which logical changes this activity contributes, and on
what lines of descent this version appears.'
Surely '.. this version contributes...'
Yup! Fixed.
what does 'and on what lines of descent this version appears.' mean?
This emphasizes that an activity both defines a change set and a line
of descent from the root version.
11.3.1 DAV:unreserved
all instances of 'checked-out resource' should read 'checked-out
version-controlled resource'.
They could be working resources (where a working resource
is a kind of checked-out resource).
para. 2
'... merge the latest version of that activity...'
consider '... of the version-controlled resource in that activity...'
I agree this needs to be fixed, but that didn't quite do it for me.
I rewrote this sentence ... hopefully it is OK now.
11.3.2 DAV:activity-set
'...checked-out resource...' --> '...checked-out version-controlled
resource...'
It could be a working resource.
Why not allow this property on checked-in and checked-out resources as a
convenience? Just an idea.
We wouldn't want a lock on the checked-in resource to imply a lock
on the checked-in version resource, but we'd have to do something like
that to keep those properties consistent (standard argument for not
exposing version properties on checked-in vcr's).
11.5 MKACTIVITY Method
(DAV:activity-lcoation-ok)
consider renaming to (DAV:activity-location-not-OK)
But the precondition is that the location is "ok", not that it
is "not ok".
(DAV:create-activity) &
(DAV:initialize-activity-properties)
The corresponding postconditions for MKWORKSPACE do not distinguish
these as separate advances status codes, this should be consistent.
Fixed.
11.7 Additional OPTIONS Semantics
'An activity collection MAY be the root collection of a tree of
collections, all of which may contain activities.'
I propose that this is true of all OPTIONS reported locations (e.g.,
DAV:workspace-collection-set)
I believe that this statement appears in all of the OPTIONS that
report locations. Did I miss one?
11.10 Additional CHECKOUT Semantics
Additional Preconditions: (both)
'If there is a request activity set, ...'
consider clarifying with 'If the request body contains a
DAV:activity-set element,...'
The activity set can come from other places (e.g. a workspace)
than the activity-set element.
11.10.1
>>RESPONSE
Does not contain a cache conrol header as required per 3.1.
Fixed.
11.11 Additional CHECKIN Semantics
(DAV:linear-activity)
An example of a '...resource MUST be in the DAV:predecessor-set...'
(DAV:initialize-activity-version-set)
An example of a '...URL for the new version is added to the
DAV:activity-version-set...'
Should be consistent about describing resources added to properties or
URLs added to properties, otherwise the reader may think you are referring
to differerent things.
Fixed.
11.(13?)
Shouldn't there be Additional UNCHECKOUT Semantics to remove the
checked-out version-controlled resource URL from a
DAV:activity-checkout-set? If so, this is likely true for more methods that
have additional CHECKIN/CHECKOUT semantics.
The DAV:activity-checkout-set is a computed property, and those are
defined to change whenever the data upon which they are based changes
(see section 1.5.3).
12.4 Additional PUT Semantics
I know that I've been banging this drum for 12+ months. but I don't like
the idea of <version URL>/member URLs.
OK, I'll just add a DAV:version-controlled-bindings-set property to
a version, and get rid of collection version members. It appears that
you, Greg, and I are the only ones that care deeply about this so far,
and Greg said he's not supporting slashing through collection versions,
irrespective of what the protocol says. So you're against it, Greg's
against it, and I don't care.
I'll fire up a separate thread for this, so let me know if anyone has
any objections to this change.
12.9 Additional CHECKIN Semantics
(DAV:no-eclipsed-baseline-controlled-collection-members)
I don't understand the rationale for this precondition.
I agree that it is overkill. It's enough for a client to be
able to detect when eclipses happen. I've added a DAV:eclipse-set
property for collections, so that this can be discovered by a client.
13.1.1 DAV:checkout-fork
'<!ELEMENT checkout-fork (ok | discouraged | forbidden )>'
Consider using ANY for future extensions.
Done.
13.1.2 DAV:checkin-fork
'<!ELEMENT checkin-fork (ok | discouraged | forbidden )>'
Consider using ANY for future extensions.
Done.
22 Authors' Addresses
Check Jim W's contact details.
Fixed.
23.1.1 DAV:comment
Why do we have an extra DAV:string element?
Why can you have any number of them (how would a client choose which one
to display)?
This was a change requested by Yaron to support internationalization.
You can have the comment string in multiple languages this way.
23.2 Response Bodies ...
para. 2
'...each method precondition defined in this document...' -->
'...each method precondition and postcondition defined in this
document...'
Fixed.
Should mention the purpose of the postcondition stati.
Done.
'...the appropriate XML element MUST be returned in the response
body.' -->
'... in the response body unless otherwise negotiated by the request.'
Fixed.
23.3 Clarification of COPY ...
para. 4.
'Roy Fielding (an author..'
consider striking this paragraph (and moving it to the book of why)
Done.
23.4 REPORT Method
Marshalling:
'The request body of a REPORT request...' -->
'The body of a REPORT request...'
Fixed.
Minutae:
(This stuff is way below trivia, but caught my eye when reading the
document. I'm almost embarrassed to send these in.)
Trivia???! After 182 versions, I want this document *perfect* (or at
least, typo free :-). Thanks for finding these!
Abstract
Add comma after 'URL namespace versioning'
Done.
2.10 Additional COPY semantics
Extra space before the text '(DAV:initialize-precursor)'
Fixed. Word *insists* on doing that whenever I delete or move
the conditions around ... if anyone knows how to tell it to *stop*,
please let me know (:-). I periodically scan for all occurrences
of "<lf><sp>(" to fix this.
5.8 Additional VERSION-CONTROL Semantics
(DAV:new-version-history)
double space in '..for that version history that MUST NOT...'
Fixed.
Scanned for all occurrences of two spaces not following a "."
(and found several more). And just to show how anal one can be,
I also made sure that every new sentence was preceded by *exactly*
two spaces (not one, not three ... :-).
5.9 Additional CHECKIN Semantics
(DAV:preserve-version-history)
the document already uses (DAV:preserve-history) consider consistent
naming.
Done.
8.2 MERGE method
para. 2
double space in '...the DAV:auto-merge-set property of the...'
Fixed.
10.11 Additional UPDATE Semantics
(DAV:set-baseline-controlled-collection-members)
double space in '... identifies a baseline, then the
version-controlled...'
Fixed.
11.11 Additional CHECKIN Semantics
(DAV:linear-activity)
double space in '...the DAV:predecessor-set of the checked...'
Fixed.
23.5.1 Example - DAV:property-report
>>REQUEST
last three lines have extra space '</D:property >' -->
'</D:property>'
Fixed.
--------------------------
Again, Tim, thanks for the outstanding review!
Hopefully I didn't break more than I fixed in the process of making
these changes (:-).
Cheers,
Geoff
Received on Thursday, 18 January 2001 14:58:56 UTC