Code review comment for lp:~jelmer/bzr/overwrite-tags

On 02/03/2012 02:10 PM, Vincent Ladeuil wrote:
>> Am 03/02/12 09:55, schrieb Vincent Ladeuil:
>>> Having slept on the topic, I had some more ideas.
>>>
>>> My main objection here is that we break the API at the command-line level
>>> which is bad for plugins overriding pull or push.
>> Why is this a problem while if we're going to do another beta release?
>> AFAIK I'm the only person who works on plugins that override pull or
>> push, and I'm happy to do new releases of them before 2.5.0.
>
> I don't think bzr-pipeline is impacted but what about bzr-loom ?
bzr-loom isn't impacted either as far as I can tell - it just passes
'overwrite' along as argument to the underlying Branch implementation.
>
>>> Not super user-friendly but workable.
>> Is it really a problem if we add an option (--overwrite-tags) that is
>> specific to formats with non-versioned tags? In the future it will just
>> become irrelevant for some formats, but it will stay useful for older
>> formats and some foreign formats anyway. Changing optparse seems much
>> more intrusive than just adding another option, and it's less well
>> discoverable.
>
> I'm not talking about chnaging optparse, I'm talking of introducing a
> specific option.
Still, it's changing an existing option and adding a bunch of logic to
deal with possible arguments to options. I don't see how that's better
than adding a new (more discoverable) option.

>
>>> 2) We don't introduce an '--overwrite-tags' cmdline option but
instead add
>>> config options: 'pull.overwrite' and 'push.overwrite' defaulting to
>>> 'history,tags' (or 'revisions' instead of 'history' ?).
>>>
>>> In this case, neither the cmdline interface is changed nor any internal
>>> APIs, the interpretation of 'overwrite' is just delayed.
>>>
>>> This seems less intrusive, will provide some help via 'bzr help
>>> pull.overwrite' and is fully backward compatible.
>>>
>>> This can then be used as 'bzr pull --overwrite -Opull.overwrite=tags'.
>>>
>>> This proposal then becomes a bugfix that can very well land in 2.5.
>
>> I don't see how this helps - it is still a contract change.
>
> A less intrusive one, that's the point. If the options are used with older
> bzr, nothing happens.
Something does happen, because history will be overwritten *too* (since
the underlying implementation doesn't look at pull.overwrite), not just
the tags. The same is the case with my change.

Also, isn't -O new in 2.5 anyway ?
>
>> This still requires the foreign plugins to be updated, because they have
>> to add support for this option.
>
> How is your proposal different in this respect ?
It isn't different in this respect, but neither does the alternative
provide any benefits in this regard.
> These options also don't require updating the plugins unless they override
> bzr internals. If they do they need to be updated whatever solution is
used.
Why do any plugins other than bzr-{svn,git,hg} have to be updated in the
case of my MP?

Cheers,

Jelmer