You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Hudson <gh...@MIT.EDU> on 2002/10/28 23:21:51 UTC
Laundry list
For those curious, here is a list of 30 perceived problems I have
found in Subversion during the past week or so. Most of them are too
small or too controversial to file issues on, and I hope to address
many of them myself (not by unilateral action in the controversial
cases, of course). But for the sake of having stuff out in the open,
here's a list:
There are a few opportunities for exception-handling cleanups:
* svn_error_clear_all() is dangerous, and must be replaced with an
svn_error_clear().
* We should replace svn_error_locate() with extra arguments to
svn_create_error()'s internal name.
* We should always collect line and file information, but only
expose it with the appropriate flag set. This will help with
debugging and will simplify the internal interface.
In some places, our implementations of functions go to special effort
to allow caller behavior not documented in the contract. For
instance, svn_path_uri_encode() allows a NULL path parameter. There
is no advantage to these allowances, and after 1.0 they can trap
future implementations into the lax contracts. I would like to squash
these cases under the principles:
* Never go to special effort to allow caller behavior not specified
in the contract.
* Don't allow caller behavior in the contract which doesn't have a
legitimate purpose. (URI-encoding NULL doesn't make sense.)
(Detecting improper arguments and aborting, or returning an exception,
is just fine.)
We use _cstring to denote null-terminated strings in some places, and
_nts in others. We should really have only one convention here. I
find _cstring more intuitive, and it's an older convention in our code
base as well. In many cases, the designation shouldn't be necessary
at all; for instance, the svn_stringbuf_t svn_path functions should
probably just go away, and the _nts be removed from the other
functions.
ra_dav and ra_local have separate implementations of split_url.
ra_loader.c uses a void * baton to communicate between
svn_ra_init_ra_libs() and svn_ra_get_ra_library(), when a real live
type would do just fine.
svn_ra_get_ra_library() searches a hash table with a linear traversal.
This isn't a real problem (the table is small), but it's weird.
The "svn diff" syntax we agreed upon in prior discussion was never
implemented.
The ra interface has no pool parameters, so implementations are forced
to use a pool stashed in the session baton. This is not a serious
practical problem, but it would be cleaner to have pool parameters.
The ra interface's check_path function is inconsistent with the rest
of the interface with regard to argument order; it places the return
parameter before the session baton, instead of just afterwards.
The ra interface documentation doesn't specify when a rev can be
specified as SVN_INVALID_REVNUM (meaning "whatever's youngest at the
time of processing").
The ra interface passes dates as strings (in svn_time_to_nts()
format), for implementation convenience. apr_time_t would make for a
cleaner interface, though it's more work on the back end.
log_message_receiver returns changed_paths as a hash table, I think
for the convenience of the current libsvn_fs implementation. An array
would make more sense from the caller's perspective.
Outside of the libsvn_fs internals, "created_rev" should probably be
called "checkin_rev" or something. Otherwise people may guess that
it's the creation rev of the node, rather than of the node-revision.
We never implemented an editor interface which allows either the
driver or consumer to decide to use plain text instead of deltas.
There are various ideas on how this should work; it would
significantly speed up ra_local if one of the ideas was implemented
and applied such that editors running over ra_local don't use deltas.
We never replaced getdate.y. We have an unfinished patch to do so.
Our comments and documentation use ` as a left quote all over the
place. The ` is supposed to be a grave accent (which is pretty
useless, since you can't put it on top of anthing), not a left quote.
Modern fonts display ` and ' asymmetrically (` slants down to the
right but ' is vertical), making `foo' look bad. We should use 'foo'
instead of `foo'.
Someone added an svn_stream_dup() function to my stream interface.
Nothing uses it. The only purpose would be to move a stream object
from one pool to another (it doesn't actually split the stream), and I
can't imagine why any legitimately designed code would want to do
that. I think it should go away.
svn_log_changed_path_t uses a char instead of an enum to specify file
status. Sloppy.
Our build system's Makefiles use CFLAGS and LDFLAGS. The GNU Makefile
standards say:
If there are C compiler options that *must* be used for proper
compilation of certain files, do not include them in `CFLAGS'.
Users expect to be able to specify `CFLAGS' freely themselves.
We should change gen-make.py to use ALL_CFLAGS for everything except
optimization options, and then rename EXTRA_CFLAGS to CFLAGS. Same
deal for CPPFLAGS and LDFLAGS.
svn_wc_get_actual_target() sets *target to NULL when anchor is the
target. That was probably right at the time, but these days we use ""
to mean an empty relative path, not NULL. As a consequence of this
function's behavior, a number of contracts are unnecessarily lax,
including the ra interface's update/switch/status functions.
The wc diff editor performs a couple of operations while totally
ignoring errors. This leaks small amounts of memory and is also
unsafe; errors other than "not found" could happen and be ignored.
svn_wc__close_text_base() takes an int argument, which should be an
svn_boolean_t.
close_adm_file() uses a pathname as an error message. This might
yield cryptic errors.
adm_files.c's extend_with_adm_name() doubles as a path concatenation
function. That seems a little bizarre, but it's pretty entrenched, so
I'm not likely to do anything about it.
adm_files.c's v_extend_with_adm_name() checks for empty paths as it's
joining paths onto the end. That's not necessary; joining an empty
path to the end of a path has no effect.
Some code in mod_dav_svn contains tabs. Doing a codebase-wide sweep
for tabs might be good.
svn_path_basename()'s contract could be to return a pointer into the
argument string, instead of duplicating the result into a pool
argument. This would make some code simpler.
While building with shared libraries, I ran into a libtool relink
problem when installing. (Red Hat 7.3.) I've seen this problem
before, in other code, but forget what the cause was. I should
investigate this.
svnlook --help isn't very helpful.
props and wc-props files still have no suffix, so they show up in
"find". (Arguably, all these files should have prefixes as well as
suffixes, for added protection.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by cm...@collab.net.
=?UTF-8?B?QnJhbmtvIMSMaWJlag==?= <br...@xbc.nu> writes:
> Speaking of laundry lists, I'm sick and tired of how our Python test
> harness depends on "svn import" working correctly. Instead, I propose to
> commit a dumped repository (db_dump, not svnadmin dump) containing the
> basic greek tree, and db_loading that to initialize the test
> repositories. Then we can create
> an import_tests.py and finally treat import like any other command.
>
> Thoughts? Objections?
Just don't use db_dump with the -p flag -- it uses non-portable system
calls to determine which characters to escape.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by Branko Čibej <br...@xbc.nu>.
Glenn A. Thompson wrote:
>
>
> Branko Čibej wrote:
>
>> Speaking of laundry lists, I'm sick and tired of how our Python test
>> harness depends on "svn import" working correctly. Instead, I propose to
>> commit a dumped repository (db_dump, not svnadmin dump) containing the
>> basic greek tree, and db_loading that to initialize the test
>> repositories. Then we can create
>> an import_tests.py and finally treat import like any other command.
>>
>> Thoughts? Objections?
>>
>>
> Potententially, this means that every supported DB would have to be
> dumped and committed using their respective dump and load programs.
> Right?
Yes. That's quite all right -- after all, we'd have to repeat the tests
for each supported back-end anyway.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by "Glenn A. Thompson" <gt...@cdr.net>.
Branko Čibej wrote:
>Speaking of laundry lists, I'm sick and tired of how our Python test
>harness depends on "svn import" working correctly. Instead, I propose to
>commit a dumped repository (db_dump, not svnadmin dump) containing the
>basic greek tree, and db_loading that to initialize the test
>repositories. Then we can create
>an import_tests.py and finally treat import like any other command.
>
>Thoughts? Objections?
>
>
Potententially, this means that every supported DB would have to be
dumped and committed using their respective dump and load programs.
Right?
gat
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by Branko Čibej <br...@xbc.nu>.
Speaking of laundry lists, I'm sick and tired of how our Python test
harness depends on "svn import" working correctly. Instead, I propose to
commit a dumped repository (db_dump, not svnadmin dump) containing the
basic greek tree, and db_loading that to initialize the test
repositories. Then we can create
an import_tests.py and finally treat import like any other command.
Thoughts? Objections?
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Greg Hudson <gh...@MIT.EDU> writes:
> For those curious, here is a list of 30 perceived problems I have
> found in Subversion during the past week or so. Most of them are too
> small or too controversial to file issues on, and I hope to address
> many of them myself (not by unilateral action in the controversial
> cases, of course). But for the sake of having stuff out in the open,
> here's a list:
Wow! Greg, thanks for writing this up.
Only a few of these are controversial, and you seem to know which
ones. For the others, are there any that you plan to take care of
yourself? Whichever ones remain, we should file issues about. The
issues database is one way new volunteers figure out what needs
doing. If we can add a bunch of new `bite-sized' tasks (which a lot
of these are), maybe we could lure in some more volunteers...
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by cm...@collab.net.
cmpilato@collab.net writes:
> =?UTF-8?B?QnJhbmtvIMSMaWJlag==?= <br...@xbc.nu> writes:
>
> > > * We should replace svn_error_locate() with extra arguments to
> > > svn_create_error()'s internal name.
> > >
> > How do you propose to wrap svn_error_creatf without using C99 varargs
> > macros? I don't see how it's possible short of explicitly passing
> > __FILE__ and __LINE__ to every svn_error_createf call.
>
> svn_error_create and svn_error_createf can become macros that
> optionally pass this information to some real underlying function.
> This is the same way I did our pool debugging stuff, and the way
> Sander moved that to APR:
>
> APR_DECLARE(apr_status_t) apr_pool_create_ex(apr_pool_t **newpool,
> apr_pool_t *parent,
> apr_abortfunc_t abort_fn,
> apr_allocator_t *allocator);
> APR_DECLARE(apr_status_t) apr_pool_create_ex_debug(apr_pool_t **newpool,
> apr_pool_t *parent,
> apr_abortfunc_t abort_fn,
> apr_allocator_t *allocator,
> const char *file_line);
> #if APR_POOL_DEBUG
> #define apr_pool_create_ex(newpool, parent, abort_fn, allocator) \
> apr_pool_create_ex_debug(newpool, parent, abort_fn, allocator, \
> APR_POOL__FILE_LINE__)
> #endif
Ignore me. I'm not paying attention tonight. We already do the above
in our error generation. And it's not even relevant to the
discussion.
cmpilato goes off to hide somewhere...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by cm...@collab.net.
=?UTF-8?B?QnJhbmtvIMSMaWJlag==?= <br...@xbc.nu> writes:
> > * We should replace svn_error_locate() with extra arguments to
> > svn_create_error()'s internal name.
> >
> How do you propose to wrap svn_error_creatf without using C99 varargs
> macros? I don't see how it's possible short of explicitly passing
> __FILE__ and __LINE__ to every svn_error_createf call.
svn_error_create and svn_error_createf can become macros that
optionally pass this information to some real underlying function.
This is the same way I did our pool debugging stuff, and the way
Sander moved that to APR:
APR_DECLARE(apr_status_t) apr_pool_create_ex(apr_pool_t **newpool,
apr_pool_t *parent,
apr_abortfunc_t abort_fn,
apr_allocator_t *allocator);
APR_DECLARE(apr_status_t) apr_pool_create_ex_debug(apr_pool_t **newpool,
apr_pool_t *parent,
apr_abortfunc_t abort_fn,
apr_allocator_t *allocator,
const char *file_line);
#if APR_POOL_DEBUG
#define apr_pool_create_ex(newpool, parent, abort_fn, allocator) \
apr_pool_create_ex_debug(newpool, parent, abort_fn, allocator, \
APR_POOL__FILE_LINE__)
#endif
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by "Glenn A. Thompson" <gt...@cdr.net>.
>
>
>>But perhaps relying on
>>"svnadmin load" would be a good compromise. That way the test harness
>>wouldn't rely on as much Subversion machinery working, but the same
>>input data would work for any filesystem back end.
>>
>>
>>
>No, I don't think so. Other FS backends will presumably have their own
>dump/load mechanisms, also independent of Subversion, so we can just use
>those. When the schema changes, we can just dump a new greek tree, no
>problems there.
>
How about a prompt: Is greek tree preloaded (y or n)?
or a cmd line argument?
gat
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by Branko Čibej <br...@xbc.nu>.
Greg Hudson wrote:
>On Mon, 2002-10-28 at 18:57, Branko Čibej wrote:
>
>>> * We should always collect line and file information, but only
>>> expose it with the appropriate flag set. This will help with
>>> debugging and will simplify the internal interface.
>>>
>>>
>>No, we should _collect_ it only with the appropriate flag set
>>
>>
>
>Why?
>
Argh, ignore me. My though processes must have fossilized for a moment.
From a separate message:
>
>
>>I'm sick and tired of how our Python test harness depends on "svn
>>import" working correctly.
>>
>>
>
>I don't see how this is a serious problem.
>
I think it is. For one thing, it slows down the tests (on my machine,
DAV tests -- just the client stuff, without fs-tests -- take almost two
hours...). But more importantly, a bug in "svn import" means that you
get _no_ useful information at all from a test run, and it could
concievably give false passes, not just false failures. It's never a
good idea to use the tool you're testing as part of its test harness.
>But perhaps relying on
>"svnadmin load" would be a good compromise. That way the test harness
>wouldn't rely on as much Subversion machinery working, but the same
>input data would work for any filesystem back end.
>
No, I don't think so. Other FS backends will presumably have their own
dump/load mechanisms, also independent of Subversion, so we can just use
those. When the schema changes, we can just dump a new greek tree, no
problems there.
>My quibble about the test harness is that when a cmdline test fails, I
>can't easily bring up the same command in a debugger to find out what
>went wrong. I'd like it if tests.log contained a log of commands run so
>I can find out which command failed. Plus, if a command seg faults, I'd
>like a note to that effect; as it stands, I get an unexplained "FAIL:"
>and I have to notice the core file lying around.
>
Ah, that can be arranged, certainly.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2002-10-28 at 18:57, Branko Čibej wrote:
> How do you propose to wrap svn_error_creatf without using C99 varargs
> macros? I don't see how it's possible short of explicitly passing
> __FILE__ and __LINE__ to every svn_error_createf call.
Oh, I hadn't thought about that. Fooey.
I don't suppose people would go for having to write
svn_error_createf(apr_err, src_err, child, (fmt, args)), either.
> > * We should always collect line and file information, but only
> > expose it with the appropriate flag set. This will help with
> > debugging and will simplify the internal interface.
> No, we should _collect_ it only with the appropriate flag set
Why?
Re: Laundry list
Posted by Branko Čibej <br...@xbc.nu>.
I long list, so I'll go for the impossibles first...
Greg Hudson wrote:
> * We should replace svn_error_locate() with extra arguments to
> svn_create_error()'s internal name.
>
How do you propose to wrap svn_error_creatf without using C99 varargs
macros? I don't see how it's possible short of explicitly passing
__FILE__ and __LINE__ to every svn_error_createf call.
> * We should always collect line and file information, but only
> expose it with the appropriate flag set. This will help with
> debugging and will simplify the internal interface.
>
No, we should _collect_ it only with the appropriate flag set, but I
agree this should be a runtime, not compile-time optoin.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by Nuutti Kotivuori <na...@iki.fi>.
Greg Hudson wrote:
> props and wc-props files still have no suffix, so they show up in
> "find".
I complained about this as well - but now when I check out a new
working copy, everything seems fine, the suffixes seem to be there.
So could it be that it has been fixed in a backward-compatible way for
working copies?
-- Naked
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Greg Hudson <gh...@MIT.EDU> writes:
> I'll work on filing issues for some of this stuff in a bit.
Thanks.
> > I recall some part of the RA interface being very relaxed about empty
> > strings versus NULL points. Something dealing with the authors and
> > dates and stuff. I certainly have no problem tightening up stuff
> > like that.
>
> Right now, we don't generally assume that a revision has an author or
> date; in practice, revision 0 doesn't have them any all other revisions
> do. We might be able to clean this situation up by doing something
> intelligent with revision 0 and treating it as a database corruption
> error if any other revision doesn't have commit info.
We should certainly make the interfaces consistent (i.e., always use
NULL, not "", to mean the property isn't present). But I don't think
we should treat it as corruption if a revision doesn't have commit
info. The revision tree is still good, and the association of an
author, or whatever, with a change is not something the filesystem
should enforce. One can imagine setups where it wouldn't be
meaningful, and therefore would just be annoying bureaucracy for the
repository admin.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Nov 05, 2002 at 09:59:00AM +0200, Michael Wood wrote:
> On Mon, Nov 04, 2002 at 06:40:07PM +0000, Philip Martin wrote:
>...
> > Is that because they are all created using our clients and libraries?
> > What if some other client, perhaps using other libraries, creates a
> > revision?
It is certainly possible, even using our libraries, to create revisions
without a log message, author, or date.
>...
> cvs2svn.py with Marco's patches actually creates some revisions without
> authors for a CVS repository I tried it on. I think they always have
> dates, though.
That would be a bug in those patches. All revisions should have a date,
author, and log message. For something like a tag, cvs2svn should put in an
assumed username (e.g. 'cvs2svn') and a log message describing the tag
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by Michael Wood <mw...@its.uct.ac.za>.
On Mon, Nov 04, 2002 at 06:40:07PM +0000, Philip Martin wrote:
> Greg Hudson <gh...@MIT.EDU> writes:
>
> > Right now, we don't generally assume that a revision has an author or
> > date; in practice, revision 0 doesn't have them any all other revisions
> > do.
>
> Is that because they are all created using our clients and libraries?
> What if some other client, perhaps using other libraries, creates a
> revision?
Are you referring to the "all other revisions do" part? If so, then
yes.
cvs2svn.py with Marco's patches actually creates some revisions without
authors for a CVS repository I tried it on. I think they always have
dates, though.
Doing an "svn pl -rREV" on one of the revisions just returned:
svn:date
i.e. no svn:log or svn:author.
--
Michael Wood <mw...@its.uct.ac.za>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by Philip Martin <ph...@codematters.co.uk>.
Greg Hudson <gh...@MIT.EDU> writes:
> Right now, we don't generally assume that a revision has an author or
> date; in practice, revision 0 doesn't have them any all other revisions
> do.
Is that because they are all created using our clients and libraries?
What if some other client, perhaps using other libraries, creates a
revision?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by Greg Hudson <gh...@MIT.EDU>.
I'll work on filing issues for some of this stuff in a bit.
On Sun, 2002-11-03 at 18:22, Greg Stein wrote:
> Hmm. Did we come to a resolution on the on this? Seems there was still some
> open discussion on whether this would excessively bulk up the non-debug
> builds.
Well, there are 736 instances of svn_error_create in our code base. At
16 bytes per __FILE__ instance, that's about 12K. And that's if you can
find a compiler which doesn't collapse string literals.
> > ra_dav and ra_local have separate implementations of split_url.
>
> Hmm? I don't see a split_url in ra_dav.
I meant mod_dav_svn, which has dav_svn_split_uri(). They don't behave
exactly the same (they combine some hostname verification with the
repository search), but I'm thinking there should be an
svn_repos_find_repository() which accepts a pathname and returns a
repository and fs_path.
> I recall some part of the RA interface being very relaxed about empty
> strings versus NULL points. Something dealing with the authors and dates and
> stuff. I certainly have no problem tightening up stuff like that.
Right now, we don't generally assume that a revision has an author or
date; in practice, revision 0 doesn't have them any all other revisions
do. We might be able to clean this situation up by doing something
intelligent with revision 0 and treating it as a database corruption
error if any other revision doesn't have commit info.
> >...
> > svn_path_basename()'s contract could be to return a pointer into the
> > argument string, instead of duplicating the result into a pool
> > argument. This would make some code simpler.
>
> I thought about this, but the caller would never know if the passed-in
> string is part of the output or not. Today, the caller can change the input
> string at will since the contract says the result will always be a new
> string in the provided pool. (the doc specifically states it will make a
> copy, even when it isn't strictly necessary)
Oh, my idea was that the output would always be part of the passed-in
string (unless the output is "", maybe), and the caller could
apr_pstrdup the output if it wanted to change the input string.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Laundry list
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Oct 28, 2002 at 06:21:51PM -0500, Greg Hudson wrote:
>...
> There are a few opportunities for exception-handling cleanups:
> * svn_error_clear_all() is dangerous, and must be replaced with an
> svn_error_clear().
Yup.
> * We should replace svn_error_locate() with extra arguments to
> svn_create_error()'s internal name.
> * We should always collect line and file information, but only
> expose it with the appropriate flag set. This will help with
> debugging and will simplify the internal interface.
Hmm. Did we come to a resolution on the on this? Seems there was still some
open discussion on whether this would excessively bulk up the non-debug
builds.
[ on the rest, I agree with all that you stated, but call out some stuff ]
>...
> ra_dav and ra_local have separate implementations of split_url.
Hmm? I don't see a split_url in ra_dav.
>...
> The ra interface passes dates as strings (in svn_time_to_nts()
> format), for implementation convenience. apr_time_t would make for a
> cleaner interface, though it's more work on the back end.
Yes, this is not good. I think we also use a string in some of the 'svn log'
types of operations.
>...
> Someone added an svn_stream_dup() function to my stream interface.
> Nothing uses it. The only purpose would be to move a stream object
> from one pool to another (it doesn't actually split the stream), and I
> can't imagine why any legitimately designed code would want to do
> that. I think it should go away.
+1 Toss it.
> svn_log_changed_path_t uses a char instead of an enum to specify file
> status. Sloppy.
Yah... never liked that. Some other repos stuff (the nodes stuff, iirc) also
does this.
> Our build system's Makefiles use CFLAGS and LDFLAGS. The GNU Makefile
> standards say:
> If there are C compiler options that *must* be used for proper
> compilation of certain files, do not include them in `CFLAGS'.
> Users expect to be able to specify `CFLAGS' freely themselves.
> We should change gen-make.py to use ALL_CFLAGS for everything except
> optimization options, and then rename EXTRA_CFLAGS to CFLAGS. Same
> deal for CPPFLAGS and LDFLAGS.
This might need an issue to track it well enough. It sounds kind of tricky.
> svn_wc_get_actual_target() sets *target to NULL when anchor is the
> target. That was probably right at the time, but these days we use ""
> to mean an empty relative path, not NULL. As a consequence of this
> function's behavior, a number of contracts are unnecessarily lax,
> including the ra interface's update/switch/status functions.
I recall some part of the RA interface being very relaxed about empty
strings versus NULL points. Something dealing with the authors and dates and
stuff. I certainly have no problem tightening up stuff like that.
>...
> adm_files.c's extend_with_adm_name() doubles as a path concatenation
> function. That seems a little bizarre, but it's pretty entrenched, so
> I'm not likely to do anything about it.
Be nice if there was a way to use svn_path_join_many(). Would probably need
a variant of the join which takes a valist.
>...
> svn_path_basename()'s contract could be to return a pointer into the
> argument string, instead of duplicating the result into a pool
> argument. This would make some code simpler.
I thought about this, but the caller would never know if the passed-in
string is part of the output or not. Today, the caller can change the input
string at will since the contract says the result will always be a new
string in the provided pool. (the doc specifically states it will make a
copy, even when it isn't strictly necessary)
> While building with shared libraries, I ran into a libtool relink
> problem when installing. (Red Hat 7.3.) I've seen this problem
> before, in other code, but forget what the cause was. I should
> investigate this.
Hmm. We definitely relink at install time, but I haven't seen any errors
recently.
>...
> props and wc-props files still have no suffix, so they show up in
> "find". (Arguably, all these files should have prefixes as well as
> suffixes, for added protection.)
Ah. Interesting thought. Prefixes would be interesting. At this rate, we
should just choose a unique name and store that into the 'entries'. It would
save us a good amount of pain with all this prefix/suffix stuff.
[ of course, that would make debugging hard. "what does ekz45931 correspond
to? grr..." ]
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org