[PATCH] cmdutil: extract duplicate definitions of matchessubrepo in files() and remove()

Matt Harbison mharbison72 at gmail.com
Thu Jul 21 22:20:19 EDT 2016


> On Jul 21, 2016, at 7:04 AM, Zergling <zerstroyer at gmail.com> wrote:
> 
> I tried to create a bug and dug a little bit into the code, and although it wrongly goes into the if statement, subdirmatcher seems to perform the same check again anyways. Or did you observe a bug? Here is what i tried: 
> 
> mkdir r && cd r && hg init;
> 
> echo s = s > .hgsub;
> echo sub = sub >> .hgsub;
> 
> hg add .hgsub;
> 
> mkdir s sub && hg init s && hg init sub;
> touch s/a;
> touch sub/a;
> 
> hg -R s add s/a;
> hg -R sub add sub/a;
> 
> hg -R s commit -m "first subrepo first commit";
> hg -R sub commit -m "second subrepo first commit";
> 
> hg commit -m "root repo first commit";
> 
> hg files -S s/a;
> 
> hg remove --verbose s/a; 

I didn't try it recently, but I thought I ran into an issue in the past, which caused me to append the '/' where it is.  That said, I couldn't get it to fail with a quick experiment either.  A better test is the form 'hg files -S s', without naming the file in the subrepo.  I thought that made the subrepo matcher match everything after -S forces it to recurse.   

But it seems to be ok.  So I guess wait until after the freeze, but definitely put the method on the matcher class.


> #Output:
> #s/a
> #removing s/a
> 
> 
> 
>> On Wed, Jul 20, 2016 at 3:09 AM, Matt Harbison <mharbison72 at gmail.com> wrote:
>> (+CC list)
>> 
>> On Tue, 19 Jul 2016 09:13:51 -0400, Augie Fackler <raf at durin42.com> wrote:
>> 
>>>> On Mon, Jul 18, 2016 at 07:56:54PM +0000, Zergling wrote:
>>>> Hm sorry, that was the the wrong patch version. I'm gonna send again with
>>>> whitespace fixed if it's worth it and i guess i should patch against
>>>> default for this.
>>> 
>>> Looks like a good cleanup, I might make matchessubrepo have a leading
>>> _ so it's clear it's module-private (unless you have plans for it
>>> beyond cmdutil).
>>> 
>>> We just entered a code freeze for hg 3.9, so if you could send this on
>>> or after August 1 that'll be best. Thanks!
>> 
>> Actually, I think this might fix a bug.  Note that '/' is appended to
>> subpath before doing the prefix test in the second hunk, but not the
>> third.  Therefore, 'path/to/subrepo' matches 'path/to/subrepo/foo' in the
>> first case, but also matches 'path/to/subrepository/foo' in the second.
>> scmutil.addremove() also has this bit of code, and needs to be fixed I
>> think.  I meant to fix this awhile ago.
>> 
>> Hannes- maybe append '/' to the two cases that don't have it to keep this
>> simple, and submit now (with tests).  Then after the freeze lifts on Aug
>> 1, add the utility routine to the matcher class and use it in these three
>> places.
>> 
>>>> 
>>>> On Sun, Jul 17, 2016 at 7:56 AM, Hannes Oldenburg <
>>>> hannes.christian.oldenburg at gmail.com> wrote:
>>>> 
>>>> > # HG changeset patch
>>>> > # User Hannes Oldenburg zerstroyer at gmail.com
>>>> > # Date 1468660399 0
>>>> > #      Sat Jul 16 09:13:19 2016 +0000
>>>> > # Branch stable
>>>> > # Node ID eb4a974d3a18826632600dabe82c8fe453b65e20
>>>> > # Parent  a7d1532b26a17bbaace43124cd415dcb709b08e2
>>>> > cmdutil: extract duplicate definitions of matchessubrepo in files() and
>>>> > remove()
>>>> >
>>>> > diff -r a7d1532b26a1 -r eb4a974d3a18 mercurial/cmdutil.py
>>>> > --- a/mercurial/cmdutil.py      Sat Jul 02 09:41:40 2016 -0700
>>>> > +++ b/mercurial/cmdutil.py      Sat Jul 16 09:13:19 2016 +0000
>>>> > @@ -2396,6 +2396,10 @@
>>>> >      forgot.extend(f for f in forget if f not in rejected)
>>>> >      return bad, forgot
>>>> >
>>>> > +def matchessubrepo(m,subpath):
>>>> > +    return (m.exact(subpath)
>>>> > +            or any(f.startswith(subpath + '/') for f in m.files()))
>>>> > +
>>>> >  def files(ui, ctx, m, fm, fmt, subrepos):
>>>> >      rev = ctx.rev()
>>>> >      ret = 1
>>>> > @@ -2413,11 +2417,7 @@
>>>> >          ret = 0
>>>> >
>>>> >      for subpath in sorted(ctx.substate):
>>>> > -        def matchessubrepo(subpath):
>>>> > -            return (m.exact(subpath)
>>>> > -                    or any(f.startswith(subpath + '/') for f in
>>>> > m.files()))
>>>> > -
>>>> > -        if subrepos or matchessubrepo(subpath):
>>>> > +        if subrepos or matchessubrepo(m,subpath):
>>>> >              sub = ctx.sub(subpath)
>>>> >              try:
>>>> >                  submatch = matchmod.subdirmatcher(subpath, m)
>>>> > @@ -2448,14 +2448,6 @@
>>>> >      total = len(subs)
>>>> >      count = 0
>>>> >      for subpath in subs:
>>>> > -        def matchessubrepo(matcher, subpath):
>>>> > -            if matcher.exact(subpath):
>>>> > -                return True
>>>> > -            for f in matcher.files():
>>>> > -                if f.startswith(subpath):
>>>> > -                    return True
>>>> > -            return False
>>>> > -
>>>> >          count += 1
>>>> >          if subrepos or matchessubrepo(m, subpath):
>>>> >              ui.progress(_('searching'), count, total=total,
>>>> > unit=_('subrepos'))
>>>> > _______________________________________________
>>>> > Mercurial-devel mailing list
>>>> > Mercurial-devel at mercurial-scm.org
>>>> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>>> >
>>> 
>>>> _______________________________________________
>>>> Mercurial-devel mailing list
>>>> Mercurial-devel at mercurial-scm.org
>>>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel at mercurial-scm.org
>>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20160721/48de96bb/attachment.html>


More information about the Mercurial-devel mailing list