Re: [PATCHv8 5/5] pathspec: allow querying for attributes
Stefan Beller writes: > $ grep -r "cat" |grep "<<-"|wc -l > 915 > $ grep -r "cat" |grep "<<"|grep -v "<<-"| wc -l > 1329 > > I was undecided what the prevailing style is, some did indent, > others did not. FWIW, newer ones tend to use "<<-"; just FYI. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 5/5] pathspec: allow querying for attributes
On Thu, May 19, 2016 at 11:53 AM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> diff --git a/Documentation/glossary-content.txt
>> b/Documentation/glossary-content.txt
>> index cafc284..aa9f220 100644
>> --- a/Documentation/glossary-content.txt
>> +++ b/Documentation/glossary-content.txt
>> @@ -384,6 +384,23 @@ full pathname may have special meaning:
>> +
>> Glob magic is incompatible with literal magic.
>>
>> +attr;;
>> + Additionally to matching the pathspec, the path must have the
>> + attribute as specified. The syntax for specifying the required
>> + attributes is "`attr: [mode] [=value]`"
>> ++
>> +Attributes can have 4 states (Set, Unset, Set to a value, unspecified) and
>> +you can query each attribute for certain states. The "`[mode]`" is a special
>> +character to indicate which attribute states are looked for. The following
>> +modes are available:
>> +
>> + - an empty "`[mode]`" matches if the attribute is set
>> + - "`-`" the attribute must be unset
>> + - "`!`" the attribute must be unspecified
>> + - an empty "`[mode]`" combined with "`[=value]`" matches if the attribute
>> has
>> + the given value.
>> ++
>
> As an initial design, I find this much more agreeable than the
> previous rounds. I however find the phrasing of the above harder
> than necessary to understand, for a few reasons.
>
> * Mixed use of "X matches if ..." and "... must be Y" makes it
>unclear if they are talking about different kind of things, or
>the same kind of things in merely different ways.
>
> * It does not make it clear "=value" is only meaningful when [mode]
>is empty.
>
> Perhaps dropping the '[mode]' thing altogether and instead saying
>
> After `attr:` comes a space separated list of "attribute
> requirements", all of which must be met in order for the
> path to be considered a match; this is in addition to the
> usual non-magic pathspec pattern matching.
>
> Each of the attribute requirements for the path takes one of
> these forms:
>
> - "`ATTR`" requires that the attribute `ATTR` must be set.
>
> - "`-ATTR`" requires that the attribute `ATTR` must be unset.
>
> - "`ATTR=VALUE`" requires that the attribute `ATTR` must be
> set to the string `VALUE`.
>
> - "`!ATTR`" requires that the attribute `ATTR` must be
> unspecified.
>
> would make the resulting text easier to read?
That is way better!
>
>> +static int match_attrs(const char *name, int namelen,
>> +const struct pathspec_item *item)
>> +{
>> + int i;
>> +
>> + git_check_attr_counted(name, namelen, item->attr_check);
>> + for (i = 0; i < item->attr_match_nr; i++) {
>> + const char *value;
>> + int matched;
>> + enum attr_match_mode match_mode;
>> +
>> + value = item->attr_check->check[i].value;
>> + match_mode = item->attr_match[i].match_mode;
>> +
>> + if (ATTR_TRUE(value))
>> + matched = match_mode == MATCH_SET;
>> + else if (ATTR_FALSE(value))
>> + matched = match_mode == MATCH_UNSET;
>> + else if (ATTR_UNSET(value))
>> + matched = match_mode == MATCH_UNSPECIFIED;
>
> readability nit:
>
> matched = (match_mode == MATCH_WHATEVER);
>
> would be easier to view
ok.
>
>> + else
>> + matched = (match_mode == MATCH_VALUE &&
>> +!strcmp(item->attr_match[i].value, value));
>
> and would match the last case above better.
>
>> +static void parse_pathspec_attr_match(struct pathspec_item *item, const
>> char *value)
>> +{
>> + struct string_list_item *si;
>> + struct string_list list = STRING_LIST_INIT_DUP;
>> +
>> +
>> + if (!value || !strlen(value))
>> + die(_("attr spec must not be empty"));
>> +
>> + string_list_split(&list, value, ' ', -1);
>> + string_list_remove_empty_items(&list, 0);
>> +
>> + if (!item->attr_check)
>> + item->attr_check = git_attr_check_alloc();
>> + else
>> + die(_("Only one 'attr:' specification is allowed."));
>> +
>> + ALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr,
>> item->attr_match_alloc);
>> +
>> + for_each_string_list_item(si, &list) {
>> + size_t attr_len;
>> +
>> + int j = item->attr_match_nr++;
>> + const char *attr = si->string;
>> + struct attr_match *am = &item->attr_match[j];
>> +
>> + if (attr[0] == '!')
>> + am->match_mode = MATCH_UNSPECIFIED;
>> + else if (attr[0] == '-')
>> + am->match_mode = MATCH_UNSET;
>> + else
>> + am->match_mode = MATCH_SET;
>> +
>> + if (am->match_mode != MATCH_SET)
>> + /* skip first character */
>> + attr++;
>> +
Re: [PATCHv8 5/5] pathspec: allow querying for attributes
Stefan Beller writes: > +test_expect_success 'setup a tree' ' > + mkdir sub && > + for p in fileA fileB fileC fileAB fileAC fileBC fileNoLabel > fileUnsetLabel fileSetLabel fileValue fileWrongLabel; do > + : >$p && > + git add $p && > + : >sub/$p > + git add sub/$p > + done && > + git commit -m $p && What does this $p refer to? > + git ls-files >actual && > + cat+fileA > +fileAB > +fileAC > +fileB > +fileBC > +fileC > +fileNoLabel > +fileSetLabel > +fileUnsetLabel > +fileValue > +fileWrongLabel > +sub/fileA > +sub/fileAB > +sub/fileAC > +sub/fileB > +sub/fileBC > +sub/fileC > +sub/fileNoLabel > +sub/fileSetLabel > +sub/fileUnsetLabel > +sub/fileValue > +sub/fileWrongLabel > +EOF > + test_cmp expect actual > +' If I were doing this, I'd prepare the list of paths (i.e. expect) first and then create these paths using that list, i.e. test_expect_success 'setup a tree' ' cat <<-\EOF >expect && fileA fileAB ... sub/fileWrongLabel EOF mkdir sub && while read path do : >$path && git add $path || return 1 done actual && test_cmp expect actual ' -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 5/5] pathspec: allow querying for attributes
Stefan Beller writes:
> diff --git a/Documentation/glossary-content.txt
> b/Documentation/glossary-content.txt
> index cafc284..aa9f220 100644
> --- a/Documentation/glossary-content.txt
> +++ b/Documentation/glossary-content.txt
> @@ -384,6 +384,23 @@ full pathname may have special meaning:
> +
> Glob magic is incompatible with literal magic.
>
> +attr;;
> + Additionally to matching the pathspec, the path must have the
> + attribute as specified. The syntax for specifying the required
> + attributes is "`attr: [mode] [=value]`"
> ++
> +Attributes can have 4 states (Set, Unset, Set to a value, unspecified) and
> +you can query each attribute for certain states. The "`[mode]`" is a special
> +character to indicate which attribute states are looked for. The following
> +modes are available:
> +
> + - an empty "`[mode]`" matches if the attribute is set
> + - "`-`" the attribute must be unset
> + - "`!`" the attribute must be unspecified
> + - an empty "`[mode]`" combined with "`[=value]`" matches if the attribute
> has
> + the given value.
> ++
As an initial design, I find this much more agreeable than the
previous rounds. I however find the phrasing of the above harder
than necessary to understand, for a few reasons.
* Mixed use of "X matches if ..." and "... must be Y" makes it
unclear if they are talking about different kind of things, or
the same kind of things in merely different ways.
* It does not make it clear "=value" is only meaningful when [mode]
is empty.
Perhaps dropping the '[mode]' thing altogether and instead saying
After `attr:` comes a space separated list of "attribute
requirements", all of which must be met in order for the
path to be considered a match; this is in addition to the
usual non-magic pathspec pattern matching.
Each of the attribute requirements for the path takes one of
these forms:
- "`ATTR`" requires that the attribute `ATTR` must be set.
- "`-ATTR`" requires that the attribute `ATTR` must be unset.
- "`ATTR=VALUE`" requires that the attribute `ATTR` must be
set to the string `VALUE`.
- "`!ATTR`" requires that the attribute `ATTR` must be
unspecified.
would make the resulting text easier to read?
> +static int match_attrs(const char *name, int namelen,
> +const struct pathspec_item *item)
> +{
> + int i;
> +
> + git_check_attr_counted(name, namelen, item->attr_check);
> + for (i = 0; i < item->attr_match_nr; i++) {
> + const char *value;
> + int matched;
> + enum attr_match_mode match_mode;
> +
> + value = item->attr_check->check[i].value;
> + match_mode = item->attr_match[i].match_mode;
> +
> + if (ATTR_TRUE(value))
> + matched = match_mode == MATCH_SET;
> + else if (ATTR_FALSE(value))
> + matched = match_mode == MATCH_UNSET;
> + else if (ATTR_UNSET(value))
> + matched = match_mode == MATCH_UNSPECIFIED;
readability nit:
matched = (match_mode == MATCH_WHATEVER);
would be easier to view
> + else
> + matched = (match_mode == MATCH_VALUE &&
> +!strcmp(item->attr_match[i].value, value));
and would match the last case above better.
> +static void parse_pathspec_attr_match(struct pathspec_item *item, const char
> *value)
> +{
> + struct string_list_item *si;
> + struct string_list list = STRING_LIST_INIT_DUP;
> +
> +
> + if (!value || !strlen(value))
> + die(_("attr spec must not be empty"));
> +
> + string_list_split(&list, value, ' ', -1);
> + string_list_remove_empty_items(&list, 0);
> +
> + if (!item->attr_check)
> + item->attr_check = git_attr_check_alloc();
> + else
> + die(_("Only one 'attr:' specification is allowed."));
> +
> + ALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr,
> item->attr_match_alloc);
> +
> + for_each_string_list_item(si, &list) {
> + size_t attr_len;
> +
> + int j = item->attr_match_nr++;
> + const char *attr = si->string;
> + struct attr_match *am = &item->attr_match[j];
> +
> + if (attr[0] == '!')
> + am->match_mode = MATCH_UNSPECIFIED;
> + else if (attr[0] == '-')
> + am->match_mode = MATCH_UNSET;
> + else
> + am->match_mode = MATCH_SET;
> +
> + if (am->match_mode != MATCH_SET)
> + /* skip first character */
> + attr++;
> + attr_len = strcspn(attr, "=");
> + if (attr[attr_len] == '=') {
> + am->match_mode = MATCH_VALUE;
> + am->value = xstrdup(&attr[attr_len + 1]);
> + if (strchr(am->valu
