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

Proposed by Jelmer Vernooij on 2012-02-02
Status: Rejected
Rejected by: Jelmer Vernooij on 2012-04-16
Proposed branch: lp:~jelmer/bzr/overwrite-tags
Merge into: lp:bzr/2.5
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.5.txt (+3/-0)
To merge this branch: bzr merge lp:~jelmer/bzr/overwrite-tags
Reviewer Review Type Date Requested Status
Jelmer Vernooij (community) Disapprove on 2012-04-16
Vincent Ladeuil 2012-02-02 Approve on 2012-04-13
Review via email: mp+91277@code.launchpad.net

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?

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

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

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

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

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

Any chance of this in 2.5.1 perhaps?

Vincent Ladeuil (vila) wrote :

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

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

Merge bzr.dev.

Jelmer Vernooij (jelmer) wrote :

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

Unmerged revisions

6167. By Jelmer Vernooij on 2012-04-16

Merge bzr.dev.

6166. By Jelmer Vernooij on 2012-02-02

Merge 2.5.

6165. By Jelmer Vernooij on 2012-01-30

Fix conflict resolution.

6164. By Jelmer Vernooij on 2012-01-30

Merge 2.5 branch.

6163. By Jelmer Vernooij on 2011-11-04

Add --overwrite-tags flag.

6162. By Jelmer Vernooij on 2011-09-22

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

6161. By Jelmer Vernooij on 2011-09-22

Add some tests for 'bzr pull --overwrite-tags'.

6160. By Jelmer Vernooij on 2011-09-22

Add some tests for 'bzr push --overwrite-tags'.

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-01-27 14:56:06 +0000
3+++ bzrlib/branch.py 2012-02-02 14:31:53 +0000
4@@ -3346,6 +3346,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@@ -3516,15 +3525,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@@ -3608,13 +3620,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-01-31 15:43:17 +0000
64+++ bzrlib/builtins.py 2012-02-02 14:31:53 +0000
65@@ -1141,7 +1141,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@@ -1149,7 +1151,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@@ -1291,6 +1300,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@@ -1298,9 +1309,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-01-19 17:23:00 +0000
123+++ bzrlib/push.py 2012-02-02 14:31:53 +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-05 17:46:44 +0000
138+++ bzrlib/tests/blackbox/test_pull.py 2012-02-02 14:31:53 +0000
139@@ -527,6 +527,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 2011-12-24 10:10:59 +0000
166+++ bzrlib/tests/blackbox/test_push.py 2012-02-02 14:31:53 +0000
167@@ -605,6 +605,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.5.txt'
192--- doc/en/release-notes/bzr-2.5.txt 2012-02-01 17:41:55 +0000
193+++ doc/en/release-notes/bzr-2.5.txt 2012-02-02 14:31:53 +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 * Support for colocated branches is now available in the default
202 format ("2a"). (Jelmer Vernooij)
203

Subscribers

People subscribed via source and target branches