[PATCH 2 of 5] extdata: add revset support for extdata

[Pierre-Yves David]

On 10/03/2016 09:15 PM, Matt Mackall wrote:
> On Tue, 2016-09-27 at 14:41 +0200, Pierre-Yves David wrote:
>>
>> On 09/25/2016 12:43 AM, Matt Mackall wrote:
>>>
>>> On Sat, 2016-09-24 at 00:22 +0200, Pierre-Yves David wrote:
>>>>
>>>>
>>>> On 09/23/2016 07:34 PM, Matt Mackall wrote:
>>>>>
>>>>>
>>>>> On Fri, 2016-09-23 at 19:49 +0900, FUJIWARA Katsunori wrote:
>>>>>>
>>>>>>
>>>>>> At Thu, 22 Sep 2016 13:21:36 -0500,
>>>>>> Matt Mackall wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> # HG changeset patch
>>>>>>> # User Matt Mackall <mpm at selenic.com>
>>>>>>> # Date 1474293900 18000
>>>>>>> #      Mon Sep 19 09:05:00 2016 -0500
>>>>>>> # Node ID 9c8847df32a0c5045e60aded2e03a9c97507f909
>>>>>>> # Parent  19bf2776dfe39befdc479253e1e7d030b41c08f9
>>>>>>> extdata: add revset support for extdata
>>>>>>>
>>>>>>> This inserts extdata into the revset function support. Planned
>>>>>>> extensions of extdata support arguments, so this is the most
>>>>>>> appropriate place for it.
>>>>>>>
>>>>>>> Unfortunately, the registrar framework is not a good fit here.
>>>>>>> First,
>>>>>>> setting an appropriate load point is still an unsolved problem (we
>>>>>>> want the code to live in revset.py, but that module may never be
>>>>>>> loaded).
>>>>>>> Second, registered methods become global and the data sources are
>>>>>>> likely
>>>>>>> to
>>>>>>> be
>>>>>>> repo-specific. This won't work well in a context like hgwebdir.
>>>>>> Is there any reason not to define extdata() revset predicate (or
>>>>>> template function), which requires external data source name like as
>>>>>> extdata('filedata') ? (for convenience ?)
>>>>> It's mostly convenience. But I also plan to add support for arguments.
>>>> I think I really like foozy idea about using a generic 'extdata("key")'
>>>> predicate. That will probably be okay for many case and prevent
>>>> unexpected collision with other revsets. If needed, the user can easily
>>>> define a revset alias for the sources in needs easy access to. As
>>>> configuration of the source is needed anyway, this does seems like a
>>>> bearable burden.
>>>>
>>>> If I remember correctly, it does not seems to have limitation in the
>>>> current implementation of revset that would prevent use to do
>>>> 'extdata("key", arg1, arg2)'
>>> You get to implement this version, because I think it's an awful idea.'ll
>
>> hu? I'm not sure of when the Mercurial review process switched to from
>> "reviewers give feedback to submitter" to "reviewer can update submitter
>> patch them-self if they don't like it". That seems quite the opposite of
>> what you have been teaching us in the past 10 years.
>
> I've always been aware that there's a risk that contributors will walk away if
> I'm too nitpicky with their patches or ask them to make a change that they find
> distasteful. And I've often taken imperfect or incomplete patches because of it.
> Just so long as forward progress is made and no regressions are introduced.
>
> In rare cases, where I find a feature desirable, but can't get a contributor to
> do it to my satisfaction, I've written it myself (merge-tools comes to mind).
> The downside of this approach is that there's no reason for the original
> contributor to feel vested in it further, and I am thus on the hook for
> maintaining it.

I think I'm quite aware of your review policy as I've been going through 
it for the past 5 years, you can be certain it has driven away numerous 
contributors. I'm still not exactly very keen to see seasoned member of 
the community issuing one aggressive line of reply to a suggestion 
backed by 3 major contributors.

> This extdata() proposal is a rather silly suggestion because every other
> namespace in Mercurial already collides. And they collide intentionally because
> it's convenient for users. We already have revset aliases creating the exact
> same collision risk so the idea that a) extdata() is somehow protecting me from
> something but b) I can still get the effect I want by also adding a revset alias
> is a bit of a self-contradiction. Never mind that I put out an RFC on this topic
> months ago and that the prototype has been in use for six.

Sure, we have namespace colliding for labels, but we also have explicit 
way to request for each namespaces individually (eg: bookmark("foo") vs 
tags("foo")). What is introduced here is collision at the revset 
predicate level, we should be careful about it as it cannot be 
disambiguated and have deeper consequence. In the same way, we avoid 
command name collision.

Revset alias can create collision, but that is also why they are treated 
differently. Users provided input will use the alias definition but 
internally issued revset will not.

I've been using the prototype for 6 months and from that experience, I 
can tell that I personally am actually using the feature only through 
some high level -command-alias- or -changesets-style-. All theses alias 
could be using the long "extdata()" form without any extra inconvenience 
for me. adding alias or template when really needed seems a good 
compromises here.

I'm not sure why Foozy did not comment on the RFC initially, but this 
kind of things happens. Myself I sorry to have missed it initially, I 
took July as a vacation Month.

> But the real issue is that I am still completely exhausted with arguing with you
> personally, so when you decided to latch on to this proposal, I was immediately
> reminded of how much I was looking forward to doing other things with my time.

If you saw that as a real issue, I imagine you would have spend some 
minimal effort to try to solve it. Instead you spent the last 6 months 
avoiding any chance to chat and issued a steady stream of one way and 
aggressive way email with woobly content. This was bad enough when they 
were privates, but not what you have moved aggressive behavior to public 
space this is just plain bad for the community atmosphere. I really 
would be happier if you could either move to a constructive stance or be 
coherent with your wish to spend your time on something else, actually 
spending it somewhere else.

> So I'm not going to write the extdata() version. You can either shrug and say "I
> guess we won't have that feature" or you can do it.

I guess we'll get that later if the needs for it becomes bad enough.

Cheers,

-- 
Pierre-Yves David