Patchwork [1,of,2,STABLE] largefiles: update largefiles in a subrepo when updating the parent

login
register
mail settings
Submitter Matt Harbison
Date Jan. 4, 2013, 2:42 a.m.
Message ID <5da07921976bf249809c.1357267346@Envy>
Download mbox | patch
Permalink /patch/388/
State Superseded
Headers show

Comments

Matt Harbison - Jan. 4, 2013, 2:42 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison at yahoo.com>
# Date 1357181569 18000
# Branch stable
# Node ID 5da07921976bf249809cc0934c80c52025cf5e09
# Parent  cd64a8a2d0da5178de30105ff69ccff32a73a536
largefiles: update largefiles in a subrepo when updating the parent

Starting with 17c030014ddf, largefiles in a subrepo weren't checked out when
updating the parent repo.  That cset changed hg.clean() and hg.update() to call
the newly introduced hg.updaterepo(), and also changed hgsubrepo.get() to call
updaterepo() directly.  Since only clean and update were overridden, largefiles
in subrepos were no longer being updated.

The original issue was noticed by Francois Tr?ton <ftreton at dxo.com> [1].

Unfortunately, this changes the relative order of printing the largefile changes
and the normal updated/merged/removed/unresolved line, though the actual
operations remain in the same order.  The alternative is to conditionally call
hg.clean() or hg.update() in hgsubrepo.get().  However, making the override as
narrow as possible seems better, and avoids the potential trap of something in
the future calling an updaterepo() that isn't overridden.

[1] http://www.selenic.com/pipermail/mercurial/2013-January/044829.html
Mads Kiilerich - Jan. 4, 2013, 3:28 p.m.
On 01/04/2013 03:42 AM, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison at yahoo.com>
> # Date 1357181569 18000
> # Branch stable
> # Node ID 5da07921976bf249809cc0934c80c52025cf5e09
> # Parent  cd64a8a2d0da5178de30105ff69ccff32a73a536
> largefiles: update largefiles in a subrepo when updating the parent
>
> Starting with 17c030014ddf, largefiles in a subrepo weren't checked out when
> updating the parent repo.  That cset changed hg.clean() and hg.update() to call
> the newly introduced hg.updaterepo(), and also changed hgsubrepo.get() to call
> updaterepo() directly.  Since only clean and update were overridden, largefiles
> in subrepos were no longer being updated.
>
> The original issue was noticed by Francois Tr?ton <ftreton at dxo.com> [1].
>
> Unfortunately, this changes the relative order of printing the largefile changes
> and the normal updated/merged/removed/unresolved line, though the actual
> operations remain in the same order.  The alternative is to conditionally call
> hg.clean() or hg.update() in hgsubrepo.get().  However, making the override as
> narrow as possible seems better, and avoids the potential trap of something in
> the future calling an updaterepo() that isn't overridden.

hg.updaterepo doesn't show any status info. It is not so elegant that 
largefiles override it with something that displays status info.

I think I would prefer a simple wrapping of hgsubrepo.get ... if that 
would work. Especially for stable.

(I don't know/remember why hgsubrepo.get ignores the result from 
hg.updaterepo and apparently doesn't show any update status. That seems 
like a bug.)

Do the test in patch 2 cover this change? Then it should be in the same 
patch. (And if not: There should be some test coverage for this.)

/Mads
Matt Harbison - Jan. 6, 2013, 6:42 a.m.
Mads Kiilerich wrote:
> On 01/04/2013 03:42 AM, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison at yahoo.com>
>> # Date 1357181569 18000
>> # Branch stable
>> # Node ID 5da07921976bf249809cc0934c80c52025cf5e09
>> # Parent cd64a8a2d0da5178de30105ff69ccff32a73a536
>> largefiles: update largefiles in a subrepo when updating the parent
>>
>> Starting with 17c030014ddf, largefiles in a subrepo weren't checked
>> out when
>> updating the parent repo. That cset changed hg.clean() and hg.update()
>> to call
>> the newly introduced hg.updaterepo(), and also changed hgsubrepo.get()
>> to call
>> updaterepo() directly. Since only clean and update were overridden,
>> largefiles
>> in subrepos were no longer being updated.
>>
>> The original issue was noticed by Francois Tr?ton <ftreton at dxo.com> [1].
>>
>> Unfortunately, this changes the relative order of printing the
>> largefile changes
>> and the normal updated/merged/removed/unresolved line, though the actual
>> operations remain in the same order. The alternative is to
>> conditionally call
>> hg.clean() or hg.update() in hgsubrepo.get(). However, making the
>> override as
>> narrow as possible seems better, and avoids the potential trap of
>> something in
>> the future calling an updaterepo() that isn't overridden.
>
> hg.updaterepo doesn't show any status info. It is not so elegant that
> largefiles override it with something that displays status info.

Good catch.

> I think I would prefer a simple wrapping of hgsubrepo.get ... if that
> would work. Especially for stable.

I don't think it would.  I tried putting hg.clean() back like it was in 
hgsubrepo, and conditionally call it or hg.update() (both of which are 
already overridden), and I ended up with a bunch of extra lines from 
showstats() in some tests.  I think the problem is update() always 
prints stats, whereas the previous clean() invocation and current 
updaterepo() never do.

If hgsubrepo.get() were overridden, the code from the clean() and 
update() overrides would have to be copy/pasted in, which doesn't feel 
right.  Neither does _not_ calling the base class implementation and 
instead implementing the override in terms of clean and update, because 
then the implementations start to substantively differ.

> (I don't know/remember why hgsubrepo.get ignores the result from
> hg.updaterepo

I always thought it just aborted, but I got that impression from the 
error when the subrepo default path can't be found.  I'll look at this 
when I get a chance, but not in this series.

 > and apparently doesn't show any update status. That seems
> like a bug.)

I thought it was strange too, but it explicitly didn't when it 
originally called hg.clean().  Based on the extra messages I mentioned, 
this probably warrants digging into further.

> Do the test in patch 2 cover this change? Then it should be in the same
> patch. (And if not: There should be some test coverage for this.)

Not specifically.  There is a test in the largefile.t (admittedly lost 
in the noise) for this specific case.  Since the change that introduced 
this bug introduced the concept of updating subrepos (instead of just 
cleaning them), I wanted to make sure there was test coverage for what 
happens if the largefile subrepo is dirty vs clean.  I thought about 
folding them because they're closely related, but it's a test that 
should have been in the original change (or maybe even before).  I guess 
I could expand the commit message, or if you think they should be 
folded, that's probably fine too.

I've got a slightly tuned series for default that passes the tests 
cleanly that I'll submit tomorrow after I review it with fresh eyes. 
Basically, it adds a preliminary patch to move the rest of the content 
of clean() and update() into updaterepo() (they are substantially the 
same anyway), with a parameter to control whether it prints stats.  The 
override in largefiles then restores the status quo ante.  At that point 
we can look at flipping the switch to print the stats for subrepos 
(which can't currently be done with updaterepo() never printing).

--Matt

Patch

diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -638,18 +638,20 @@ 
     finally:
         wlock.release()
 
-def hgupdate(orig, repo, node):
-    # Only call updatelfiles the standins that have changed to save time
-    oldstandins = lfutil.getstandinsstate(repo)
-    result = orig(repo, node)
-    newstandins = lfutil.getstandinsstate(repo)
-    filelist = lfutil.getlfilestoupdate(oldstandins, newstandins)
+def hgupdaterepo(orig, repo, node, overwrite):
+    if not overwrite:
+        # Only call updatelfiles on the standins that have changed to save time
+        oldstandins = lfutil.getstandinsstate(repo)
+
+    result = orig(repo, node, overwrite)
+    filelist = None
+
+    if not overwrite:
+        newstandins = lfutil.getstandinsstate(repo)
+        filelist = lfutil.getlfilestoupdate(oldstandins, newstandins)
+
     lfcommands.updatelfiles(repo.ui, repo, filelist=filelist, printmessage=True)
-    return result
 
-def hgclean(orig, repo, node, show_stats=True):
-    result = orig(repo, node, show_stats)
-    lfcommands.updatelfiles(repo.ui, repo)
     return result
 
 def hgmerge(orig, repo, node, force=None, remind=True):
diff --git a/hgext/largefiles/uisetup.py b/hgext/largefiles/uisetup.py
--- a/hgext/largefiles/uisetup.py
+++ b/hgext/largefiles/uisetup.py
@@ -104,11 +104,7 @@ 
     entry = extensions.wrapfunction(commands, 'revert',
                                     overrides.overriderevert)
 
-    # clone uses hg._update instead of hg.update even though they are the
-    # same function... so wrap both of them)
-    extensions.wrapfunction(hg, 'update', overrides.hgupdate)
-    extensions.wrapfunction(hg, '_update', overrides.hgupdate)
-    extensions.wrapfunction(hg, 'clean', overrides.hgclean)
+    extensions.wrapfunction(hg, 'updaterepo', overrides.hgupdaterepo)
     extensions.wrapfunction(hg, 'merge', overrides.hgmerge)
 
     extensions.wrapfunction(archival, 'archive', overrides.overridearchive)
diff --git a/tests/test-issue3084.t b/tests/test-issue3084.t
--- a/tests/test-issue3084.t
+++ b/tests/test-issue3084.t
@@ -16,9 +16,9 @@ 
   $ hg commit -m "Add foo as a largefile"
 
   $ hg update -r 0
-  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
   getting changed largefiles
   0 largefiles updated, 1 removed
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
 
   $ echo "normal" > foo
   $ hg add foo
diff --git a/tests/test-largefiles-cache.t b/tests/test-largefiles-cache.t
--- a/tests/test-largefiles-cache.t
+++ b/tests/test-largefiles-cache.t
@@ -45,29 +45,29 @@ 
 "missing"(!) file.
 
   $ hg update
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   large: can't get file locally
   (no default or default-push path set in hgrc)
   0 largefiles updated, 0 removed
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg status
   ! large
 
 Update working directory to null: this cleanup .hg/largefiles/dirstate
 
   $ hg update null
-  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
   getting changed largefiles
   0 largefiles updated, 0 removed
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
 
 Update working directory to tip, again.
 
   $ hg update
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   large: can't get file locally
   (no default or default-push path set in hgrc)
   0 largefiles updated, 0 removed
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg status
   ! large
   $ cd ..
diff --git a/tests/test-largefiles-small-disk.t b/tests/test-largefiles-small-disk.t
--- a/tests/test-largefiles-small-disk.t
+++ b/tests/test-largefiles-small-disk.t
@@ -57,7 +57,6 @@ 
   adding file changes
   added 1 changesets with 1 changes to 1 files
   updating to branch default
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   abort: No space left on device
   [255]
diff --git a/tests/test-largefiles.t b/tests/test-largefiles.t
--- a/tests/test-largefiles.t
+++ b/tests/test-largefiles.t
@@ -485,9 +485,9 @@ 
 Test addremove with -R
 
   $ hg up -C
-  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   1 largefiles updated, 0 removed
+  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ rm normal3
   $ rm sub/large4
   $ echo "testing addremove with patterns" > testaddremove.dat
@@ -503,9 +503,9 @@ 
 Test 3364
   $ hg clone . ../addrm
   updating to branch default
-  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   3 largefiles updated, 0 removed
+  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cd ../addrm
   $ cat >> .hg/hgrc <<EOF
   > [hooks]
@@ -583,9 +583,9 @@ 
   C sub2/large6
   C sub2/large7
   $ hg up -C '.^'
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   0 largefiles updated, 0 removed
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg remove large
   $ hg addremove --traceback
   $ hg ci -m "removed large"
@@ -694,9 +694,9 @@ 
 
   $ hg clone . ../b
   updating to branch default
-  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   3 largefiles updated, 0 removed
+  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cd ../b
   $ hg log --template '{rev}:{node|short}  {desc|firstline}\n'
   7:daea875e9014  add/edit more largefiles
@@ -724,9 +724,9 @@ 
   adding file changes
   added 4 changesets with 10 changes to 4 files
   updating to branch default
-  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   2 largefiles updated, 0 removed
+  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cd c
   $ hg log --template '{rev}:{node|short}  {desc|firstline}\n'
   3:9e8fbc4bce62  copy files
@@ -746,9 +746,9 @@ 
 tests update).
 
   $ hg update -r 1
-  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   1 largefiles updated, 0 removed
+  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cat large1
   large11
   $ cat sub/large2
@@ -760,17 +760,17 @@ 
   $ rm "${USERCACHE}"/*
   $ hg clone --all-largefiles a a-backup
   updating to branch default
-  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   3 largefiles updated, 0 removed
+  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   8 additional largefiles cached
 
   $ rm "${USERCACHE}"/*
   $ hg clone --all-largefiles -u 0 a a-clone0
   updating to branch default
-  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   2 largefiles updated, 0 removed
+  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
   9 additional largefiles cached
   $ hg -R a-clone0 sum
   parent: 0:30d30fe6a5be 
@@ -782,9 +782,9 @@ 
   $ rm "${USERCACHE}"/*
   $ hg clone --all-largefiles -u 1 a a-clone1
   updating to branch default
-  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   2 largefiles updated, 0 removed
+  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
   8 additional largefiles cached
   $ hg -R a-clone1 sum
   parent: 1:ce8896473775 
@@ -807,9 +807,9 @@ 
   $ hg clone ../a
   destination directory: a
   updating to branch default
-  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   3 largefiles updated, 0 removed
+  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cd ..
 
 Ensure base clone command argument validation
@@ -833,9 +833,9 @@ 
   adding file changes
   added 2 changesets with 8 changes to 4 files
   updating to branch default
-  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   2 largefiles updated, 0 removed
+  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ rm "${USERCACHE}"/*
   $ cd a-backup
   $ hg pull --all-largefiles --config paths.default-push=bogus/path
@@ -856,9 +856,9 @@ 
 
   $ hg clone a d
   updating to branch default
-  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   3 largefiles updated, 0 removed
+  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cd b
   $ echo large4-modified > sub/large4
   $ echo normal3-modified > normal3
@@ -876,9 +876,9 @@ 
   $ cd ..
   $ hg clone d e
   updating to branch default
-  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   3 largefiles updated, 0 removed
+  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cd d
 
 More rebase testing, but also test that the largefiles are downloaded from
@@ -1008,9 +1008,9 @@ 
 "update --clean" leaves correct largefiles in working copy.
 
   $ hg update --clean
-  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   1 largefiles updated, 0 removed
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cat normal3
   normal3-modified
   $ cat sub/normal4
@@ -1024,13 +1024,13 @@ 
 
 Now "update check" is happy.
   $ hg update --check 8
-  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   1 largefiles updated, 0 removed
+  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg update --check
-  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   1 largefiles updated, 0 removed
+  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
 Test removing empty largefiles directories on update
   $ test -d sub2 && echo "sub2 exists"
@@ -1119,14 +1119,14 @@ 
   adding file changes
   added 8 changesets with 24 changes to 10 files
   updating to branch default
-  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   3 largefiles updated, 0 removed
+  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg clone temp f
   updating to branch default
-  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   3 largefiles updated, 0 removed
+  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
 # Delete the largefiles in the largefiles system cache so that we have an
 # opportunity to test that caching after a pull works.
   $ rm "${USERCACHE}"/*
@@ -1207,9 +1207,9 @@ 
   adding file changes
   added 9 changesets with 26 changes to 10 files
   updating to branch default
-  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   3 largefiles updated, 0 removed
+  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cd g
   $ hg transplant -s ../d 598410d3eb9a
   searching for changes
@@ -1493,9 +1493,9 @@ 
   adding file changes
   added 1 changesets with 1 changes to 1 files
   updating to branch default
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   1 largefiles updated, 0 removed
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cd ..
   $ chmod -R u+w alice/pubrepo
   $ HOME="$ORIGHOME"
@@ -1665,6 +1665,18 @@ 
   lf_subrepo_archive/subrepo/large.txt
   lf_subrepo_archive/subrepo/normal.txt
 
+Test that largefiles in subrepos are checked out
+
+  $ hg clone . ../update_on_clone
+  updating to branch default
+  cloning subrepo subrepo from $TESTTMP/statusmatch/subrepo
+  getting changed largefiles
+  1 largefiles updated, 0 removed
+  getting changed largefiles
+  1 largefiles updated, 0 removed
+  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg -R ../update_on_clone status -S
+
 Test archiving a revision that references a subrepo that is not yet
 cloned (see test-subrepo-recursion.t):
 
diff --git a/tests/test-lfconvert.t b/tests/test-lfconvert.t
--- a/tests/test-lfconvert.t
+++ b/tests/test-lfconvert.t
@@ -78,9 +78,9 @@ 
 "lfconvert" converts content correctly
   $ cd largefiles-repo
   $ hg up
-  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   2 largefiles updated, 0 removed
+  4 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg locate
   .hglf/large
   .hglf/sub/maybelarge.dat
@@ -187,9 +187,9 @@ 
   normal1
   stuff/normal2
   $ hg update
-  3 files updated, 0 files merged, 0 files removed, 0 files unresolved
   getting changed largefiles
   1 largefiles updated, 0 removed
+  3 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cat stuff/normal2
   alsonormal
   blah