Patchwork [7,of,7] largefiles: use the optional badfn argument when building a matcher

login
register
mail settings
Submitter Matt Harbison
Date June 6, 2015, 3:54 a.m.
Message ID <2e18852e1324a67455c3.1433562866@Envy>
Download mbox | patch
Permalink /patch/9544/
State Accepted
Commit 378a8e700e02794e991d3521423a4f581b635666
Headers show

Comments

Matt Harbison - June 6, 2015, 3:54 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1433559195 14400
#      Fri Jun 05 22:53:15 2015 -0400
# Node ID 2e18852e1324a67455c39a1a39b5a2c266c1d387
# Parent  63faca7efc382bd10477106373596e81b7129576
largefiles: use the optional badfn argument when building a matcher

The monkey patching in cat() can't be fixed, because it still delegates to the
original bad().  Overriding commands.cat() should go away in favor overriding
cmdutil.cat() anyway, and that matcher can be wrapped with matchmod.badmatch().
Martin von Zweigbergk - June 8, 2015, 4:52 p.m.
I've pushed this to the clowncopter, thanks.

There are still 3 cases of ".bad = " in the codebase. Did you intentionally
not update those? Will they be dealt with in another series?

On Fri, Jun 5, 2015 at 9:00 PM Matt Harbison <mharbison72@gmail.com> wrote:

> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1433559195 14400
> #      Fri Jun 05 22:53:15 2015 -0400
> # Node ID 2e18852e1324a67455c39a1a39b5a2c266c1d387
> # Parent  63faca7efc382bd10477106373596e81b7129576
> largefiles: use the optional badfn argument when building a matcher
>
> The monkey patching in cat() can't be fixed, because it still delegates to
> the
> original bad().  Overriding commands.cat() should go away in favor
> overriding
> cmdutil.cat() anyway, and that matcher can be wrapped with
> matchmod.badmatch().
>
> diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
> --- a/hgext/largefiles/lfutil.py
> +++ b/hgext/largefiles/lfutil.py
> @@ -241,16 +241,18 @@
>  def getstandinmatcher(repo, rmatcher=None):
>      '''Return a match object that applies rmatcher to the standin
> directory'''
>      standindir = repo.wjoin(shortname)
> +
> +    # no warnings about missing files or directories
> +    badfn = lambda f, msg: None
> +
>      if rmatcher and not rmatcher.always():
>          pats = [os.path.join(standindir, pat) for pat in rmatcher.files()]
> -        match = scmutil.match(repo[None], pats)
> +        match = scmutil.match(repo[None], pats, badfn=badfn)
>          # if pats is empty, it would incorrectly always match, so clear
> _always
>          match._always = False
>      else:
>          # no patterns: relative to repo root
> -        match = scmutil.match(repo[None], [standindir])
> -    # no warnings about missing files or directories
> -    match.bad = lambda f, msg: None
> +        match = scmutil.match(repo[None], [standindir], badfn=badfn)
>      return match
>
>  def composestandinmatcher(repo, rmatcher):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Matt Harbison - June 9, 2015, 12:35 a.m.
On Mon, 08 Jun 2015 12:52:07 -0400, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> I've pushed this to the clowncopter, thanks.
>
> There are still 3 cases of ".bad = " in the codebase. Did you  
> intentionally
> not update those? Will they be dealt with in another series?

Thanks for raising this.  I couldn't explain it in patches unrelated to  
these uses.

It was intentional for different reasons:

- largefiles/overrides.py: explained below

- context.py: super._matchstatus() creates the matcher, and then the  
override patches it.  So we would have to add a badfn to this signature,  
default it to None, and no caller other than the override should ever use  
it.  Then, does the overriding method need to change for consistency with  
what it is overriding, even though it shouldn't be used by a caller?  Not  
sure if it is worth it, especially since the unpatched matcher isn't  
available anywhere.

- localrepo.py: commit optionally takes in a matcher, and creates one  
locally if None is provided.  So one path could use badmatch(), the other  
could pass a bad() callback (except I didn't think to add it to  
matchmod.always()).  But since this method doesn't use subrepo style  
narrowing and recursion, there aren't any issues that I can see with the  
patching here.  I wasn't sure if it was worth messing with.  (This method  
also patches matcher.explicitdir() in a similar way).

If the goal of this was to check-code ban this idiom, I'm not sure we can  
because matchmod.badmatch() needs to use it.


> On Fri, Jun 5, 2015 at 9:00 PM Matt Harbison <mharbison72@gmail.com>  
> wrote:
>
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1433559195 14400
>> #      Fri Jun 05 22:53:15 2015 -0400
>> # Node ID 2e18852e1324a67455c39a1a39b5a2c266c1d387
>> # Parent  63faca7efc382bd10477106373596e81b7129576
>> largefiles: use the optional badfn argument when building a matcher
>>
>> The monkey patching in cat() can't be fixed, because it still delegates  
>> to
>> the
>> original bad().  Overriding commands.cat() should go away in favor
>> overriding
>> cmdutil.cat() anyway, and that matcher can be wrapped with
>> matchmod.badmatch().
>>
>> diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
>> --- a/hgext/largefiles/lfutil.py
>> +++ b/hgext/largefiles/lfutil.py
>> @@ -241,16 +241,18 @@
>>  def getstandinmatcher(repo, rmatcher=None):
>>      '''Return a match object that applies rmatcher to the standin
>> directory'''
>>      standindir = repo.wjoin(shortname)
>> +
>> +    # no warnings about missing files or directories
>> +    badfn = lambda f, msg: None
>> +
>>      if rmatcher and not rmatcher.always():
>>          pats = [os.path.join(standindir, pat) for pat in  
>> rmatcher.files()]
>> -        match = scmutil.match(repo[None], pats)
>> +        match = scmutil.match(repo[None], pats, badfn=badfn)
>>          # if pats is empty, it would incorrectly always match, so clear
>> _always
>>          match._always = False
>>      else:
>>          # no patterns: relative to repo root
>> -        match = scmutil.match(repo[None], [standindir])
>> -    # no warnings about missing files or directories
>> -    match.bad = lambda f, msg: None
>> +        match = scmutil.match(repo[None], [standindir], badfn=badfn)
>>      return match
>>
>>  def composestandinmatcher(repo, rmatcher):
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> https://selenic.com/mailman/listinfo/mercurial-devel
Martin von Zweigbergk - June 9, 2015, 4:02 a.m.
On Mon, Jun 8, 2015 at 5:35 PM Matt Harbison <mharbison72@gmail.com> wrote:

> On Mon, 08 Jun 2015 12:52:07 -0400, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>
> > I've pushed this to the clowncopter, thanks.
> >
> > There are still 3 cases of ".bad = " in the codebase. Did you
> > intentionally
> > not update those? Will they be dealt with in another series?
>
> Thanks for raising this.  I couldn't explain it in patches unrelated to
> these uses.
>
> It was intentional for different reasons:
>
> - largefiles/overrides.py: explained below
>
> - context.py: super._matchstatus() creates the matcher, and then the
> override patches it.  So we would have to add a badfn to this signature,
> default it to None, and no caller other than the override should ever use
> it.  Then, does the overriding method need to change for consistency with
> what it is overriding, even though it shouldn't be used by a caller?  Not
> sure if it is worth it, especially since the unpatched matcher isn't
> available anywhere.
>
> - localrepo.py: commit optionally takes in a matcher, and creates one
> locally if None is provided.  So one path could use badmatch(), the other
> could pass a bad() callback (except I didn't think to add it to
> matchmod.always()).


Or they could both use badmatch() perhaps, after the None-path has created
an always-matcher. But I don't really care that much. I think it's fine as
it is.


>   But since this method doesn't use subrepo style
> narrowing and recursion, there aren't any issues that I can see with the
> patching here.  I wasn't sure if it was worth messing with.  (This method
> also patches matcher.explicitdir() in a similar way).
>
> If the goal of this was to check-code ban this idiom, I'm not sure we can
> because matchmod.badmatch() needs to use it.
>

That wasn't my goal at least. I'm happy to just hear that you had not
simply missed them. Thanks for the usual detailed explanation!


>
>
> > On Fri, Jun 5, 2015 at 9:00 PM Matt Harbison <mharbison72@gmail.com>
> > wrote:
> >
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1433559195 14400
> >> #      Fri Jun 05 22:53:15 2015 -0400
> >> # Node ID 2e18852e1324a67455c39a1a39b5a2c266c1d387
> >> # Parent  63faca7efc382bd10477106373596e81b7129576
> >> largefiles: use the optional badfn argument when building a matcher
> >>
> >> The monkey patching in cat() can't be fixed, because it still delegates
> >> to
> >> the
> >> original bad().  Overriding commands.cat() should go away in favor
> >> overriding
> >> cmdutil.cat() anyway, and that matcher can be wrapped with
> >> matchmod.badmatch().
> >>
> >> diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
> >> --- a/hgext/largefiles/lfutil.py
> >> +++ b/hgext/largefiles/lfutil.py
> >> @@ -241,16 +241,18 @@
> >>  def getstandinmatcher(repo, rmatcher=None):
> >>      '''Return a match object that applies rmatcher to the standin
> >> directory'''
> >>      standindir = repo.wjoin(shortname)
> >> +
> >> +    # no warnings about missing files or directories
> >> +    badfn = lambda f, msg: None
> >> +
> >>      if rmatcher and not rmatcher.always():
> >>          pats = [os.path.join(standindir, pat) for pat in
> >> rmatcher.files()]
> >> -        match = scmutil.match(repo[None], pats)
> >> +        match = scmutil.match(repo[None], pats, badfn=badfn)
> >>          # if pats is empty, it would incorrectly always match, so clear
> >> _always
> >>          match._always = False
> >>      else:
> >>          # no patterns: relative to repo root
> >> -        match = scmutil.match(repo[None], [standindir])
> >> -    # no warnings about missing files or directories
> >> -    match.bad = lambda f, msg: None
> >> +        match = scmutil.match(repo[None], [standindir], badfn=badfn)
> >>      return match
> >>
> >>  def composestandinmatcher(repo, rmatcher):
> >> _______________________________________________
> >> Mercurial-devel mailing list
> >> Mercurial-devel@selenic.com
> >> https://selenic.com/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
--- a/hgext/largefiles/lfutil.py
+++ b/hgext/largefiles/lfutil.py
@@ -241,16 +241,18 @@ 
 def getstandinmatcher(repo, rmatcher=None):
     '''Return a match object that applies rmatcher to the standin directory'''
     standindir = repo.wjoin(shortname)
+
+    # no warnings about missing files or directories
+    badfn = lambda f, msg: None
+
     if rmatcher and not rmatcher.always():
         pats = [os.path.join(standindir, pat) for pat in rmatcher.files()]
-        match = scmutil.match(repo[None], pats)
+        match = scmutil.match(repo[None], pats, badfn=badfn)
         # if pats is empty, it would incorrectly always match, so clear _always
         match._always = False
     else:
         # no patterns: relative to repo root
-        match = scmutil.match(repo[None], [standindir])
-    # no warnings about missing files or directories
-    match.bad = lambda f, msg: None
+        match = scmutil.match(repo[None], [standindir], badfn=badfn)
     return match
 
 def composestandinmatcher(repo, rmatcher):