[PATCH 5 of 7] changegroup: deprecate 'getlocalchangroup' (API)
Martin von Zweigbergk
martinvonz at google.com
Mon May 8 12:15:44 EDT 2017
On Mon, May 8, 2017 at 9:10 AM, Durham Goode <durham at 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 at ens-lyon.org <mailto:pierre-yves.david at 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 at ens-lyon.org
>> <mailto:pierre-yves.david at 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 at ens-lyon.org
>> <mailto:pierre-yves.david at 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 at ens-lyon.org
>> <mailto:pierre-yves.david at ens-lyon.org>> wrote:
>>
>>
>>
>> # HG changeset patch
>> # User Pierre-Yves David
>> <pierre-yves.david at octobus.net
>> <mailto:pierre-yves.david at 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.
More information about the Mercurial-devel
mailing list