Re: [PATCH 0/6] wildmatch part 2

[Junio C Hamano <gitster@pobox.com>]

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

> 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.