Re: Blame callback: remove start/end parameters

> Bert Huijben wrote:
> > Not sure if that would be as helpful as the old code and/or can be done
> > with a compatibility wrapper.

It already is done with the compatibility wrapper.

> > The problem is that the output arguments are
> > available only, long after the blame reporting is done, [...]
> > Perhaps if we document that the output arguments are set before the first
> > invocation of the callback,

I intended to document that, but forgot. Documented in r1851268.

> > but that would be an API-first.

I'm pretty sure it would not, though I don't have an example at hand.

Branko Čibej wrote:
> Good point. And also we may not be able to guarantee that in general.

If you mean another implementation might not be able to supply those outputs before it first calls the callback, then that is no different from the previous situation in which the implementation was required to calculate and supply those outputs in the first callback (and in every subsequent one).

> It doesn't really hurt for the callback to get redundant parameters. It
> does hurt the API user if they have to play tricks to make sure they
> update the callback's context (baton) before the callback is invoked.
>
> So, on balance ... this is not an improvement.

I think it *does* hurt to send redundant or misplaced parameters, and so this is an improvement.

I don't want to waste much time arguing about it, however, so I will revert it if you still think so after this reply.

First let me try to say why I like it. The main thing is I think we throw too many parameters into many of our public functions, and this absolutely does make the (overall) SVN API harder to use than it need be. As this was being revved anyway, I see this as an opportunity to make a small change.

In terms of the specifics here, apart from the simple "DRY" argument, it's because these are so closely related to the 'start' and 'end' input parameters. In principle the resolving of revision numbers is a preliminary step, before the blame algorithm runs. It's just a detail that we delay resolution until somewhere inside the blame function. A good alternative interface would be to have svn_client_blame6() take mutable revision specification objects, which the called function resolves (if not already resolved) at its first good opportunity. If we had this kind of interface, it would be reasonable to specify that it gets resolved before the first callback call. That's conceptually identical to what I've implemented.

I will also point out that these callback arguments had never yet been added to the JavaHL wrapper. If we reinstate them, then we should add them there too.

- Julian
Received on 2019-01-16 17:30:44 CET