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

[Zergling]

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;

#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/34c9f61b/attachment.html>