Patchwork D7714: subrepo: fix a crash when archiving an svn or git subrepo

login
register
mail settings
Submitter phabricator
Date Dec. 24, 2019, 7:04 a.m.
Message ID <differential-rev-PHID-DREV-ydlhdgydswxfk6inj7xx-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/44035/
State Superseded
Headers show

Comments

phabricator - Dec. 24, 2019, 7:04 a.m.
mharbison72 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Only hgsubrepos have a repository attribute.  This is pretty hacky, but probably
  the best we can do on stable.  Pushing the lfstatus check down into the wrapper
  for hgsubrepo (and dropping the check for lfstatus at the top of
  `hgsubrepoarchive()`) resulted in various test failures because:
  
  1. hgsubrepoarchive isn't returning the number of files archived at the bottom, resulting in an error about += NoneType
  2. These copypasta archive wrappers don't use progress bars
  3. Largefiles are *not* currently archived when using extdiff (68822b7cdd01 <https://phab.mercurial-scm.org/rHG68822b7cdd01a84d1c5fa335be492b3706daf510>), but pushing this context manager down into the subrepo resulted in it apparently doing so (as evidenced by progress bars being dropped)
  
  The other uses of `lfstatus()` are not in the substate processing loop, so they
  shouldn't be an issue.
  
  I initially put testcases in this test for largefiles-{on,off}, and it flagged
  a bunch of exit code differences for `cat` and `diff`, so I backed that off.

REPOSITORY
  rHG Mercurial

BRANCH
  stable

REVISION DETAIL
  https://phab.mercurial-scm.org/D7714

AFFECTED FILES
  hgext/largefiles/overrides.py
  tests/test-subrepo-git.t

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: mercurial-devel

Patch

diff --git a/tests/test-subrepo-git.t b/tests/test-subrepo-git.t
--- a/tests/test-subrepo-git.t
+++ b/tests/test-subrepo-git.t
@@ -376,6 +376,15 @@ 
   a
   s/g
 
+  $ hg -R ../tc archive -S ../lf_archive.tgz --prefix '.' \
+  >       --config extensions.largefiles= 2>/dev/null
+  $ tar -tzf ../lf_archive.tgz | sort | grep -v pax_global_header
+  .hg_archival.txt
+  .hgsub
+  .hgsubstate
+  a
+  s/g
+
 create nested repo
 
   $ cd ..
diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -1208,7 +1208,16 @@ 
             sub = ctx.workingsub(subpath)
             submatch = matchmod.subdirmatcher(subpath, match)
             subprefix = prefix + subpath + b'/'
-            with lfstatus(sub._repo):
+
+            # TODO: Only hgsubrepo instances have `_repo`, so figure out how to
+            # infer and possibly set lfstatus in hgsubrepoarchive.  That would
+            # allow only hgsubrepos to set this, instead of the current scheme
+            # where the parent sets this for the child.
+            with (
+                util.safehasattr(sub, '_repo')
+                and lfstatus(sub._repo)
+                or util.nullcontextmanager()
+            ):
                 sub.archive(archiver, subprefix, submatch)
 
     archiver.done()
@@ -1266,7 +1275,15 @@ 
         sub = ctx.workingsub(subpath)
         submatch = matchmod.subdirmatcher(subpath, match)
         subprefix = prefix + subpath + b'/'
-        with lfstatus(sub._repo):
+        # TODO: Only hgsubrepo instances have `_repo`, so figure out how to
+        # infer and possibly set lfstatus at the top of this function.  That
+        # would allow only hgsubrepos to set this, instead of the current scheme
+        # where the parent sets this for the child.
+        with (
+            util.safehasattr(sub, '_repo')
+            and lfstatus(sub._repo)
+            or util.nullcontextmanager()
+        ):
             sub.archive(archiver, subprefix, submatch, decode)