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

[Masahiro Yamada <masahiroy@kernel.org>]

On Fri, Feb 10, 2023 at 10:20 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> 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...
>
> > 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


I wouldn't mind seeing somebody use this for tree-wide cleanups.
I do not see any reason to do that in this patch set, either.





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



--
Best Regards
Masahiro Yamada