Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes

From: Junio C Hamano <gitster@pobox.com>
Date: May 18, 2016, 15:39 UTC
Message-ID: <xmqqa8jntlyd.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqposkv3c9.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 27 quoted lines

> The mention of the possibility is purely as a hint useful for a
> possible enhancement in the far future.  If we ever want to support
> something like this:
>
> 	":(attr-expression (VAR1=VAL1 | VAR1=VAL2) & VAR2)"
>
> you can remember that you can put VAR1 and VAR2 in attr_check to
> grab values for VAR1 and VAR2 (even though VAR1 is mentioned twice
> in the expression), and use them in the evaluation you will perform.
>
>> So for the matching we would need to get the order right, i.e.
>>
>>     const char *inspect_name = git_attr_name(item.attr_match[i].attr);
>>     for (j=0; j <  p.attr_check.check_nr; j++) {
>>         const char *cur_name = git_attr_name(p.attr_check.check[j].attr);
>>         if (!strcmp(inspect_name, cur_name))
>>             break;
>
> You do not strcmp() when you have attributes.  They are interned so
> that you can compare their addresses.  That makes it somewhat
> cheaper.
>
> Once you start "expression over random attributes", you'd need to
> map attr => value somehow.  The format attr_check structure gives
> you, i.e. a list of <attr, value>, is aimed at compactness than
> random hashmap-like access.  If the caller wants a hashmap-like
> access for performance purposes, the caller does that itself.

To expand this a bit, I actually do not think hashmap-like access is necessary even in such an application.

An implementation of the evaluator, at least a production-quality one, for the attr expression example shown above is unlikely to keep the expression in a single string "(VAR1=VAL1 | VAR1=VAL2) & VAR2". Instead it would use a parse tree with nodes that represent operators (e.g. OR, AND, "=", etc.) and terms (e.g. attribute reference VAR1, VAR2, and constatnts "VAL1", "VAL2") as its internal representation.

And a node in such a parse tree that refers to an attribute VAR1 would unlikely to keep a "const char *" that is "VAR1". The node would have a field that stores an index into the attr_check.check[] array when the textual expression is "compiled" into a parse tree and the set of attributes the expression uses (hence it needs to ask the attributes API about, to prepare attr_check array) is identified.

When "evaluating" the parse-tree, a node that refers to an attribute has an index into attr_check.check[] array, so there is no need for a loop like the one shown above at all.

When "showing" the expression (for debugging purposes), it would grab the "index into the attr_check.check[] array" from the node, and the element in that array is an <attr, value> pair, so it can use git_attr_name() to obtain the attribute name.