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

[Jelmer Vernooij]

Add --overwrite-tags option to push and pull.

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?

(There is also a merge proposal open for lp:bzr/2.5, this is a resubmission against lp:bzr)

[Vincent Ladeuil (vila)]

I'm not sure we understood each other here :-/

What I objected about was the addition of the --overwrite-tags parameter, all the rest is ~fine by me (I'm still wondering about _fix_overwrite_type being private but needed by the plugins but that's a minor concern).

The rationale here is that we already know that the long term solution is to have versioned tags. Once we get there, --overwrite-tags will have to be deprecated/deleted as it will be replaced by some moral equivalent of 'bzr resolve --take-other'.

Therefore, I'd rather implement this behavior behind a config option (that can still be used from the command line with -O) to address the edge case and not add a parameter that we'll have to remove. The option can be documented to explain that it's a temporary one that will eventually get removed once tags are versioned.

[Jelmer Vernooij (jelmer)]

Am 13/03/12 09:10, schrieb Vincent Ladeuil:
> I'm not sure we understood each other here :-/
>
> What I objected about was the addition of the --overwrite-tags parameter, all the rest is ~fine by me (I'm still wondering about _fix_overwrite_type being private but needed by the plugins but that's a minor concern).
>
> The rationale here is that we already know that the long term solution is to have versioned tags. Once we get there, --overwrite-tags will have to be deprecated/deleted as it will be replaced by some moral equivalent of 'bzr resolve --take-other'.
Even when we have versioned tags I still think it would be useful to
overwrite tags in some cases.
> Therefore, I'd rather implement this behavior behind a config option (that can still be used from the command line with -O) to address the edge case and not add a parameter that we'll have to remove. The option can be documented to explain that it's a temporary one that will eventually get removed once tags are versioned.
There are always going to be formats in bzr that don't support versioned tags, so I don't think we'd ever be able to delete it. Having it as a configuration option just makes it less explicit in the API I think.

Cheers,

jelmer

[Vincent Ladeuil (vila)]

> There are always going to be formats in bzr that don't support versioned tags,
> so I don't think we'd ever be able to delete it. Having it as a configuration
> option just makes it less explicit in the API I think.

Right, that's exactly where we disagree :) Tag conflicts occur rarely so only a few people are concerned by this feature and I argue that these people will be equally well served by a config option. *But* even the option and the command line parameter fall short about addressing the issue I faced for the 2.3.2 bzr release where lp:bzr/2.3 needed to have its tags overwritten but was (still is and will be) unreachable (without asking a losa which is cheating).

How about landing this stuff behind a config option and *then* discuss about adding the parameter to have some progress ? That would even allow landing in 2.5 in this case as this will not break compatibility (assuming you update the foreign plugins accordingly).

[Jelmer Vernooij (jelmer)]

Am 13/03/12 10:15, schrieb Vincent Ladeuil:
>> There are always going to be formats in bzr that don't support versioned tags,
>> so I don't think we'd ever be able to delete it. Having it as a configuration
>> option just makes it less explicit in the API I think.
> Right, that's exactly where we disagree :) Tag conflicts occur rarely so only a few people are concerned by this feature and I argue that these people will be equally well served by a config option. *But* even the option and the command line parameter fall short about addressing the issue I faced for the 2.3.2 bzr release where lp:bzr/2.3 needed to have its tags overwritten but was (still is and will be) unreachable (without asking a losa which is cheating).
It's come up a few times (the emacs folks have asked about it too), so I
would argue it's not unheard of. I don't really see the harm in adding a
option for "bzr push" / "bzr pull" and a command-line option is a lot
more discoverable than a configuration option. This also doesn't really
seem appropriate as a configuration option since you wouldn't really
want to set this option in e.g. branch.conf any more than you would want
to set 'overwrite = True'.

We can't directly push to lp: bzr/2.3 so I don't understand how that is
relevant to the discussion about 'bzr push --overwrite-tags' ?
>
> How about landing this stuff behind a config option and *then* discuss about adding the parameter to have some progress ? That would even allow landing in 2.5 in this case as this will not break compatibility (assuming you update the foreign plugins accordingly).
Doing this will break compatibility no more or less than my patch does,
in that anything that doesn't support it will ignore it and anything
that does support it will.

I don't really want to update the foreign plugins for this now, as it
means rolling three new releases and getting those packaged, etc.

Cheers,

Jelmer

[Vincent Ladeuil (vila)]

> It's come up a few times (the emacs folks have asked about it too), so I
> would argue it's not unheard of. I don't really see the harm in adding a
> option for "bzr push" / "bzr pull" and a command-line option is a lot
> more discoverable than a configuration option.

The fact that it's more exposed means more work associated with removing it
which is my point. Yes, some people need this option, I agree with that or I
wouldn't search for a way to land the associated feature :)

> This also doesn't really
> seem appropriate as a configuration option since you wouldn't really
> want to set this option in e.g. branch.conf any more than you would want
> to set 'overwrite = True'.

I disagree with that, if I maintaining an official branch mirrored to
apublic website I very well want to set this option locally.

>
> We can't directly push to lp: bzr/2.3 so I don't understand how that is
> relevant to the discussion about 'bzr push --overwrite-tags' ?

This is precicely what versioned tags could address :)

> >
> > How about landing this stuff behind a config option and *then* discuss about
> adding the parameter to have some progress ? That would even allow landing in
> 2.5 in this case as this will not break compatibility (assuming you update the
> foreign plugins accordingly).
> Doing this will break compatibility no more or less than my patch does,
> in that anything that doesn't support it will ignore it and anything
> that does support it will.

It won't break the command-line compatibility which is exactly what I'm
arguing against.

[Vincent Ladeuil (vila)]

> > We can't directly push to lp: bzr/2.3 so I don't understand how that is
> > relevant to the discussion about 'bzr push --overwrite-tags' ?
>
> This is precicely what versioned tags could address :)

I.e. since --overwrite-tags does not address what versioned tags should
address I'd rather not add a parameter that only partially address the use
case.

[Jelmer Vernooij (jelmer)]

Am 13/03/12 10:40, schrieb Vincent Ladeuil:
>> It's come up a few times (the emacs folks have asked about it too), so I
>> would argue it's not unheard of. I don't really see the harm in adding a
>> option for "bzr push" / "bzr pull" and a command-line option is a lot
>> more discoverable than a configuration option.
> The fact that it's more exposed means more work associated with removing it
> which is my point. Yes, some people need this option, I agree with that or I
> wouldn't search for a way to land the associated feature :)
I don't think we'll ever want to remove this option if we add it. There
are plenty of formats that don't support versioned tags and some of them
aren't going to go away (e.g. git, 2a, etc).
>
>> This also doesn't really
>> seem appropriate as a configuration option since you wouldn't really
>> want to set this option in e.g. branch.conf any more than you would want
>> to set 'overwrite = True'.
> I disagree with that, if I maintaining an official branch mirrored to
> apublic website I very well want to set this option locally.
By that reasoning "overwrite" should be a configuration option rather
than a command-line option too.... ?
>>> How about landing this stuff behind a config option and *then* discuss about
>> adding the parameter to have some progress ? That would even allow landing in
>> 2.5 in this case as this will not break compatibility (assuming you update the
>> foreign plugins accordingly).
>> Doing this will break compatibility no more or less than my patch does,
>> in that anything that doesn't support it will ignore it and anything
>> that does support it will.
> It won't break the command-line compatibility which is exactly what I'm
> arguing against.
How is adding a new command-line option breaking command-line compatibility?

If you mean that we'll break compatibility if we remove this
command-line option later then I think that a configuration option is no
better. In fact, in the case of a configuration option we'll *silently*
ignore what the user specified, which is probably a lot more confusing.

Cheers,

Jelmer

[Jelmer Vernooij (jelmer)]

Marking as approved, since the same branch is now approved for landing in 2.5.

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2012-03-29 12:27:52 +0000
+++ bzrlib/branch.py	2012-04-16 11:12:22 +0000
@@ -3270,6 +3270,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."""
 
@@ -3440,15 +3449,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
 
@@ -3532,13 +3544,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-04-11 02:22:42 +0000
+++ bzrlib/builtins.py	2012-04-16 11:12:22 +0000
@@ -1153,7 +1153,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'
@@ -1161,7 +1163,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
@@ -1305,6 +1314,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'
@@ -1312,9 +1323,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-02-14 17:22:37 +0000
+++ bzrlib/push.py	2012-04-16 11:12:22 +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-18 14:09:19 +0000
+++ bzrlib/tests/blackbox/test_pull.py	2012-04-16 11:12:22 +0000
@@ -533,6 +533,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	2012-01-18 14:09:19 +0000
+++ bzrlib/tests/blackbox/test_push.py	2012-04-16 11:12:22 +0000
@@ -613,6 +613,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.6.txt'
--- doc/en/release-notes/bzr-2.6.txt	2012-04-16 02:00:46 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-04-16 11:12:22 +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)
+
 Improvements
 ************