Blame callback: remove start/end parameters

[Julian Foad <ju...@apache.org>]

The svn_client_blame_receiver4_t parameters "start_revnum" and "end_revnum" do not really belong here because they are not per-line data. They are the "resolved" versions of the input revnums to svn_client_blame6(). I introduced them in r955895, applying a patch by Johan. I propose to move them to svn_client_blame6() output parameters.

(svn_client_blame6() and svn_client_blame_receiver4_t APIs are currently being revved so this is a good time to make such a change.)

-- 
- Julian

Re: Blame callback: remove start/end parameters

[Branko Čibej <br...@apache.org>]

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

Re: Blame callback: remove start/end parameters

[Julian Foad <ju...@apache.org>]

> 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

Re: Blame callback: remove start/end parameters

[Branko Čibej <br...@apache.org>]

On 15.01.2019 10:49, Bert Huijben wrote:
> Not sure if that would be as helpful as the old code and/or can be done
> with a compatibility wrapper. The problem is that the output arguments are
> available only, long after the blame reporting is done, while with the
> current implementation intermediate callbacks can be used more efficiently.
>
> Perhaps if we document that the output arguments are set before the first
> invocation of the callback, but that would be an API-first.


Good point. And also we may not be able to guarantee that in general.

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.

-- Brane


> On Tue, Jan 15, 2019 at 1:30 AM Johan Corveleyn <jc...@gmail.com> wrote:
>
>> On Mon, Jan 14, 2019 at 4:13 PM Julian Foad <ju...@apache.org> wrote:
>>> Julian Foad wrote:
>>>> The svn_client_blame_receiver4_t parameters "start_revnum" and
>>>> "end_revnum" do not really belong here because they are not per-line
>>>> data. They are the "resolved" versions of the input revnums to
>>>> svn_client_blame6(). I introduced them in r955895, applying a patch by
>>>> Johan. I propose to move them to svn_client_blame6() output parameters.
>> Ooh, my very first patch :-). I had no clue what I was doing back
>> then, and I still don't most of the time :-). Thanks for improving
>> things ...
>>
>> --
>> Johan
>>

Re: Blame callback: remove start/end parameters

[Bert Huijben <be...@qqmail.nl>]

Not sure if that would be as helpful as the old code and/or can be done
with a compatibility wrapper. The problem is that the output arguments are
available only, long after the blame reporting is done, while with the
current implementation intermediate callbacks can be used more efficiently.

Perhaps if we document that the output arguments are set before the first
invocation of the callback, but that would be an API-first.

    Bert


On Tue, Jan 15, 2019 at 1:30 AM Johan Corveleyn <jc...@gmail.com> wrote:

> On Mon, Jan 14, 2019 at 4:13 PM Julian Foad <ju...@apache.org> wrote:
> >
> > Julian Foad wrote:
> > > The svn_client_blame_receiver4_t parameters "start_revnum" and
> > > "end_revnum" do not really belong here because they are not per-line
> > > data. They are the "resolved" versions of the input revnums to
> > > svn_client_blame6(). I introduced them in r955895, applying a patch by
> > > Johan. I propose to move them to svn_client_blame6() output parameters.
>
> Ooh, my very first patch :-). I had no clue what I was doing back
> then, and I still don't most of the time :-). Thanks for improving
> things ...
>
> --
> Johan
>

Re: Blame callback: remove start/end parameters

[Johan Corveleyn <jc...@gmail.com>]

On Mon, Jan 14, 2019 at 4:13 PM Julian Foad <ju...@apache.org> wrote:
>
> Julian Foad wrote:
> > The svn_client_blame_receiver4_t parameters "start_revnum" and
> > "end_revnum" do not really belong here because they are not per-line
> > data. They are the "resolved" versions of the input revnums to
> > svn_client_blame6(). I introduced them in r955895, applying a patch by
> > Johan. I propose to move them to svn_client_blame6() output parameters.

Ooh, my very first patch :-). I had no clue what I was doing back
then, and I still don't most of the time :-). Thanks for improving
things ...

-- 
Johan

Re: Blame callback: remove start/end parameters

[Julian Foad <ju...@apache.org>]

Julian Foad wrote:
> The svn_client_blame_receiver4_t parameters "start_revnum" and 
> "end_revnum" do not really belong here because they are not per-line 
> data. They are the "resolved" versions of the input revnums to 
> svn_client_blame6(). I introduced them in r955895, applying a patch by 
> Johan. I propose to move them to svn_client_blame6() output parameters.

r1851265.

-- 
- Julian