Re: [PATCH/RFC 4/4] attr: avoid heavy work when we know the specified attr is not defined

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 10, 2014, 00:18 UTC
Message-ID: <xmqqsigojr0u.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1418133205-18213-5-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 12 quoted lines

> +static void collect_selected_attrs(const char *path, int num,
> +				   struct git_attr_check *check)
> +{
> +	struct attr_stack *stk;
> +	int i, pathlen, rem, dirlen;
> +	int basename_offset;
> +
> +	pathlen = split_path(path, &dirlen, &basename_offset);
> +	prepare_attr_stack(path, dirlen);
> +	if (cannot_trust_maybe_real) {
> +		for (i = 0; i < git_attr_nr; i++)
> +			check_all_attr[i].value = ATTR__UNKNOWN;

Judging from the fact that

 (1) the only caller calls this function in this fashion based on the
     setting of "cannot-trust" bit,

 (2) this and the other function the only caller calls share the
     same code in their beginning part, and

 (3) the body of the if() statement here duplicates the code from
     collect_all_attrs(),

I smell that a much better split is possible.

Why isn't this all inside a single function collect_all_attrs()? That single function may no longer be collect_ALL_attrs, so renaming it to collect_attrs() is fine, but then that function may have this if () to initialize all of them to ATTR__UNKNOWN or do the else part we see below, and when organized that way we do not need to have duplicated code (or split_path() helper function), no?

Show 32 quoted lines

> +	} else {
> +		rem = num;
> +		for (i = 0; i < num; i++) {
> +			struct git_attr_check *c;
> +			c = check_all_attr + check[i].attr->attr_nr;
> +			if (check[i].attr->maybe_real)
> +				c->value = ATTR__UNKNOWN;
> +			else {
> +				c->value = ATTR__UNSET;
> +				rem--;
> +			}
> +		}
> +		if (!rem)
> +			return;
> +	}
> +	rem = git_attr_nr;
> +	for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
> +		rem = fill(path, pathlen, basename_offset, stk, rem);
> +}
> +
>  int git_check_attr(const char *path, int num, struct git_attr_check *check)
>  {
>  	int i;
>  
> -	collect_all_attrs(path);
> +	if (cannot_trust_maybe_real)
> +		collect_all_attrs(path);
> +	else
> +		collect_selected_attrs(path, num, check);
>  
>  	for (i = 0; i < num; i++) {
>  		const char *value = check_all_attr[check[i].attr->attr_nr].value;