Patchwork [5,of,7] changegroup: deprecate 'getlocalchangroup' (API)

login
register
mail settings
Submitter Pierre-Yves David
Date May 5, 2017, 6:26 a.m.
Message ID <2f51cfeac5bcf8ee266a.1493965615@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/20450/
State Accepted
Headers show

Comments

Pierre-Yves David - May 5, 2017, 6:26 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1493894621 -7200
#      Thu May 04 12:43:41 2017 +0200
# Node ID 2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
# Parent  1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
# EXP-Topic changegroup.cleanup
# Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
#              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 2f51cfeac5bc
changegroup: deprecate 'getlocalchangroup' (API)

We have 'getchangegroup' with a shorter name for the exactly same feature. Now
that all users are gone we can formally deprecate it.
via Mercurial-devel - May 5, 2017, 4:41 p.m.
On Thu, May 4, 2017 at 11:26 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # Date 1493894621 -7200
> #      Thu May 04 12:43:41 2017 +0200
> # Node ID 2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
> # Parent  1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
> # EXP-Topic changegroup.cleanup
> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 2f51cfeac5bc
> changegroup: deprecate 'getlocalchangroup' (API)
>
> We have 'getchangegroup' with a shorter name for the exactly same feature. Now
> that all users are gone we can formally deprecate it.

We usually just delete methods that we no longer use internally. Why
not do that here?
Pierre-Yves David - May 5, 2017, 6:35 p.m.
On 05/05/2017 06:41 PM, Martin von Zweigbergk wrote:
> On Thu, May 4, 2017 at 11:26 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>> # Date 1493894621 -7200
>> #      Thu May 04 12:43:41 2017 +0200
>> # Node ID 2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
>> # Parent  1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
>> # EXP-Topic changegroup.cleanup
>> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 2f51cfeac5bc
>> changegroup: deprecate 'getlocalchangroup' (API)
>>
>> We have 'getchangegroup' with a shorter name for the exactly same feature. Now
>> that all users are gone we can formally deprecate it.
>
> We usually just delete methods that we no longer use internally. Why
> not do that here?

When function are likely to be used by extension we try to avoid 
dropping them and we deprecated them for a version. This helps third 
party extensions to smoothly detect the API changes (usually through 
test run) and smoothly upgrade their code over the version cycle.

The function are dropped in the version released after their deprecation 
(cf recent localrepo API cleanup and vfs movement).

We have been using this approach for a couple of year and this has 
provent extremely useful.

Cheers,
via Mercurial-devel - May 5, 2017, 6:38 p.m.
On Fri, May 5, 2017 at 11:35 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 05/05/2017 06:41 PM, Martin von Zweigbergk wrote:
>>
>> On Thu, May 4, 2017 at 11:26 PM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>>
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>>> # Date 1493894621 -7200
>>> #      Thu May 04 12:43:41 2017 +0200
>>> # Node ID 2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
>>> # Parent  1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
>>> # EXP-Topic changegroup.cleanup
>>> # Available At
>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>> #              hg pull
>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 2f51cfeac5bc
>>> changegroup: deprecate 'getlocalchangroup' (API)
>>>
>>> We have 'getchangegroup' with a shorter name for the exactly same
>>> feature. Now
>>> that all users are gone we can formally deprecate it.
>>
>>
>> We usually just delete methods that we no longer use internally. Why
>> not do that here?
>
>
> When function are likely to be used by extension we try to avoid dropping
> them and we deprecated them for a version. This helps third party extensions
> to smoothly detect the API changes (usually through test run) and smoothly
> upgrade their code over the version cycle.

In this case I suspect it won't help many of them because I probably
broke most or all anyway with
https://www.mercurial-scm.org/repo/hg/rev/282b288aa20c. I thought we
were okay with that kind of changes. Are you saying I should have
instead added duplicate methods with the new signatures and only
deprecated the existing methods?

>
> The function are dropped in the version released after their deprecation (cf
> recent localrepo API cleanup and vfs movement).
>
> We have been using this approach for a couple of year and this has provent
> extremely useful.
>
> Cheers,
>
> --
> Pierre-Yves David
Pierre-Yves David - May 5, 2017, 6:44 p.m.
On 05/05/2017 08:38 PM, Martin von Zweigbergk wrote:
> On Fri, May 5, 2017 at 11:35 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>> On 05/05/2017 06:41 PM, Martin von Zweigbergk wrote:
>>>
>>> On Thu, May 4, 2017 at 11:26 PM, Pierre-Yves David
>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>>>> # Date 1493894621 -7200
>>>> #      Thu May 04 12:43:41 2017 +0200
>>>> # Node ID 2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
>>>> # Parent  1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
>>>> # EXP-Topic changegroup.cleanup
>>>> # Available At
>>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>>> #              hg pull
>>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 2f51cfeac5bc
>>>> changegroup: deprecate 'getlocalchangroup' (API)
>>>>
>>>> We have 'getchangegroup' with a shorter name for the exactly same
>>>> feature. Now
>>>> that all users are gone we can formally deprecate it.
>>>
>>>
>>> We usually just delete methods that we no longer use internally. Why
>>> not do that here?
>>
>>
>> When function are likely to be used by extension we try to avoid dropping
>> them and we deprecated them for a version. This helps third party extensions
>> to smoothly detect the API changes (usually through test run) and smoothly
>> upgrade their code over the version cycle.
>
> In this case I suspect it won't help many of them because I probably
> broke most or all anyway with
> https://www.mercurial-scm.org/repo/hg/rev/282b288aa20c. I thought we
> were okay with that kind of changes. Are you saying I should have
> instead added duplicate methods with the new signatures and only
> deprecated the existing methods?

Generating changegroup is a quite core feature in Mercurial. I suspect 
their are extension out there using these and I tried to be careful. 
That is a gut feeling. I'm did not looked at actual data. (that gut 
feeling proved right for "vfs", but might be wrong here)

(so, yes I would have been more careful with your API change too)

Cheers,
via Mercurial-devel - May 5, 2017, 7:16 p.m.
On Fri, May 5, 2017 at 11:44 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 05/05/2017 08:38 PM, Martin von Zweigbergk wrote:
>>
>> On Fri, May 5, 2017 at 11:35 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>>
>>>
>>>
>>> On 05/05/2017 06:41 PM, Martin von Zweigbergk wrote:
>>>>
>>>>
>>>> On Thu, May 4, 2017 at 11:26 PM, Pierre-Yves David
>>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>>
>>>>>
>>>>> # HG changeset patch
>>>>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>>>>> # Date 1493894621 -7200
>>>>> #      Thu May 04 12:43:41 2017 +0200
>>>>> # Node ID 2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
>>>>> # Parent  1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
>>>>> # EXP-Topic changegroup.cleanup
>>>>> # Available At
>>>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>>>> #              hg pull
>>>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r
>>>>> 2f51cfeac5bc
>>>>> changegroup: deprecate 'getlocalchangroup' (API)
>>>>>
>>>>> We have 'getchangegroup' with a shorter name for the exactly same
>>>>> feature. Now
>>>>> that all users are gone we can formally deprecate it.
>>>>
>>>>
>>>>
>>>> We usually just delete methods that we no longer use internally. Why
>>>> not do that here?
>>>
>>>
>>>
>>> When function are likely to be used by extension we try to avoid dropping
>>> them and we deprecated them for a version. This helps third party
>>> extensions
>>> to smoothly detect the API changes (usually through test run) and
>>> smoothly
>>> upgrade their code over the version cycle.
>>
>>
>> In this case I suspect it won't help many of them because I probably
>> broke most or all anyway with
>> https://www.mercurial-scm.org/repo/hg/rev/282b288aa20c. I thought we
>> were okay with that kind of changes. Are you saying I should have
>> instead added duplicate methods with the new signatures and only
>> deprecated the existing methods?
>
>
> Generating changegroup is a quite core feature in Mercurial. I suspect their
> are extension out there using these and I tried to be careful. That is a gut
> feeling. I'm did not looked at actual data. (that gut feeling proved right
> for "vfs", but might be wrong here)
>
> (so, yes I would have been more careful with your API change too)

Well, given that my patch probably broke most of those extensions, I
don't see much reason for your series to be more careful. But there's
little harm in doing it, so I'll queue it once tests have run etc.

>
> Cheers,
>
> --
> Pierre-Yves David
Augie Fackler - May 5, 2017, 10:44 p.m.
On Fri, May 05, 2017 at 12:16:50PM -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> On Fri, May 5, 2017 at 11:44 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
> >
> >
> > On 05/05/2017 08:38 PM, Martin von Zweigbergk wrote:
> >>
> >> On Fri, May 5, 2017 at 11:35 AM, Pierre-Yves David
> >> <pierre-yves.david@ens-lyon.org> wrote:
> >>>
> >>>
> >>>
> >>> On 05/05/2017 06:41 PM, Martin von Zweigbergk wrote:
> >>>>
> >>>>
> >>>> On Thu, May 4, 2017 at 11:26 PM, Pierre-Yves David
> >>>> <pierre-yves.david@ens-lyon.org> wrote:
> >>>>>
> >>>>>
> >>>>> # HG changeset patch
> >>>>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> >>>>> # Date 1493894621 -7200
> >>>>> #      Thu May 04 12:43:41 2017 +0200
> >>>>> # Node ID 2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
> >>>>> # Parent  1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
> >>>>> # EXP-Topic changegroup.cleanup
> >>>>> # Available At
> >>>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
> >>>>> #              hg pull
> >>>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r
> >>>>> 2f51cfeac5bc
> >>>>> changegroup: deprecate 'getlocalchangroup' (API)
> >>>>>
> >>>>> We have 'getchangegroup' with a shorter name for the exactly same
> >>>>> feature. Now
> >>>>> that all users are gone we can formally deprecate it.
> >>>>
> >>>>
> >>>>
> >>>> We usually just delete methods that we no longer use internally. Why
> >>>> not do that here?
> >>>
> >>>
> >>>
> >>> When function are likely to be used by extension we try to avoid dropping
> >>> them and we deprecated them for a version. This helps third party
> >>> extensions
> >>> to smoothly detect the API changes (usually through test run) and
> >>> smoothly
> >>> upgrade their code over the version cycle.
> >>
> >>
> >> In this case I suspect it won't help many of them because I probably
> >> broke most or all anyway with
> >> https://www.mercurial-scm.org/repo/hg/rev/282b288aa20c. I thought we
> >> were okay with that kind of changes. Are you saying I should have
> >> instead added duplicate methods with the new signatures and only
> >> deprecated the existing methods?
> >
> >
> > Generating changegroup is a quite core feature in Mercurial. I suspect their
> > are extension out there using these and I tried to be careful. That is a gut
> > feeling. I'm did not looked at actual data. (that gut feeling proved right
> > for "vfs", but might be wrong here)
> >
> > (so, yes I would have been more careful with your API change too)
>
> Well, given that my patch probably broke most of those extensions, I
> don't see much reason for your series to be more careful. But there's
> little harm in doing it, so I'll queue it once tests have run etc.

FWIW, my gut feeling is that few-to-no extensions are likely to
generate bundles. I've done lots of awful invasive stuff in extensions
and never actually generated bundles as part of them.

(I don't feel strongly about preserving compat either, but my feeling
on API stability is that our warnings should be seen by extension
authors as best-effort and not reliable.)

>
> >
> > Cheers,
> >
> > --
> > Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - May 6, 2017, 6:57 a.m.
On 05/05/2017 09:16 PM, Martin von Zweigbergk wrote:
> On Fri, May 5, 2017 at 11:44 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>> On 05/05/2017 08:38 PM, Martin von Zweigbergk wrote:
>>>
>>> On Fri, May 5, 2017 at 11:35 AM, Pierre-Yves David
>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>
>>>>
>>>>
>>>> On 05/05/2017 06:41 PM, Martin von Zweigbergk wrote:
>>>>>
>>>>>
>>>>> On Thu, May 4, 2017 at 11:26 PM, Pierre-Yves David
>>>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>>>
>>>>>>
>>>>>> # HG changeset patch
>>>>>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>>>>>> # Date 1493894621 -7200
>>>>>> #      Thu May 04 12:43:41 2017 +0200
>>>>>> # Node ID 2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
>>>>>> # Parent  1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
>>>>>> # EXP-Topic changegroup.cleanup
>>>>>> # Available At
>>>>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>>>>> #              hg pull
>>>>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r
>>>>>> 2f51cfeac5bc
>>>>>> changegroup: deprecate 'getlocalchangroup' (API)
>>>>>>
>>>>>> We have 'getchangegroup' with a shorter name for the exactly same
>>>>>> feature. Now
>>>>>> that all users are gone we can formally deprecate it.
>>>>>
>>>>>
>>>>>
>>>>> We usually just delete methods that we no longer use internally. Why
>>>>> not do that here?
>>>>
>>>>
>>>>
>>>> When function are likely to be used by extension we try to avoid dropping
>>>> them and we deprecated them for a version. This helps third party
>>>> extensions
>>>> to smoothly detect the API changes (usually through test run) and
>>>> smoothly
>>>> upgrade their code over the version cycle.
>>>
>>>
>>> In this case I suspect it won't help many of them because I probably
>>> broke most or all anyway with
>>> https://www.mercurial-scm.org/repo/hg/rev/282b288aa20c. I thought we
>>> were okay with that kind of changes. Are you saying I should have
>>> instead added duplicate methods with the new signatures and only
>>> deprecated the existing methods?
>>
>>
>> Generating changegroup is a quite core feature in Mercurial. I suspect their
>> are extension out there using these and I tried to be careful. That is a gut
>> feeling. I'm did not looked at actual data. (that gut feeling proved right
>> for "vfs", but might be wrong here)
>>
>> (so, yes I would have been more careful with your API change too)
>
> Well, given that my patch probably broke most of those extensions, I
> don't see much reason for your series to be more careful. But there's
> little harm in doing it, so I'll queue it once tests have run etc.

I agree doing both approach at the same time is sub-optimal :-)
We should either restore some of the (deprecated) compatibility dropped 
your series or remove the function deprecated in mine.

I've digged up a bit and  already found a couple of Facebook extensions 
using getlocalchangegroup so I think it is safer to assume there are 
external users.

Cheers,
via Mercurial-devel - May 6, 2017, 1:55 p.m.
On May 5, 2017 23:57, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:



On 05/05/2017 09:16 PM, Martin von Zweigbergk wrote:

> On Fri, May 5, 2017 at 11:44 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>
>>
>>
>> On 05/05/2017 08:38 PM, Martin von Zweigbergk wrote:
>>
>>>
>>> On Fri, May 5, 2017 at 11:35 AM, Pierre-Yves David
>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>
>>>>
>>>>
>>>>
>>>> On 05/05/2017 06:41 PM, Martin von Zweigbergk wrote:
>>>>
>>>>>
>>>>>
>>>>> On Thu, May 4, 2017 at 11:26 PM, Pierre-Yves David
>>>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> # HG changeset patch
>>>>>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>>>>>> # Date 1493894621 -7200
>>>>>> #      Thu May 04 12:43:41 2017 +0200
>>>>>> # Node ID 2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
>>>>>> # Parent  1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
>>>>>> # EXP-Topic changegroup.cleanup
>>>>>> # Available At
>>>>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>>>>> #              hg pull
>>>>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r
>>>>>> 2f51cfeac5bc
>>>>>> changegroup: deprecate 'getlocalchangroup' (API)
>>>>>>
>>>>>> We have 'getchangegroup' with a shorter name for the exactly same
>>>>>> feature. Now
>>>>>> that all users are gone we can formally deprecate it.
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> We usually just delete methods that we no longer use internally. Why
>>>>> not do that here?
>>>>>
>>>>
>>>>
>>>>
>>>> When function are likely to be used by extension we try to avoid
>>>> dropping
>>>> them and we deprecated them for a version. This helps third party
>>>> extensions
>>>> to smoothly detect the API changes (usually through test run) and
>>>> smoothly
>>>> upgrade their code over the version cycle.
>>>>
>>>
>>>
>>> In this case I suspect it won't help many of them because I probably
>>> broke most or all anyway with
>>> https://www.mercurial-scm.org/repo/hg/rev/282b288aa20c. I thought we
>>> were okay with that kind of changes. Are you saying I should have
>>> instead added duplicate methods with the new signatures and only
>>> deprecated the existing methods?
>>>
>>
>>
>> Generating changegroup is a quite core feature in Mercurial. I suspect
>> their
>> are extension out there using these and I tried to be careful. That is a
>> gut
>> feeling. I'm did not looked at actual data. (that gut feeling proved right
>> for "vfs", but might be wrong here)
>>
>> (so, yes I would have been more careful with your API change too)
>>
>
> Well, given that my patch probably broke most of those extensions, I
> don't see much reason for your series to be more careful. But there's
> little harm in doing it, so I'll queue it once tests have run etc.
>

I agree doing both approach at the same time is sub-optimal :-)
We should either restore some of the (deprecated) compatibility dropped
your series or remove the function deprecated in mine.

I've digged up a bit and  already found a couple of Facebook extensions
using getlocalchangegroup so I think it is safer to assume there are
external users.


I don't care much. Durham, do you guys care enough so we should back out
the change. I don't care enough to clean up that I'm going to bother with
deprecation.


Cheers,
Gregory Szorc - May 6, 2017, 6:20 p.m.
On Fri, May 5, 2017 at 11:44 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 05/05/2017 08:38 PM, Martin von Zweigbergk wrote:
>
>> On Fri, May 5, 2017 at 11:35 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>>
>>>
>>> On 05/05/2017 06:41 PM, Martin von Zweigbergk wrote:
>>>
>>>>
>>>> On Thu, May 4, 2017 at 11:26 PM, Pierre-Yves David
>>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>
>>>>>
>>>>> # HG changeset patch
>>>>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>>>>> # Date 1493894621 -7200
>>>>> #      Thu May 04 12:43:41 2017 +0200
>>>>> # Node ID 2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
>>>>> # Parent  1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
>>>>> # EXP-Topic changegroup.cleanup
>>>>> # Available At
>>>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>>>> #              hg pull
>>>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r
>>>>> 2f51cfeac5bc
>>>>> changegroup: deprecate 'getlocalchangroup' (API)
>>>>>
>>>>> We have 'getchangegroup' with a shorter name for the exactly same
>>>>> feature. Now
>>>>> that all users are gone we can formally deprecate it.
>>>>>
>>>>
>>>>
>>>> We usually just delete methods that we no longer use internally. Why
>>>> not do that here?
>>>>
>>>
>>>
>>> When function are likely to be used by extension we try to avoid dropping
>>> them and we deprecated them for a version. This helps third party
>>> extensions
>>> to smoothly detect the API changes (usually through test run) and
>>> smoothly
>>> upgrade their code over the version cycle.
>>>
>>
>> In this case I suspect it won't help many of them because I probably
>> broke most or all anyway with
>> https://www.mercurial-scm.org/repo/hg/rev/282b288aa20c. I thought we
>> were okay with that kind of changes. Are you saying I should have
>> instead added duplicate methods with the new signatures and only
>> deprecated the existing methods?
>>
>
> Generating changegroup is a quite core feature in Mercurial. I suspect
> their are extension out there using these and I tried to be careful. That
> is a gut feeling. I'm did not looked at actual data. (that gut feeling
> proved right for "vfs", but might be wrong here)
>

The changegroup APIs are horrible and are low-level. I favor deleting
legacy ones that are no longer used in core. Extensions can test for
function presence at run-time.
Augie Fackler - May 6, 2017, 6:22 p.m.
> On May 6, 2017, at 2:20 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> 
>>> In this case I suspect it won't help many of them because I probably
>>> broke most or all anyway with
>>> https://www.mercurial-scm.org/repo/hg/rev/282b288aa20c. I thought we
>>> were okay with that kind of changes. Are you saying I should have
>>> instead added duplicate methods with the new signatures and only
>>> deprecated the existing methods?
>> 
>> Generating changegroup is a quite core feature in Mercurial. I suspect their are extension out there using these and I tried to be careful. That is a gut feeling. I'm did not looked at actual data. (that gut feeling proved right for "vfs", but might be wrong here)
>> 
> The changegroup APIs are horrible and are low-level. I favor deleting legacy ones that are no longer used in core. Extensions can test for function presence at run-time.

Horrible in what way? Data loss, or just awkward?
Gregory Szorc - May 6, 2017, 6:25 p.m.
On Sat, May 6, 2017 at 11:22 AM, Augie Fackler <raf@durin42.com> wrote:

>
> > On May 6, 2017, at 2:20 PM, Gregory Szorc <gregory.szorc@gmail.com>
> wrote:
> >
> >>> In this case I suspect it won't help many of them because I probably
> >>> broke most or all anyway with
> >>> https://www.mercurial-scm.org/repo/hg/rev/282b288aa20c. I thought we
> >>> were okay with that kind of changes. Are you saying I should have
> >>> instead added duplicate methods with the new signatures and only
> >>> deprecated the existing methods?
> >>
> >> Generating changegroup is a quite core feature in Mercurial. I suspect
> their are extension out there using these and I tried to be careful. That
> is a gut feeling. I'm did not looked at actual data. (that gut feeling
> proved right for "vfs", but might be wrong here)
> >>
> > The changegroup APIs are horrible and are low-level. I favor deleting
> legacy ones that are no longer used in core. Extensions can test for
> function presence at run-time.
>
> Horrible in what way? Data loss, or just awkward?


Awkward. Remember the long series I sent last year to refactor the entire
thing to inject some sanity? It was queued then unqueued, which is why the
API is still horrible (although slightly less bad with this series). There
is a bunch of redundancy and it isn't clear what the purpose of each
function is.
Durham Goode - May 8, 2017, 4:10 p.m.
On 5/6/17 6:55 AM, Martin von Zweigbergk wrote:
>
>
> On May 5, 2017 23:57, "Pierre-Yves David"
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>
>
>     On 05/05/2017 09:16 PM, Martin von Zweigbergk wrote:
>
>         On Fri, May 5, 2017 at 11:44 AM, Pierre-Yves David
>         <pierre-yves.david@ens-lyon.org
>         <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>
>
>
>             On 05/05/2017 08:38 PM, Martin von Zweigbergk wrote:
>
>
>                 On Fri, May 5, 2017 at 11:35 AM, Pierre-Yves David
>                 <pierre-yves.david@ens-lyon.org
>                 <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>
>
>
>
>                     On 05/05/2017 06:41 PM, Martin von Zweigbergk wrote:
>
>
>
>                         On Thu, May 4, 2017 at 11:26 PM, Pierre-Yves David
>                         <pierre-yves.david@ens-lyon.org
>                         <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>
>
>
>                             # HG changeset patch
>                             # User Pierre-Yves David
>                             <pierre-yves.david@octobus.net
>                             <mailto:pierre-yves.david@octobus.net>>
>                             # Date 1493894621 -7200
>                             #      Thu May 04 12:43:41 2017 +0200
>                             # Node ID
>                             2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
>                             # Parent
>                             1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
>                             # EXP-Topic changegroup.cleanup
>                             # Available At
>                             https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>                             <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_repo_users_marmoute_mercurial_&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=Z_zpDW7T_nj2SSo3wEjhKP-brPY2ADWcYxKbzEEGLRE&s=0JDb_ZGCm6BNaMnooV8bgQveC_poVDKoEdXVyHMo7ZY&e=>
>                             #              hg pull
>                             https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>                             <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_repo_users_marmoute_mercurial_&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=Z_zpDW7T_nj2SSo3wEjhKP-brPY2ADWcYxKbzEEGLRE&s=0JDb_ZGCm6BNaMnooV8bgQveC_poVDKoEdXVyHMo7ZY&e=>
>                             -r
>                             2f51cfeac5bc
>                             changegroup: deprecate 'getlocalchangroup' (API)
>
>                             We have 'getchangegroup' with a shorter name
>                             for the exactly same
>                             feature. Now
>                             that all users are gone we can formally
>                             deprecate it.
>
>
>
>
>                         We usually just delete methods that we no longer
>                         use internally. Why
>                         not do that here?
>
>
>
>
>                     When function are likely to be used by extension we
>                     try to avoid dropping
>                     them and we deprecated them for a version. This
>                     helps third party
>                     extensions
>                     to smoothly detect the API changes (usually through
>                     test run) and
>                     smoothly
>                     upgrade their code over the version cycle.
>
>
>
>                 In this case I suspect it won't help many of them
>                 because I probably
>                 broke most or all anyway with
>                 https://www.mercurial-scm.org/repo/hg/rev/282b288aa20c
>                 <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_repo_hg_rev_282b288aa20c&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=Z_zpDW7T_nj2SSo3wEjhKP-brPY2ADWcYxKbzEEGLRE&s=U3i9CQImO2kFvpDtk8ZXAjlE18k-V4aNzoFlnx_5l8E&e=>.
>                 I thought we
>                 were okay with that kind of changes. Are you saying I
>                 should have
>                 instead added duplicate methods with the new signatures
>                 and only
>                 deprecated the existing methods?
>
>
>
>             Generating changegroup is a quite core feature in Mercurial.
>             I suspect their
>             are extension out there using these and I tried to be
>             careful. That is a gut
>             feeling. I'm did not looked at actual data. (that gut
>             feeling proved right
>             for "vfs", but might be wrong here)
>
>             (so, yes I would have been more careful with your API change
>             too)
>
>
>         Well, given that my patch probably broke most of those extensions, I
>         don't see much reason for your series to be more careful. But
>         there's
>         little harm in doing it, so I'll queue it once tests have run etc.
>
>
>     I agree doing both approach at the same time is sub-optimal :-)
>     We should either restore some of the (deprecated) compatibility
>     dropped your series or remove the function deprecated in mine.
>
>     I've digged up a bit and  already found a couple of Facebook
>     extensions using getlocalchangegroup so I think it is safer to
>     assume there are external users.
>
>
> I don't care much. Durham, do you guys care enough so we should back out
> the change. I don't care enough to clean up that I'm going to bother
> with deprecation.

In the few places we use it, we're just using it to get a changegroup 
generator to pass to bundlepart(data=...) or 
util.chunkbuffer(...).read().  As long as there is some equivalent 
function that takes a repo and 'outgoing' and returns that same thing, 
then we don't mind it being removed.
via Mercurial-devel - May 8, 2017, 4:15 p.m.
On Mon, May 8, 2017 at 9:10 AM, Durham Goode <durham@fb.com> wrote:
>
>
> On 5/6/17 6:55 AM, Martin von Zweigbergk wrote:
>>
>>
>>
>> On May 5, 2017 23:57, "Pierre-Yves David"
>> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>> wrote:
>>
>>
>>
>>     On 05/05/2017 09:16 PM, Martin von Zweigbergk wrote:
>>
>>         On Fri, May 5, 2017 at 11:44 AM, Pierre-Yves David
>>         <pierre-yves.david@ens-lyon.org
>>         <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>>
>>
>>
>>             On 05/05/2017 08:38 PM, Martin von Zweigbergk wrote:
>>
>>
>>                 On Fri, May 5, 2017 at 11:35 AM, Pierre-Yves David
>>                 <pierre-yves.david@ens-lyon.org
>>                 <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>>
>>
>>
>>
>>                     On 05/05/2017 06:41 PM, Martin von Zweigbergk wrote:
>>
>>
>>
>>                         On Thu, May 4, 2017 at 11:26 PM, Pierre-Yves David
>>                         <pierre-yves.david@ens-lyon.org
>>                         <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>>
>>
>>
>>                             # HG changeset patch
>>                             # User Pierre-Yves David
>>                             <pierre-yves.david@octobus.net
>>                             <mailto:pierre-yves.david@octobus.net>>
>>                             # Date 1493894621 -7200
>>                             #      Thu May 04 12:43:41 2017 +0200
>>                             # Node ID
>>                             2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
>>                             # Parent
>>                             1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
>>                             # EXP-Topic changegroup.cleanup
>>                             # Available At
>>
>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>
>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_repo_users_marmoute_mercurial_&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=Z_zpDW7T_nj2SSo3wEjhKP-brPY2ADWcYxKbzEEGLRE&s=0JDb_ZGCm6BNaMnooV8bgQveC_poVDKoEdXVyHMo7ZY&e=>
>>                             #              hg pull
>>
>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>
>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_repo_users_marmoute_mercurial_&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=Z_zpDW7T_nj2SSo3wEjhKP-brPY2ADWcYxKbzEEGLRE&s=0JDb_ZGCm6BNaMnooV8bgQveC_poVDKoEdXVyHMo7ZY&e=>
>>                             -r
>>                             2f51cfeac5bc
>>                             changegroup: deprecate 'getlocalchangroup'
>> (API)
>>
>>                             We have 'getchangegroup' with a shorter name
>>                             for the exactly same
>>                             feature. Now
>>                             that all users are gone we can formally
>>                             deprecate it.
>>
>>
>>
>>
>>                         We usually just delete methods that we no longer
>>                         use internally. Why
>>                         not do that here?
>>
>>
>>
>>
>>                     When function are likely to be used by extension we
>>                     try to avoid dropping
>>                     them and we deprecated them for a version. This
>>                     helps third party
>>                     extensions
>>                     to smoothly detect the API changes (usually through
>>                     test run) and
>>                     smoothly
>>                     upgrade their code over the version cycle.
>>
>>
>>
>>                 In this case I suspect it won't help many of them
>>                 because I probably
>>                 broke most or all anyway with
>>                 https://www.mercurial-scm.org/repo/hg/rev/282b288aa20c
>>
>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_repo_hg_rev_282b288aa20c&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=Z_zpDW7T_nj2SSo3wEjhKP-brPY2ADWcYxKbzEEGLRE&s=U3i9CQImO2kFvpDtk8ZXAjlE18k-V4aNzoFlnx_5l8E&e=>.
>>
>>                 I thought we
>>                 were okay with that kind of changes. Are you saying I
>>                 should have
>>                 instead added duplicate methods with the new signatures
>>                 and only
>>                 deprecated the existing methods?
>>
>>
>>
>>             Generating changegroup is a quite core feature in Mercurial.
>>             I suspect their
>>             are extension out there using these and I tried to be
>>             careful. That is a gut
>>             feeling. I'm did not looked at actual data. (that gut
>>             feeling proved right
>>             for "vfs", but might be wrong here)
>>
>>             (so, yes I would have been more careful with your API change
>>             too)
>>
>>
>>         Well, given that my patch probably broke most of those extensions,
>> I
>>         don't see much reason for your series to be more careful. But
>>         there's
>>         little harm in doing it, so I'll queue it once tests have run etc.
>>
>>
>>     I agree doing both approach at the same time is sub-optimal :-)
>>     We should either restore some of the (deprecated) compatibility
>>     dropped your series or remove the function deprecated in mine.
>>
>>     I've digged up a bit and  already found a couple of Facebook
>>     extensions using getlocalchangegroup so I think it is safer to
>>     assume there are external users.
>>
>>
>> I don't care much. Durham, do you guys care enough so we should back out
>> the change. I don't care enough to clean up that I'm going to bother
>> with deprecation.
>
>
> In the few places we use it, we're just using it to get a changegroup
> generator to pass to bundlepart(data=...) or util.chunkbuffer(...).read().
> As long as there is some equivalent function that takes a repo and
> 'outgoing' and returns that same thing, then we don't mind it being removed.

Great. Let's just leave it as is then (i.e. with the deprecwarn that's
unlikely to fire because the signature will probably mismatch anyway).
It doesn't seem worth discussing more.
Pierre-Yves David - May 8, 2017, 6:02 p.m.
On 05/06/2017 03:55 PM, Martin von Zweigbergk wrote:
>
> On May 5, 2017 23:57, "Pierre-Yves David"
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>  […]
>> I've digged up a bit and  already found a couple of Facebook
>> extensions using getlocalchangegroup so I think it is safer to
>> assume there are external users.
>
> I don't care much. Durham, do you guys care enough so we should back out
> the change. I don't care enough to clean up that I'm going to bother
> with deprecation.

The point is not so much about how Facebook will adapt to the API 
change. They have a good continuous test coverage and many dev about to 
do the adjustment. My point is more than if we can find usage of that 
API in Facebook extension, it is a good hint that other extensions might 
do the same too.

It is not too important, I'll do a cleanup path on this in the next 
couple of weeks and align things one way or another (dropping the old 
one or adding some deprecwarn).

On 05/06/2017 08:20 PM, Gregory Szorc wrote:
 > The changegroup APIs are horrible and are low-level. I favor deleting
 > legacy ones that are no longer used in core. Extensions can test for
 > function presence at run-time.

Yep! We are dropping that function and cleaning the API in all cases. We 
are just discussing keeping it around for a couple of month to help with 
third party extension.
Pierre-Yves David - May 8, 2017, 6:09 p.m.
On 05/08/2017 06:15 PM, Martin von Zweigbergk wrote:
> On Mon, May 8, 2017 at 9:10 AM, Durham Goode <durham@fb.com> wrote:
>>
>>
>> On 5/6/17 6:55 AM, Martin von Zweigbergk wrote:
>>>
>>>
>>>
>>> On May 5, 2017 23:57, "Pierre-Yves David"
>>> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>>> wrote:
>>>
>>>
>>>
>>>     On 05/05/2017 09:16 PM, Martin von Zweigbergk wrote:
>>>
>>>         On Fri, May 5, 2017 at 11:44 AM, Pierre-Yves David
>>>         <pierre-yves.david@ens-lyon.org
>>>         <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>>>
>>>
>>>
>>>             On 05/05/2017 08:38 PM, Martin von Zweigbergk wrote:
>>>
>>>
>>>                 On Fri, May 5, 2017 at 11:35 AM, Pierre-Yves David
>>>                 <pierre-yves.david@ens-lyon.org
>>>                 <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>>>
>>>
>>>
>>>
>>>                     On 05/05/2017 06:41 PM, Martin von Zweigbergk wrote:
>>>
>>>
>>>
>>>                         On Thu, May 4, 2017 at 11:26 PM, Pierre-Yves David
>>>                         <pierre-yves.david@ens-lyon.org
>>>                         <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>>>
>>>
>>>
>>>                             # HG changeset patch
>>>                             # User Pierre-Yves David
>>>                             <pierre-yves.david@octobus.net
>>>                             <mailto:pierre-yves.david@octobus.net>>
>>>                             # Date 1493894621 -7200
>>>                             #      Thu May 04 12:43:41 2017 +0200
>>>                             # Node ID
>>>                             2f51cfeac5bcf8ee266a6fada56517d5d44d9b6b
>>>                             # Parent
>>>                             1e8427b7d0b9ce66c5ba34c2cdb64821ff909267
>>>                             # EXP-Topic changegroup.cleanup
>>>                             # Available At
>>>
>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>>
>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_repo_users_marmoute_mercurial_&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=Z_zpDW7T_nj2SSo3wEjhKP-brPY2ADWcYxKbzEEGLRE&s=0JDb_ZGCm6BNaMnooV8bgQveC_poVDKoEdXVyHMo7ZY&e=>
>>>                             #              hg pull
>>>
>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>>
>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_repo_users_marmoute_mercurial_&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=Z_zpDW7T_nj2SSo3wEjhKP-brPY2ADWcYxKbzEEGLRE&s=0JDb_ZGCm6BNaMnooV8bgQveC_poVDKoEdXVyHMo7ZY&e=>
>>>                             -r
>>>                             2f51cfeac5bc
>>>                             changegroup: deprecate 'getlocalchangroup'
>>> (API)
>>>
>>>                             We have 'getchangegroup' with a shorter name
>>>                             for the exactly same
>>>                             feature. Now
>>>                             that all users are gone we can formally
>>>                             deprecate it.
>>>
>>>
>>>
>>>
>>>                         We usually just delete methods that we no longer
>>>                         use internally. Why
>>>                         not do that here?
>>>
>>>
>>>
>>>
>>>                     When function are likely to be used by extension we
>>>                     try to avoid dropping
>>>                     them and we deprecated them for a version. This
>>>                     helps third party
>>>                     extensions
>>>                     to smoothly detect the API changes (usually through
>>>                     test run) and
>>>                     smoothly
>>>                     upgrade their code over the version cycle.
>>>
>>>
>>>
>>>                 In this case I suspect it won't help many of them
>>>                 because I probably
>>>                 broke most or all anyway with
>>>                 https://www.mercurial-scm.org/repo/hg/rev/282b288aa20c
>>>
>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_repo_hg_rev_282b288aa20c&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=Z_zpDW7T_nj2SSo3wEjhKP-brPY2ADWcYxKbzEEGLRE&s=U3i9CQImO2kFvpDtk8ZXAjlE18k-V4aNzoFlnx_5l8E&e=>.
>>>
>>>                 I thought we
>>>                 were okay with that kind of changes. Are you saying I
>>>                 should have
>>>                 instead added duplicate methods with the new signatures
>>>                 and only
>>>                 deprecated the existing methods?
>>>
>>>
>>>
>>>             Generating changegroup is a quite core feature in Mercurial.
>>>             I suspect their
>>>             are extension out there using these and I tried to be
>>>             careful. That is a gut
>>>             feeling. I'm did not looked at actual data. (that gut
>>>             feeling proved right
>>>             for "vfs", but might be wrong here)
>>>
>>>             (so, yes I would have been more careful with your API change
>>>             too)
>>>
>>>
>>>         Well, given that my patch probably broke most of those extensions,
>>> I
>>>         don't see much reason for your series to be more careful. But
>>>         there's
>>>         little harm in doing it, so I'll queue it once tests have run etc.
>>>
>>>
>>>     I agree doing both approach at the same time is sub-optimal :-)
>>>     We should either restore some of the (deprecated) compatibility
>>>     dropped your series or remove the function deprecated in mine.
>>>
>>>     I've digged up a bit and  already found a couple of Facebook
>>>     extensions using getlocalchangegroup so I think it is safer to
>>>     assume there are external users.
>>>
>>>
>>> I don't care much. Durham, do you guys care enough so we should back out
>>> the change. I don't care enough to clean up that I'm going to bother
>>> with deprecation.
>>
>> In the few places we use it, we're just using it to get a changegroup
>> generator to pass to bundlepart(data=...) or util.chunkbuffer(...).read().
>> As long as there is some equivalent function that takes a repo and
>> 'outgoing' and returns that same thing, then we don't mind it being removed.
>
> Great. Let's just leave it as is then (i.e. with the deprecwarn that's
> unlikely to fire because the signature will probably mismatch anyway).
> It doesn't seem worth discussing more.

(my reply[1] written AFK went out before I saw this)

As pointed in my other reply, I'll do a cleanup pass on this soon™. I 
agree we should not sink more time discussing this.

Patch

diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
--- a/mercurial/changegroup.py
+++ b/mercurial/changegroup.py
@@ -976,8 +976,10 @@  def getchangegroup(repo, source, outgoin
     bundler = getbundler(version, repo)
     return getsubset(repo, outgoing, bundler, source)
 
-# deprecate me once all users are gone
-getlocalchangegroup = getchangegroup
+def getlocalchangegroup(repo, *args, **kwargs):
+    repo.ui.deprecwarn('getlocalchangegroup is deprecated, use getchangegroup',
+                       '4.3')
+    return getchangegroup(repo, *args, **kwargs)
 
 def changegroup(repo, basenodes, source):
     # to avoid a race we use changegroupsubset() (issue1320)