Madan U Sreenivasan <madan@collab.net> writes:
> > * 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 )
Ah, I see. What I meant to express is: there's a difference between
revving an API to take a new struct2, and actually changing the code
behind that API to *use* the new fields in struct2. I'm only
proposing the former; I agree that changing the code behind those
other APIs can happen in a separate stage.
> > * 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.
Yes -- good call, thanks for pointing it out.
And in fact, the revving of the APIs themselves could happen in a
separate patch, if you want. (I.e., introduce
svn_client_commit_info2_t, with its new field, everywhere, but don't
use it -- then the next patch would make use of it in ra_dav).
> > * 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.
Well, thank *you* for your patience. I realize there are a lot of
conventions to learn here!
> > 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.
So, I think to keep the patches manageable (i.e., reviewable :-) ),
the stages could be:
1. Introduce svn_client_commit_info2_t and its constructor.
2. Rev all APIs that take svn_client_commit_info_t to take
svn_client_commit_info2_t instead, but don't actually do
anything with the new field yet. IOW, no behavioral change,
just the introduction of a new type, in preparation for
stages 3, 4, and 5 below.
3. In one RA layer, start using the new field.
4. Same for the next RA layer :-).
5. Same for yet another RA layer :-).
And if you can think of other intermediate stages that ought to be
inserted into this process, go ahead -- the above is just a rough
outline.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jul 7 16:08:26 2005