Patchwork [1,of,2] lfs: respect narrowmatcher when testing to add 'lfs' requirement (issue5794)

login
register
mail settings
Submitter Matt Harbison
Date March 27, 2018, 3:14 a.m.
Message ID <19b73408a61866697920.1522120445@Envy>
Download mbox | patch
Permalink /patch/29894/
State Accepted
Headers show

Comments

Matt Harbison - March 27, 2018, 3:14 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1522117116 14400
#      Mon Mar 26 22:18:36 2018 -0400
# Node ID 19b73408a618666979209b9654182e6fa72364d2
# Parent  aaabd709df720e456d7f93a1c790a0dbed051b38
lfs: respect narrowmatcher when testing to add 'lfs' requirement (issue5794)

There's a similar test in lfs.wrapper.convertsink(), but I didn't update that
because I don't think that the sink repo in a convert can be narrow.

It seems reasonable that a narrow clone of an LFS repo may not necessarily be an
LFS repo.  The only potential issue is that LFS has a hard requirement for
changegroup v3, which that extension enables.  The use of treemanifest will
enable changegroup v3 in narrow clones, because allsupportedversions() in
changegroup.py preserves it when it sees a 'treemanifest' requirement.  But I
don't see where changegroup v3 is enabled for a flat manifest.
Yuya Nishihara - March 27, 2018, 12:05 p.m.
On Mon, 26 Mar 2018 23:14:05 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1522117116 14400
> #      Mon Mar 26 22:18:36 2018 -0400
> # Node ID 19b73408a618666979209b9654182e6fa72364d2
> # Parent  aaabd709df720e456d7f93a1c790a0dbed051b38
> lfs: respect narrowmatcher when testing to add 'lfs' requirement (issue5794)
> 
> There's a similar test in lfs.wrapper.convertsink(), but I didn't update that
> because I don't think that the sink repo in a convert can be narrow.
> 
> It seems reasonable that a narrow clone of an LFS repo may not necessarily be an
> LFS repo.  The only potential issue is that LFS has a hard requirement for
> changegroup v3, which that extension enables.  The use of treemanifest will
> enable changegroup v3 in narrow clones, because allsupportedversions() in
> changegroup.py preserves it when it sees a 'treemanifest' requirement.  But I
> don't see where changegroup v3 is enabled for a flat manifest.
> 
> diff --git a/hgext/lfs/__init__.py b/hgext/lfs/__init__.py
> --- a/hgext/lfs/__init__.py
> +++ b/hgext/lfs/__init__.py
> @@ -226,9 +226,10 @@ def reposetup(ui, repo):
>                      s = repo.set('%n:%n', _bin(kwargs[r'node']), _bin(last))
>                  else:
>                      s = repo.set('%n', _bin(kwargs[r'node']))
> +            match = lambda f: f in ctx and repo.narrowmatch()(f)

It's probably better not to call repo.narrowmatcher() in loop. Whether it
is filecached or not is implementation detail.

> -                if any(ctx[f].islfs() for f in ctx.files() if f in ctx):
> +                if any(ctx[f].islfs() for f in ctx.files() if match(f)):

Patch

diff --git a/hgext/lfs/__init__.py b/hgext/lfs/__init__.py
--- a/hgext/lfs/__init__.py
+++ b/hgext/lfs/__init__.py
@@ -226,9 +226,10 @@  def reposetup(ui, repo):
                     s = repo.set('%n:%n', _bin(kwargs[r'node']), _bin(last))
                 else:
                     s = repo.set('%n', _bin(kwargs[r'node']))
+            match = lambda f: f in ctx and repo.narrowmatch()(f)
             for ctx in s:
                 # TODO: is there a way to just walk the files in the commit?
-                if any(ctx[f].islfs() for f in ctx.files() if f in ctx):
+                if any(ctx[f].islfs() for f in ctx.files() if match(f)):
                     repo.requirements.add('lfs')
                     repo._writerequirements()
                     repo.prepushoutgoinghooks.add('lfs', wrapper.prepush)
diff --git a/tests/test-narrow-commit.t b/tests/test-narrow-commit.t
--- a/tests/test-narrow-commit.t
+++ b/tests/test-narrow-commit.t
@@ -28,7 +28,11 @@  create full repo
 
   $ cd ..
 
-  $ hg clone --narrow ssh://user@dummy/master narrow --include inside
+(The lfs extension does nothing here, but this test ensures that its hook that
+determines whether to add the lfs requirement, respects the narrow boundaries.)
+
+  $ hg --config extensions.lfs= clone --narrow ssh://user@dummy/master narrow \
+  >    --include inside
   requesting all changes
   adding changesets
   adding manifests