Re: [PATCH 0/6] wildmatch part 2

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 4, 2012, 17:43 UTC
Message-ID: <7vwqz6yvyj.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1349336392-1772-1-git-send-email-pclouds@gmail.com>

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

Show 12 quoted lines

> On Thu, Oct 4, 2012 at 1:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Perhaps the wildmatch code may not be what we want X-<.
>
> When I imported wildmatch I was hoping to make minimum changes to it.
> But wildmatch is probably the only practical way to support "**" even
> if we later need to change it the way we want. Other options are base
> our work on top of compat/fnmatch.c, which is an #ifdef spaghetti
> mess, or write a new fnmatch()-compatible function. Both unattractive
> to me.
>
> Anyway, this is on top of nd/wildmatch, which makes "ab**cd" match
> full pathname.

I do not think we are in a hurry to push "**" support in before we know what semantics we want to get out of it. Pushing half-baked "this is good enough at least to me for now" topics before they are ready will cost the users in the longer term.

On the other hand, the three patches (2/3/4) in this series look like a good improvement regardless of what kind of matching engine we use. I would have preferred to see them _before_ nd/wildmatch.

I do not agree with the reasoning behind [1/6] that changes

	union {
		char *pattern;
		strict git_attr *attr;
	} u;
	char is_macro;

to

	char *pattern;
	strict git_attr *attr;
	char is_macro;

by the way.

The union is much less about space saving but is more about the nature of the usage; we use pattern or attr but not both at the same time. Even if the pattern becomes richer with later patches, that does not change the fundamental premise that depending on the value of "is_macro", either "attr" is used or "pattern" is used but they won't be in effect at the same time. The evolution of this should go more like this, I think:

	union {
		struct {
			const char *string;
			int pattern_length;
			int prefix_literal_length;
			int flags;
		} pattern;
		struct git_attr *attr;
	} u;
	char is_macro;

Thanks.