Hi Karl,
> manner described in the following IRC conversation, which I will quote
> in full, so you can see the love and care with which I enter into code
> reviews:
I totally understand Karl. :)
[snip]
> Okay, at this point enough questions have been raised in the log
> message that I think it doesn't make sense for me to review the patch.
> Instead, can you produce a new patch that
>
> * Avoids the need for svn_ra_find_protocol() and the conditionals
> resulting therefrom.
> * Really deprecates the things it deprecates, everywhere, and revs
> APIs as necessary to compensate.
hmmmm... that was what my earlier patches on #443 did - using the new
APIs all the way. But then along the way, while refactoring some
functions, and, I thought if the issue was taken care in phases ( ra_dav
first, ra_local and ra_svn subsequently), two things can be achieved...
- Ease of review : as the patch is only half the size
- Help as a inherent test of the current use of the deprecable API
functions ( that is I have NOT broken existing API usage )
>
> * Provides a constructor function for any structures we had to rev,
> and documents on the new versions of the structures that they
> should always be allocated by the constructor, so that at least
> future revs will not be necessary.
hmmmm, This should be done as a separate patch, before introducing the
new structures, shouldn't it? IMHumbleO, The intent of introducing the
constructor type of function doesnt inherently figure in the purpose of
#443.
>
> * Has a log message in the more compact, more standard style that
> I've tried to point out above.
There a lot for me to take home from your review. Thank you for the
detailed review and suggestions. I will spend time on your comments. You
can expect better logs from me next time on. Thank you.
>
> Or, if any of these things (especially the first three) seem like bad
> ideas to you, please explain why, and we'll figure out the best design
> on the list here.
Oh, no, nothing wrong with the idea.... not at all, in fact I would have
been happy to do that ( specifically, I wasnt happy to introduce the
svn_ra_find_protocol() function at all - just had to do it to reduce the
size of the patch.)
Summary : If a longer patch is okay, I will take care of 1,2. Point 3 to
be taken care as a separate patch prior to #443. Point 4 is a MUST DO
for me. I will take care of this in future. Thanks.
Karl, pl. let me know what you think.
Regards,
Madan.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jul 7 09:07:05 2005