Code review comment for lp:~vila/bzr/deprecation-warning-preference

Revision history for this message
Vincent Ladeuil (vila) wrote :

This patch supersedes https://code.edge.launchpad.net/~ted/bzr/deprecation-warning-preference/+merge/15455

It's a patch pilot attempt to land a work started by fullermd far too long ago and re-started by ted more recently.

I tried to capture most of the comments by Ian, Martin and John without spending too much time on it (but still...).

This patch adds:
- specifying a suppress_warnings variable in any configuration file,
- explicitly requiring a config variable as a list,
- a suppres_warning() method to config files,
- checks for format_deprecation at lock time on branch objects (which can then provide
  their config to the repo before locking it),
- checks for format_deprecation at lock on repo objects but relying on bazaar.conf only
  in that case.

I may file a bug or three for the remaining points so please comment if you disagree
with some decision made here but mention if you agree with postponing it (good) or you
think it's required to land (bad :).
The idea was to implement something that we could tweak further
but that will not require some upgrade dance in the future (so mainly: keep the same
variable name, don't force setting it in bazaar.conf so the default behaviour for
new branches is still to raise the warning).

These remaining points are:
- the actual implementation use a single global variable for the whole code base,
  forget about being warned for the second repo accessed (or the 20th for bzrlib users),
  that should be implemented as a repo attribute instead,
- working trees can raise format deprecation warnings too, they should use the new
  facility (I don't think we want to proliferate warning variables gratuitously)
- hpss remote repositories doesn't raise the warning locally (AFAICS but feel free
  to prove me wrong),
- no distinction is made between read and write access (I'm not sure I understand
  the need here),
- there is no config file for repositories were such an option can be specified.

My intent here is to provide a base for future work without addressing some existing problems (some of which have been revealed while writing this patch).

I didn't address John idea about filtering by format as I think we plan to
reduce the number of formats and push people to use the default ones.
The rationale is that: 1) you care or you don't care about being warned, 2) you can set
the option in various ways, the finest grain being the branch. I think that's
good enough.

« Back to merge proposal