On Wednesday 04 May 2005 16:31, kfogel@collab.net wrote:
> John Szakmeister <john@szakmeister.net> writes:
> > Hmph... I was following the pattern that I saw in other files (the
> > internal functions were document a lot less formally than
> > public/semi-public ones). I can go back and update this though.
>
> Thanks! (See "Document everything" in HACKING, btw. It is lamentable
> that not all of our internal functions live up to this ideal.)
Thanks.
> > I assume that it should be doxygenified? :-)
>
> Nope, just use all caps for parameter names.
>
> > svn_opt_revision_working, svn_opt_revision_base, and
> > svn_opt_revision_comitted are the acceptable values. I'll make sure
> > to document that as well as put in a few assert() statements.
>
> Cool.
>
> But not assert()! Just return an error, and document what the error
> will be. Assertions check "can't happen" cases, stuff that's worth
> exiting immediately for (i.e., there is a bug in the code itself).
> Error checks are for situations where the code is good but the input
> might be bad, which is what we'd have here.
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. :-)
> > I missed some of the peg discussions, so I wasn't certain if it was
> > really feasible to specify a peg revision on a local path. I
> > actually tried it on the command line, and the peg revision is
> > something other than unspecified if you @REVISION syntax
> > (my-local-file@BASE, for example). From my understanding of the peg
> > revision, it's where we're supposed to start looking as far as
> > history tracing is concerned. If it's anything other than
> > unspecified, base, or committed, then I assumed we'd have to ask the
> > repository for that information (how would I know if the file was
> > called x at revision y without contacting the repository?). Hence
> > the check. If I'm wrong, please enlighten me, and I'll either remove
> > the redundant code or document why I chose to check the peg revision.
>
> No, you're right. We would have to contact the repository to
> determine the node identity, but (interestingly enough) we might not
> have to contact the repository for the actual *contents*, since we
> might have them locally. Your code is correct as it stands, so I'm
> not suggesting a change. If you want to add a comment, that'd be
> great, but no big deal.
Cool. I thought I missed something substantial. :-)
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu May 5 12:02:05 2005