Merge lp:~jelmer/bzr/move-interbranch-fetch2 into lp:bzr

Proposed by Jelmer Vernooij on 2011-04-27
Status: Merged
Approved by: Jelmer Vernooij on 2011-04-28
Approved revision: 5753
Merged at revision: 5810
Proposed branch: lp:~jelmer/bzr/move-interbranch-fetch2
Merge into: lp:bzr
Prerequisite: lp:~jelmer/bzr/move-interbranch-fetch1
Diff against target: 223 lines (+22/-61)
5 files modified
bzrlib/branch.py (+18/-45)
bzrlib/merge.py (+1/-8)
bzrlib/tests/test_revisionspec.py (+2/-4)
bzrlib/workingtree_4.py (+1/-1)
doc/en/release-notes/bzr-2.4.txt (+0/-3)
To merge this branch: bzr merge lp:~jelmer/bzr/move-interbranch-fetch2
Reviewer Review Type Date Requested Status
John A Meinel 2011-04-27 Pending
Andrew Bennetts 2011-04-27 Pending
Review via email: mp+59204@code.launchpad.net

This proposal supersedes a proposal from 2011-04-20.

Commit message

Make Branch.fetch() no longer take a fetch_spec

Description of the change

Make Branch.fetch() no longer take a fetch_spec. This makes it possible for Merger._maybe_fetch() to fetch the branch revisions without specifying a fetch_spec (which doesn't work with remote foreign repositories that don't allow direct graph access).

Since the fetch_spec argument was added in 2.4, I don't think there is a need to deprecate it first.

To post a comment you must log in.
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew-
Can you look at this one? I'm not sure that I prefer this method. I
think it is fine to change how we search for items, but I felt that
FetchSpec was the more generic interface.
John
=:->

- -------- Original Message --------
Subject: [Merge] lp:~jelmer/bzr/move-interbranch-fetch2 into lp:bzr
Date: Sat, 26 Mar 2011 15:18:05 -0000
From: Jelmer Vernooij <email address hidden>
Reply-To: <email address hidden>
To: <email address hidden>, Andrew Bennetts
<email address hidden>

Jelmer Vernooij has proposed merging
lp:~jelmer/bzr/move-interbranch-fetch2 into lp:bzr with
lp:~jelmer/bzr/move-interbranch-fetch1 as a prerequisite.

Requested reviews:
  bzr-core (bzr-core)
  Andrew Bennetts (spiv): code
Related bugs:
  Bug #741760 in Bazaar: ""bzr merge" always looks at revision graph"
  https://bugs.launchpad.net/bzr/+bug/741760

For more details, see:
https://code.launchpad.net/~jelmer/bzr/move-interbranch-fetch2/+merge/54958

Make Branch.fetch() take a fetch_tags argument rather than fetch_spec.
This makes it possible for Merger._maybe_fetch() to fetch the branch
revisions without specifying a fetch_spec (which doesn't work with
remote foreign repositories that don't allow direct graph access).

Since the fetch_spec argument was added in 2.4, I don't think there is a
need to deprecate it first.
- --
https://code.launchpad.net/~jelmer/bzr/move-interbranch-fetch2/+merge/54958
Your team bzr-core is requested to review the proposed merge of
lp:~jelmer/bzr/move-interbranch-fetch2 into lp:bzr.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2YkekACgkQJdeBCYSNAAPDPwCdGs5DDx1uCCMyP5UcOGdbdE7W
wTkAoJukPhEgwObgCNBt3tP5S5vGYIeV
=rRiH
-----END PGP SIGNATURE-----

Jelmer Vernooij (jelmer) wrote : Posted in a previous version of this proposal

On Sun, 2011-04-03 at 15:31 +0000, John A Meinel wrote:
> Andrew-
> Can you look at this one? I'm not sure that I prefer this method. I
> think it is fine to change how we search for items, but I felt that
> FetchSpec was the more generic interface.
FWIW The generic interface is still usable, it just requires you to call
fetch on the repository rather than on the branch.

Cheers,

Jelmer

> - -------- Original Message --------
> Subject: [Merge] lp:~jelmer/bzr/move-interbranch-fetch2 into lp:bzr
> Date: Sat, 26 Mar 2011 15:18:05 -0000
> From: Jelmer Vernooij <email address hidden>
> Reply-To: <email address hidden>
> To: <email address hidden>, Andrew Bennetts
> <email address hidden>
>
> Jelmer Vernooij has proposed merging
> lp:~jelmer/bzr/move-interbranch-fetch2 into lp:bzr with
> lp:~jelmer/bzr/move-interbranch-fetch1 as a prerequisite.
>
> Requested reviews:
> bzr-core (bzr-core)
> Andrew Bennetts (spiv): code
> Related bugs:
> Bug #741760 in Bazaar: ""bzr merge" always looks at revision graph"
> https://bugs.launchpad.net/bzr/+bug/741760
>
> For more details, see:
> https://code.launchpad.net/~jelmer/bzr/move-interbranch-fetch2/+merge/54958
>
> Make Branch.fetch() take a fetch_tags argument rather than fetch_spec.
> This makes it possible for Merger._maybe_fetch() to fetch the branch
> revisions without specifying a fetch_spec (which doesn't work with
> remote foreign repositories that don't allow direct graph access).
>
> Since the fetch_spec argument was added in 2.4, I don't think there is a
> need to deprecate it first.
> - --
> https://code.launchpad.net/~jelmer/bzr/move-interbranch-fetch2/+merge/54958
> Your team bzr-core is requested to review the proposed merge of
> lp:~jelmer/bzr/move-interbranch-fetch2 into lp:bzr.
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (Cygwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAk2YkekACgkQJdeBCYSNAAPDPwCdGs5DDx1uCCMyP5UcOGdbdE7W
> wTkAoJukPhEgwObgCNBt3tP5S5vGYIeV
> =rRiH
> -----END PGP SIGNATURE-----
>

Andrew Bennetts (spiv) wrote : Posted in a previous version of this proposal

Special-casing tags over other revisions that may be related to a branch (like loom threads) feels weird to me.

So looking at this I think my initial impression on IRC is right: the core problem is that the purpose of Branch.fetch/InterBranch.fetch isn't clear.

Most of bzrlib itself uses Repository.fetch. Branch.fetch in pure bzrlib (without your patch) is basically a convenience method for “do a repo fetch of the source branch's tip”. That raises two obvious questions for me: 1) why bother accepting stop_revision or fetch_spec at all (if you need an unusual fetch, just use the repo), and 2) why doesn't it fetch tags if its purpose is to be a convenient and sensible default fetch for branches.

[For 2) the immediate answer is that during development of my fetch-all-tags fixes that Branch.fetch probably never showed up as relevant to making tagged revisions be fetched during 'bzr branch/pull/update/merge', which is a pretty strong indication it is not used much.]

So given that, I'd be tempted to delete Branch.fetch entirely! But I'm guessing it may be useful for foreign branch support? I can see at a glance that e.g. bzr-svn implements its own InterBranch.fetch etc, but I can't tell from a glance if that's functionality that can be easily implemented elsewhere (e.g. heads_to_fetch) or not. What do you think?

If deleting Branch.fetch isn't an (easy) option, what about going the other direction to this patch, and removing its arguments entirely? So in bzrlib it would become:

"""
class GenericInterBranch(InterBranch):
    # …
    def fetch(self):
        must_have_revs, optional_revs = self.source.heads_to_fetch()
        return self.target.repository.fetch(fetch_spec=NotInOtherForHeads(self.target.repository, self.source.repository, must_have_revs, optional_revs)
"""

What do you think?

Jelmer Vernooij (jelmer) wrote : Posted in a previous version of this proposal

> Special-casing tags over other revisions that may be related to a branch (like
> loom threads) feels weird to me.
>
> So looking at this I think my initial impression on IRC is right: the core
> problem is that the purpose of Branch.fetch/InterBranch.fetch isn't clear.
>
> Most of bzrlib itself uses Repository.fetch. Branch.fetch in pure bzrlib
> (without your patch) is basically a convenience method for “do a repo fetch of
> the source branch's tip”. That raises two obvious questions for me: 1) why
> bother accepting stop_revision or fetch_spec at all (if you need an unusual
> fetch, just use the repo), and 2) why doesn't it fetch tags if its purpose is
> to be a convenient and sensible default fetch for branches.
>
> [For 2) the immediate answer is that during development of my fetch-all-tags
> fixes that Branch.fetch probably never showed up as relevant to making tagged
> revisions be fetched during 'bzr branch/pull/update/merge', which is a pretty
> strong indication it is not used much.]
>
> So given that, I'd be tempted to delete Branch.fetch entirely! But I'm
> guessing it may be useful for foreign branch support? I can see at a glance
> that e.g. bzr-svn implements its own InterBranch.fetch etc, but I can't tell
> from a glance if that's functionality that can be easily implemented elsewhere
> (e.g. heads_to_fetch) or not. What do you think?
Branch.fetch() is very useful for foreign branches, as it makes it possible to determine the tags to fetch and fetching them in one go. heads_to_fetch() requires opening a connection to discover what tags to fetch, then later opening a new connection when actually fetching them.

> If deleting Branch.fetch isn't an (easy) option, what about going the other
> direction to this patch, and removing its arguments entirely? So in bzrlib it
> would become:
>
> """
> class GenericInterBranch(InterBranch):
> # …
> def fetch(self):
> must_have_revs, optional_revs = self.source.heads_to_fetch()
> return self.target.repository.fetch(fetch_spec=NotInOtherForHeads(self
> .target.repository, self.source.repository, must_have_revs, optional_revs)
> """
>
> What do you think?
Removing fetch_tags might make sense (it just seems to be set to true everywhere). Removing stop_revision would make it impossible to use Branch.fetch from e.g. Merger._maybe_fetch(), which specifies a revision id.

Your proposed GenericInterBranch.fetch() implementation seems reasonable, though perhaps we could add a stop_revision there (and in heads_to_fetch()) too?

Cheers,

Jelmer

Andrew Bennetts (spiv) wrote : Posted in a previous version of this proposal

Jelmer Vernooij wrote:
[…]
> > So given that, I'd be tempted to delete Branch.fetch entirely! But I'm
> > guessing it may be useful for foreign branch support? I can see at a glance
> > that e.g. bzr-svn implements its own InterBranch.fetch etc, but I can't tell
> > from a glance if that's functionality that can be easily implemented elsewhere
> > (e.g. heads_to_fetch) or not. What do you think?
> Branch.fetch() is very useful for foreign branches, as it makes it possible to
> determine the tags to fetch and fetching them in one go. heads_to_fetch()
> requires opening a connection to discover what tags to fetch, then later
> opening a new connection when actually fetching them.

Ah. That's a shame. Ok I guess we keep it then.

[…]
> Removing fetch_tags might make sense (it just seems to be set to true
> everywhere). Removing stop_revision would make it impossible to use
> Branch.fetch from e.g. Merger._maybe_fetch(), which specifies a revision id.
>
> Your proposed GenericInterBranch.fetch() implementation seems reasonable,
> though perhaps we could add a stop_revision there (and in heads_to_fetch())
> too?

Ok, that sounds like a good plan to me.

Jelmer Vernooij (jelmer) wrote : Posted in a previous version of this proposal

Updated to pass the stop_revision and include_tags on to Branch.heads_to_fetch.

I had to leave include_tags in as InterBranch.update_revisions() has a fetch_tags argument.

Does this seem more reasonable?

Andrew Bennetts (spiv) wrote : Posted in a previous version of this proposal

I'm confused: this patch still has fetch_tags on Branch.fetch and InterBranch.fetch. Wasn't the revised plan that these would only take stop_revision and no other args?

I'm not very comfortable with heads_to_fetch taking include_tags, although it's less troubling than having it on fetch. I'm ok with it as an intermediate step to making things work if that's what's needed, but I don't think it's the long term answer.

It makes me wonder if heads_to_fetch should actually take no args but return something more structured, with room for optional extra fields, e.g.: something like a two-tuple of (heads_dict, type_advice), which looks like:

  heads_dict: {'tip': [revid], 'tags': [revid, revid, …]}
  type_advice: {'tip': ['must fetch'], 'tags': ['optional', 'ghost ok']}

(Probably an actual formal object would be better, but you get the idea)

Foreign implementations could add new things into the data returned, e.g. 'loom threads', and advise whether they are must-fetch or not, etc. This would also resolve the XXX about fetching a different stop_revision than tip without potentially removing an important revid (if it's the tip and a loom thread, then replacing the 'tip' value won't remove it from the heads-to-fetch info entirely).

Your change to the heads_to_fetch HPSS code isn't sufficiently tested. I don't think we allow None as an RPC argument. And there should at least be tests on the client and server what happens when a stop_revision is and isn't specified (and ditto include_tags)

review: Needs Fixing
Jelmer Vernooij (jelmer) wrote : Posted in a previous version of this proposal

Hi spiv,

On Wed, 2011-04-06 at 05:51 +0000, Andrew Bennetts wrote:
> Review: Needs Fixing

> I'm confused: this patch still has fetch_tags on
> Branch.fetch and InterBranch.fetch. Wasn't the revised plan that these
> would only take stop_revision and no other args?
See my comment earlier: Branch.update_revisions takes a fetch_tags
argument, hence we still needed fetch_tags/include_tags as an argument.
Alternatively, we could consider dropping that argument to
update_revisions too, I'm not sure where it's used.

> I'm not very comfortable with heads_to_fetch taking include_tags,
> although it's less troubling than having it on fetch. I'm ok with it
> as an intermediate step to making things work if that's what's needed,
> but I don't think it's the long term answer.
>
> It makes me wonder if heads_to_fetch should actually take no args but
> return something more structured, with room for optional extra fields,
> e.g.: something like a two-tuple of (heads_dict, type_advice), which
> looks like:
>
> heads_dict: {'tip': [revid], 'tags': [revid, revid, …]}
> type_advice: {'tip': ['must fetch'], 'tags': ['optional', 'ghost ok']}
>
> (Probably an actual formal object would be better, but you get the idea)

> Foreign implementations could add new things into the data returned,
> e.g. 'loom threads', and advise whether they are must-fetch or not,
> etc.
That would give us more flexibility, but I'm not sure what that would
actually gain us. It means the caller has to be aware of the kinds of
things that are being returned, and it makes the API harder to use (as
you'd need to do post-processing on the return value). The separation
between just "always fetch" and "fetch if present" seems sufficient to
me.

> This would also resolve the XXX about fetching a different
> stop_revision than tip without potentially removing an important revid
> (if it's the tip and a loom thread, then replacing the 'tip' value
> won't remove it from the heads-to-fetch info entirely).
This discarding (if you mean the discarding in make_fetch_spec) no
longer happens now that stop_revision is passed into
Branch.heads_to_fetch.

Cheers,

Jelmer

John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...
>> (Probably an actual formal object would be better, but you get the idea)
>
>> Foreign implementations could add new things into the data returned,
>> e.g. 'loom threads', and advise whether they are must-fetch or not,
>> etc.
> That would give us more flexibility, but I'm not sure what that would
> actually gain us. It means the caller has to be aware of the kinds of
> things that are being returned, and it makes the API harder to use (as
> you'd need to do post-processing on the return value). The separation
> between just "always fetch" and "fetch if present" seems sufficient to
> me.

We also need a "fetch into a stacked branch if present in master
branch". We *might* just make that 'fetch if present'.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2cP8kACgkQJdeBCYSNAANy+gCdECaVBBa2dWP19irl1+ua3fmc
fPMAn1Izq/Zlqwpovb6bOwMOXBQqZYp7
=fLp+
-----END PGP SIGNATURE-----

Andrew Bennetts (spiv) wrote : Posted in a previous version of this proposal

Jelmer Vernooij wrote:
> Hi spiv,
>
> On Wed, 2011-04-06 at 05:51 +0000, Andrew Bennetts wrote:
> > Review: Needs Fixing
>
> > I'm confused: this patch still has fetch_tags on
> > Branch.fetch and InterBranch.fetch. Wasn't the revised plan that these
> > would only take stop_revision and no other args?
> See my comment earlier: Branch.update_revisions takes a fetch_tags
> argument, hence we still needed fetch_tags/include_tags as an argument.
> Alternatively, we could consider dropping that argument to
> update_revisions too, I'm not sure where it's used.

It's used in exactly 2 places, I think: BzrBranch._basic_push and
GenericInterBranch._pull, and I don't think it's really possible to remove
stop_revision from those.

I don't think that matters though, because update_revisions could simply
use self.target.repository.fetch instead of self.target.fetch. By the
time it invokes fetch it's already determined what it wants to fetch and
can just deal with repository directly.

[…]
> > It makes me wonder if heads_to_fetch should actually take no args but
> > return something more structured, with room for optional extra fields,
> > e.g.: something like a two-tuple of (heads_dict, type_advice), which
> > looks like:
> >
> > heads_dict: {'tip': [revid], 'tags': [revid, revid, …]}
> > type_advice: {'tip': ['must fetch'], 'tags': ['optional', 'ghost ok']}
> >
> > (Probably an actual formal object would be better, but you get the idea)
>
> > Foreign implementations could add new things into the data returned,
> > e.g. 'loom threads', and advise whether they are must-fetch or not,
> > etc.
> That would give us more flexibility, but I'm not sure what that would
> actually gain us. It means the caller has to be aware of the kinds of
> things that are being returned, and it makes the API harder to use (as
> you'd need to do post-processing on the return value). The separation
> between just "always fetch" and "fetch if present" seems sufficient to
> me.

Well, if we returned an object rather than just dicts the object could
have methods to take care of the post-processing.

> > This would also resolve the XXX about fetching a different
> > stop_revision than tip without potentially removing an important revid
> > (if it's the tip and a loom thread, then replacing the 'tip' value
> > won't remove it from the heads-to-fetch info entirely).
> This discarding (if you mean the discarding in make_fetch_spec) no
> longer happens now that stop_revision is passed into
> Branch.heads_to_fetch.

Yes, that's the discarding I mean. I realise that passing stop_revision
to heads_to_fetch avoids that problem, but I don't much like doing that
:)

Martin Pool (mbp) wrote : Posted in a previous version of this proposal

Based on spiv's last comment, I'm going to set this back to wip, just to get it off the queue. Please push it back whenever you want more review or help.

John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

I don't think we want to change 'heads_to_fetch' at this point. We default to the local operation for bzr branches anyway, so I'm not sure of the benefit of adding an RPC that defaults to not being used. However, I don't think changing it benefits us much, and it will be incompatible with people running bzr-2.4b1 (I would be okay with that if we had an actual benefit).

I think moving the FetchSpec from being in update_revisions to being in InterBranch.fetch() simplifies a lot of code paths, so I think it is a good way to go.
I don't think we're at "Ideal" here, but we are at "better than existing".

review: Needs Fixing
Jelmer Vernooij (jelmer) wrote : Posted in a previous version of this proposal

Updated to back out the changes to heads_to_fetch as discussed with jam.

John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

The NEWS comment is wrong, because I think you got rid of fetch_tags. anyway, the rest looks good.

review: Approve
Jelmer Vernooij (jelmer) wrote :

sent to pqm by email

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 2011-04-20 19:32:00 +0000
3+++ bzrlib/branch.py 2011-04-27 11:26:57 +0000
4@@ -669,20 +669,15 @@
5 raise errors.UnsupportedOperation(self.get_reference_info, self)
6
7 @needs_write_lock
8- def fetch(self, from_branch, last_revision=None, fetch_spec=None):
9+ def fetch(self, from_branch, last_revision=None):
10 """Copy revisions from from_branch into this branch.
11
12 :param from_branch: Where to copy from.
13 :param last_revision: What revision to stop at (None for at the end
14 of the branch.
15- :param fetch_spec: If specified, a SearchResult or
16- PendingAncestryResult that describes which revisions to copy. This
17- allows copying multiple heads at once. Mutually exclusive with
18- last_revision.
19 :return: None
20 """
21- return InterBranch.get(from_branch, self).fetch(last_revision,
22- fetch_spec)
23+ return InterBranch.get(from_branch, self).fetch(last_revision)
24
25 def get_bound_location(self):
26 """Return the URL of the branch we are bound to.
27@@ -1000,7 +995,7 @@
28 return (0, _mod_revision.NULL_REVISION)
29
30 def update_revisions(self, other, stop_revision=None, overwrite=False,
31- graph=None, fetch_tags=True):
32+ graph=None):
33 """Pull in new perfect-fit revisions.
34
35 :param other: Another Branch to pull from
36@@ -1009,12 +1004,10 @@
37 to see if it is a proper descendant.
38 :param graph: A Graph object that can be used to query history
39 information. This can be None.
40- :param fetch_tags: Flag that specifies if tags from other should be
41- fetched too.
42 :return: None
43 """
44 return InterBranch.get(other, self).update_revisions(stop_revision,
45- overwrite, graph, fetch_tags=fetch_tags)
46+ overwrite, graph)
47
48 @deprecated_method(deprecated_in((2, 4, 0)))
49 def import_last_revision_info(self, source_repo, revno, revid):
50@@ -1045,14 +1038,7 @@
51 (should only be different from the arguments when lossy=True)
52 """
53 if not self.repository.has_same_location(source.repository):
54- try:
55- tags_to_fetch = set(source.tags.get_reverse_tag_dict())
56- except errors.TagsNotSupported:
57- tags_to_fetch = set()
58- fetch_spec = _mod_graph.NotInOtherForRevs(self.repository,
59- source.repository, [revid],
60- if_present_ids=tags_to_fetch).execute()
61- self.repository.fetch(source.repository, fetch_spec=fetch_spec)
62+ self.fetch(source, revid)
63 self.set_last_revision_info(revno, revid)
64 return (revno, revid)
65
66@@ -3302,7 +3288,7 @@
67
68 @needs_write_lock
69 def update_revisions(self, stop_revision=None, overwrite=False,
70- graph=None, fetch_tags=True):
71+ graph=None):
72 """Pull in new perfect-fit revisions.
73
74 :param stop_revision: Updated until the given revision
75@@ -3310,8 +3296,6 @@
76 to see if it is a proper descendant.
77 :param graph: A Graph object that can be used to query history
78 information. This can be None.
79- :param fetch_tags: Flag that specifies if tags from source should be
80- fetched too.
81 :return: None
82 """
83 raise NotImplementedError(self.update_revisions)
84@@ -3335,11 +3319,10 @@
85 raise NotImplementedError(self.copy_content_into)
86
87 @needs_write_lock
88- def fetch(self, stop_revision=None, fetch_spec=None):
89+ def fetch(self, stop_revision=None):
90 """Fetch revisions.
91
92 :param stop_revision: Last revision to fetch
93- :param fetch_spec: Fetch spec.
94 """
95 raise NotImplementedError(self.fetch)
96
97@@ -3383,25 +3366,26 @@
98 self.source.tags.merge_to(self.target.tags)
99
100 @needs_write_lock
101- def fetch(self, stop_revision=None, fetch_spec=None):
102- if fetch_spec is not None and stop_revision is not None:
103- raise AssertionError(
104- "fetch_spec and last_revision are mutually exclusive.")
105+ def fetch(self, stop_revision=None):
106 if self.target.base == self.source.base:
107 return (0, [])
108 self.source.lock_read()
109 try:
110- if stop_revision is None and fetch_spec is None:
111- stop_revision = self.source.last_revision()
112- stop_revision = _mod_revision.ensure_null(stop_revision)
113+ fetch_spec_factory = fetch.FetchSpecFactory()
114+ fetch_spec_factory.source_branch = self.source
115+ fetch_spec_factory.source_branch_stop_revision_id = stop_revision
116+ fetch_spec_factory.source_repo = self.source.repository
117+ fetch_spec_factory.target_repo = self.target.repository
118+ fetch_spec_factory.target_repo_kind = fetch.TargetRepoKinds.PREEXISTING
119+ fetch_spec = fetch_spec_factory.make_fetch_spec()
120 return self.target.repository.fetch(self.source.repository,
121- revision_id=stop_revision, fetch_spec=fetch_spec)
122+ fetch_spec=fetch_spec)
123 finally:
124 self.source.unlock()
125
126 @needs_write_lock
127 def update_revisions(self, stop_revision=None, overwrite=False,
128- graph=None, fetch_tags=True):
129+ graph=None):
130 """See InterBranch.update_revisions()."""
131 other_revno, other_last_revision = self.source.last_revision_info()
132 stop_revno = None # unknown
133@@ -3419,18 +3403,7 @@
134 # case of having something to pull, and so that the check for
135 # already merged can operate on the just fetched graph, which will
136 # be cached in memory.
137- if fetch_tags:
138- fetch_spec_factory = fetch.FetchSpecFactory()
139- fetch_spec_factory.source_branch = self.source
140- fetch_spec_factory.source_branch_stop_revision_id = stop_revision
141- fetch_spec_factory.source_repo = self.source.repository
142- fetch_spec_factory.target_repo = self.target.repository
143- fetch_spec_factory.target_repo_kind = fetch.TargetRepoKinds.PREEXISTING
144- fetch_spec = fetch_spec_factory.make_fetch_spec()
145- else:
146- fetch_spec = _mod_graph.NotInOtherForRevs(self.target.repository,
147- self.source.repository, revision_ids=[stop_revision]).execute()
148- self.target.fetch(self.source, fetch_spec=fetch_spec)
149+ self.fetch(stop_revision=stop_revision)
150 # Check to see if one is an ancestor of the other
151 if not overwrite:
152 if graph is None:
153
154=== modified file 'bzrlib/merge.py'
155--- bzrlib/merge.py 2011-04-17 23:06:22 +0000
156+++ bzrlib/merge.py 2011-04-27 11:26:57 +0000
157@@ -563,14 +563,7 @@
158
159 def _maybe_fetch(self, source, target, revision_id):
160 if not source.repository.has_same_location(target.repository):
161- try:
162- tags_to_fetch = set(source.tags.get_reverse_tag_dict())
163- except errors.TagsNotSupported:
164- tags_to_fetch = None
165- fetch_spec = _mod_graph.NotInOtherForRevs(target.repository,
166- source.repository, [revision_id],
167- if_present_ids=tags_to_fetch).execute()
168- target.fetch(source, fetch_spec=fetch_spec)
169+ target.fetch(source, revision_id)
170
171 def find_base(self):
172 revisions = [_mod_revision.ensure_null(self.this_basis),
173
174=== modified file 'bzrlib/tests/test_revisionspec.py'
175--- bzrlib/tests/test_revisionspec.py 2011-04-14 07:53:38 +0000
176+++ bzrlib/tests/test_revisionspec.py 2011-04-27 11:26:57 +0000
177@@ -397,8 +397,7 @@
178 """We can get any revision id in the repository"""
179 # XXX: This may change in the future, but for now, it is true
180 self.tree2.commit('alt third', rev_id='alt_r3')
181- self.tree.branch.repository.fetch(self.tree2.branch.repository,
182- revision_id='alt_r3')
183+ self.tree.branch.fetch(self.tree2.branch, 'alt_r3')
184 self.assertInHistoryIs(None, 'alt_r3', 'revid:alt_r3')
185
186 def test_unicode(self):
187@@ -475,8 +474,7 @@
188 def test_alt_no_parents(self):
189 new_tree = self.make_branch_and_tree('new_tree')
190 new_tree.commit('first', rev_id='new_r1')
191- self.tree.branch.repository.fetch(new_tree.branch.repository,
192- revision_id='new_r1')
193+ self.tree.branch.fetch(new_tree.branch, 'new_r1')
194 self.assertInHistoryIs(0, 'null:', 'before:revid:new_r1')
195
196 def test_as_revision_id(self):
197
198=== modified file 'bzrlib/workingtree_4.py'
199--- bzrlib/workingtree_4.py 2011-04-17 23:06:22 +0000
200+++ bzrlib/workingtree_4.py 2011-04-27 11:26:57 +0000
201@@ -2016,7 +2016,7 @@
202 def make_source_parent_tree(source, target):
203 """Change the source tree into a parent of the target."""
204 revid = source.commit('record tree')
205- target.branch.repository.fetch(source.branch.repository, revid)
206+ target.branch.fetch(source.branch, revid)
207 target.set_parent_ids([revid])
208 return target.basis_tree(), target
209
210
211=== modified file 'doc/en/release-notes/bzr-2.4.txt'
212--- doc/en/release-notes/bzr-2.4.txt 2011-04-21 13:22:08 +0000
213+++ doc/en/release-notes/bzr-2.4.txt 2011-04-27 11:26:57 +0000
214@@ -365,9 +365,6 @@
215 ``RepositoryFormat.supports_leaving_lock`` flags have been added.
216 (Jelmer Vernooij)
217
218-* ``Branch.fetch`` implementations must now accept an optional
219- ``fetch_spec`` keyword argument. (Andrew Bennetts)
220-
221 * ``Branch.import_last_revision_info`` is deprecated. Use the
222 ``import_last_revision_info_and_tags`` method instead.
223 (Andrew Bennetts)