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

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