Merge lp:~jelmer/bzr/overwrite-tags into lp:bzr/2.5

[Jelmer Vernooij]

Add --overwrite-tags argument to "bzr push" and "bzr pull".

I'm not entirely sure about the API change. I don't really want to end up with overwrite-history, overwrite-tags, overwrite-references, etc so using a tuple seemed reasonable. Users can still specify a boolean if they want to overwrite everything. Thoughts?

[Vincent Ladeuil (vila)]

I'm not sure we're closing all holes here but until we have proper versioning for tags this seems like a valuable step.

Aren't we breaking plugins that redefine push/pull here ? (from irc discussion, yes :-/)

I think we need to think a bit more about it and re-targeting bzr.dev sounds more appropriate ?

review: Needs Information

[Vincent Ladeuil (vila)]

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.

A less important objection is that the way we override the meaning of
--overwrite is a bit hackish and hard to document for plugins as well as
users.

A deeper one is that this smells like a work-around waiting for a more
robust story for tag handling which need to be versioned.

I can think of two ways to address these issues:

1) Create a specific Overwrite optparse option that will accept being used
as either: '--overwrite' or '--overwrite tags'. Internally we can still
consider that overwrite=False means nothing is overwritten and is a list of
components to overwrite otherwise. Since we used 'if overwrite' in the code
and 'overwrite=False' in signatures this should work.

This works less well as the command-line level as we need to make sure we
correctly interpret '--overwrite <url>' by leaving <url> alone (restricting
to 'tags' or ':tags', the latter avoiding ambiguities.

Not super user-friendly but workable.

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.

Thoughts ?

[Jelmer Vernooij (jelmer)]

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.
> A less important objection is that the way we override the meaning of
> --overwrite is a bit hackish and hard to document for plugins as well as
> users.
>
> A deeper one is that this smells like a work-around waiting for a more
> robust story for tag handling which need to be versioned.
>
> I can think of two ways to address these issues:
>
> 1) Create a specific Overwrite optparse option that will accept being used
> as either: '--overwrite' or '--overwrite tags'. Internally we can still
> consider that overwrite=False means nothing is overwritten and is a list of
> components to overwrite otherwise. Since we used 'if overwrite' in the code
> and 'overwrite=False' in signatures this should work.
>
> This works less well as the command-line level as we need to make sure we
> correctly interpret '--overwrite <url>' by leaving <url> alone (restricting
> to 'tags' or ':tags', the latter avoiding ambiguities.
>
> 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.
> 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. This still
requires the foreign plugins to be updated, because they have to add
support for this option. Without support for that option in the foreign
plugins they will always just overwrite history and tags, which is just
as bad.

Cheers,

Jelmer

[Vincent Ladeuil (vila)]

> 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 ?

> > 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.

> > 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.

> 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 ?

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.

[Jelmer Vernooij (jelmer)]

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

[Jelmer Vernooij (jelmer)]

Any chance of this in 2.5.1 perhaps?

[Vincent Ladeuil (vila)]

Since versioned tags won't come soon, it makes sense to address the gap *now*, in 2.5, please land.

review: Approve

[Jelmer Vernooij (jelmer)]

This can't land on 2.5 anymore since it's a fairly major API change that will break bzr-git, bzr-svn and bzr-hg.:-(

review: Disapprove

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2012-01-27 14:56:06 +0000
+++ bzrlib/branch.py	2012-02-02 14:31:53 +0000
@@ -3346,6 +3346,15 @@
         raise NotImplementedError(self.fetch)
 
 
+def _fix_overwrite_type(overwrite):
+    if isinstance(overwrite, bool):
+        if overwrite:
+            return ["history", "tags"]
+        else:
+            return []
+    return overwrite
+
+
 class GenericInterBranch(InterBranch):
     """InterBranch implementation that uses public Branch functions."""
 
@@ -3516,15 +3525,18 @@
         result.target_branch = self.target
         result.old_revno, result.old_revid = self.target.last_revision_info()
         self.source.update_references(self.target)
+        overwrite = _fix_overwrite_type(overwrite)
         if result.old_revid != stop_revision:
             # We assume that during 'push' this repository is closer than
             # the target.
             graph = self.source.repository.get_graph(self.target.repository)
-            self._update_revisions(stop_revision, overwrite=overwrite,
-                    graph=graph)
+            self._update_revisions(stop_revision,
+                overwrite=("history" in overwrite),
+                graph=graph)
         if self.source._push_should_merge_tags():
             result.tag_updates, result.tag_conflicts = (
-                self.source.tags.merge_to(self.target.tags, overwrite))
+                self.source.tags.merge_to(
+                self.target.tags, "tags" in overwrite))
         result.new_revno, result.new_revid = self.target.last_revision_info()
         return result
 
@@ -3608,13 +3620,16 @@
             # -- JRV20090506
             result.old_revno, result.old_revid = \
                 self.target.last_revision_info()
-            self._update_revisions(stop_revision, overwrite=overwrite,
+            overwrite = _fix_overwrite_type(overwrite)
+            self._update_revisions(stop_revision,
+                overwrite=("history" in overwrite),
                 graph=graph)
             # TODO: The old revid should be specified when merging tags, 
             # so a tags implementation that versions tags can only 
             # pull in the most recent changes. -- JRV20090506
             result.tag_updates, result.tag_conflicts = (
-                self.source.tags.merge_to(self.target.tags, overwrite,
+                self.source.tags.merge_to(self.target.tags,
+                    "tags" in overwrite,
                     ignore_master=not merge_tags_to_master))
             result.new_revno, result.new_revid = self.target.last_revision_info()
             if _hook_master:
 
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2012-01-31 15:43:17 +0000
+++ bzrlib/builtins.py	2012-02-02 14:31:53 +0000
@@ -1141,7 +1141,9 @@
                  "the master branch."
             ),
         Option('show-base',
-            help="Show base revision text in conflicts.")
+            help="Show base revision text in conflicts."),
+        Option('overwrite-tags',
+            help="Overwrite tags only."),
         ]
     takes_args = ['location?']
     encoding_type = 'replace'
@@ -1149,7 +1151,14 @@
     def run(self, location=None, remember=None, overwrite=False,
             revision=None, verbose=False,
             directory=None, local=False,
-            show_base=False):
+            show_base=False, overwrite_tags=False):
+
+        if overwrite:
+            overwrite = ["history", "tags"]
+        elif overwrite_tags:
+            overwrite = ["tags"]
+        else:
+            overwrite = []
         # FIXME: too much stuff is in the command class
         revision_id = None
         mergeable = None
@@ -1291,6 +1300,8 @@
         Option('no-tree',
                help="Don't populate the working tree, even for protocols"
                " that support it."),
+        Option('overwrite-tags',
+              help="Overwrite tags only."),
         ]
     takes_args = ['location?']
     encoding_type = 'replace'
@@ -1298,9 +1309,17 @@
     def run(self, location=None, remember=None, overwrite=False,
         create_prefix=False, verbose=False, revision=None,
         use_existing_dir=False, directory=None, stacked_on=None,
-        stacked=False, strict=None, no_tree=False):
+        stacked=False, strict=None, no_tree=False,
+        overwrite_tags=False):
         from bzrlib.push import _show_push_branch
 
+        if overwrite:
+            overwrite = ["history", "tags"]
+        elif overwrite_tags:
+            overwrite = ["tags"]
+        else:
+            overwrite = []
+
         if directory is None:
             directory = '.'
         # Get the source branch
 
=== modified file 'bzrlib/push.py'
--- bzrlib/push.py	2012-01-19 17:23:00 +0000
+++ bzrlib/push.py	2012-02-02 14:31:53 +0000
@@ -68,8 +68,8 @@
     :param location: the url of the destination
     :param to_file: the output stream
     :param verbose: if True, display more output than normal
-    :param overwrite: if False, a current branch at the destination may not
-        have diverged from the source, otherwise the push fails
+    :param overwrite: list of things to overwrite ("history", "tags")
+        or boolean indicating for everything
     :param remember: if True, store the location as the push location for
         the source branch
     :param stacked_on: the url of the branch, if any, to stack on;
 
=== modified file 'bzrlib/tests/blackbox/test_pull.py'
--- bzrlib/tests/blackbox/test_pull.py	2012-01-05 17:46:44 +0000
+++ bzrlib/tests/blackbox/test_pull.py	2012-02-02 14:31:53 +0000
@@ -527,6 +527,23 @@
         self.assertEqual(out,
             ('1 tag(s) updated.\n', ''))
 
+    def test_overwrite_tags(self):
+        """--overwrite-tags only overwrites tags, not revisions."""
+        from_tree = self.make_branch_and_tree('from')
+        from_tree.branch.tags.set_tag("mytag", "somerevid")
+        to_tree = self.make_branch_and_tree('to')
+        to_tree.branch.tags.set_tag("mytag", "anotherrevid")
+        revid1 = to_tree.commit('my commit')
+        out = self.run_bzr(['pull', '-d', 'to', 'from'], retcode=1)
+        self.assertEquals(out,
+            ('No revisions to pull.\nConflicting tags:\n    mytag\n', ''))
+        out = self.run_bzr(['pull', '-d', 'to', '--overwrite-tags', 'from'])
+        self.assertEquals(out, ('1 tag(s) updated.\n', ''))
+
+        self.assertEquals(to_tree.branch.tags.lookup_tag('mytag'),
+                          'somerevid')
+        self.assertEquals(to_tree.branch.last_revision(), revid1)
+
     def test_pull_tag_overwrite(self):
         """pulling tags with --overwrite only reports changed tags."""
         # create a branch, see that --show-base fails
 
=== modified file 'bzrlib/tests/blackbox/test_push.py'
--- bzrlib/tests/blackbox/test_push.py	2011-12-24 10:10:59 +0000
+++ bzrlib/tests/blackbox/test_push.py	2012-02-02 14:31:53 +0000
@@ -605,6 +605,22 @@
         self.assertEqual('', out)
         self.assertEqual('Created new branch.\n', err)
 
+    def test_overwrite_tags(self):
+        """--overwrite-tags only overwrites tags, not revisions."""
+        from_tree = self.make_branch_and_tree('from')
+        from_tree.branch.tags.set_tag("mytag", "somerevid")
+        to_tree = self.make_branch_and_tree('to')
+        to_tree.branch.tags.set_tag("mytag", "anotherrevid")
+        revid1 = to_tree.commit('my commit')
+        out = self.run_bzr(['push', '-d', 'from', 'to'])
+        self.assertEquals(out,
+            ('Conflicting tags:\n    mytag\n', 'No new revisions to push.\n'))
+        out = self.run_bzr(['push', '-d', 'from', '--overwrite-tags', 'to'])
+        self.assertEquals(out, ('', '1 tag updated.\n'))
+        self.assertEquals(to_tree.branch.tags.lookup_tag('mytag'),
+                          'somerevid')
+        self.assertEquals(to_tree.branch.last_revision(), revid1)
+
 
 class RedirectingMemoryTransport(memory.MemoryTransport):
 
 
=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2012-02-01 17:41:55 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2012-02-02 14:31:53 +0000
@@ -20,6 +20,9 @@
 
 .. New commands, options, etc that users may wish to try out.
 
+* New option ``--overwrite-tags`` for ``bzr pull`` and ``bzr push``.
+  (Jelmer Vernooij, #681792)
+
 * Support for colocated branches is now available in the default
   format ("2a"). (Jelmer Vernooij)