Code review comment for lp:~spiv/bzr/stacking-friendly-revision-history-verb

> Does this need a NEWS entry?

Yes, I'll add one.

> Please put the bug url in a comment in the tests to give more context.

Ok.

> The rest of these are general comments are needn't delay merging:
>
> 65 + # This figure represent the amount of work to perform this use case.
> It
> 66 + # is entirely ok to reduce this number if a test fails due to
> rpc_count
> 67 + # being too low. If rpc_count increases, more network roundtrips
> have
> 68 + # become necessary for this use case. Please do not adjust this
> number
> 69 + # upwards without agreement from bzr's network support maintainers.
> 70 + self.assertLength(43, self.hpss_calls)
>
> This feels like it should be a specific assertion rather than copy and pasted
> so many times...

Yeah. Another possibility is a centralised list of ratchet values somewhere, and then the scary "DO NOT CHANGE UPWARDS" comment can be at that one place.

> It's unfortunate to hardcode 1.9 in these tests but we should probably scan
> for them later.

Yeah. That's how Robert and I feel about the hardcoded 1.9 in these tests too.

> 88 + except unstackable_format_errors, e:
> 89 + raise TestNotApplicable('Format does not support stacking.')
>
> Some people would say you should check at the start of the test whether it
> promises to support stacking.

Well, I'm doing what nearby tests do. (Also, you can't do it at the very start, because at least for the Remote* tests you can't ask the format if it supports stacking unless there is an underlying object for the Remote*Format to ask).

> Looks good.

Thanks!