Bruce,
The reorganization in subversion/client/ is appreciated. Something of
the sort needed to be done. However, I do have some concerns:
- Have you read the HACKING file, which explains how to write log
messages? Thorough log messages for changes like this are
*really* important, because a lot of the old code is still in
place and being used -- so we need a summary of exactly what new
stuff was added, and how it interacts with what was already
there.
- You've created several new data types, whose usage is not
entirely obvious, and didn't document them:
I think I mostly grok how to use svn_cl__t_cmd_desc, but some
comments would be nice. I don't understand the need for the
`name_len' field, and think it maintaining it is likely to be
error-prone.
The svn_cl__t_cmd_proc type is great, but there's no comment
explaining what the arguments look like. For example, are argc
and argv already past the first two command line args, or not?
Just looking at the definition of `svn_cl__te_command', I have
not a clue what it's for. Perhaps if I grep for it in the code,
I'll get a sense, but it should really be documented right there
in cl.h.
Also, is there some reason you're putting `t_' first, instead of
putting `_t' at the end like the rest of Subversion?
- The brief comment above svn_cl__t_cmd_desc in cl.h refers
mysteriously to `tOptions', which is mentioned nowhere else in
the code. (Just a leftover?)
- The help message from "svn --help" is problematic. Specifically,
it gives no indication that certain commands are shorter synonyms
of others. (Is this a result of tabularization? If so, then
it's a pretty huge disadvantage!) And it doesn't say, for
second-level help, what order the subcommand and the "--help"
have relative to each other. Try running "cvs --help" for a good
example of a concise, yet precise, help message. Here's the
current "svn --help", for reference:
What command do you need help with?
You must type the command you need help with along with the `--help'
command line option. Choose from the following commands:
ad add checkout ci co commit del
delete help new pf pfind propfind rm
st stat status up update
I realize that having any help message at all is an improvement
over not having one; but the above makes me worry that you're not
trying hard enough to see things from the user's point of
view...
- You've named the C files after the short abbreviations of the
commands they represent, instead of the full names. This seems
needlessly confusing, especially considering that the function
names defined in those files are full, not abbreviated.
In short, please be a lot more careful and thorough. Try to abide by
the conventions the rest of Subversion is using. Please don't use
highly compressed abbreviations in a public or semi-public symbol
names, or file names, unless things are getting too long (it's a
judgement call, I admit). If you do abbreviate something, at least
say what the abbreviation means. At the moment, I have no idea what
"_te_command" stands for.
Right now I don't know whether to revert the change or not. I think
it's probably good, if it were documented and made consistent with how
the rest of Subversion works. Can you fix the above problems?
Thank you,
-Karl
Received on Sat Oct 21 14:36:14 2006