Duy Nguyen [off-list ref] writes:
quoted
So we would want to do any adjustment to the fix when we merge up to
maint.
OK. Then Junio, you may need to resolve the conflict with something
like this. Originally match_basename uses fnmatch, not wildmatch. But
using wildmatch there too should be fine, now that both
match_{base,path}name share fnmatch_icase_mem().
Thanks.
The result still smells somewhat funny, though.
fnmatch_icase_mem() is meant to be a wrapper of fnmatch_icase() for
counted strings and its matching semantics should be the same as
fnmatch_icase().
With the merge-fix, fnmatch_icase_mem() calls into wildmatch(), but
fnmatch_icase() still calls into fnmatch().
The latter's flags are meant to be taken from FNM_* family, but the
former takes flags from WM_* family of bits, no?
I think you are running with USE_WILDMATCH which may make the
differences harder to notice, but the name fnmatch_icase_mem() that
is not in the same family as fnmatch but is from the wildmatch()
family smells like an accident waiting to happen.
I tend to think in the longer term it may be a good idea to build
with USE_WILDMATCH unconditionally (we can lose compat/fnmatch), so
in the end this may not matter that much, but before that happens,
soon after we merge the regression fix with this merge-fix, we may
want to update the codebase as if we applied a series that were
based on 'maint' as you suggested, i.e. using raw wildmatch()
consistently in the match_{base,path}name() codepath.
Opinions?
quoted hunk
-- 8< --
diff --git a/dir.c b/dir.c
index 73a08af..84744df 100644
--- a/dir.c
+++ b/dir.c
@@ -81,7 +81,9 @@ static int fnmatch_icase_mem(const char *pattern, int patternlen,
use_str = str_buf.buf;
}
- match_status = fnmatch_icase(use_pat, use_str, flags);
+ if (ignore_case)
+ flags |= WM_CASEFOLD;
+ match_status = wildmatch(use_pat, use_str, flags, NULL);
strbuf_release(&pat_buf);
strbuf_release(&str_buf);
@@ -564,7 +566,7 @@ int match_pathname(const char *pathname, int pathlen,
return fnmatch_icase_mem(pattern, patternlen,
name, namelen,
- FNM_PATHNAME) == 0;
+ WM_PATHNAME) == 0;
}
/*
-- 8< --
On Fri, Mar 29, 2013 at 09:44:32AM -0700, Junio C Hamano wrote:
Duy Nguyen [off-list ref] writes:
quoted
quoted
So we would want to do any adjustment to the fix when we merge up to
maint.
OK. Then Junio, you may need to resolve the conflict with something
like this. Originally match_basename uses fnmatch, not wildmatch. But
using wildmatch there too should be fine, now that both
match_{base,path}name share fnmatch_icase_mem().
Thanks.
The result still smells somewhat funny, though.
fnmatch_icase_mem() is meant to be a wrapper of fnmatch_icase() for
counted strings and its matching semantics should be the same as
fnmatch_icase().
With the merge-fix, fnmatch_icase_mem() calls into wildmatch(), but
fnmatch_icase() still calls into fnmatch().
The latter's flags are meant to be taken from FNM_* family, but the
former takes flags from WM_* family of bits, no?
Yeah, that does not seem right. If match_pathname has learned to call
into wildmatch instead of fnmatch_icase in the interim, then the right
resolution is to convert its call to fnmatch_icase_mem to a new
wildmatch_mem. Presumably that can be done by either tweaking
fnmatch_icase_mem, or, if wildmatch is ready to take counted strings,
calling into it with the right options.
I think you are running with USE_WILDMATCH which may make the
differences harder to notice, but the name fnmatch_icase_mem() that is
not in the same family as fnmatch but is from the wildmatch() family
smells like an accident waiting to happen.
Agreed.
I tend to think in the longer term it may be a good idea to build with
USE_WILDMATCH unconditionally (we can lose compat/fnmatch), so in the
end this may not matter that much
Yeah, I think that is a sane long-term goal.
-Peff
On Fri, Mar 29, 2013 at 09:44:32AM -0700, Junio C Hamano wrote:
I tend to think in the longer term it may be a good idea to build
with USE_WILDMATCH unconditionally (we can lose compat/fnmatch), so
in the end this may not matter that much
I was thinking about that yesterday. After all, it's the purpose of
USE_WILDMATCH and nd/retire-fnmatch series. So how about this patch to
enable USE_WILDMATCH for a wider audience? I think it could get merged
down to master. If wildmatch's bug history is messy by the time we
enter rc cycles, we could revert the patch and cut new releases with
fnmatch. If not, after 1.9 is released, I'll submit a patch to remove
fnmatch compatiblity later and we will be on wildmatch only.
-- 8< --
diff --git a/Makefile b/Makefile
index 598d631..8a740f8 100644
--- a/Makefile
+++ b/Makefile
@@ -106,7 +106,7 @@ all::
# Define NO_FNMATCH_CASEFOLD if your fnmatch function doesn't have the
# FNM_CASEFOLD GNU extension.
#
-# Define USE_WILDMATCH if you want to use Git's wildmatch
+# Define NO_USE_WILDMATCH if you do not want to use Git's wildmatch
# implementation as fnmatch
#
# Define NO_GECOS_IN_PWENT if you don't have pw_gecos in struct passwd
@@ -1255,7 +1255,7 @@ ifdef NO_FNMATCH_CASEFOLD
COMPAT_OBJS += compat/fnmatch/fnmatch.o
endif
endif
-ifdef USE_WILDMATCH
+ifndef NO_USE_WILDMATCH
COMPAT_CFLAGS += -DUSE_WILDMATCH
endif
ifdef NO_SETENV
-- 8< --