[Rt-devel] PATCH: Link symmetry

[Todd Chapman]

On Sat, Nov 05, 2005 at 01:04:46AM +0100, Rolf Grossmann wrote:
> Todd Chapman wrote:
> 
> > OK gang,
> > 
> > Here is my stab at fixing the asymmetrical link
> > transaction problem. Added a couple of tests
> > and everything passes. See any problems?
> 
> You've been a busy beaver, thank you.

Yes I have. :) I also submitted 3 other patches to rt-bugs today,
including a bug in HasRight that bit me while writing RightsMatrix.

> 
> I guess you didn't test the other scrips actually not running (the
> default would be on, right?), because there is a typo in you patch:
> ActtivateScrips (note the extra t) on both _NewTransaction calls.

Your right. I didn't test the scrips firing/not firing. Not sure how
to do that in regression tests. I'll fix the typo and resubmit the patch.

> 
> Also, I usually prefer to have the simpler branch of a condition handled
> first, especially when it involves a return clause. You've switched that
> for the one
> 	if ( $args{'Silent'} ) {
> I guess you wanted to collect the returning at the end of the function.

See other mail. Nitpicking is not appreciated.

> 
> Ok, now that I'm done nitpicking (sorry for that ;)), I'd also like to
> remark what you're suggesting now is to always have the second
> transaction, but to only conditionally run the scrips for it. I don't
> think that's the right thing to do. I think the argument was to always
> run scrips with a transaction. If you're being backwards compatible, you
> can do without the extra transaction aswell. So I'd suggest to
> completely scrap the extra (misspelled ;)) option and add the condition
> with the
> 	if ( $remote_uri->IsLocal )
> like this:
> 	if ( $remote_uri->IsLocal && $RT::LinkMakes2Transactions)
> 
> What do you think?

I agree with Ruslan. One transaction is a bug. 2 is the right way. Defaulting
to one scrip is necessary for backwards compatability.

> 
> Rolf.