[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: mod-dav-svn high level logging compatibility (long)

From: Eric Gillespie <epg_at_pretzelnet.org>
Date: 2007-11-17 06:51:49 CET

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

This is an archived mail posted to the Subversion Dev mailing list.