| 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
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 >
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
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):