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