Code review comment for lp:~jelmer/bzr/move-interbranch-fetch2

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?