Code review comment for lp:~jameinel/bzr/2.4-overridAttr-non-existant

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 11/6/2012 12:39 PM, Vincent Ladeuil wrote:
> > IIRC we didn't implement the unexisting attr case because we didn't
> > need it.
> >
> > Later on, testtools provided 'patch' which takes this case into
> > account.
> >
> > I think it would be better to use the testtools version for your
> > specific case.
> >
> > Also, unless you intend to cut a release for 2.4, I think we should
> > avoid landing patches on stable releases for which we don't intend
> > to make a release, the SRU process is resource intensive and we
> > still haven't a 2.6.0 release yet...
> >
>
> So I intended to do the SSH fdatasync patch in the release that added
> fdatasync (2.4). I'd rather have that patch in a 2.4 series that
> people can download if they want even if we don't SRU it, rather than
> only land it in something newer.

Daggy fixing as you did is enough (and a Good Thing anyway) for people to easily get the patch if they need.

>
> I don't plan on SRUing this,

Then don't make a potentially critical and required SRU harder by adding code that is not mandatory.

> I tried grepping through the codebase, and could find no references to
> testtools.monkey.patch. Even in bzr.dev tip.

Yes, because the existing overrideAttr cover all the needed cases.

>
> Also, reading the code in monkey.py it *doesn't* handle the delattr
> case. At least in 0.9.14.final.0 that I have here.

It is there in 0.9.12 and... still there on tip, check again (hint: restore()).