Patchwork [STABLE] largefiles: access to specific fields only if largefiles enabled (issue4547)

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Feb. 25, 2015, 9:06 p.m.
Message ID <8049e948fa335fb6cfcc.1424898406@feefifofum>
Download mbox | patch
Permalink /patch/7830/
State Accepted
Commit d414c28db84d592f9f27c683414bc02d18a5e344
Headers show

Comments

Katsunori FUJIWARA - Feb. 25, 2015, 9:06 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1424898219 -32400
#      Thu Feb 26 06:03:39 2015 +0900
# Branch stable
# Node ID 8049e948fa335fb6cfcce88197dc9963cf4698b7
# Parent  942a5a34b2d0611ab284380fbe45b9bb1897af98
largefiles: access to specific fields only if largefiles enabled (issue4547)

Even if largefiles extension is enabled in a repository, "repo"
object, which isn't "largefiles.reposetup()"-ed, is passed to
overridden functions in the cases below unexpectedly, because
extensions are enabled for each repositories strictly.

  (1) clone without -U:
  (2) pull with -U:
  (3) pull with --rebase:

    combination of "enabled@src", "disabled@dst" and
    "not-required@src" cause this situation.

       largefiles     requirement
    @src     @dst     @src            result
    -------- -------- --------------- --------------------
    enabled  disabled not-required    aborted unexpectedly
                      required        requirement error (intentional)
    -------- -------- --------------- --------------------
    enabled  enabled  *               success
    -------- -------- --------------- --------------------
    disabled enabled  *               success (only for "pull")
    -------- -------- --------------- --------------------
    disabled disabled not-required    success
                      required        requirement error (intentional)
    -------- -------- --------------- --------------------

  (4) update/revert with a subrepo disabling largefiles

In these cases, overridden functions cause accessing to largefiles
specific fields of not "largefiles.reposetup()"-ed "repo" object, and
execution is aborted.

  - (1), (2), (4) cause accessing to "_lfstatuswriters" in
    "getstatuswriter()" invoked via "updatelfiles()"

  - (3) causes accessing to "_lfcommithooks" in "overriderebase()"

For safe accessing to these fields, this patch examines whether passed
"repo" object is "largefiles.reposetup()"-ed or not before accessing
to them.

This patch chooses examining existence of newly introduced
"_largefilesenabled" instead of "_lfcommithooks" and
"_lfstatuswriters" directly, because the former is better name for the
generic "largefiles is enabled in this repo" mark than the latter.

In the future, all other overridden functions should avoid largefiles
specific processing for efficiency, and "_largefilesenabled" is better
also for such purpose.

BTW, "lfstatus" can't be used for such purpose, because some code
paths set it forcibly regardless of existence of it in specified
"repo" object.
Matt Mackall - March 2, 2015, 6:51 a.m.
On Thu, 2015-02-26 at 06:06 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1424898219 -32400
> #      Thu Feb 26 06:03:39 2015 +0900
> # Branch stable
> # Node ID 8049e948fa335fb6cfcce88197dc9963cf4698b7
> # Parent  942a5a34b2d0611ab284380fbe45b9bb1897af98
> largefiles: access to specific fields only if largefiles enabled (issue4547)

Queued for stable, thanks.
Matt Mackall - March 2, 2015, 7:30 a.m.
On Thu, 2015-02-26 at 06:06 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1424898219 -32400
> #      Thu Feb 26 06:03:39 2015 +0900
> # Branch stable
> # Node ID 8049e948fa335fb6cfcce88197dc9963cf4698b7
> # Parent  942a5a34b2d0611ab284380fbe45b9bb1897af98
> largefiles: access to specific fields only if largefiles enabled (issue4547)

When I merged this into stable, I got excessive recursion depth in
test-largefiles-misc.t. I ended up pushing it anyway because I'd already
pushed out stable and was unlikely to figure out what was going on on my
own. Please take a look.
Katsunori FUJIWARA - March 2, 2015, 11:01 a.m.
At Mon, 02 Mar 2015 01:30:47 -0600,
Matt Mackall wrote:
> 
> On Thu, 2015-02-26 at 06:06 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1424898219 -32400
> > #      Thu Feb 26 06:03:39 2015 +0900
> > # Branch stable
> > # Node ID 8049e948fa335fb6cfcce88197dc9963cf4698b7
> > # Parent  942a5a34b2d0611ab284380fbe45b9bb1897af98
> > largefiles: access to specific fields only if largefiles enabled (issue4547)
> 
> When I merged this into stable, I got excessive recursion depth in
> test-largefiles-misc.t. I ended up pushing it anyway because I'd already
> pushed out stable and was unlikely to figure out what was going on on my
> own. Please take a look.

Until my this patch, there is no test with subrepos in which
largefiles is disabled locally.

In such case, "openlfdirstate" invocation in "overriderevert"
introduced by 79c2c29c71ae causes "excessive recursion depth", because
"openlfdirstate" invokes already overridden "scmutil.match" when there
is no "lfdirstate" file: "disabling largefiles" also satisfies this
condition.

  --- a/hgext/largefiles/overrides.py
  +++ b/hgext/largefiles/overrides.py
  @@ -716,10 +716,17 @@ def overriderevert(orig, ui, repo, *pats
                   default='relpath'):
               match = oldmatch(ctx, pats, opts, globbed, default)
               m = copy.copy(match)
  +
  +            # revert supports recursing into subrepos, and though largefiles
  +            # currently doesn't work correctly in that case, this match is
  +            # called, so the lfdirstate above may not be the correct one for
  +            # this invocation of match.
  +            lfdirstate = lfutil.openlfdirstate(ctx._repo.ui, ctx._repo)
  +
               def tostandin(f):
                   if lfutil.standin(f) in ctx:
                       return lfutil.standin(f)
  -                elif lfutil.standin(f) in repo[None]:
  +                elif lfutil.standin(f) in repo[None] or lfdirstate[f] == 'r':
                       return None
                   return f
               m._files = [tostandin(f) for f in m._files]


IMHO, we should do:

(1) avoid invocation of "overriding 'match' function" in repos
    disabling largefiles for efficiency

  ====================
  diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
  --- a/hgext/largefiles/overrides.py
  +++ b/hgext/largefiles/overrides.py
  @@ -61,8 +61,13 @@ def installmatchfn(f):
       '''monkey patch the scmutil module with a custom match function.
       Warning: it is monkey patching the _module_ on runtime! Not thread safe!''$
       oldmatch = scmutil.match
  -    setattr(f, 'oldmatch', oldmatch)
  -    scmutil.match = f
  +    def switchmatch(ctx, *args, **kwargs):
  +        if not util.safehasattr(ctx._repo, '_largefilesenabled'):
  +            return oldmatch(ctx, *args, **kwargs)
  +        else:
  +            return f(ctx, *args, **kwargs)
  +    setattr(switchmatch, 'oldmatch', oldmatch)
  +    scmutil.match = switchmatch
       return oldmatch
  
   def restorematchfn():
  ====================

(2) avoid accidental recursive invocation

  Even if largefiles is enabled in the repository, unexpected missing
  lfdirstate file also causes "getstandinmatcher()" invocation in
  "openlfdirstate()".

  To avoid accidental recursive invocation in such case, we have to
  pass "False" as "create" to "openlfdirstate()".

  ====================
  diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
  --- a/hgext/largefiles/overrides.py
  +++ b/hgext/largefiles/overrides.py
  @@ -721,7 +721,7 @@ def overriderevert(orig, ui, repo, *pats
               # currently doesn't work correctly in that case, this match is
               # called, so the lfdirstate above may not be the correct one for
               # this invocation of match.
  -            lfdirstate = lfutil.openlfdirstate(ctx._repo.ui, ctx._repo)
  +            lfdirstate = lfutil.openlfdirstate(ctx._repo.ui, ctx._repo, False)
  
               def tostandin(f):
                   if lfutil.standin(f) in ctx:
  ====================


> -- 
> Mathematics is the supreme nostalgia of our time.
> 
> 
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Katsunori FUJIWARA - March 2, 2015, 1:42 p.m.
At Mon, 02 Mar 2015 20:01:19 +0900,
FUJIWARA Katsunori wrote:
> 
> At Mon, 02 Mar 2015 01:30:47 -0600,
> Matt Mackall wrote:
> > 
> > On Thu, 2015-02-26 at 06:06 +0900, FUJIWARA Katsunori wrote:
> > > # HG changeset patch
> > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > # Date 1424898219 -32400
> > > #      Thu Feb 26 06:03:39 2015 +0900
> > > # Branch stable
> > > # Node ID 8049e948fa335fb6cfcce88197dc9963cf4698b7
> > > # Parent  942a5a34b2d0611ab284380fbe45b9bb1897af98
> > > largefiles: access to specific fields only if largefiles enabled (issue4547)
> > 
> > When I merged this into stable, I got excessive recursion depth in
> > test-largefiles-misc.t. I ended up pushing it anyway because I'd already
> > pushed out stable and was unlikely to figure out what was going on on my
> > own. Please take a look.
> 
> Until my this patch, there is no test with subrepos in which
> largefiles is disabled locally.
> 
> In such case, "openlfdirstate" invocation in "overriderevert"
> introduced by 79c2c29c71ae causes "excessive recursion depth", because
> "openlfdirstate" invokes already overridden "scmutil.match" when there
> is no "lfdirstate" file: "disabling largefiles" also satisfies this
> condition.
> 
>   --- a/hgext/largefiles/overrides.py
>   +++ b/hgext/largefiles/overrides.py
>   @@ -716,10 +716,17 @@ def overriderevert(orig, ui, repo, *pats
>                    default='relpath'):
>                match = oldmatch(ctx, pats, opts, globbed, default)
>                m = copy.copy(match)
>   +
>   +            # revert supports recursing into subrepos, and though largefiles
>   +            # currently doesn't work correctly in that case, this match is
>   +            # called, so the lfdirstate above may not be the correct one for
>   +            # this invocation of match.
>   +            lfdirstate = lfutil.openlfdirstate(ctx._repo.ui, ctx._repo)
>   +
>                def tostandin(f):
>                    if lfutil.standin(f) in ctx:
>                        return lfutil.standin(f)
>   -                elif lfutil.standin(f) in repo[None]:
>   +                elif lfutil.standin(f) in repo[None] or lfdirstate[f] == 'r':
>                        return None
>                    return f
>                m._files = [tostandin(f) for f in m._files]
> 
> 
> IMHO, we should do:
> 
> (1) avoid invocation of "overriding 'match' function" in repos
>     disabling largefiles for efficiency
> 
>   ====================
>   diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
>   --- a/hgext/largefiles/overrides.py
>   +++ b/hgext/largefiles/overrides.py
>   @@ -61,8 +61,13 @@ def installmatchfn(f):
>        '''monkey patch the scmutil module with a custom match function.
>        Warning: it is monkey patching the _module_ on runtime! Not thread safe!''$
>        oldmatch = scmutil.match
>   -    setattr(f, 'oldmatch', oldmatch)
>   -    scmutil.match = f
>   +    def switchmatch(ctx, *args, **kwargs):
>   +        if not util.safehasattr(ctx._repo, '_largefilesenabled'):
>   +            return oldmatch(ctx, *args, **kwargs)
>   +        else:
>   +            return f(ctx, *args, **kwargs)
>   +    setattr(switchmatch, 'oldmatch', oldmatch)
>   +    scmutil.match = switchmatch
>        return oldmatch
>   
>    def restorematchfn():
>   ====================

I'm also working to override "cmdutil.revert" instead of
"commands.revert" for backout, histedit, shelve and
hgsubrepo.filerevert invoking the former directly.

And I noticed that it causes nested wrapping match function
unexpectedly in nested subrepositories.

I'll post changes (1) above or so, after more investigation around
"cmdutil.revert" and "installmatchfn".

On the other hand, I'll post changes (2) below soon, to pass
test-largefiles-misc.t successfully.


> (2) avoid accidental recursive invocation
> 
>   Even if largefiles is enabled in the repository, unexpected missing
>   lfdirstate file also causes "getstandinmatcher()" invocation in
>   "openlfdirstate()".
> 
>   To avoid accidental recursive invocation in such case, we have to
>   pass "False" as "create" to "openlfdirstate()".
> 
>   ====================
>   diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
>   --- a/hgext/largefiles/overrides.py
>   +++ b/hgext/largefiles/overrides.py
>   @@ -721,7 +721,7 @@ def overriderevert(orig, ui, repo, *pats
>                # currently doesn't work correctly in that case, this match is
>                # called, so the lfdirstate above may not be the correct one for
>                # this invocation of match.
>   -            lfdirstate = lfutil.openlfdirstate(ctx._repo.ui, ctx._repo)
>   +            lfdirstate = lfutil.openlfdirstate(ctx._repo.ui, ctx._repo, False)
>   
>                def tostandin(f):
>                    if lfutil.standin(f) in ctx:
>   ====================
> 
> 
> > -- 
> > Mathematics is the supreme nostalgia of our time.
> > 
> > 
> > 
> 
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Matt Harbison - March 3, 2015, 1:58 a.m.
On Mon, 02 Mar 2015 08:42:32 -0500, FUJIWARA Katsunori  
<foozy@lares.dti.ne.jp> wrote:

> At Mon, 02 Mar 2015 20:01:19 +0900,
> FUJIWARA Katsunori wrote:
>>
>> At Mon, 02 Mar 2015 01:30:47 -0600,
>> Matt Mackall wrote:
>> >
>> > On Thu, 2015-02-26 at 06:06 +0900, FUJIWARA Katsunori wrote:
>> > > # HG changeset patch
>> > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>> > > # Date 1424898219 -32400
>> > > #      Thu Feb 26 06:03:39 2015 +0900
>> > > # Branch stable
>> > > # Node ID 8049e948fa335fb6cfcce88197dc9963cf4698b7
>> > > # Parent  942a5a34b2d0611ab284380fbe45b9bb1897af98
>> > > largefiles: access to specific fields only if largefiles enabled  
>> (issue4547)
>> >
>> > When I merged this into stable, I got excessive recursion depth in
>> > test-largefiles-misc.t. I ended up pushing it anyway because I'd  
>> already
>> > pushed out stable and was unlikely to figure out what was going on on  
>> my
>> > own. Please take a look.
>>
>> Until my this patch, there is no test with subrepos in which
>> largefiles is disabled locally.
>>
>> In such case, "openlfdirstate" invocation in "overriderevert"
>> introduced by 79c2c29c71ae causes "excessive recursion depth", because
>> "openlfdirstate" invokes already overridden "scmutil.match" when there
>> is no "lfdirstate" file: "disabling largefiles" also satisfies this
>> condition.
>>
>>   --- a/hgext/largefiles/overrides.py
>>   +++ b/hgext/largefiles/overrides.py
>>   @@ -716,10 +716,17 @@ def overriderevert(orig, ui, repo, *pats
>>                    default='relpath'):
>>                match = oldmatch(ctx, pats, opts, globbed, default)
>>                m = copy.copy(match)
>>   +
>>   +            # revert supports recursing into subrepos, and though  
>> largefiles
>>   +            # currently doesn't work correctly in that case, this  
>> match is
>>   +            # called, so the lfdirstate above may not be the correct  
>> one for
>>   +            # this invocation of match.
>>   +            lfdirstate = lfutil.openlfdirstate(ctx._repo.ui,  
>> ctx._repo)
>>   +
>>                def tostandin(f):
>>                    if lfutil.standin(f) in ctx:
>>                        return lfutil.standin(f)
>>   -                elif lfutil.standin(f) in repo[None]:
>>   +                elif lfutil.standin(f) in repo[None] or  
>> lfdirstate[f] == 'r':
>>                        return None
>>                    return f
>>                m._files = [tostandin(f) for f in m._files]
>>
>>
>> IMHO, we should do:
>>
>> (1) avoid invocation of "overriding 'match' function" in repos
>>     disabling largefiles for efficiency

Is it just the monkey patched scmutil.match() that needs to be avoided?   
It looks like revert and copy are the only things left that use it.  I'm  
not sure how to avoid the largefile tweaking done to the other matchers  
that are passed in via narrowmatcher() (though maybe that's harmless).

>>   ====================
>>   diff --git a/hgext/largefiles/overrides.py  
>> b/hgext/largefiles/overrides.py
>>   --- a/hgext/largefiles/overrides.py
>>   +++ b/hgext/largefiles/overrides.py
>>   @@ -61,8 +61,13 @@ def installmatchfn(f):
>>        '''monkey patch the scmutil module with a custom match function.
>>        Warning: it is monkey patching the _module_ on runtime! Not  
>> thread safe!''$
>>        oldmatch = scmutil.match
>>   -    setattr(f, 'oldmatch', oldmatch)
>>   -    scmutil.match = f
>>   +    def switchmatch(ctx, *args, **kwargs):
>>   +        if not util.safehasattr(ctx._repo, '_largefilesenabled'):
>>   +            return oldmatch(ctx, *args, **kwargs)
>>   +        else:
>>   +            return f(ctx, *args, **kwargs)
>>   +    setattr(switchmatch, 'oldmatch', oldmatch)
>>   +    scmutil.match = switchmatch
>>        return oldmatch
>>
>>    def restorematchfn():
>>   ====================
>
> I'm also working to override "cmdutil.revert" instead of
> "commands.revert" for backout, histedit, shelve and
> hgsubrepo.filerevert invoking the former directly.

+1 on that, and thanks for mentioning it, because I was toying with that  
too.  I think everything I did so far has been landed, except an  
unpolished change to pass matchers instead of pattern lists to  
cmdutil.revert().  If you need it, I can toss it out on the ML.  There's  
some other stuff I'm working on in the next few days anyway, and I don't  
want to disrupt you from fixing this.

I'm a bit baffled by the revert implementation.  It just clicked now that  
subrepo.filerevert --all hardcodes 'set:modified()' solely to generate the  
backup files.  I still don't understand why it is changing the subrepo  
state in hgsubrepo.revert().  It seems it should only be touching files,  
not potentially changing branches, etc.  Not that any of this is specific  
to largefiles, but maybe it needs consideration when changing pats ->  
matcher on cmdutil.revert()?

--Matt

> And I noticed that it causes nested wrapping match function
> unexpectedly in nested subrepositories.
>
> I'll post changes (1) above or so, after more investigation around
> "cmdutil.revert" and "installmatchfn".
>
> On the other hand, I'll post changes (2) below soon, to pass
> test-largefiles-misc.t successfully.
>
>
>> (2) avoid accidental recursive invocation
>>
>>   Even if largefiles is enabled in the repository, unexpected missing
>>   lfdirstate file also causes "getstandinmatcher()" invocation in
>>   "openlfdirstate()".
>>
>>   To avoid accidental recursive invocation in such case, we have to
>>   pass "False" as "create" to "openlfdirstate()".
>>
>>   ====================
>>   diff --git a/hgext/largefiles/overrides.py  
>> b/hgext/largefiles/overrides.py
>>   --- a/hgext/largefiles/overrides.py
>>   +++ b/hgext/largefiles/overrides.py
>>   @@ -721,7 +721,7 @@ def overriderevert(orig, ui, repo, *pats
>>                # currently doesn't work correctly in that case, this  
>> match is
>>                # called, so the lfdirstate above may not be the correct  
>> one for
>>                # this invocation of match.
>>   -            lfdirstate = lfutil.openlfdirstate(ctx._repo.ui,  
>> ctx._repo)
>>   +            lfdirstate = lfutil.openlfdirstate(ctx._repo.ui,  
>> ctx._repo, False)
>>
>>                def tostandin(f):
>>                    if lfutil.standin(f) in ctx:
>>   ====================
>>
>>
>> > --
>> > Mathematics is the supreme nostalgia of our time.
>> >
>> >
>> >
>>
>> ----------------------------------------------------------------------
>> [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel
>
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Katsunori FUJIWARA - March 5, 2015, 1:16 p.m.
At Mon, 02 Mar 2015 20:58:10 -0500,
Matt Harbison wrote:
> 
> On Mon, 02 Mar 2015 08:42:32 -0500, FUJIWARA Katsunori  
> <foozy@lares.dti.ne.jp> wrote:
> 
> > At Mon, 02 Mar 2015 20:01:19 +0900,
> > FUJIWARA Katsunori wrote:
> >>
> >> At Mon, 02 Mar 2015 01:30:47 -0600,
> >> Matt Mackall wrote:
> >> >
> >> > On Thu, 2015-02-26 at 06:06 +0900, FUJIWARA Katsunori wrote:
> >> > > # HG changeset patch
> >> > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> >> > > # Date 1424898219 -32400
> >> > > #      Thu Feb 26 06:03:39 2015 +0900
> >> > > # Branch stable
> >> > > # Node ID 8049e948fa335fb6cfcce88197dc9963cf4698b7
> >> > > # Parent  942a5a34b2d0611ab284380fbe45b9bb1897af98
> >> > > largefiles: access to specific fields only if largefiles enabled  
> >> (issue4547)
> >> >
> >> > When I merged this into stable, I got excessive recursion depth in
> >> > test-largefiles-misc.t. I ended up pushing it anyway because I'd  
> >> already
> >> > pushed out stable and was unlikely to figure out what was going on on  
> >> my
> >> > own. Please take a look.
> >>
> >> Until my this patch, there is no test with subrepos in which
> >> largefiles is disabled locally.
> >>
> >> In such case, "openlfdirstate" invocation in "overriderevert"
> >> introduced by 79c2c29c71ae causes "excessive recursion depth", because
> >> "openlfdirstate" invokes already overridden "scmutil.match" when there
> >> is no "lfdirstate" file: "disabling largefiles" also satisfies this
> >> condition.
> >>
> >>   --- a/hgext/largefiles/overrides.py
> >>   +++ b/hgext/largefiles/overrides.py
> >>   @@ -716,10 +716,17 @@ def overriderevert(orig, ui, repo, *pats
> >>                    default='relpath'):
> >>                match = oldmatch(ctx, pats, opts, globbed, default)
> >>                m = copy.copy(match)
> >>   +
> >>   +            # revert supports recursing into subrepos, and though  
> >> largefiles
> >>   +            # currently doesn't work correctly in that case, this  
> >> match is
> >>   +            # called, so the lfdirstate above may not be the correct  
> >> one for
> >>   +            # this invocation of match.
> >>   +            lfdirstate = lfutil.openlfdirstate(ctx._repo.ui,  
> >> ctx._repo)
> >>   +
> >>                def tostandin(f):
> >>                    if lfutil.standin(f) in ctx:
> >>                        return lfutil.standin(f)
> >>   -                elif lfutil.standin(f) in repo[None]:
> >>   +                elif lfutil.standin(f) in repo[None] or  
> >> lfdirstate[f] == 'r':
> >>                        return None
> >>                    return f
> >>                m._files = [tostandin(f) for f in m._files]
> >>
> >>
> >> IMHO, we should do:
> >>
> >> (1) avoid invocation of "overriding 'match' function" in repos
> >>     disabling largefiles for efficiency
> 
> Is it just the monkey patched scmutil.match() that needs to be avoided?   
> It looks like revert and copy are the only things left that use it.  I'm  
> not sure how to avoid the largefile tweaking done to the other matchers  
> that are passed in via narrowmatcher() (though maybe that's harmless).

Sorry for late response.

"(1)" was proposed just for fixing "excessive recursion depth".

I think that overriding functions other than one for "scmutil.match"
should be also avoided in repos disabling largefiles for efficiency in
the future.


> > I'm also working to override "cmdutil.revert" instead of
> > "commands.revert" for backout, histedit, shelve and
> > hgsubrepo.filerevert invoking the former directly.
> 
> +1 on that, and thanks for mentioning it, because I was toying with that  
> too.  I think everything I did so far has been landed, except an  
> unpolished change to pass matchers instead of pattern lists to  
> cmdutil.revert().  If you need it, I can toss it out on the ML.  There's  
> some other stuff I'm working on in the next few days anyway, and I don't  
> want to disrupt you from fixing this.

In fact, I have to finish some other patches for largefiles before
working for "overriderevert". So, you would have enough time to work
for it :-)


> I'm a bit baffled by the revert implementation.  It just clicked now that  
> subrepo.filerevert --all hardcodes 'set:modified()' solely to generate the  
> backup files.

Me too, about "set:modified()" for "--all" !


> I still don't understand why it is changing the subrepo  
> state in hgsubrepo.revert().  It seems it should only be touching files,  
> not potentially changing branches, etc. 

Current revert implementation of hgsubrepo/gitsubrepo seems to support
not "revert specified files in subrepo to specified revision" but only
"revert whole subrepo to specified revision", even though "revert" of
them takes "pats" argument.

It may be reason why "cmdutil.revert" still doesn't take "match"
argument instead of patterns: partial reverting should want
"cmdutil.revert" to take "match" argument for easily narrowing.


> Not that any of this is specific  
> to largefiles, but maybe it needs consideration when changing pats ->  
> matcher on cmdutil.revert()?
>
> --Matt
> 
> > And I noticed that it causes nested wrapping match function
> > unexpectedly in nested subrepositories.
> >
> > I'll post changes (1) above or so, after more investigation around
> > "cmdutil.revert" and "installmatchfn".
> >
> > On the other hand, I'll post changes (2) below soon, to pass
> > test-largefiles-misc.t successfully.
> >
> >
> >> (2) avoid accidental recursive invocation
> >>
> >>   Even if largefiles is enabled in the repository, unexpected missing
> >>   lfdirstate file also causes "getstandinmatcher()" invocation in
> >>   "openlfdirstate()".
> >>
> >>   To avoid accidental recursive invocation in such case, we have to
> >>   pass "False" as "create" to "openlfdirstate()".
> >>
> >>   ====================
> >>   diff --git a/hgext/largefiles/overrides.py  
> >> b/hgext/largefiles/overrides.py
> >>   --- a/hgext/largefiles/overrides.py
> >>   +++ b/hgext/largefiles/overrides.py
> >>   @@ -721,7 +721,7 @@ def overriderevert(orig, ui, repo, *pats
> >>                # currently doesn't work correctly in that case, this  
> >> match is
> >>                # called, so the lfdirstate above may not be the correct  
> >> one for
> >>                # this invocation of match.
> >>   -            lfdirstate = lfutil.openlfdirstate(ctx._repo.ui,  
> >> ctx._repo)
> >>   +            lfdirstate = lfutil.openlfdirstate(ctx._repo.ui,  
> >> ctx._repo, False)
> >>
> >>                def tostandin(f):
> >>                    if lfutil.standin(f) in ctx:
> >>   ====================

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
--- a/hgext/largefiles/lfutil.py
+++ b/hgext/largefiles/lfutil.py
@@ -577,7 +577,7 @@ 
     Otherwise, this returns the function to always write out (or
     ignore if ``not forcibly``) status.
     '''
-    if forcibly is None:
+    if forcibly is None and util.safehasattr(repo, '_largefilesenabled'):
         return repo._lfstatuswriters[-1]
     else:
         if forcibly:
diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -832,6 +832,9 @@ 
     return result
 
 def overriderebase(orig, ui, repo, **opts):
+    if not util.safehasattr(repo, '_largefilesenabled'):
+        return orig(ui, repo, **opts)
+
     resuming = opts.get('continue')
     repo._lfcommithooks.append(lfutil.automatedcommithook(resuming))
     repo._lfstatuswriters.append(lambda *msg, **opts: None)
diff --git a/hgext/largefiles/reposetup.py b/hgext/largefiles/reposetup.py
--- a/hgext/largefiles/reposetup.py
+++ b/hgext/largefiles/reposetup.py
@@ -24,6 +24,9 @@ 
         return
 
     class lfilesrepo(repo.__class__):
+        # the mark to examine whether "repo" object enables largefiles or not
+        _largefilesenabled = True
+
         lfstatus = False
         def status_nolfiles(self, *args, **kwargs):
             return super(lfilesrepo, self).status(*args, **kwargs)
diff --git a/tests/test-largefiles-misc.t b/tests/test-largefiles-misc.t
--- a/tests/test-largefiles-misc.t
+++ b/tests/test-largefiles-misc.t
@@ -907,6 +907,77 @@ 
   $ "$TESTDIR/killdaemons.py" $DAEMON_PIDS
 #endif
 
+Test overridden functions work correctly even for repos disabling
+largefiles (issue4547)
+
+  $ hg showconfig extensions | grep largefiles
+  extensions.largefiles=!
+
+(test updating implied by clone)
+
+  $ hg init enabled-but-no-largefiles
+  $ echo normal1 > enabled-but-no-largefiles/normal1
+  $ hg -R enabled-but-no-largefiles add enabled-but-no-largefiles/normal1
+  $ hg -R enabled-but-no-largefiles commit -m '#0@enabled-but-no-largefiles'
+  Invoking status precommit hook
+  A normal1
+  $ cat >> enabled-but-no-largefiles/.hg/hgrc <<EOF
+  > [extensions]
+  > # enable locally
+  > largefiles=
+  > EOF
+  $ hg clone -q enabled-but-no-largefiles no-largefiles
+
+(test rebasing implied by pull: precommit while rebasing unexpectedly
+shows "normal3" as "?", because lfdirstate isn't yet written out at
+that time)
+
+  $ echo normal2 > enabled-but-no-largefiles/normal2
+  $ hg -R enabled-but-no-largefiles add enabled-but-no-largefiles/normal2
+  $ hg -R enabled-but-no-largefiles commit -m '#1@enabled-but-no-largefiles'
+  Invoking status precommit hook
+  A normal2
+
+  $ echo normal3 > no-largefiles/normal3
+  $ hg -R no-largefiles add no-largefiles/normal3
+  $ hg -R no-largefiles commit -m '#1@no-largefiles'
+  Invoking status precommit hook
+  A normal3
+
+  $ hg -R no-largefiles -q pull --rebase
+  Invoking status precommit hook
+  ? normal3
+
+(test reverting)
+
+  $ hg init subrepo-root
+  $ cat >> subrepo-root/.hg/hgrc <<EOF
+  > [extensions]
+  > # enable locally
+  > largefiles=
+  > EOF
+  $ echo large > subrepo-root/large
+  $ hg -R subrepo-root add --large subrepo-root/large
+  $ hg clone -q no-largefiles subrepo-root/no-largefiles
+  $ cat > subrepo-root/.hgsub <<EOF
+  > no-largefiles = no-largefiles
+  > EOF
+  $ hg -R subrepo-root add subrepo-root/.hgsub
+  $ hg -R subrepo-root commit -m '#0'
+  Invoking status precommit hook
+  A .hgsub
+  A large
+  ? .hgsubstate
+  $ echo dirty >> subrepo-root/large
+  $ echo dirty >> subrepo-root/no-largefiles/normal1
+  $ hg -R subrepo-root status -S
+  M large
+  M no-largefiles/normal1
+  $ hg -R subrepo-root revert --all
+  reverting subrepo-root/.hglf/large (glob)
+  reverting subrepo no-largefiles
+  reverting subrepo-root/no-largefiles/normal1 (glob)
+
   $ cd ..