Karl Fogel wrote:
> Thorough log messages for changes like this are *really* important,
There are a couple of things to do, so I'll submit a long log message
with my tweaks and leave the one-liner for the massive part of the change.
> 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.
That field is a dinkleberry. I was considering segregating the table
by size and then sort, which really meant it could have been inside
a comment field and worked as well. But the table is no longer sortable,
so it is definitely moot.
The boolean is anticipatory.
*When* svn becomes a shell :), certain commands will need to run
in the parent process (e.g. "exit" and "cd"), while nearly all
others will run in a subprocess (so they can exit with failure
without killing the main loop). But there will be needs of other
considerations as well. (e.g. if running non-interactively,
maybe the fork should not be done so that a failure exit stops
the process.) In any event, it is not now used.
> 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?
Yep. They *should* be, but are not. Yet. They should be because
the option processing mumbo jumbo ought to be done before the
command procedures are called out. Unfortunately, the current
option processing mechanism requires a complex exchange of information
between it and the command procedure, so it cannot be done yet.
So, *FOR THE TIME BEING*, it must be the initial argc/argv.
> Also, is there some reason you're putting `t_' first, instead of
> putting `_t' at the end like the rest of Subversion?
POSIX. POSIX has claimed the entire name space that matches the
pattern: [a-zA-Z_][a-zA-Z_0-9]*_t
Disgusting land grab, but true.
> - 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?)
But of course :-), but I cannot find it anywhere! ;-)
> - The help message from "svn --help" is problematic.
It was also incomplete. I like what you did with it. Thanks.
> 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...
No. It really means I am trying to check in early and often.
But I definitely need to be less terse about it so my intentions
are clearer. My personal approach is to check in if it works
reliably. Clean up dinkleberries asap afterwards (I do not leave
them for very long).
> - 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.
The abbreviations fit well in the context of something else
I was doing but did not check in. But the "something else" is
now, but for a finale gasp, essentially dead.
> At the moment, I have no idea what "_te_command" stands for.
"t_" is a struct type. er, I mean, "_t" is a struct type.
svn_cl__te_command is an SVN client command enumeration type.
> Can you fix the above problems?
No. You beat me to it, mostly. ;-) Thank you.
I am cleaning up some as I type this, tho.
Received on Sat Oct 21 14:36:14 2006