Re: [PATCH 1/5] git-compat-util: add isblank() and isgraph()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 10, 2023, 13:16 UTC
Message-ID: <230210.861qmxwtbz.gmgdl@evledraar.gmail.com>
In-Reply-To: <20230210075939.44949-2-masahiroy@kernel.org>

On Fri, Feb 10 2023, Masahiro Yamada wrote:

> git-compat-util.h implements most of is*() macros.
>
> Add isblank() and isgraph(), which are useful to clean up wildmatch.c
> in a consistent way (in this and later commits).

You are on a journey to fix wildmatch.c, so...

Show 22 quoted lines

> The same issue already exists for isspace().
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> [...]
> -#ifdef isblank
> -# define ISBLANK(c) (ISASCII(c) && isblank(c))
> -#else
> -# define ISBLANK(c) ((c) == ' ' || (c) == '\t')
> -#endif
> -
> -#ifdef isgraph
> -# define ISGRAPH(c) (ISASCII(c) && isgraph(c))
> -#else
> -# define ISGRAPH(c) (ISASCII(c) && isprint(c) && !isspace(c))
> -#endif
> -
> +#define ISBLANK(c) (ISASCII(c) && isblank(c))
> +#define ISGRAPH(c) (ISASCII(c) && isgraph(c))
>  #define ISPRINT(c) (ISASCII(c) && isprint(c))
>  #define ISDIGIT(c) (ISASCII(c) && isdigit(c))
>  #define ISALNUM(c) (ISASCII(c) && isalnum(c))

You make this change, but not others in tree.

Personally I wouldn't mind seeing this expanded to fix the various other
trivially convertable cases in-tree, e.g.:
	
	$ git -P grep -n "(' '|'\\\t').*\|\|.*(' '|'\\\t')"
	builtin/am.c:602:               if (*sb.buf == '\t' || *sb.buf == ' ')
	compat/regex/regex_internal.h:60:# define isblank(ch) ((ch) == ' ' || (ch) == '\t')
	compat/regex/regex_internal.h:73:   return (c == ' ' || c == '\t');
	config.c:893:   while (c == ' ' || c == '\t')
	fsck.c:837:     while (*p == ' ' || *p == '\t')
	mailinfo.c:749:     (line->buf[0] == ' ' || line->buf[0] == '\t')) {
	sequencer.c:2476:                (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p) &&
	sequencer.c:2536:                  (*bol == ' ' || *bol == '\t')) {
	sequencer.c:2540:                                 (*bol == ' ' || *bol == '\t')) {
	sequencer.c:2594:       if (is_command(TODO_PICK, &bol) && (*bol == ' ' || *bol == '\t'))
	sequencer.c:2597:                (*bol == ' ' || *bol == '\t'))
	t/helper/test-json-writer.c:443:                if (c == '\n' || c == '\r' || c == ' ' || c == '\t')
	t/helper/test-json-writer.c:449:        while (*buf == ' ' || *buf == '\t')
	t/t4256/1/mailinfo.c:737:           (line->buf[0] == ' ' || line->buf[0] == '\t')) {
	t/t4256/1/mailinfo.c.orig:726:      (line->buf[0] == ' ' || line->buf[0] == '\t')) {
	trailer.c:630:          if (c != line && (*c == ' ' || *c == '\t')) {
	wildmatch.c:34:# define ISBLANK(c) ((c) == ' ' || (c) == '\t')

Some of those are false positves, but most are not.