> 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 understand. I will do that. thank you for pointing out.
>
>
>>>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.
okay, np... I'll give it another try too...
>
>
>>>>+/** @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.
> */
I will do this. Thanks.
>
> 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 will try my best not to repeat this. Thanks for pointing out.
>
>>>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.
oh, no... not at all...its a learning experience for me. I should thank you
for that. Sometimes, I feel that I have been a little rude in
conversations... apologize if you felt so about any of my statements.
Again, thanks a lot for helping me out.
Regards,
Madan.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun May 22 18:24:33 2005