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

[Matt Harbison]

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