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
Received on Tue Oct 29 00:22:41 2002