Re: [PATCH v2 10/12] attr: convert git_all_attrs() to use "struct git_attr_check"

From: Junio C Hamano <gitster@pobox.com>
Date: May 17, 2016, 17:00 UTC
Message-ID: <xmqqeg90y5yz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160516210545.6591-11-gitster@pobox.com>

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

Show 18 quoted lines

> +struct git_attr_check *git_attr_check_alloc(void)
> +{
> +	return xcalloc(1, sizeof(struct git_attr_check));
> +}
> +
> +void git_attr_check_append(struct git_attr_check *check, const char *name)
> +{
> +	struct git_attr *attr;
> +
> +	if (check->finalized)
> +		die("BUG: append after git_attr_check structure is finalized");
> +
> +	attr = git_attr(name);
> +	if (!attr)
> +		die("%s: not a valid attribute name", name);
> +	ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);
> +	check->check[check->check_nr++].attr = attr;
> +}

Given that one of the two expected callers, namely, "check-attr" and Stefan's pathspec label magic, of this "alloc and then append" side of the API wants to have an access to git_attr(name), I think the function signature for this one should be updated to take not "const char *name" but instead take "struct git_attr *attr", i.e.

void git_attr_check_append(struct git_attr_check *check,
			   struct git_attr *attr);

Then the loop in check-attr below

> +		check = git_attr_check_alloc();
> +		for (i = 0; i < cnt; i++)
> +			git_attr_check_append(check, argv[i]);

would become

		check = git_attr_check_alloc();
		for (i = 0; i < cnt; i++)
			git_attr_check_append(check, git_attr(argv[i]));

And this part in $gmane/294855

    Then while parsing ":(attr:VAR1=VAL1 -VAR2 VAR3...)path/to/dir/",
    you would first do:

            p->attr_check = git_attr_check_alloc();

    once, and then send each of VAR1=VAL2, -VAR2, VAR3... to your
    parse_one_item() helper function which would:

     * parse the match-mode like your code does;

     * parse out the attribute name (i.e. VAR1, VAR2 and VAR3), and
       instead of keeping it as a "(const) char *", call git_attr()
       to intern it (and keep it in local variable "attr"), and save
       it in p->attr_match[p->attr_nr].attr;

     * call git_attr_check_append(p->attr_check, git_attr_name(attr))

would look like

	... saw ":(attr:", expect "VAR1=VAL1 -VAR2...)" to follow;
        ... char *scan points at byte after attr: now.

	item->attr_check = git_attr_check_alloc();
	while (scan) {
		const char *var, *val;
                enum attr_match_mode mode;
		struct git_attr *attr;
		struct attr_match *match;

        	scan = parse_attr_match(scan, &var, &val, &mode);
                ... var points at VAR1, val points at VAL,
                ... mode becomes MATCH_VALUE; scan skips
                ... forward to pint at -VAR2.  parse_attr_match()
                ... would return NULL once input runs out.

                ALLOC_GROW(item->attr_match, ...);
                match = &item->attr_match[item->attr_match_nr++];

        	attr = git_attr(var);

		match->attr = attr;
                match->value = val;
                match->match_mode = mode;
                
                git_attr_check_append(item->attr_check, attr);
	}

Then matching part would get the "item" and would do:

	... after making sure that the path matches
        ... the pathspec the magic is attached to ...

	git_check_attr(path, item->attr_check);
	for (i = 0; i < item->attr_match_nr; i++) {
		struct git_attr *attr = item->attr_check->check[i].attr;
		const char *value = item->attr_check->check[i].value;
                struct attr_match *match = &item->attr_match[i];

                ... compare what "match" expects with <attr, value> ...
        }