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

[Jelmer Vernooij]

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.

[John A Meinel (jameinel)]

-----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)]

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)]

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)]

> 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)]

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)]

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)]

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)]

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)]

-----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)]

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)]

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.

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2011-04-19 13:56:32 +0000
+++ bzrlib/branch.py	2011-04-20 19:55:54 +0000
@@ -669,20 +669,15 @@
         raise errors.UnsupportedOperation(self.get_reference_info, self)
 
     @needs_write_lock
-    def fetch(self, from_branch, last_revision=None, fetch_spec=None):
+    def fetch(self, from_branch, last_revision=None):
         """Copy revisions from from_branch into this branch.
 
         :param from_branch: Where to copy from.
         :param last_revision: What revision to stop at (None for at the end
                               of the branch.
-        :param fetch_spec: If specified, a SearchResult or
-            PendingAncestryResult that describes which revisions to copy.  This
-            allows copying multiple heads at once.  Mutually exclusive with
-            last_revision.
         :return: None
         """
-        return InterBranch.get(from_branch, self).fetch(last_revision,
-            fetch_spec)
+        return InterBranch.get(from_branch, self).fetch(last_revision)
 
     def get_bound_location(self):
         """Return the URL of the branch we are bound to.
@@ -1000,7 +995,7 @@
             return (0, _mod_revision.NULL_REVISION)
 
     def update_revisions(self, other, stop_revision=None, overwrite=False,
-                         graph=None, fetch_tags=True):
+            graph=None):
         """Pull in new perfect-fit revisions.
 
         :param other: Another Branch to pull from
@@ -1009,12 +1004,10 @@
             to see if it is a proper descendant.
         :param graph: A Graph object that can be used to query history
             information. This can be None.
-        :param fetch_tags: Flag that specifies if tags from other should be
-            fetched too.
         :return: None
         """
         return InterBranch.get(other, self).update_revisions(stop_revision,
-            overwrite, graph, fetch_tags=fetch_tags)
+            overwrite, graph)
 
     @deprecated_method(deprecated_in((2, 4, 0)))
     def import_last_revision_info(self, source_repo, revno, revid):
@@ -1045,14 +1038,7 @@
             (should only be different from the arguments when lossy=True)
         """
         if not self.repository.has_same_location(source.repository):
-            try:
-                tags_to_fetch = set(source.tags.get_reverse_tag_dict())
-            except errors.TagsNotSupported:
-                tags_to_fetch = set()
-            fetch_spec = _mod_graph.NotInOtherForRevs(self.repository,
-                source.repository, [revid],
-                if_present_ids=tags_to_fetch).execute()
-            self.repository.fetch(source.repository, fetch_spec=fetch_spec)
+            self.fetch(source, revid)
         self.set_last_revision_info(revno, revid)
         return (revno, revid)
 
@@ -1535,18 +1521,22 @@
         else:
             raise AssertionError("invalid heads: %r" % (heads,))
 
-    def heads_to_fetch(self):
+    def heads_to_fetch(self, stop_revision=None):
         """Return the heads that must and that should be fetched to copy this
         branch into another repo.
 
+        :param stop_revision: Last branch revision to fetch, if not tip.
+
         :returns: a 2-tuple of (must_fetch, if_present_fetch).  must_fetch is a
             set of heads that must be fetched.  if_present_fetch is a set of
             heads that must be fetched if present, but no error is necessary if
             they are not present.
         """
-        # For bzr native formats must_fetch is just the tip, and if_present_fetch
-        # are the tags.
-        must_fetch = set([self.last_revision()])
+        # For bzr native formats must_fetch is just the tip, and
+        # if_present_fetch are the tags.
+        if stop_revision is None:
+            stop_revision = self.last_revision()
+        must_fetch = set([stop_revision])
         try:
             if_present_fetch = set(self.tags.get_reverse_tag_dict())
         except errors.TagsNotSupported:
@@ -3298,7 +3288,7 @@
 
     @needs_write_lock
     def update_revisions(self, stop_revision=None, overwrite=False,
-                         graph=None, fetch_tags=True):
+            graph=None):
         """Pull in new perfect-fit revisions.
 
         :param stop_revision: Updated until the given revision
@@ -3306,8 +3296,6 @@
             to see if it is a proper descendant.
         :param graph: A Graph object that can be used to query history
             information. This can be None.
-        :param fetch_tags: Flag that specifies if tags from source should be
-            fetched too.
         :return: None
         """
         raise NotImplementedError(self.update_revisions)
@@ -3331,11 +3319,10 @@
         raise NotImplementedError(self.copy_content_into)
 
     @needs_write_lock
-    def fetch(self, stop_revision=None, fetch_spec=None):
+    def fetch(self, stop_revision=None):
         """Fetch revisions.
 
         :param stop_revision: Last revision to fetch
-        :param fetch_spec: Fetch spec.
         """
         raise NotImplementedError(self.fetch)
 
@@ -3379,25 +3366,26 @@
             self.source.tags.merge_to(self.target.tags)
 
     @needs_write_lock
-    def fetch(self, stop_revision=None, fetch_spec=None):
-        if fetch_spec is not None and stop_revision is not None:
-            raise AssertionError(
-                "fetch_spec and last_revision are mutually exclusive.")
+    def fetch(self, stop_revision=None):
         if self.target.base == self.source.base:
             return (0, [])
         self.source.lock_read()
         try:
-            if stop_revision is None and fetch_spec is None:
-                stop_revision = self.source.last_revision()
-                stop_revision = _mod_revision.ensure_null(stop_revision)
+            fetch_spec_factory = fetch.FetchSpecFactory()
+            fetch_spec_factory.source_branch = self.source
+            fetch_spec_factory.source_branch_stop_revision_id = stop_revision
+            fetch_spec_factory.source_repo = self.source.repository
+            fetch_spec_factory.target_repo = self.target.repository
+            fetch_spec_factory.target_repo_kind = fetch.TargetRepoKinds.PREEXISTING
+            fetch_spec = fetch_spec_factory.make_fetch_spec()
             return self.target.repository.fetch(self.source.repository,
-                revision_id=stop_revision, fetch_spec=fetch_spec)
+                fetch_spec=fetch_spec)
         finally:
             self.source.unlock()
 
     @needs_write_lock
     def update_revisions(self, stop_revision=None, overwrite=False,
-        graph=None, fetch_tags=True):
+            graph=None):
         """See InterBranch.update_revisions()."""
         other_revno, other_last_revision = self.source.last_revision_info()
         stop_revno = None # unknown
@@ -3415,18 +3403,7 @@
         # case of having something to pull, and so that the check for
         # already merged can operate on the just fetched graph, which will
         # be cached in memory.
-        if fetch_tags:
-            fetch_spec_factory = fetch.FetchSpecFactory()
-            fetch_spec_factory.source_branch = self.source
-            fetch_spec_factory.source_branch_stop_revision_id = stop_revision
-            fetch_spec_factory.source_repo = self.source.repository
-            fetch_spec_factory.target_repo = self.target.repository
-            fetch_spec_factory.target_repo_kind = fetch.TargetRepoKinds.PREEXISTING
-            fetch_spec = fetch_spec_factory.make_fetch_spec()
-        else:
-            fetch_spec = _mod_graph.NotInOtherForRevs(self.target.repository,
-                self.source.repository, revision_ids=[stop_revision]).execute()
-        self.target.fetch(self.source, fetch_spec=fetch_spec)
+        self.fetch(stop_revision=stop_revision)
         # Check to see if one is an ancestor of the other
         if not overwrite:
             if graph is None:

=== modified file 'bzrlib/fetch.py'
--- bzrlib/fetch.py	2011-04-17 23:06:22 +0000
+++ bzrlib/fetch.py	2011-04-20 19:55:54 +0000
@@ -375,7 +375,7 @@
     def add_revision_ids(self, revision_ids):
         """Add revision_ids to the set of revision_ids to be fetched."""
         self._explicit_rev_ids.update(revision_ids)
-        
+
     def make_fetch_spec(self):
         """Build a SearchResult or PendingAncestryResult or etc."""
         if self.target_repo_kind is None or self.source_repo is None:
@@ -392,15 +392,8 @@
                     self.target_repo, self.source_repo).execute()
         heads_to_fetch = set(self._explicit_rev_ids)
         if self.source_branch is not None:
-            must_fetch, if_present_fetch = self.source_branch.heads_to_fetch()
-            if self.source_branch_stop_revision_id is not None:
-                # Replace the tip rev from must_fetch with the stop revision
-                # XXX: this might be wrong if the tip rev is also in the
-                # must_fetch set for other reasons (e.g. it's the tip of
-                # multiple loom threads?), but then it's pretty unclear what it
-                # should mean to specify a stop_revision in that case anyway.
-                must_fetch.discard(self.source_branch.last_revision())
-                must_fetch.add(self.source_branch_stop_revision_id)
+            must_fetch, if_present_fetch = self.source_branch.heads_to_fetch(
+                stop_revision=self.source_branch_stop_revision_id)
             heads_to_fetch.update(must_fetch)
         else:
             if_present_fetch = set()

=== modified file 'bzrlib/merge.py'
--- bzrlib/merge.py	2011-04-17 23:06:22 +0000
+++ bzrlib/merge.py	2011-04-20 19:55:54 +0000
@@ -563,14 +563,7 @@
 
     def _maybe_fetch(self, source, target, revision_id):
         if not source.repository.has_same_location(target.repository):
-            try:
-                tags_to_fetch = set(source.tags.get_reverse_tag_dict())
-            except errors.TagsNotSupported:
-                tags_to_fetch = None
-            fetch_spec = _mod_graph.NotInOtherForRevs(target.repository,
-                source.repository, [revision_id],
-                if_present_ids=tags_to_fetch).execute()
-            target.fetch(source, fetch_spec=fetch_spec)
+            target.fetch(source, revision_id)
 
     def find_base(self):
         revisions = [_mod_revision.ensure_null(self.this_basis),

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2011-04-19 13:56:32 +0000
+++ bzrlib/remote.py	2011-04-20 19:55:54 +0000
@@ -3046,32 +3046,33 @@
         self._ensure_real()
         return self._real_branch.set_push_location(location)
 
-    def heads_to_fetch(self):
+    def heads_to_fetch(self, stop_revision=None):
         if self._format._use_default_local_heads_to_fetch():
             # We recognise this format, and its heads-to-fetch implementation
             # is the default one (tip + tags).  In this case it's cheaper to
             # just use the default implementation rather than a special RPC as
             # the tip and tags data is cached.
-            return branch.Branch.heads_to_fetch(self)
+            return branch.Branch.heads_to_fetch(self, stop_revision)
         medium = self._client._medium
         if medium._is_remote_before((2, 4)):
-            return self._vfs_heads_to_fetch()
+            return self._vfs_heads_to_fetch(stop_revision)
         try:
-            return self._rpc_heads_to_fetch()
+            return self._rpc_heads_to_fetch(stop_revision)
         except errors.UnknownSmartMethod:
             medium._remember_remote_is_before((2, 4))
-            return self._vfs_heads_to_fetch()
+            return self._vfs_heads_to_fetch(stop_revision)
 
-    def _rpc_heads_to_fetch(self):
-        response = self._call('Branch.heads_to_fetch', self._remote_path())
+    def _rpc_heads_to_fetch(self, stop_revision):
+        response = self._call('Branch.heads_to_fetch', self._remote_path(),
+            stop_revision)
         if len(response) != 2:
             raise errors.UnexpectedSmartServerResponse(response)
         must_fetch, if_present_fetch = response
         return set(must_fetch), set(if_present_fetch)
 
-    def _vfs_heads_to_fetch(self):
+    def _vfs_heads_to_fetch(self, stop_revision):
         self._ensure_real()
-        return self._real_branch.heads_to_fetch()
+        return self._real_branch.heads_to_fetch(stop_revision)
 
 
 class RemoteConfig(object):

=== modified file 'bzrlib/smart/branch.py'
--- bzrlib/smart/branch.py	2011-02-25 04:44:53 +0000
+++ bzrlib/smart/branch.py	2011-04-20 19:55:54 +0000
@@ -144,14 +144,14 @@
 
 class SmartServerBranchHeadsToFetch(SmartServerBranchRequest):
 
-    def do_with_branch(self, branch):
+    def do_with_branch(self, branch, stop_revision):
         """Return the heads-to-fetch for a Branch as two bencoded lists.
         
         See Branch.heads_to_fetch.
 
         New in 2.4.
         """
-        must_fetch, if_present_fetch = branch.heads_to_fetch()
+        must_fetch, if_present_fetch = branch.heads_to_fetch(stop_revision)
         return SuccessfulSmartServerResponse(
             (list(must_fetch), list(if_present_fetch)))
 

=== modified file 'bzrlib/tests/per_branch/test_branch.py'
--- bzrlib/tests/per_branch/test_branch.py	2011-04-14 07:53:38 +0000
+++ bzrlib/tests/per_branch/test_branch.py	2011-04-20 19:55:54 +0000
@@ -476,6 +476,14 @@
         must_fetch, should_fetch = tree.branch.heads_to_fetch()
         self.assertTrue('rev2' in must_fetch)
 
+    def test_heads_to_fetch_stop_revision(self):
+        tree = self.make_branch_and_tree('a')
+        tree.commit('first commit', rev_id='rev1')
+        tree.commit('second commit', rev_id='rev2')
+        must_fetch, should_fetch = tree.branch.heads_to_fetch('rev1')
+        self.assertTrue('rev1' in must_fetch)
+        self.assertFalse('rev2' in must_fetch)
+
     def test_heads_to_fetch_not_null_revision(self):
         # NULL_REVISION does not appear in the result of heads_to_fetch, even
         # for an empty branch.

=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py	2011-04-05 17:37:53 +0000
+++ bzrlib/tests/test_remote.py	2011-04-20 19:55:54 +0000
@@ -1203,7 +1203,7 @@
             'Branch.get_stacked_on_url', ('quack/',),
             'error', ('NotStacked',))
         client.add_expected_call(
-            'Branch.heads_to_fetch', ('quack/',),
+            'Branch.heads_to_fetch', ('quack/', None),
             'success', (['tip'], ['tagged-1', 'tagged-2']))
         transport.mkdir('quack')
         transport = transport.clone('quack')

=== modified file 'bzrlib/tests/test_revisionspec.py'
--- bzrlib/tests/test_revisionspec.py	2011-04-14 07:53:38 +0000
+++ bzrlib/tests/test_revisionspec.py	2011-04-20 19:55:54 +0000
@@ -397,8 +397,7 @@
         """We can get any revision id in the repository"""
         # XXX: This may change in the future, but for now, it is true
         self.tree2.commit('alt third', rev_id='alt_r3')
-        self.tree.branch.repository.fetch(self.tree2.branch.repository,
-                                          revision_id='alt_r3')
+        self.tree.branch.fetch(self.tree2.branch, 'alt_r3')
         self.assertInHistoryIs(None, 'alt_r3', 'revid:alt_r3')
 
     def test_unicode(self):
@@ -475,8 +474,7 @@
     def test_alt_no_parents(self):
         new_tree = self.make_branch_and_tree('new_tree')
         new_tree.commit('first', rev_id='new_r1')
-        self.tree.branch.repository.fetch(new_tree.branch.repository,
-                                          revision_id='new_r1')
+        self.tree.branch.fetch(new_tree.branch, 'new_r1')
         self.assertInHistoryIs(0, 'null:', 'before:revid:new_r1')
 
     def test_as_revision_id(self):

=== modified file 'bzrlib/workingtree_4.py'
--- bzrlib/workingtree_4.py	2011-04-17 23:06:22 +0000
+++ bzrlib/workingtree_4.py	2011-04-20 19:55:54 +0000
@@ -2016,7 +2016,7 @@
     def make_source_parent_tree(source, target):
         """Change the source tree into a parent of the target."""
         revid = source.commit('record tree')
-        target.branch.repository.fetch(source.branch.repository, revid)
+        target.branch.fetch(source.branch, revid)
         target.set_parent_ids([revid])
         return target.basis_tree(), target
 

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-04-19 14:12:43 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-04-20 19:55:54 +0000
@@ -357,7 +357,7 @@
   (Jelmer Vernooij)
 
 * ``Branch.fetch`` implementations must now accept an optional
-  ``fetch_spec`` keyword argument. (Andrew Bennetts)
+  ``fetch_tags`` keyword argument. (Andrew Bennetts)
 
 * ``Branch.import_last_revision_info`` is deprecated.  Use the
   ``import_last_revision_info_and_tags`` method instead.