{"thread":{"id":"59312","subject":"[PATCH v2 0/5] Clean up wildmatch.c","startedAt":"2023-02-26T11:50:59Z","lastAt":"2023-02-27T20:07:30Z","messageCount":9,"participants":["Masahiro Yamada","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"472746","messageId":"20230226115021.1681834-1-masahiroy@kernel.org","threadId":"59312","inReplyTo":null,"subject":"[PATCH v2 0/5] Clean up wildmatch.c","fromName":"Masahiro Yamada","fromEmail":"masahiroy@kernel.org","sentAt":"2023-02-26T11:50:16Z","receivedAt":"2023-02-26T11:50:59Z","isPatch":true,"sender":{"key":"masahiroy@kernel.org","avatar":"https://gravatar.com/avatar/de07e07e05be0542e420cb9b79a723c58893c54989cddfee4a69e05e5dac4a9f?d=mp&s=160"},"body":"This file was imported from rsysc, but it was already largely modified\nfor GIT. Why not further cleanups? I see a lot of unneeded stubs.\n\n\nMasahiro Yamada (5):\n  git-compat-util: add isblank() and isgraph()\n  wildmatch: remove IS*() macros\n  wildmatch: remove NEGATE_CLASS(2) macros with trivial refactoring\n  wildmatch: use char instead of uchar\n  wildmatch: more cleanups after killing uchar\n\n git-compat-util.h |  14 ++++++\n wildmatch.c       | 108 +++++++++++++---------------------------------\n 2 files changed, 45 insertions(+), 77 deletions(-)\n\n-- \n2.34.1\n\n"},{"id":"472747","messageId":"20230226115021.1681834-2-masahiroy@kernel.org","threadId":"59312","inReplyTo":"20230226115021.1681834-1-masahiroy@kernel.org","subject":"[PATCH v2 1/5] git-compat-util: add isblank() and isgraph()","fromName":"Masahiro Yamada","fromEmail":"masahiroy@kernel.org","sentAt":"2023-02-26T11:50:17Z","receivedAt":"2023-02-26T11:50:59Z","isPatch":true,"sender":{"key":"masahiroy@kernel.org","avatar":"https://gravatar.com/avatar/de07e07e05be0542e420cb9b79a723c58893c54989cddfee4a69e05e5dac4a9f?d=mp&s=160"},"body":"git-compat-util.h implements most of is*() macros.\n\nAdd isblank() and isgraph(), which are useful to clean up wildmatch.c\nin a consistent way (in this and later commits).\n\nIn the previous submission, I just moved isblank() and isgraph() as\nimplemented in wildmatch.c. I knew they were not robust against the\npointer increment like isblank(*s++), but I thought it was the same\npattern as isprint(), which has the same issue. Unfortunately, it was\nmore controversial than I had expected...\n\nThis version implements them as inline functions because we ran out\nall bits in the sane_ctype[] table. This is the same pattern as\nislower() and isupper().\n\nOnce we refactor ctype.c to create more room in sane_ctype[], isblank()\nand isgraph() will be able to use sane_istest(). Probably so will\nislower() and isupper(). The ctype in Linux kernel (lib/ctype.c) has\nthe LOWER and UPPER bits separately.\n\nSigned-off-by: Masahiro Yamada <masahiroy@kernel.org>\n---\n\nChanges in v2:\n  - Use inline functions\n\n git-compat-util.h | 14 ++++++++++++++\n wildmatch.c       | 14 ++------------\n 2 files changed, 16 insertions(+), 12 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 4f0028ce60..b29c238f02 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1212,10 +1212,12 @@ extern const unsigned char tolower_trans_tbl[256];\n /* Sane ctype - no locale, and works with signed chars */\n #undef isascii\n #undef isspace\n+#undef isblank\n #undef isdigit\n #undef isalpha\n #undef isalnum\n #undef isprint\n+#undef isgraph\n #undef islower\n #undef isupper\n #undef tolower\n@@ -1236,10 +1238,12 @@ extern const unsigned char sane_ctype[256];\n #define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)\n #define isascii(x) (((x) & ~0x7f) == 0)\n #define isspace(x) sane_istest(x,GIT_SPACE)\n+#define isblank(x) sane_isblank(x)\n #define isdigit(x) sane_istest(x,GIT_DIGIT)\n #define isalpha(x) sane_istest(x,GIT_ALPHA)\n #define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)\n #define isprint(x) ((x) >= 0x20 && (x) <= 0x7e)\n+#define isgraph(x) sane_isgraph(x)\n #define islower(x) sane_iscase(x, 1)\n #define isupper(x) sane_iscase(x, 0)\n #define is_glob_special(x) sane_istest(x,GIT_GLOB_SPECIAL)\n@@ -1270,6 +1274,16 @@ static inline int sane_iscase(int x, int is_lower)\n \t\treturn (x & 0x20) == 0;\n }\n \n+static inline int sane_isblank(int c)\n+{\n+\treturn c == ' ' || c == '\\t';\n+}\n+\n+static inline int sane_isgraph(int c)\n+{\n+\treturn isprint(c) && !isspace(c);\n+}\n+\n /*\n  * Like skip_prefix, but compare case-insensitively. Note that the comparison\n  * is done via tolower(), so it is strictly ASCII (no multi-byte characters or\ndiff --git a/wildmatch.c b/wildmatch.c\nindex 7e5a7ea1ea..85c4c7f8a7 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -28,18 +28,8 @@ typedef unsigned char uchar;\n # define ISASCII(c) isascii(c)\n #endif\n \n-#ifdef isblank\n-# define ISBLANK(c) (ISASCII(c) && isblank(c))\n-#else\n-# define ISBLANK(c) ((c) == ' ' || (c) == '\\t')\n-#endif\n-\n-#ifdef isgraph\n-# define ISGRAPH(c) (ISASCII(c) && isgraph(c))\n-#else\n-# define ISGRAPH(c) (ISASCII(c) && isprint(c) && !isspace(c))\n-#endif\n-\n+#define ISBLANK(c) (ISASCII(c) && isblank(c))\n+#define ISGRAPH(c) (ISASCII(c) && isgraph(c))\n #define ISPRINT(c) (ISASCII(c) && isprint(c))\n #define ISDIGIT(c) (ISASCII(c) && isdigit(c))\n #define ISALNUM(c) (ISASCII(c) && isalnum(c))\n-- \n2.34.1\n\n"},{"id":"472748","messageId":"20230226115021.1681834-3-masahiroy@kernel.org","threadId":"59312","inReplyTo":"20230226115021.1681834-1-masahiroy@kernel.org","subject":"[PATCH v2 2/5] wildmatch: remove IS*() macros","fromName":"Masahiro Yamada","fromEmail":"masahiroy@kernel.org","sentAt":"2023-02-26T11:50:18Z","receivedAt":"2023-02-26T11:51:00Z","isPatch":true,"sender":{"key":"masahiroy@kernel.org","avatar":"https://gravatar.com/avatar/de07e07e05be0542e420cb9b79a723c58893c54989cddfee4a69e05e5dac4a9f?d=mp&s=160"},"body":"This file was imported from rsync, which has some compatibility\nlayer because it relies on <ctypes.h> in C standard library.\n\nIn contrast, GIT has its own implementations in git-compat-util.h.\n\n[1] isprint, isgraph, isblank\n\n   They check the given char range in an obvious way\n\n[2] isspace, isdigit, isalpha, isalnum, islower, isupper, iscntr, ispunct\n\n   They look up sane_ctype[], which fills the range 0x80-0xff with 0.\n\n[3] isxdigit\n\n   It looks up hexval_table[], which fills the range 0x80-0xff with -1.\n\nFor all of these, ISACII() is a redundant check.\n\nRemove IS*() macros, and directly use is*() in dowild().\n\nSigned-off-by: Masahiro Yamada <masahiroy@kernel.org>\n---\n\n(no changes since v1)\n\n wildmatch.c | 55 ++++++++++++++++++-----------------------------------\n 1 file changed, 18 insertions(+), 37 deletions(-)\n\ndiff --git a/wildmatch.c b/wildmatch.c\nindex 85c4c7f8a7..a510b3fd23 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -22,25 +22,6 @@ typedef unsigned char uchar;\n \t\t\t\t    && *(class) == *(litmatch) \\\n \t\t\t\t    && strncmp((char*)class, litmatch, len) == 0)\n \n-#if defined STDC_HEADERS || !defined isascii\n-# define ISASCII(c) 1\n-#else\n-# define ISASCII(c) isascii(c)\n-#endif\n-\n-#define ISBLANK(c) (ISASCII(c) && isblank(c))\n-#define ISGRAPH(c) (ISASCII(c) && isgraph(c))\n-#define ISPRINT(c) (ISASCII(c) && isprint(c))\n-#define ISDIGIT(c) (ISASCII(c) && isdigit(c))\n-#define ISALNUM(c) (ISASCII(c) && isalnum(c))\n-#define ISALPHA(c) (ISASCII(c) && isalpha(c))\n-#define ISCNTRL(c) (ISASCII(c) && iscntrl(c))\n-#define ISLOWER(c) (ISASCII(c) && islower(c))\n-#define ISPUNCT(c) (ISASCII(c) && ispunct(c))\n-#define ISSPACE(c) (ISASCII(c) && isspace(c))\n-#define ISUPPER(c) (ISASCII(c) && isupper(c))\n-#define ISXDIGIT(c) (ISASCII(c) && isxdigit(c))\n-\n /* Match pattern \"p\" against \"text\" */\n static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n {\n@@ -52,9 +33,9 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\tuchar t_ch, prev_ch;\n \t\tif ((t_ch = *text) == '\\0' && p_ch != '*')\n \t\t\treturn WM_ABORT_ALL;\n-\t\tif ((flags & WM_CASEFOLD) && ISUPPER(t_ch))\n+\t\tif ((flags & WM_CASEFOLD) && isupper(t_ch))\n \t\t\tt_ch = tolower(t_ch);\n-\t\tif ((flags & WM_CASEFOLD) && ISUPPER(p_ch))\n+\t\tif ((flags & WM_CASEFOLD) && isupper(p_ch))\n \t\t\tp_ch = tolower(p_ch);\n \t\tswitch (p_ch) {\n \t\tcase '\\\\':\n@@ -133,11 +114,11 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t */\n \t\t\t\tif (!is_glob_special(*p)) {\n \t\t\t\t\tp_ch = *p;\n-\t\t\t\t\tif ((flags & WM_CASEFOLD) && ISUPPER(p_ch))\n+\t\t\t\t\tif ((flags & WM_CASEFOLD) && isupper(p_ch))\n \t\t\t\t\t\tp_ch = tolower(p_ch);\n \t\t\t\t\twhile ((t_ch = *text) != '\\0' &&\n \t\t\t\t\t       (match_slash || t_ch != '/')) {\n-\t\t\t\t\t\tif ((flags & WM_CASEFOLD) && ISUPPER(t_ch))\n+\t\t\t\t\t\tif ((flags & WM_CASEFOLD) && isupper(t_ch))\n \t\t\t\t\t\t\tt_ch = tolower(t_ch);\n \t\t\t\t\t\tif (t_ch == p_ch)\n \t\t\t\t\t\t\tbreak;\n@@ -186,7 +167,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\t}\n \t\t\t\t\tif (t_ch <= p_ch && t_ch >= prev_ch)\n \t\t\t\t\t\tmatched = 1;\n-\t\t\t\t\telse if ((flags & WM_CASEFOLD) && ISLOWER(t_ch)) {\n+\t\t\t\t\telse if ((flags & WM_CASEFOLD) && islower(t_ch)) {\n \t\t\t\t\t\tuchar t_ch_upper = toupper(t_ch);\n \t\t\t\t\t\tif (t_ch_upper <= p_ch && t_ch_upper >= prev_ch)\n \t\t\t\t\t\t\tmatched = 1;\n@@ -208,42 +189,42 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\t\tcontinue;\n \t\t\t\t\t}\n \t\t\t\t\tif (CC_EQ(s,i, \"alnum\")) {\n-\t\t\t\t\t\tif (ISALNUM(t_ch))\n+\t\t\t\t\t\tif (isalnum(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t} else if (CC_EQ(s,i, \"alpha\")) {\n-\t\t\t\t\t\tif (ISALPHA(t_ch))\n+\t\t\t\t\t\tif (isalpha(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t} else if (CC_EQ(s,i, \"blank\")) {\n-\t\t\t\t\t\tif (ISBLANK(t_ch))\n+\t\t\t\t\t\tif (isblank(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t} else if (CC_EQ(s,i, \"cntrl\")) {\n-\t\t\t\t\t\tif (ISCNTRL(t_ch))\n+\t\t\t\t\t\tif (iscntrl(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t} else if (CC_EQ(s,i, \"digit\")) {\n-\t\t\t\t\t\tif (ISDIGIT(t_ch))\n+\t\t\t\t\t\tif (isdigit(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t} else if (CC_EQ(s,i, \"graph\")) {\n-\t\t\t\t\t\tif (ISGRAPH(t_ch))\n+\t\t\t\t\t\tif (isgraph(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t} else if (CC_EQ(s,i, \"lower\")) {\n-\t\t\t\t\t\tif (ISLOWER(t_ch))\n+\t\t\t\t\t\tif (islower(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t} else if (CC_EQ(s,i, \"print\")) {\n-\t\t\t\t\t\tif (ISPRINT(t_ch))\n+\t\t\t\t\t\tif (isprint(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t} else if (CC_EQ(s,i, \"punct\")) {\n-\t\t\t\t\t\tif (ISPUNCT(t_ch))\n+\t\t\t\t\t\tif (ispunct(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t} else if (CC_EQ(s,i, \"space\")) {\n-\t\t\t\t\t\tif (ISSPACE(t_ch))\n+\t\t\t\t\t\tif (isspace(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t} else if (CC_EQ(s,i, \"upper\")) {\n-\t\t\t\t\t\tif (ISUPPER(t_ch))\n+\t\t\t\t\t\tif (isupper(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n-\t\t\t\t\t\telse if ((flags & WM_CASEFOLD) && ISLOWER(t_ch))\n+\t\t\t\t\t\telse if ((flags & WM_CASEFOLD) && islower(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t} else if (CC_EQ(s,i, \"xdigit\")) {\n-\t\t\t\t\t\tif (ISXDIGIT(t_ch))\n+\t\t\t\t\t\tif (isxdigit(t_ch))\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t} else /* malformed [:class:] string */\n \t\t\t\t\t\treturn WM_ABORT_ALL;\n-- \n2.34.1\n\n"},{"id":"472749","messageId":"20230226115021.1681834-6-masahiroy@kernel.org","threadId":"59312","inReplyTo":"20230226115021.1681834-1-masahiroy@kernel.org","subject":"[PATCH v2 5/5] wildmatch: more cleanups after killing uchar","fromName":"Masahiro Yamada","fromEmail":"masahiroy@kernel.org","sentAt":"2023-02-26T11:50:21Z","receivedAt":"2023-02-26T11:51:02Z","isPatch":true,"sender":{"key":"masahiroy@kernel.org","avatar":"https://gravatar.com/avatar/de07e07e05be0542e420cb9b79a723c58893c54989cddfee4a69e05e5dac4a9f?d=mp&s=160"},"body":"Remove the local function dowild(), which is now equivalent to\nwildmatch().\n\nRemove the local variable, slash.\n\nSigned-off-by: Masahiro Yamada <masahiroy@kernel.org>\n---\n\n(no changes since v1)\n\n wildmatch.c | 17 +++++------------\n 1 file changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/wildmatch.c b/wildmatch.c\nindex 7dffd783cb..24577e9b8e 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -17,7 +17,7 @@\n \t\t\t\t    && strncmp(class, litmatch, len) == 0)\n \n /* Match pattern \"p\" against \"text\" */\n-static int dowild(const char *p, const char *text, unsigned int flags)\n+int wildmatch(const char *p, const char *text, unsigned int flags)\n {\n \tchar p_ch;\n \tconst char *pattern = p;\n@@ -66,7 +66,7 @@ static int dowild(const char *p, const char *text, unsigned int flags)\n \t\t\t\t\t * both foo/bar and foo/a/bar.\n \t\t\t\t\t */\n \t\t\t\t\tif (p[0] == '/' &&\n-\t\t\t\t\t    dowild(p + 1, text, flags) == WM_MATCH)\n+\t\t\t\t\t    wildmatch(p + 1, text, flags) == WM_MATCH)\n \t\t\t\t\t\treturn WM_MATCH;\n \t\t\t\t\tmatch_slash = 1;\n \t\t\t\t} else /* WM_PATHNAME is set */\n@@ -88,10 +88,9 @@ static int dowild(const char *p, const char *text, unsigned int flags)\n \t\t\t\t * with WM_PATHNAME matches the next\n \t\t\t\t * directory\n \t\t\t\t */\n-\t\t\t\tconst char *slash = strchr(text, '/');\n-\t\t\t\tif (!slash)\n+\t\t\t\ttext = strchr(text, '/');\n+\t\t\t\tif (!text)\n \t\t\t\t\treturn WM_NOMATCH;\n-\t\t\t\ttext = slash;\n \t\t\t\t/* the slash is consumed by the top-level for loop */\n \t\t\t\tbreak;\n \t\t\t}\n@@ -121,7 +120,7 @@ static int dowild(const char *p, const char *text, unsigned int flags)\n \t\t\t\t\tif (t_ch != p_ch)\n \t\t\t\t\t\treturn WM_NOMATCH;\n \t\t\t\t}\n-\t\t\t\tif ((matched = dowild(p, text, flags)) != WM_NOMATCH) {\n+\t\t\t\tif ((matched = wildmatch(p, text, flags)) != WM_NOMATCH) {\n \t\t\t\t\tif (!match_slash || matched != WM_ABORT_TO_STARSTAR)\n \t\t\t\t\t\treturn matched;\n \t\t\t\t} else if (!match_slash && t_ch == '/')\n@@ -231,9 +230,3 @@ static int dowild(const char *p, const char *text, unsigned int flags)\n \n \treturn *text ? WM_NOMATCH : WM_MATCH;\n }\n-\n-/* Match the \"pattern\" against the \"text\" string. */\n-int wildmatch(const char *pattern, const char *text, unsigned int flags)\n-{\n-\treturn dowild(pattern, text, flags);\n-}\n-- \n2.34.1\n\n"},{"id":"472750","messageId":"20230226115021.1681834-4-masahiroy@kernel.org","threadId":"59312","inReplyTo":"20230226115021.1681834-1-masahiroy@kernel.org","subject":"[PATCH v2 3/5] wildmatch: remove NEGATE_CLASS(2) macros with trivial refactoring","fromName":"Masahiro Yamada","fromEmail":"masahiroy@kernel.org","sentAt":"2023-02-26T11:50:19Z","receivedAt":"2023-02-26T11:51:03Z","isPatch":true,"sender":{"key":"masahiroy@kernel.org","avatar":"https://gravatar.com/avatar/de07e07e05be0542e420cb9b79a723c58893c54989cddfee4a69e05e5dac4a9f?d=mp&s=160"},"body":"The other glob patterns are hard-coded in dowild(). There is no need\nto macrofy '!' or '^'. Remove the NEGATE_CLASS and REGATE_CLASS2\ndefines, then refactor the code.\n\nSigned-off-by: Masahiro Yamada <masahiroy@kernel.org>\n---\n\n(no changes since v1)\n\n wildmatch.c | 10 +---------\n 1 file changed, 1 insertion(+), 9 deletions(-)\n\ndiff --git a/wildmatch.c b/wildmatch.c\nindex a510b3fd23..93800b8eac 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -14,10 +14,6 @@\n \n typedef unsigned char uchar;\n \n-/* What character marks an inverted character class? */\n-#define NEGATE_CLASS\t'!'\n-#define NEGATE_CLASS2\t'^'\n-\n #define CC_EQ(class, len, litmatch) ((len) == sizeof (litmatch)-1 \\\n \t\t\t\t    && *(class) == *(litmatch) \\\n \t\t\t\t    && strncmp((char*)class, litmatch, len) == 0)\n@@ -137,12 +133,8 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\treturn WM_ABORT_ALL;\n \t\tcase '[':\n \t\t\tp_ch = *++p;\n-#ifdef NEGATE_CLASS2\n-\t\t\tif (p_ch == NEGATE_CLASS2)\n-\t\t\t\tp_ch = NEGATE_CLASS;\n-#endif\n \t\t\t/* Assign literal 1/0 because of \"matched\" comparison. */\n-\t\t\tnegated = p_ch == NEGATE_CLASS ? 1 : 0;\n+\t\t\tnegated = p_ch == '!' || p_ch == '^' ? 1 : 0;\n \t\t\tif (negated) {\n \t\t\t\t/* Inverted character class. */\n \t\t\t\tp_ch = *++p;\n-- \n2.34.1\n\n"},{"id":"472751","messageId":"20230226115021.1681834-5-masahiroy@kernel.org","threadId":"59312","inReplyTo":"20230226115021.1681834-1-masahiroy@kernel.org","subject":"[PATCH v2 4/5] wildmatch: use char instead of uchar","fromName":"Masahiro Yamada","fromEmail":"masahiroy@kernel.org","sentAt":"2023-02-26T11:50:20Z","receivedAt":"2023-02-26T11:51:06Z","isPatch":true,"sender":{"key":"masahiroy@kernel.org","avatar":"https://gravatar.com/avatar/de07e07e05be0542e420cb9b79a723c58893c54989cddfee4a69e05e5dac4a9f?d=mp&s=160"},"body":"dowild() casts (char *) and (uchar *) back-and-forth, which is\nugly.\n\nThis file was imported from rsync, which started to use (unsigned char)\nsince the following commit:\n\n | commit e11c42511903adc6d27cf1671cc76fa711ea37e5\n | Author: Wayne Davison <wayned@samba.org>\n | Date:   Sun Jul 6 04:33:54 2003 +0000\n |\n |     - Added [:class:] handling to the character-class code.\n |     - Use explicit unsigned characters for proper set checks.\n |     - Made the character-class code honor backslash escapes.\n |     - Accept '^' as a class-negation character in addition to '!'.\n\nPerhaps, it was needed because rsync relies on is*() from <ctypes.h>.\n\nGIT has its own implementations, so the behavior is clear.\n\nIn fact, commit 4546738b58a0 (\"Unlocalized isspace and friends\")\nsays one of the motivations is \"we want the right signed behaviour\".\n\nsane_istest() casts the given character to (unsigned char) anyway\nbefore sane_ctype[] table lookup, so dowild() can use 'char'.\n\nSigned-off-by: Masahiro Yamada <masahiroy@kernel.org>\n---\n\n(no changes since v1)\n\n wildmatch.c | 24 +++++++++++-------------\n 1 file changed, 11 insertions(+), 13 deletions(-)\n\ndiff --git a/wildmatch.c b/wildmatch.c\nindex 93800b8eac..7dffd783cb 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -12,21 +12,19 @@\n #include \"cache.h\"\n #include \"wildmatch.h\"\n \n-typedef unsigned char uchar;\n-\n #define CC_EQ(class, len, litmatch) ((len) == sizeof (litmatch)-1 \\\n \t\t\t\t    && *(class) == *(litmatch) \\\n-\t\t\t\t    && strncmp((char*)class, litmatch, len) == 0)\n+\t\t\t\t    && strncmp(class, litmatch, len) == 0)\n \n /* Match pattern \"p\" against \"text\" */\n-static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n+static int dowild(const char *p, const char *text, unsigned int flags)\n {\n-\tuchar p_ch;\n-\tconst uchar *pattern = p;\n+\tchar p_ch;\n+\tconst char *pattern = p;\n \n \tfor ( ; (p_ch = *p) != '\\0'; text++, p++) {\n \t\tint matched, match_slash, negated;\n-\t\tuchar t_ch, prev_ch;\n+\t\tchar t_ch, prev_ch;\n \t\tif ((t_ch = *text) == '\\0' && p_ch != '*')\n \t\t\treturn WM_ABORT_ALL;\n \t\tif ((flags & WM_CASEFOLD) && isupper(t_ch))\n@@ -50,7 +48,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\tcontinue;\n \t\tcase '*':\n \t\t\tif (*++p == '*') {\n-\t\t\t\tconst uchar *prev_p = p - 2;\n+\t\t\t\tconst char *prev_p = p - 2;\n \t\t\t\twhile (*++p == '*') {}\n \t\t\t\tif (!(flags & WM_PATHNAME))\n \t\t\t\t\t/* without WM_PATHNAME, '*' == '**' */\n@@ -90,10 +88,10 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t * with WM_PATHNAME matches the next\n \t\t\t\t * directory\n \t\t\t\t */\n-\t\t\t\tconst char *slash = strchr((char*)text, '/');\n+\t\t\t\tconst char *slash = strchr(text, '/');\n \t\t\t\tif (!slash)\n \t\t\t\t\treturn WM_NOMATCH;\n-\t\t\t\ttext = (const uchar*)slash;\n+\t\t\t\ttext = slash;\n \t\t\t\t/* the slash is consumed by the top-level for loop */\n \t\t\t\tbreak;\n \t\t\t}\n@@ -160,13 +158,13 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\tif (t_ch <= p_ch && t_ch >= prev_ch)\n \t\t\t\t\t\tmatched = 1;\n \t\t\t\t\telse if ((flags & WM_CASEFOLD) && islower(t_ch)) {\n-\t\t\t\t\t\tuchar t_ch_upper = toupper(t_ch);\n+\t\t\t\t\t\tchar t_ch_upper = toupper(t_ch);\n \t\t\t\t\t\tif (t_ch_upper <= p_ch && t_ch_upper >= prev_ch)\n \t\t\t\t\t\t\tmatched = 1;\n \t\t\t\t\t}\n \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n \t\t\t\t} else if (p_ch == '[' && p[1] == ':') {\n-\t\t\t\t\tconst uchar *s;\n+\t\t\t\t\tconst char *s;\n \t\t\t\t\tint i;\n \t\t\t\t\tfor (s = p += 2; (p_ch = *p) && p_ch != ']'; p++) {} /*SHARED ITERATOR*/\n \t\t\t\t\tif (!p_ch)\n@@ -237,5 +235,5 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n /* Match the \"pattern\" against the \"text\" string. */\n int wildmatch(const char *pattern, const char *text, unsigned int flags)\n {\n-\treturn dowild((const uchar*)pattern, (const uchar*)text, flags);\n+\treturn dowild(pattern, text, flags);\n }\n-- \n2.34.1\n\n"},{"id":"472757","messageId":"36cd059e-c676-2aa2-68d9-41a7b0db57f0@web.de","threadId":"59312","inReplyTo":"20230226115021.1681834-2-masahiroy@kernel.org","subject":"Re: [PATCH v2 1/5] git-compat-util: add isblank() and isgraph()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-02-26T18:45:22Z","receivedAt":"2023-02-26T18:45:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.02.23 um 12:50 schrieb Masahiro Yamada:\n> git-compat-util.h implements most of is*() macros.\n>\n> Add isblank() and isgraph(), which are useful to clean up wildmatch.c\n> in a consistent way (in this and later commits).\n>\n> In the previous submission, I just moved isblank() and isgraph() as\n> implemented in wildmatch.c. I knew they were not robust against the\n> pointer increment like isblank(*s++), but I thought it was the same\n> pattern as isprint(), which has the same issue. Unfortunately, it was\n> more controversial than I had expected...\n\nNot sure we need that story in the commit message, but it gave me an\nidea: To go back to the isprint() version from 1c149ab2dd (ctype:\nsupport iscntrl, ispunct, isxdigit and isprint, 2012-10-15), which\nevaluates its argument only once:\n\n#define isprint(x) (sane_istest(x, GIT_ALPHA | GIT_DIGIT | GIT_SPACE | \\\n\t\tGIT_PUNCT | GIT_REGEX_SPECIAL | GIT_GLOB_SPECIAL | \\\n\t\tGIT_PATHSPEC_MAGIC))\n\nBut then I realized that it wrongly classifies \\t, \\r and \\n as\nbeing printing characters; 567342fc77 (test-ctype: test iscntrl,\nispunct, isxdigit and isprint, 2023-02-13) shows it.  So it's not so\neasy, however, ...\n\n> This version implements them as inline functions because we ran out\n> all bits in the sane_ctype[] table. This is the same pattern as\n> islower() and isupper().\n\n... if you remove GIT_SPACE from the definition above you get a\nmacro version of isgraph() that uses a single table lookup.\n\n> Once we refactor ctype.c to create more room in sane_ctype[], isblank()\n> and isgraph() will be able to use sane_istest(). Probably so will\n> islower() and isupper(). The ctype in Linux kernel (lib/ctype.c) has\n> the LOWER and UPPER bits separately.\n\nIf we're out of bits then isblank() is a good choice to implement\nwithout a table lookup, as this class only contains two characters\nand two comparisons should be quite fast.\n\nStepping back a bit: Is using the unlocalized is* macros in\nwildmatch() safe, i.e. do we get the same results as before\nregardless of locale?  Junio's remark in\nhttps://lore.kernel.org/git/xmqq3579crsd.fsf@gitster.g/ sounds\nconvincing to me if we don't care about single-byte code pages\nand require plain ASCII or UTF-8.  I think it's a good idea to\naddress that point in the commit message.\n\nAnd adding tests to t/helper/test-ctype.c would be nice.\n\n> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>\n> ---\n>\n> Changes in v2:\n>   - Use inline functions\n>\n>  git-compat-util.h | 14 ++++++++++++++\n>  wildmatch.c       | 14 ++------------\n>  2 files changed, 16 insertions(+), 12 deletions(-)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 4f0028ce60..b29c238f02 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -1212,10 +1212,12 @@ extern const unsigned char tolower_trans_tbl[256];\n>  /* Sane ctype - no locale, and works with signed chars */\n>  #undef isascii\n>  #undef isspace\n> +#undef isblank\n>  #undef isdigit\n>  #undef isalpha\n>  #undef isalnum\n>  #undef isprint\n> +#undef isgraph\n>  #undef islower\n>  #undef isupper\n>  #undef tolower\n> @@ -1236,10 +1238,12 @@ extern const unsigned char sane_ctype[256];\n>  #define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)\n>  #define isascii(x) (((x) & ~0x7f) == 0)\n>  #define isspace(x) sane_istest(x,GIT_SPACE)\n> +#define isblank(x) sane_isblank(x)\n>  #define isdigit(x) sane_istest(x,GIT_DIGIT)\n>  #define isalpha(x) sane_istest(x,GIT_ALPHA)\n>  #define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)\n>  #define isprint(x) ((x) >= 0x20 && (x) <= 0x7e)\n> +#define isgraph(x) sane_isgraph(x)\n>  #define islower(x) sane_iscase(x, 1)\n>  #define isupper(x) sane_iscase(x, 0)\n>  #define is_glob_special(x) sane_istest(x,GIT_GLOB_SPECIAL)\n> @@ -1270,6 +1274,16 @@ static inline int sane_iscase(int x, int is_lower)\n>  \t\treturn (x & 0x20) == 0;\n>  }\n>\n> +static inline int sane_isblank(int c)\n> +{\n> +\treturn c == ' ' || c == '\\t';\n> +}\n> +\n> +static inline int sane_isgraph(int c)\n> +{\n> +\treturn isprint(c) && !isspace(c);\n> +}\n> +\n>  /*\n>   * Like skip_prefix, but compare case-insensitively. Note that the comparison\n>   * is done via tolower(), so it is strictly ASCII (no multi-byte characters or\n> diff --git a/wildmatch.c b/wildmatch.c\n> index 7e5a7ea1ea..85c4c7f8a7 100644\n> --- a/wildmatch.c\n> +++ b/wildmatch.c\n> @@ -28,18 +28,8 @@ typedef unsigned char uchar;\n>  # define ISASCII(c) isascii(c)\n>  #endif\n>\n> -#ifdef isblank\n> -# define ISBLANK(c) (ISASCII(c) && isblank(c))\n> -#else\n> -# define ISBLANK(c) ((c) == ' ' || (c) == '\\t')\n> -#endif\n> -\n> -#ifdef isgraph\n> -# define ISGRAPH(c) (ISASCII(c) && isgraph(c))\n> -#else\n> -# define ISGRAPH(c) (ISASCII(c) && isprint(c) && !isspace(c))\n> -#endif\n> -\n> +#define ISBLANK(c) (ISASCII(c) && isblank(c))\n> +#define ISGRAPH(c) (ISASCII(c) && isgraph(c))\n>  #define ISPRINT(c) (ISASCII(c) && isprint(c))\n>  #define ISDIGIT(c) (ISASCII(c) && isdigit(c))\n>  #define ISALNUM(c) (ISASCII(c) && isalnum(c))\n\n"},{"id":"472818","messageId":"xmqqpm9usxrz.fsf@gitster.g","threadId":"59312","inReplyTo":"36cd059e-c676-2aa2-68d9-41a7b0db57f0@web.de","subject":"Re: [PATCH v2 1/5] git-compat-util: add isblank() and isgraph()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-27T19:39:44Z","receivedAt":"2023-02-27T19:40:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>> In the previous submission, I just moved isblank() and isgraph() as\n>> implemented in wildmatch.c. I knew they were not robust against the\n>> pointer increment like isblank(*s++), but I thought it was the same\n>> pattern as isprint(), which has the same issue. Unfortunately, it was\n>> more controversial than I had expected...\n>\n> Not sure we need that story in the commit message,...\n\nUsually we encourage people to write as if there were no \"previous\niterations\".  Describe how to reach the best end-result without any\ndetour, using the experience gained from failed attempts.\n\n> ... but it gave me an idea:\n\n;-)\n\n> ...  So it's not so\n> easy, however, ...\n>\n>> This version implements them as inline functions because we ran out\n>> all bits in the sane_ctype[] table. This is the same pattern as\n>> islower() and isupper().\n>\n> ... if you remove GIT_SPACE from the definition above you get a\n> macro version of isgraph() that uses a single table lookup.\n>\n> If we're out of bits then isblank() is a good choice to implement\n> without a table lookup, as this class only contains two characters\n> and two comparisons should be quite fast.\n\nImplementing sane_isblank() plus sane_isgraph() as static inlines is\nalready not too bad, but if one of them can be made into a simple\ntable look-up, that indeed is better ;-)\n\n> Stepping back a bit: Is using the unlocalized is* macros in\n> wildmatch() safe, i.e. do we get the same results as before\n> regardless of locale?  Junio's remark in\n> https://lore.kernel.org/git/xmqq3579crsd.fsf@gitster.g/ sounds\n> convincing to me if we don't care about single-byte code pages\n> and require plain ASCII or UTF-8.  I think it's a good idea to\n> address that point in the commit message.\n\nTrue.  To be honest, I wasn't thinking about non-UTF-8 single-byte\n\"code pages\".  In the context of wildmatch, the strings we deal with\nmostly interact with the paths on the filesystem.  If you interact\nwith your filesystem in latin-1 that may pose an issue, and even if\n(or rather, especially if) we are to declare that it does not matter,\nwe should document it in the proposed log message.\n>\n> And adding tests to t/helper/test-ctype.c would be nice.\n\nVery true.\n\n>> +#define isblank(x) sane_isblank(x)\n>> +#define isgraph(x) sane_isgraph(x)\n>> @@ -1270,6 +1274,16 @@ static inline int sane_iscase(int x, int is_lower)\n>>  \t\treturn (x & 0x20) == 0;\n>>  }\n>>\n>> +static inline int sane_isblank(int c)\n>> +{\n>> +\treturn c == ' ' || c == '\\t';\n>> +}\n>> +\n>> +static inline int sane_isgraph(int c)\n>> +{\n>> +\treturn isprint(c) && !isspace(c);\n>> +}\n"},{"id":"472820","messageId":"xmqqh6v6swi6.fsf@gitster.g","threadId":"59312","inReplyTo":"20230226115021.1681834-5-masahiroy@kernel.org","subject":"Re: [PATCH v2 4/5] wildmatch: use char instead of uchar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-27T20:07:13Z","receivedAt":"2023-02-27T20:07:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Masahiro Yamada <masahiroy@kernel.org> writes:\n\n> GIT has its own implementations, so the behavior is clear.\n>\n> In fact, commit 4546738b58a0 (\"Unlocalized isspace and friends\")\n> says one of the motivations is \"we want the right signed behaviour\".\n>\n> sane_istest() casts the given character to (unsigned char) anyway\n> before sane_ctype[] table lookup, so dowild() can use 'char'.\n\nUse of values taken from a char/uchar in sane_istest() is designed\nto be safe, and between using char*/uchar* to scan pieces of memory\nwould not make much difference, so changes like ...\n\n>  \t\tcase '*':\n>  \t\t\tif (*++p == '*') {\n> -\t\t\t\tconst uchar *prev_p = p - 2;\n> +\t\t\t\tconst char *prev_p = p - 2;\n\n... this is safe, and so is ...\n\n> -\t\t\t\tconst char *slash = strchr((char*)text, '/');\n> +\t\t\t\tconst char *slash = strchr(text, '/');\n\n... this.\n\nBut does the comparison between t_ch_upper and prev_ch behave the\nsame with and without this patch?\n\n> -\t\t\t\t\t\tuchar t_ch_upper = toupper(t_ch);\n> +\t\t\t\t\t\tchar t_ch_upper = toupper(t_ch);\n>  \t\t\t\t\t\tif (t_ch_upper <= p_ch && t_ch_upper >= prev_ch)\n\nHere t_ch is already known to pass islower(), so t_ch_upper would be\nwithin a reasonable range and \"char\" should be able to store it\nsafely without its value wrapping around to negative.  Do we have a\nsimilar guarantee on prev_ch, or can it be more than 128, which\nwould have caused the original to say \"t_ch_upper is smaller than\nprev_ch\" but now because prev_ch can wrap around to a negative\nvalue, leading the conditional to behave differently, or something?\n\n>  \t\t\t\t\t\t\tmatched = 1;\n>  \t\t\t\t\t}\n>  \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n>  \t\t\t\t} else if (p_ch == '[' && p[1] == ':') {\n> -\t\t\t\t\tconst uchar *s;\n> +\t\t\t\t\tconst char *s;\n>  \t\t\t\t\tint i;\n>  \t\t\t\t\tfor (s = p += 2; (p_ch = *p) && p_ch != ']'; p++) {} /*SHARED ITERATOR*/\n>  \t\t\t\t\tif (!p_ch)\n> @@ -237,5 +235,5 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n>  /* Match the \"pattern\" against the \"text\" string. */\n>  int wildmatch(const char *pattern, const char *text, unsigned int flags)\n>  {\n> -\treturn dowild((const uchar*)pattern, (const uchar*)text, flags);\n> +\treturn dowild(pattern, text, flags);\n>  }\n"}]}