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

Proposed by Jelmer Vernooij on 2012-02-24
Status: Merged
Approved by: Jelmer Vernooij on 2012-04-16
Approved revision: 6166
Merged at revision: 6522
Proposed branch: lp:~jelmer/bzr/overwrite-tags
Merge into: lp:bzr
Diff against target: 202 lines (+80/-10)
6 files modified
bzrlib/branch.py (+20/-5)
bzrlib/builtins.py (+22/-3)
bzrlib/push.py (+2/-2)
bzrlib/tests/blackbox/test_pull.py (+17/-0)
bzrlib/tests/blackbox/test_push.py (+16/-0)
doc/en/release-notes/bzr-2.6.txt (+3/-0)
To merge this branch: bzr merge lp:~jelmer/bzr/overwrite-tags
Reviewer Review Type Date Requested Status
bzr-core 2012-02-24 Pending
Review via email: mp+94551@code.launchpad.net

Commit message

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

Description of the change

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)

To post a comment you must log in.
Vincent Ladeuil (vila) wrote :

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) wrote :

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) wrote :

> 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) wrote :

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) wrote :

> 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) wrote :

> > 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) wrote :

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) wrote :

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

lp:~jelmer/bzr/overwrite-tags updated on 2012-04-16
6167. By Jelmer Vernooij on 2012-04-16

Merge bzr.dev.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/branch.py'
2--- bzrlib/branch.py 2012-03-29 12:27:52 +0000
3+++ bzrlib/branch.py 2012-04-16 11:12:22 +0000
4@@ -3270,6 +3270,15 @@
5 raise NotImplementedError(self.fetch)
6
7
8+def _fix_overwrite_type(overwrite):
9+ if isinstance(overwrite, bool):
10+ if overwrite:
11+ return ["history", "tags"]
12+ else:
13+ return []
14+ return overwrite
15+
16+
17 class GenericInterBranch(InterBranch):
18 """InterBranch implementation that uses public Branch functions."""
19
20@@ -3440,15 +3449,18 @@
21 result.target_branch = self.target
22 result.old_revno, result.old_revid = self.target.last_revision_info()
23 self.source.update_references(self.target)
24+ overwrite = _fix_overwrite_type(overwrite)
25 if result.old_revid != stop_revision:
26 # We assume that during 'push' this repository is closer than
27 # the target.
28 graph = self.source.repository.get_graph(self.target.repository)
29- self._update_revisions(stop_revision, overwrite=overwrite,
30- graph=graph)
31+ self._update_revisions(stop_revision,
32+ overwrite=("history" in overwrite),
33+ graph=graph)
34 if self.source._push_should_merge_tags():
35 result.tag_updates, result.tag_conflicts = (
36- self.source.tags.merge_to(self.target.tags, overwrite))
37+ self.source.tags.merge_to(
38+ self.target.tags, "tags" in overwrite))
39 result.new_revno, result.new_revid = self.target.last_revision_info()
40 return result
41
42@@ -3532,13 +3544,16 @@
43 # -- JRV20090506
44 result.old_revno, result.old_revid = \
45 self.target.last_revision_info()
46- self._update_revisions(stop_revision, overwrite=overwrite,
47+ overwrite = _fix_overwrite_type(overwrite)
48+ self._update_revisions(stop_revision,
49+ overwrite=("history" in overwrite),
50 graph=graph)
51 # TODO: The old revid should be specified when merging tags,
52 # so a tags implementation that versions tags can only
53 # pull in the most recent changes. -- JRV20090506
54 result.tag_updates, result.tag_conflicts = (
55- self.source.tags.merge_to(self.target.tags, overwrite,
56+ self.source.tags.merge_to(self.target.tags,
57+ "tags" in overwrite,
58 ignore_master=not merge_tags_to_master))
59 result.new_revno, result.new_revid = self.target.last_revision_info()
60 if _hook_master:
61
62=== modified file 'bzrlib/builtins.py'
63--- bzrlib/builtins.py 2012-04-11 02:22:42 +0000
64+++ bzrlib/builtins.py 2012-04-16 11:12:22 +0000
65@@ -1153,7 +1153,9 @@
66 "the master branch."
67 ),
68 Option('show-base',
69- help="Show base revision text in conflicts.")
70+ help="Show base revision text in conflicts."),
71+ Option('overwrite-tags',
72+ help="Overwrite tags only."),
73 ]
74 takes_args = ['location?']
75 encoding_type = 'replace'
76@@ -1161,7 +1163,14 @@
77 def run(self, location=None, remember=None, overwrite=False,
78 revision=None, verbose=False,
79 directory=None, local=False,
80- show_base=False):
81+ show_base=False, overwrite_tags=False):
82+
83+ if overwrite:
84+ overwrite = ["history", "tags"]
85+ elif overwrite_tags:
86+ overwrite = ["tags"]
87+ else:
88+ overwrite = []
89 # FIXME: too much stuff is in the command class
90 revision_id = None
91 mergeable = None
92@@ -1305,6 +1314,8 @@
93 Option('no-tree',
94 help="Don't populate the working tree, even for protocols"
95 " that support it."),
96+ Option('overwrite-tags',
97+ help="Overwrite tags only."),
98 ]
99 takes_args = ['location?']
100 encoding_type = 'replace'
101@@ -1312,9 +1323,17 @@
102 def run(self, location=None, remember=None, overwrite=False,
103 create_prefix=False, verbose=False, revision=None,
104 use_existing_dir=False, directory=None, stacked_on=None,
105- stacked=False, strict=None, no_tree=False):
106+ stacked=False, strict=None, no_tree=False,
107+ overwrite_tags=False):
108 from bzrlib.push import _show_push_branch
109
110+ if overwrite:
111+ overwrite = ["history", "tags"]
112+ elif overwrite_tags:
113+ overwrite = ["tags"]
114+ else:
115+ overwrite = []
116+
117 if directory is None:
118 directory = '.'
119 # Get the source branch
120
121=== modified file 'bzrlib/push.py'
122--- bzrlib/push.py 2012-02-14 17:22:37 +0000
123+++ bzrlib/push.py 2012-04-16 11:12:22 +0000
124@@ -68,8 +68,8 @@
125 :param location: the url of the destination
126 :param to_file: the output stream
127 :param verbose: if True, display more output than normal
128- :param overwrite: if False, a current branch at the destination may not
129- have diverged from the source, otherwise the push fails
130+ :param overwrite: list of things to overwrite ("history", "tags")
131+ or boolean indicating for everything
132 :param remember: if True, store the location as the push location for
133 the source branch
134 :param stacked_on: the url of the branch, if any, to stack on;
135
136=== modified file 'bzrlib/tests/blackbox/test_pull.py'
137--- bzrlib/tests/blackbox/test_pull.py 2012-01-18 14:09:19 +0000
138+++ bzrlib/tests/blackbox/test_pull.py 2012-04-16 11:12:22 +0000
139@@ -533,6 +533,23 @@
140 self.assertEqual(out,
141 ('1 tag(s) updated.\n', ''))
142
143+ def test_overwrite_tags(self):
144+ """--overwrite-tags only overwrites tags, not revisions."""
145+ from_tree = self.make_branch_and_tree('from')
146+ from_tree.branch.tags.set_tag("mytag", "somerevid")
147+ to_tree = self.make_branch_and_tree('to')
148+ to_tree.branch.tags.set_tag("mytag", "anotherrevid")
149+ revid1 = to_tree.commit('my commit')
150+ out = self.run_bzr(['pull', '-d', 'to', 'from'], retcode=1)
151+ self.assertEquals(out,
152+ ('No revisions to pull.\nConflicting tags:\n mytag\n', ''))
153+ out = self.run_bzr(['pull', '-d', 'to', '--overwrite-tags', 'from'])
154+ self.assertEquals(out, ('1 tag(s) updated.\n', ''))
155+
156+ self.assertEquals(to_tree.branch.tags.lookup_tag('mytag'),
157+ 'somerevid')
158+ self.assertEquals(to_tree.branch.last_revision(), revid1)
159+
160 def test_pull_tag_overwrite(self):
161 """pulling tags with --overwrite only reports changed tags."""
162 # create a branch, see that --show-base fails
163
164=== modified file 'bzrlib/tests/blackbox/test_push.py'
165--- bzrlib/tests/blackbox/test_push.py 2012-01-18 14:09:19 +0000
166+++ bzrlib/tests/blackbox/test_push.py 2012-04-16 11:12:22 +0000
167@@ -613,6 +613,22 @@
168 self.assertEqual('', out)
169 self.assertEqual('Created new branch.\n', err)
170
171+ def test_overwrite_tags(self):
172+ """--overwrite-tags only overwrites tags, not revisions."""
173+ from_tree = self.make_branch_and_tree('from')
174+ from_tree.branch.tags.set_tag("mytag", "somerevid")
175+ to_tree = self.make_branch_and_tree('to')
176+ to_tree.branch.tags.set_tag("mytag", "anotherrevid")
177+ revid1 = to_tree.commit('my commit')
178+ out = self.run_bzr(['push', '-d', 'from', 'to'])
179+ self.assertEquals(out,
180+ ('Conflicting tags:\n mytag\n', 'No new revisions to push.\n'))
181+ out = self.run_bzr(['push', '-d', 'from', '--overwrite-tags', 'to'])
182+ self.assertEquals(out, ('', '1 tag updated.\n'))
183+ self.assertEquals(to_tree.branch.tags.lookup_tag('mytag'),
184+ 'somerevid')
185+ self.assertEquals(to_tree.branch.last_revision(), revid1)
186+
187
188 class RedirectingMemoryTransport(memory.MemoryTransport):
189
190
191=== modified file 'doc/en/release-notes/bzr-2.6.txt'
192--- doc/en/release-notes/bzr-2.6.txt 2012-04-16 02:00:46 +0000
193+++ doc/en/release-notes/bzr-2.6.txt 2012-04-16 11:12:22 +0000
194@@ -20,6 +20,9 @@
195
196 .. New commands, options, etc that users may wish to try out.
197
198+* New option ``--overwrite-tags`` for ``bzr pull`` and ``bzr push``.
199+ (Jelmer Vernooij, #681792)
200+
201 Improvements
202 ************
203