[PATCH] revset: add hook after tree parsing

[Durham Goode]

On 3/30/15 4:25 PM, Pierre-Yves David wrote:
>
>
> On 03/30/2015 04:16 PM, Matt Mackall wrote:
>> On Mon, 2015-03-30 at 18:05 -0500, Matt Mackall wrote:
>>> On Mon, 2015-03-30 at 15:37 -0700, Pierre-Yves David wrote:
>>>>
>>>> On 03/30/2015 03:28 PM, Matt Mackall wrote:
>>>>> On Mon, 2015-03-30 at 13:08 -0700, Laurent Charignon wrote:
>>>>>> # HG changeset patch
>>>>>> # User Laurent Charignon <lcharignon at fb.com>
>>>>>> # Date 1427232295 25200
>>>>>> #      Tue Mar 24 14:24:55 2015 -0700
>>>>>> # Node ID 3ca44121d0afad1aee19675e45ae78d4bbef3cf1
>>>>>> # Parent  5b85a5bc5bbb9d8365953609d98e4dce7110e9b0
>>>>>> revset: add hook after tree parsing
>>>>>>
>>>>>> This will be useful to execute actions after the tree is parsed 
>>>>>> and before the
>>>>>> revset returns a match. It can be used to enable direct hash 
>>>>>> access of revs
>>>>>> that are hidden.
>>>>>
>>>>> This is not very clear. Apparently "enable direct hash access of revs
>>>>> that are hidden" means "a user can do hg log -r <hiddenhash> 
>>>>> without the
>>>>> --hidden flag", amirite?
>>>>>
>>>>> And I guess you'll do this by walking the parse tree and finding 
>>>>> nodes
>>>>> of the form ('symbol', 'abcd') and unhiding them before evaluating 
>>>>> the
>>>>> revset, right? This would be very hard for anyone to guess who wasn't
>>>>> already vaguely aware of what you were working on.
>>>>
>>>> The actual logic about that  lives into a dedicated extension in 
>>>> evolve,
>>>> discussion regarding this should probably happen on of patch touching
>>>> this extension. I do not believe more context is required in this 
>>>> hook.
>>>
>>> Good luck getting me to pull from clowncopter then.
>>
>> Seriously, clowncopter is not a fast-path to get your coworkers' work
>> into the tree when I'm asking for better commit messages. If you're
>> going to try to use it that way, I'm simply not going to pull from it.
>
> I'm definitely not trying to use it that way. Simple and harmless hook 
> such as this one have been added multiple time for multiple out of 
> tree extension with this being seen as a bit deal.
>
>   `localrepo.checkpush`  being a prime exemple of such hook (used by 
> MQ). The document just says "This is a hook used by extension to do 
> stuff at a specific time.
>
> I still think that discussion the actual implementation of the direct 
> access feature should not end up in the description of this specific 
> patch.
I'm with Matt on the commit description point.  In a years time, a 
random person reading this commit message would not have enough context 
to understand why it exists, or where to find more info about what 
"direct access of revs that are hidden" means.