On 16.01.2019 17:30, Julian Foad wrote: >> 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.
On reflection, I agree with your arguments. I only went and did r1851525 for consistency, though. > 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. True, those have never been exposed in JavaHL. Now is a perfect time to do that. -- Brane
