Madan US wrote:
>>Madan US wrote:
>>
>>>[[[
>>> Partial Fix for Issue #443: post-commit hook script (error) output
>>> lost
>>>
>>
>>Please begin the log message with a description of what the patch does
>>(and why, if that's not obvious).
>
> Above is a snip from the description of the issue at
> subversion.tigris.org... Anyways, I would put in more info hereon.
The few words above indicate what problem this patch addresses, but because it
is a "Partial fix" you need to say what this patch does towards fixing the problem.
>>I don't think it's acceptable to duplicate several large functions like
>>this one just to change two lines in them. You will have to find a
>>way to factor out the common parts.
>
> True... I ran out of ideas here... could you pl. suggest an alternate
> implementation for this one...
Sorry, I don't have time. Maybe someone else will.
>>>+/** @deprecated Provided for backward compatibility.
>>>+ *
>>>+ * Similar to wc_to_repos_copy but uses svn_client_commit_info_t +
>>>* for the @a commit_info parameter.
>>>+ */
>>> static svn_error_t *
>>> wc_to_repos_copy (svn_client_commit_info_t **commit_info,
>>> const char *src_path,
>>
>>These comments are inappropriate. A static function isn't deprecated,
>>it's either used or deleted; in this case is it used (by a deprecated
>>function). We only use Doxygen mark-up in public API headers.
>
> Yes, so I just leave this uncommented??? How do I say that this has two
> versions and that the old version should be removed sometime in future?? Pl.
> advise.
/* Similar to wc_to_repos_copy but uses svn_client_commit_info_t +
* for the COMMIT_INFO parameter.
*/
You don't need to say that it should be removed some time in the future when it
is no longer used, since that is true for every static function, and most
compilers will warn if it is unused. You could say it is a "Helper function
for ..." to give the reader a hint.
>>or take it as a sign that there may be something wrong
>>with the patch.
>
> Something wrong with the patch??? IMHO, I dont think so... the patch is so
Not functionally, wrong, perhaps, but sylistically wrong.
> big because of deprecating functios thorughout the code ( from one end of
> the client to the other end of the server ) over all three protocols.
> Pl. correct me if I'm wrong.
The patch is so big because of duplicating code rather than factoring out the
common parts.
>>I haven't reviewed your implementation. I'm glad you're attacking the
>>problem, though.
>
> Cool, and am happy this patch is getting noticed inspite of the size.
> Thanks, Julian.
No problem, and sorry if some of what I said sounds a little harsh.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun May 22 16:12:56 2005