[PATCH 4 of 4 v5] changegroup: enable version 03 on 'lfs' requirement

Pierre-Yves David pierre-yves.david at ens-lyon.org
Mon Dec 19 01:55:48 EST 2016



On 12/16/2016 09:46 PM, Augie Fackler wrote:
> On Wed, Dec 14, 2016 at 11:58:10AM +0000, Remi Chaintron wrote:
>> # HG changeset patch
>> # User Remi Chaintron <remi at fb.com>
>> # Date 1481639041 0
>> #      Tue Dec 13 14:24:01 2016 +0000
>> # Branch stable
>> # Node ID 8120778471f01b41d01f8d13b4176b32d2b8482c
>> # Parent  217f7db4dae3309bec78e27c85cc90e924b109be
>> changegroup: enable version 03 on 'lfs' requirement
>
> This also needs to disable 01 and 02 if it spots lfs (or treemanifests
> for that matter).

TLDR; there is issue, but not were we though they were. However, we can 
just drop that patch and have 'lfs' set 'experimental.changegroup3' in 
'reposetup'



Looks at bit deeper, the change regarding changegroup version is 
actually fineĀ¹. The part that only adds '03' is 
'supportedincomingversions'. receiving '01' and '02' version is fine as 
long as their don't contains 'lfs' entries. But the peer generating the 
bundle is responsible to ensure it emit correct ones.

The second side of the coin 'supportedoutgoingversions' is already 
discarding '01' and '02' and is re-used for on disk bundle.

(to be honest, there is some confusing bit in the core implementation 
here, I've sent a small series to clarify that)


However, I've found another problematic bug in last part of the 
changesets. adding 'lfs' to "supportedformats" will make core pretend it 
can handle 'lfs' respository while it can't. Only the 'lfs' extension can.

Overall having to mention 'lfs' anywhere in core look suspicious and I 
don't think it is necessary. We already have "experimental.changegroup3" 
config. The 'lfs' extensions just need to set it to True at reposetup time.



(also, it would be nice if someone (ideally from Google) could look into 
making cg3 enable by default).

(And ideally, we would have a low level change in changegroup building 
that make sure we are not bundling anything with a flag in a changegroup 
version that don't support it. )

>> Version 03 is required by the `lfs` extension in order to send flags for
>> revlog objects over the wire.
>>
>> diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
>> --- a/mercurial/changegroup.py
>> +++ b/mercurial/changegroup.py
>> @@ -879,14 +879,16 @@
>>  # Changegroup versions that can be applied to the repo
>>  def supportedincomingversions(repo):
>>      versions = allsupportedversions(repo.ui)
>> -    if 'treemanifest' in repo.requirements:
>> +    if ('treemanifest' in repo.requirements or
>> +        'lfs' in repo.requirements):
>>          versions.add('03')
>>      return versions
>>
>>  # Changegroup versions that can be created from the repo
>>  def supportedoutgoingversions(repo):
>>      versions = allsupportedversions(repo.ui)
>> -    if 'treemanifest' in repo.requirements:
>> +    if ('treemanifest' in repo.requirements or
>> +        'lfs' in repo.requirements):
>>          # Versions 01 and 02 support only flat manifests and it's just too
>>          # expensive to convert between the flat manifest and tree manifest on
>>          # the fly. Since tree manifests are hashed differently, all of history
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -239,7 +239,7 @@
>>  class localrepository(object):
>>
>>      supportedformats = set(('revlogv1', 'generaldelta', 'treemanifest',
>> -                            'manifestv2'))
>> +                            'manifestv2', 'lfs'))
>>      _basesupported = supportedformats | set(('store', 'fncache', 'shared',
>>                                               'dotencode'))
>>      openerreqs = set(('revlogv1', 'generaldelta', 'treemanifest', 'manifestv2'))

This is the bit of code I was talking about earlier. If I understand 
this correctly, this new code seems to indicate core support 'lfs' 
natively, which is not the case.

So this code will let a vanillia mercurial open 'lfs' repository without 
actual 'lfs' support.

Cheers,

-- 
Pierre-Yves David

[1] mea culpa for making the initial flagging as that "error" in an IRC 
conversation with Augie. Sorry about that false positive.


More information about the Mercurial-devel mailing list