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

login
register
mail settings
Submitter Remi Chaintron
Date Dec. 14, 2016, 11:58 a.m.
Message ID <8120778471f01b41d01f.1481716690@remi-mbp2>
Download mbox | patch
Permalink /patch/17904/
State Accepted
Headers show

Comments

Remi Chaintron - Dec. 14, 2016, 11:58 a.m.
# HG changeset patch
# User Remi Chaintron <remi@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

Version 03 is required by the `lfs` extension in order to send flags for
revlog objects over the wire.
Augie Fackler - Dec. 16, 2016, 8:46 p.m.
On Wed, Dec 14, 2016 at 11:58:10AM +0000, Remi Chaintron wrote:
> # HG changeset patch
> # User Remi Chaintron <remi@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).

>
> 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'))
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Dec. 19, 2016, 6:55 a.m.
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@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,

Patch

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'))