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...@red-bean.com> on 2008/06/30 00:58:48 UTC
Re: svn commit: r31874 - in trunk/subversion: include libsvn_client svn tests/cmdline
cmpilato@tigris.org writes:
> --- trunk/subversion/libsvn_client/mergeinfo.c (r31873)
> +++ trunk/subversion/libsvn_client/mergeinfo.c (r31874)
> @@ -1118,13 +1118,55 @@ logs_for_mergeinfo_rangelist(const char
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +location_from_path_and_rev(const char **url,
> + svn_opt_revision_t **revision,
> + const char *path_or_url,
> + const svn_opt_revision_t *peg_revision,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
Doc string.
Seriously. That function may look obvious, but all sorts of questions
can arise: what are the legal values in the substructure fields in
*peg_revision, and how do they behave? May url or revision be NULL if
the caller doesn't care about that information? Are there any
particular error conditions it's important to promise? Etc.
I recently spent more time reviewing the r31059 group in 1.5.x/STATUS
than I should have needed, because the get_full_mergeinfo() internal
function was not -- and still is not -- documented. Let's please not
perpetuate this problem :-).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31874 - in trunk/subversion: include libsvn_client svn tests/cmdline
Posted by Karl Fogel <kf...@red-bean.com>.
"C. Michael Pilato" <cm...@collab.net> writes:
> Nono, I totally agree. For some reason docstrings on helper functions
> have slipped routinely out of scope in my review-before-commit stuff.
> Thanks for calling attention to this.
No worries. As you can see, I've decided to play doc string police on
every commit for a while -- even when I don't have time to review the
code change, I'll check that doc strings are there for new functions and
data structures.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31874 - in trunk/subversion: include libsvn_client
svn tests/cmdline
Posted by "C. Michael Pilato" <cm...@collab.net>.
Karl Fogel wrote:
> cmpilato@tigris.org writes:
>> --- trunk/subversion/libsvn_client/mergeinfo.c (r31873)
>> +++ trunk/subversion/libsvn_client/mergeinfo.c (r31874)
>> @@ -1118,13 +1118,55 @@ logs_for_mergeinfo_rangelist(const char
>> return SVN_NO_ERROR;
>> }
>>
>> +static svn_error_t *
>> +location_from_path_and_rev(const char **url,
>> + svn_opt_revision_t **revision,
>> + const char *path_or_url,
>> + const svn_opt_revision_t *peg_revision,
>> + svn_client_ctx_t *ctx,
>> + apr_pool_t *pool)
>> +{
>
> Doc string.
>
> Seriously. That function may look obvious, but all sorts of questions
> can arise: what are the legal values in the substructure fields in
> *peg_revision, and how do they behave? May url or revision be NULL if
> the caller doesn't care about that information? Are there any
> particular error conditions it's important to promise? Etc.
>
> I recently spent more time reviewing the r31059 group in 1.5.x/STATUS
> than I should have needed, because the get_full_mergeinfo() internal
> function was not -- and still is not -- documented. Let's please not
> perpetuate this problem :-).
Nono, I totally agree. For some reason docstrings on helper functions have
slipped routinely out of scope in my review-before-commit stuff. Thanks for
calling attention to this.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand