Hi Peter,
Random review comments, hope you don't mind.
On Mon, Apr 10, 2006 at 12:38:10PM -0700, lundblad@tigris.org wrote:
> ON the nonxml-entries branch:
>
> --- branches/nonxml-entries/subversion/libsvn_wc/README (original)
> +++ branches/nonxml-entries/subversion/libsvn_wc/README Mon Apr 10 12:38:08 2006
> @@ -111,13 +111,9 @@
>
> Says what version of the working copy adm format this is (so future
> clients can be backwards compatible easily).
> - The changes in each format are listed in wc.h.
>
> - Also, the presence of this file means that the entire process of
> - creating the adm area was completed, because this is always the
> - last file created. Of course, that's no guarantee that someone
> - didn't muck things up afterwards, but it's good enough for
> - existence-checking.
> + This file is deprecated and present for backwards compatibility.
> + It may be removed in the future.
>
Why delete that text? It's all still true, and (IMO) useful (or are there
better ways to portably test whether the admin area is ready to use?).
> The entries file
> ----------------
>
> +
> +The characters "|", "\" and ASCII control characters (0x01 - 0x31 and
> +0x7f) must be escaped when they occur in a field. The escaping uses
Why 0x7f?
> +the syntax "\xHH", where "HH" are the two hexadecimal digits
> +representing the escaped byte. The character encoding of the entries
Lowercase or uppercase hex-encoding, or is either allowed?
> +representing the escaped byte. The character encoding of the entries
> +file is UTF-8, with no leading BOM allowed.
That's not overly clear; perhaps it should be at the start of the section,
so we don't (i.e. _I_ don't) get confused about whether we're talking
about file encoding or field encoding.
> +
> +A field may be boolean, in which case it can have one of the values
> +'t`, meaning true or 'f`, meaning false. Unless otherwise specified,
> +boolean fields default to `f' if emtpy. Timestamps are stored in
> +the format produced by svn_time_to_cstring().
>
We don't have any booleans that default to true, do we? Ditch 'Unless
otherwise specified'?
> +checksum:
> For file entries, base-64-encoded MD5 checksum of the text-base
> file. Optional, for backwards compatibility.
>
This is a new file format, so there is no backwards compatibility to
worry about: can't we just start mandating the checksum from now?
> +XML-based entries format
> +
> +Fields added in format 7 and later are not allowed in the XML-based
> +format.
(are there any?)
> The attributes `has-props', `has-prop-mods', `cachable-props',
> +and `present-props' are only valid in format 6.
That's rather confusing, since they're also valid in format 7.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Apr 10 23:53:11 2006