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 <r...@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.
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Reply via email to