This may seem out-of-scope given the focus on the 1.5 release,
but since we've already changed the logging for 1.5, I think we
need to consider these things before we ship. The hard part was
digging out all this information; I spent my evening playing code
archaeologist. I don't think fixing all this will be a large
change, if my suggestions seem like the right way to go.
General notes:
- The biggest problem I've found is we don't log the repository!
DOH! Instead of adding this the SVN-ACTION environment
variable, I suggest adding two more variables, SVN-REPOS
(containing the full path) and SVN-REPOS-NAME (containing only
the basename). We use the basename for other things already,
such as authz with ParentPath repositories. Admins using
ParentPath (and some who aren't, e.g. one repository per vhost)
probably just want to log the basename, not clutter up the log
with the full path.
- Good, we already use svn_path_uri_encode, so spaces, newlines,
and so on are encoded and splitting a line on whitespace works.
However, the comma "," is not usually encoded, and I expect
svn_path_uri_encode not to encode it (and if I read it
correctly, it does not). We should encode it when logging
paths, as we sometimes write a comma-delimited list of paths.
- Do the '' quotes aid or hinder readability? For me, they're a
big hindrance.
Now, notes on each log action. Armed with this knowledge (and,
assuming no one objects, updated logging), I'll write a section
for the svn book. It won't be pretty or XML-formatted, but
hopefully someone will polish it up and commit it.
- diff-or-merge '/PATH' rN:N
(diff-or-merge|switch) '/PATH@N' '/PATH@N'
In 1.4 (changed in r23302):
(diff-or-merge|switch) '/PATH' 'PATH'
Why no depth?
- (checkout-or-export|update|remote-status) '/PATH' r%ld depth-(exclude|unknown|empty|files|immediates|infinity|INVALID-DEPTH)
In 1.4 (revision and depth added in r23302):
(checkout-or-export|update|remote-status) '/PATH'
- replay( '/PATH')? rN
In 1.4 (changed in r25542):
replay N( '/PATH')?
Any reason to omit the path when base_dir is empty? Isn't that
the repository root, and should be logged as '/' as in all
other log actions?
What about svn_ra_replay_range? It has start and end revision
rather than just revision, but we're only logging one of them.
- get-mergeinfo(-partial)? '/PATH'
Not present in 1.4.
Shouldn't we log more than the path? svn_ra_get_mergeinfo also
takes svn_revnum_t and svn_mergeinfo_inheritance_t but they are
not logged. Also, I don't understand the meaning of "partial";
it looks like it's present if the caller asked for mergeinfo
about multiple paths, but I don't see the leap from that to the
word "partial".
- log(-merge-sensitive)? '/PATH(,/PATH)*' rN:N 'REVPROP(,REVPROP)*'
In 1.4 (changed in r23576 r26955 (revprops) r27024 (merge-sensitive)):
log(-(all|partial) '/PATH')?
Why are the revprops in '' quotes? I think we should drop
them, or replace them with parens. Why do we log revprops but
not limit, discover_changed_paths, strict_node_history? I'm
also not sure logging one parameter to the same operation as an
option but the other as an entirely different action is right.
- (blame(-merge-sensitive)? '/PATH' rN:N
In 1.4 (changed in r23488 r27024 (merge-sensitive)):
blame '/PATH'
This is actually svn_ra_get_file_revs2/file-revs-report. If
this interface is blame-specific, it should be named
accordingly. If it is not, the log is wrong.
Maybe someone was thinking the admin ought not need to
understand the dav-svn protocol in such detail, but that's
fallacious. Admins need to understand the protocol of any
service they run just as well as they need to understand
dav-svn (i.e. knowing what a file-revs-report entry in your
dav-svn log means is analagous to knowing what GET, OPTIONS,
PUT, POST, ... entries in your http log mean)
Which raises another point. As far as I can tell, the dav-svn
protocol is woefully under-documented. I've never been able to
find a document analagous to the svn protocol's 'protocol' file.
- commit '/PATH' rN
In 1.4 (path added in r23425):
commit rN
I've been sitting on a patch for a couple of days that changes
this to find the longest common prefix of all changed paths and
log that instead of target->info->repos_path because the latter
appears to be the root of the ra session. Well, some clients
(e.g. the two I've written :) *always* open the ra session to
the repository root; this is so much easier. Commits from such
clients log '/', then, hence my change.
However, I realize now that this is completely redundant
information! It's logged in svn itself. I therefore strongly
suggest we drop the path entirely, rather than change it.
- list-dir '/PATH' rN
In 1.4 (revision added in r23302):
list-dir '/PATH'
- (un)?lock '/PATH'
Same in 1.4.
The log is incomplete here. We need to log the base revisions
of each path (PATH@REV will do nicely) and steal_lock for lock,
and break_lock for unlock.
- revprop-change rN 'REVPROP'
Same in 1.4.
Finally, things we don't log at all:
- Interfaces I don't think we log but we should:
- svn_ra_get_latest_revnum
Maybe; is it possible? Not that important.
- svn_ra_get_dated_revision dated-rev-report
svn_ra_get_locations get-locations
svn_ra_get_location_segments get-location-segments
svn_ra_get_locks get-locks-report
Looks like these map cleanly to the above reports, so they
should be easy enough.
- svn_ra_rev_proplist
svn_ra_rev_prop
svn_ra_get_file
svn_ra_get_dir
I have half an untested patch to log (node|rev|txn)-prop-(get|list)
and get-file actions, covering svn_ra_rev_proplist and
svn_ra_rev_prop, the prop-retrieval side of svn_ra_get_file
and svn_ra_get_dir, and the file-contents side of get_file.
The dirents part of get_dir is already logged as list-dir.
This means calls to get_file/get_dir show up in the log as
(list-dir node-prop-list) and (get-file node-prop-list)
pairs, which is just fine.
- svn_ra_check_path
svn_ra_stat
Maybe; are these possible? stat is at least as valuable to
log as get_log.
- svn_ra_get_lock
Looks like mod_dav_svn/lock.c:find_lock?
- svn_ra_has_capability
I can't even find where mod-dav-svn responds to
has_capability; I can find where it checks the client's
capabilities, but not where it returns its own.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Nov 17 06:51:58 2007