[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Partial fix for Issue #443: post-commit hook script (error) output lost.

From: <kfogel_at_collab.net>
Date: 2005-07-07 15:18:50 CEST

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


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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.