On Thursday 05 May 2005 16:00, kfogel@collab.net wrote:
> John Szakmeister <john@szakmeister.net> writes:
> > I guess it depends on your point of view. In this case, if I
> > document that you can only send it
> > svn_opt_revision_base/committed/working and you send it something
> > else, then you violated the API. In that case, I feel assert() is
> > okay (in much the same way that we verify that input buffers and
> > paths aren't NULL). This function leaves the job of input validation
> > to the next level up. *shrug* I don't care all that much, I'm just
> > trying to understand why a runtime error is better than assert() in
> > this case. :-)
>
> Ah. I see what you're saying. Here are my thoughts.
>
> First, an error is much easier to understand.
I've attached a patch that does most of what you asked (which was actually
a very good exercise since I forgot the unspecified revision was being
mapped to base, thank you). However, I didn't implement the input
validation. I sat down to do it twice, and I kept stumbling on how to
create an appropriate error for the user. I think it would be confusing
to say something like "Must be one of revision BASE, COMMITTED, WORKING,
or UNSPECIFIED" or that a revision is disallowed for any reason. The
user did nothing wrong, and may have supplied a completely sound
argument. So the attached patch has an assert() statement the for the
moment. Do you have suggestions for what an appropriate error might look
like? Our error messages are terse enough without confusing the user
between a programmatic error (us violating a pre-condition) and a real
user error (they specified an invalid revision). I also grepped through
the current code looking for a similar situation, and didn't find one
that was sufficiently close to this to model after.
> Second, we can force every caller to validate the revision, or it can
> just pass the revision along and count on this function to error if
> appropriate. Result: avoidance of duplicated code.
It's a static helper function, with 1 caller and the caller already has to
do range checking to know whether or not contact the repository. In this
case, it looks like it actually generates code duplication, since I'll
basically have to do the same checks in cat_local_file(). *shrug*
-John
Stop being overly restrictive on which revisions we can cat. Update a doc
string and add some verification to check revision ranges when running 'svn
cat' on a local file.
* subversion/libsvn_client/cat.c
(cat_local_file): Update doc string, and verify that revision falls within
valid ranges.
(svn_client_cat2): Allow revision WORKING. We shouldn't assume that because
we don't have -rWORKING on the command line, that it can't be passed in by a
third party.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu May 12 11:43:15 2005