You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@galois.collab.net> on 2000/11/13 16:42:50 UTC
about the recent client changes...
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
Re: about the recent client changes...
Posted by Karl Fogel <kf...@galois.collab.net>.
Bruce Korb <bk...@cruzio.com> writes:
> Sorry you had to go to so much bother.
I would have spent that time, and more, if you hadn't laid down the
foundation.
> I'll read through your changes now and see if you left anything
> undone. A 15 second glance looks like you pretty much nailed it.
Thanks; let me know if I missed anything.
-K
Re: about the recent client changes...
Posted by Bruce Korb <bk...@cruzio.com>.
Hi Karl,
Sorry you had to go to so much bother.
I'll read through your changes now and see if you left anything
undone. A 15 second glance looks like you pretty much nailed it.
Karl Fogel wrote:
>
> Bruce,
>
> The reorganization in subversion/client/ is appreciated. Something of
> the sort needed to be done. However, I do have some concerns:
Re: about the recent client changes...
Posted by Karl Fogel <kf...@galois.collab.net>.
He already got rid of the field. We'll solve the
subprocess-from-interpreter-loop problem when we come to it, years
from now when Subversion has its own United Nations seat.
-K
Branko =?ISO-8859-2?Q?=C8ibej?= <br...@xbc.nu> writes:
> Bruce Korb wrote:
>
> > 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).
>
> I'd like to stomp on this one before it happens.
>
> There exist systems where "fork" doesn't exist -- e.g., it's gruesomly
> expensive and inconvenient to implement. Is the only for creating a
> subprocess so that you can write "exit(1)" instead of "return 1"? If so,
> please consider doing without subprocesses.
>
> --
> Brane �ibej
> home: <br...@xbc.nu> http://www.xbc.nu/brane/
> work: <br...@hermes.si> http://www.hermes-softlab.com/
> ACM: <br...@acm.org> http://www.acm.org/
Re: about the recent client changes...
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Nov 14, 2000 at 07:54:11PM +0100, Branko Cibej wrote:
> Bruce Korb wrote:
>
> > 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).
>
> I'd like to stomp on this one before it happens.
>
> There exist systems where "fork" doesn't exist -- e.g., it's gruesomly
> expensive and inconvenient to implement. Is the only for creating a
> subprocess so that you can write "exit(1)" instead of "return 1"? If so,
> please consider doing without subprocesses.
+1 on that!
And I'd say torch items that are "anticipatory". If it won't be implemented
for SVN 1.0, then it probably shouldn't be in there. Sure... structural
organizations with a forward-looking view are great. But data structures,
variables, etc are just asking for maintenance and development hassles if
they don't apply to 1.0.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: about the recent client changes...
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Nov 14, 2000 at 11:20:37AM -0800, Bruce Korb wrote:
> Branko Hibej wrote:
> > Bruce Korb wrote:
>...
> > I'd like to stomp on this one before it happens.
> >
> > There exist systems where "fork" doesn't exist -- e.g., it's gruesomly
> > expensive and inconvenient to implement. Is the only for creating a
> > subprocess so that you can write "exit(1)" instead of "return 1"? If so,
> > please consider doing without subprocesses.
>
> Even WinNT and WinME have fork(). Only Win9x and earlier are that
> anemic. Are they in the target list?
Yes.
And are you sure they have fork()? I know they have "spawn" (fork/exec), but
hadn't heard about fork.
> Besides that, the `you can write'
> must refer to everyone in all client-side routines throughout SVN.
> It is easier and safer to cope with a dead child than rely on
> all routines doing the right thing. But, hey. If you want to count
> on all routines cleaning up correctly rather than letting fork/exit
> do it, cool. I was just thinking that with fork/exit you are certain
> to have bullet proof code. Deprive Win9x of the shell version, I say
> :-).
We get our clean up through APR pools. I don't see *any* problem from that
angle. Start a pool for each input line, clear it when the command is done.
Simple and clean. And it doesn't require subprocesses.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: about the recent client changes...
Posted by Branko Čibej <br...@xbc.nu>.
Greg Stein wrote:
> longjmp?!! I hope that I never see a longjmp anywhere in SVN. Its presence
> would indicate *very* serious problems with the design and architecture of
> the code. longjmp is "handy", but is a poor excuse to taking the time for
> nicely structured code.
Yes.
> And hey... Windows will probably want to use libsvn_client rather than
> client/ in any case.
Only once somebody writes a GUI client for Sobversion. Emacs and friends
will need the client on WIndows, too.
--
Brane �ibej
home: <br...@xbc.nu> http://www.xbc.nu/brane/
work: <br...@hermes.si> http://www.hermes-softlab.com/
ACM: <br...@acm.org> http://www.acm.org/
Re: about the recent client changes...
Posted by Karl Fogel <kf...@galois.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> And hey... Windows will probably want to use libsvn_client rather than
> client/ in any case.
s/probably/definitely
Re: about the recent client changes...
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Nov 14, 2000 at 08:54:15PM +0100, Branko Cibej wrote:
>...
> > Deprive Win9x of the shell version, I say
> > :-).
>
> There's really no need to do that. As Greg S said, APR pools + cleanup
> will do just fine. I may even agree to combine that with "longjmp", if
> the unwinding code becomes too hairy. But I don't think it will, because
> it hasn't. :-)
longjmp?!! I hope that I never see a longjmp anywhere in SVN. Its presence
would indicate *very* serious problems with the design and architecture of
the code. longjmp is "handy", but is a poor excuse to taking the time for
nicely structured code.
And hey... Windows will probably want to use libsvn_client rather than
client/ in any case.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: about the recent client changes...
Posted by Branko Čibej <br...@xbc.nu>.
Bruce Korb wrote:
> Branko Cibej wrote:
>
>> There exist systems where "fork" doesn't exist -- e.g., it's gruesomly
>> expensive and inconvenient to implement. Is the only for creating a
>> subprocess so that you can write "exit(1)" instead of "return 1"? If so,
>> please consider doing without subprocesses.
>
>
> Even WinNT and WinME have fork().
Not that I'm aware of. You must be thinking of cygwin, which does
implement fork -- in a "gruesomely expensive and inconvenient" way.
> Besides that, the `you can write'
> must refer to everyone in all client-side routines throughout SVN.
Absolutely.
> It is easier and safer to cope with a dead child than rely on
> all routines doing the right thing. But, hey. If you want to count
> on all routines cleaning up correctly rather than letting fork/exit
> do it, cool. I was just thinking that with fork/exit you are certain
> to have bullet proof code.
Certain, are you? Let me tell you the about the time when ... ;-)
> Deprive Win9x of the shell version, I say
> :-).
There's really no need to do that. As Greg S said, APR pools + cleanup
will do just fine. I may even agree to combine that with "longjmp", if
the unwinding code becomes too hairy. But I don't think it will, because
it hasn't. :-)
--
Brane �ibej
home: <br...@xbc.nu> http://www.xbc.nu/brane/
work: <br...@hermes.si> http://www.hermes-softlab.com/
ACM: <br...@acm.org> http://www.acm.org/
Re: about the recent client changes...
Posted by Bruce Korb <bk...@cruzio.com>.
Branko Hibej wrote:
>
> Bruce Korb wrote:
>
> > 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).
>
> I'd like to stomp on this one before it happens.
>
> There exist systems where "fork" doesn't exist -- e.g., it's gruesomly
> expensive and inconvenient to implement. Is the only for creating a
> subprocess so that you can write "exit(1)" instead of "return 1"? If so,
> please consider doing without subprocesses.
Even WinNT and WinME have fork(). Only Win9x and earlier are that
anemic. Are they in the target list? Besides that, the `you can write'
must refer to everyone in all client-side routines throughout SVN.
It is easier and safer to cope with a dead child than rely on
all routines doing the right thing. But, hey. If you want to count
on all routines cleaning up correctly rather than letting fork/exit
do it, cool. I was just thinking that with fork/exit you are certain
to have bullet proof code. Deprive Win9x of the shell version, I say
:-).
Re: about the recent client changes...
Posted by Branko Čibej <br...@xbc.nu>.
Bruce Korb wrote:
> 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).
I'd like to stomp on this one before it happens.
There exist systems where "fork" doesn't exist -- e.g., it's gruesomly
expensive and inconvenient to implement. Is the only for creating a
subprocess so that you can write "exit(1)" instead of "return 1"? If so,
please consider doing without subprocesses.
--
Brane �ibej
home: <br...@xbc.nu> http://www.xbc.nu/brane/
work: <br...@hermes.si> http://www.hermes-softlab.com/
ACM: <br...@acm.org> http://www.acm.org/
Re: about the recent client changes...
Posted by Karl Fogel <kf...@galois.collab.net>.
Bruce Korb <bk...@cruzio.com> writes:
> 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.
Okay.
Consider a log message with a terse descriptive paragraph at the top,
and then a list of detailed consequences beneath (or is that what you
meant?).
> > 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.
Okay. This will come out in the wash soon, as we revamp the option
parsing.
> > 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.
I think we're just going to assume our `svn_' prefix gives us safety,
and keep the `_t' at the end. They may own the land, but they can't
make us pay rent. :-)
> > - The help message from "svn --help" is problematic.
>
> It was also incomplete. I like what you did with it. Thanks.
Gotcha.
> > 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).
That's usually the way I work too, yeah -- commit anytime "make check"
happens to pass. If possible, it's nice to leave a `todo' comment
near stuff you know you're going to continue with, so other's don't
wonder about it. (But I don't always do that either, shame on me.)
Nice chatting on the phone,
-K
Re: about the recent client changes...
Posted by Bruce Korb <bk...@cruzio.com>.
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.
Re: about the recent client changes...
Posted by Karl Fogel <kf...@galois.collab.net>.
Bruce Korb <bk...@sco.COM> writes:
> > the rest of Subversion works. Can you fix the above problems?
>
> Tonight, when I get home.
Okay; I'll start on it, since I was wanting to make some changes in
the client anyway. Email me (or call 312-435-1294) when you're about
to start working, so I don't step on your toes.
Thanks,
-K
Re: about the recent client changes...
Posted by Bruce Korb <bk...@sco.COM>.
> the rest of Subversion works. Can you fix the above problems?
Tonight, when I get home.