[PATCH 1 of 2] revset: add "subrepo" revset symbol

[Angel Ezquerra]

On Wed, Mar 28, 2012 at 12:21 AM, Angel Ezquerra
<angel.ezquerra at gmail.com> wrote:
> On Tue, Mar 27, 2012 at 9:49 PM, Matt Mackall <mpm at selenic.com> wrote:
>> On Thu, 2012-03-22 at 23:17 +0100, Angel Ezquerra wrote:
>>> # HG changeset patch
>>> # User Angel Ezquerra <angel.ezquerra at gmail.com>
>>> # Date 1332447135 -3600
>>> # Node ID 698737a841b9d4f28eb6849a836ab90d70fff174
>>> # Parent  54df21ce2c2cc39481697a42f06b5f873f1cc075
>>> revset: add "subrepo" revset symbol
>>>
>>> This new revset symbol finds those revisions in which any subrepo whose path
>>> matches a given glob pattern were modified (e.g. "subrepo(my*repo)").
>>>
>>> If no argument is passed, find those revisions in which any subrepo was
>>> modified.
>>
>> Seems ok.
>>
>>> +def subrepo(repo, subset, x):
>>> +    """``subrepo([path])``
>>> +    Find revisions where the selected subrepo has been modified.
>>> +    With no argument, find revisions where any subrepo has been modified.
>>> +    """
>>> +    # i18n: "subrepo" is a keyword
>>> +    if x:
>>> +        pat = encoding.lower(
>>> +            getstring(x, _("subrepo requires a string or no arguments")))
>>
>> Case mangling = bad.
>
> Matt, thanks for the review. I'll fix that on the next version.
>
>>> +        def matchpat(pat, subs):
>>> +            for sub in subs:
>>> +                if fnmatch.fnmatch(sub, pat):
>>> +                    return True
>>> +            return False
>>
>> That's definitely not the pattern for filename matching in Mercurial. We
>> use our own much more powerful match module:
>>
>> http://www.selenic.com/hg/file/9952ac7e0968/mercurial/revset.py#l298
>
> Thank you for the example. I'll use it and send a new patch.
>
> I actually realized that my patch does not do what the commit message
> says. Rather than showing any revision where a given subrepo was
> modified it shows any revision that has a subrepo matching a given
> pattern.
>
> I wonder if this may come handy, and if so we could keep this (after
> fixing it with your suggestions above), perhaps with a different
> keyword name?
>
> Also I wonder if hassubrepo would not be a better name than 'subrepo',
> since there is already a ' hasfile' which returns any revision
> affecting a given file.
>
>> Mathematics is the supreme nostalgia of our time.
>
> I'm curious. Who is that quote from?
>
> Cheers,
>
> Angel

Looking at the existing keywords in more detail I see that there is a
"file" and a "contains" keyword:
- "contains" seems to do for regular files what my original patch does
for subrepos. That is, it shows the revisions that contain a given
file, even if they do not modify it at all.
- "file" does what my original patch was supposed to do. So it seems
that the keyword name was fine after all.

So I'll resend the patch with the keyword renamed to
'containssubrepo'. Once I get that right I'll tackle the subrepo
keyword, which is a bit more complicated.

Angel