Re: [PATCH 5/5] pathspec: record labels

From: Stefan Beller <sbeller@google.com>
Date: May 16, 2016, 17:55 UTC
Message-ID: <CAGZ79kbHW+qzQjoVu9gRYC0FBqpkq5bPPLU3=BdEFKeGc=U6sA@mail.gmail.com>
In-Reply-To: <xmqqk2iw78aq.fsf@gitster.mtv.corp.google.com>

Show 8 quoted lines

> I am NOT suggesting to make this enhancement in the prototype to
> allow us experiment with submodule selection use case, but this is
> an obvious place to allow
>
>         :(label=A B):(label=C D)
>
> to mean ((A & B) | (C & D)) by making item->labels an array of set
> of labels.

This is what already works with the series. Or rather:

    ":(label=A B)" ":(label=C D)"

works as you would expect for (A&B) | (C&D).

So I am a bit hesitant to replace the string list by an array of stringlists or such, as the future enhancement can also do that?

The enhancement may bring in more expressions into the label string, so it may even parse that string into a tree for Lexicographical order instead of just using an array of lists.

Show 19 quoted lines

>
> And no, I do not think arbitrary boolean expression is too complex
> to understand to the end-users, especially if we have to stay within
> the pathspec magic syntax.  And my gut feeling is that it is not
> worth it to support anything more complex than "OR of these ANDed
> ones".
>
>> +                     string_list_split(item->labels, sb.buf, ' ', -1);
>> +                     string_list_remove_empty_items(item->labels, 0);
>> +                     strbuf_release(&sb);
>> +                     continue;
>
> The data structure to record the "required labels" is shared
> knowledge between this function and the has_all_labels() and nobody
> else knows it is done with string_list, so I think this is a good
> balance between expediency and future optimization possibilities (I
> am anticipating that linear search of string list would be found as
> performance bottleneck).
>