Re: [PATCH] git-compat-util: make git_find_last_dir_sep return a const pointer

[Collin Funk <collin.funk1@gmail.com>]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 19, 2026 at 12:06:43PM +0100, Toon Claes wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >> > Looking at strchr()'s declaration in string.h, which is defined like:
>> >> >
>> >> >   #  define strchr(S, C)                                          \
>> >> >     __glibc_const_generic (S, const char *, strchr (S, C))
>> >> >
>> >> > I think the answer is probably "yes". But it also doesn't quite solve
>> >> > our problem. That would give us type-checking of callers of our
>> >> > function, but we still have to convince the compiler not to complain
>> >> > about its implementation. For that we'd need to either cast away const
>> >> > manually, I guess.
>> >> 
>> >> That macro depends on Generic selections from C11 [1]. I wasn't sure if
>> >> Git would like that, given it is conservative with other C features.
>> >
>> > We definitely can't rely on it everywhere. But if there is a solution
>> > that is conditionally compiled, and can kick in only when these extra
>> > warnings also kick in, that would be OK. Assuming the result is not too
>> > painful to look at, of course.
>> 
>> So the Git project would be okay to conditionally compile with Generic
>> selections if the compiler supports it? Seems to me this is the easiest
>> way forward to silence the errors for users who see these warnings (that
>> includes me).
>
> Yes, though I think just turning it into a macro is enough to silence
> this particular case (because macros don't have types, and so the
> compiler sees the original types passed to strchr). And as you noted,
> there are a ton of other cases that have to be looked at individually,
> which I think is the real blocker.
>
>> I did not look into any of them, but I think you (Collin) have sent out
>> patches for various of these? But they _should_ managable to address?
>
> I have quick-and-dirty fixes for these at:
>
>   https://github.com/peff/git jk/hacky-strchr-fixes
>
> I haven't been cleaning them up and sending in patches because I didn't
> want to duplicate work Collin was doing. But Collin, let us know if we
> can contribute. Dealing with the warnings is an occasional hassle during
> other work.

Please feel free to work on it. I fixed two more simple cases which are
in Junio's "next" branch. I also have one patch that that I sent on list
but is pending review [1].

I had planned to do more, but got busy working on other things. Sorry
about that.

> If you're using gcc, you can solve it by just adding
> -Wno-discarded-qualifiers to your CFLAGS. But clang doesn't know about
> that warning. Worse, if you sometimes compile with -std=c99 (which is
> necessary to build versions of Git older than e8b3bcf491) then glibc's
> preprocessor conditionals don't kick in correctly and you get:
>
>   ./git-compat-util.h:344:9: warning: returning 'const char *' from a function with result type 'char *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
>     344 |         return strrchr(path, '/');
>         |                ^~~~~~~~~~~~~~~~~~
>   /usr/include/string.h:296:3: note: expanded from macro 'strrchr'
>     296 |   __glibc_const_generic (S, const char *, strrchr (S, C))
>         |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   /usr/include/x86_64-linux-gnu/sys/cdefs.h:838:3: note: expanded from macro '__glibc_const_generic'
>     838 |   _Generic (0 ? (PTR) : (void *) 1,                     \
>         |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     839 |             const void *: (CTYPE) (CALL),               \
>         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     840 |             default: CALL)
>         |             ~~~~~~~~~~~~~~
>
> Yuck. That is not even specific to Git, and is hopefully something that
> glibc and clang folks might figure out.

I think something not immediately obvious is happening here. Hopefully
my explanation helps you.

Here is how the macro is defined in string.h:

    # if __GLIBC_USE (ISOC23) && defined __glibc_const_generic && !defined _LIBC
    #  define strrchr(S, C)						\
      __glibc_const_generic (S, const char *, strrchr (S, C))
    # endif

There are two cases that __GLIBC_USE (ISOC23) evaluates to true. The
first is if you use '-std=c23'/'-std=gnu23' (or have a compiler that
uses that version by default). The second, which I expect is happening
in your case, is if _GNU_SOURCE is defined. That flag causes GNU
extensions to be declared along with the latest standards.

Here is an example:

    $ cat main.c
    #include <string.h>
    int
    main (void)
    {
      const char *s = "hello";
      char *p = strchr (s, 'h');
      return 0;
    }
    $ gcc -std=c99 main.c
    $ gcc -std=c99 -D_GNU_SOURCE main.c
    main.c: In function ‘main’:
    main.c:6:13: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
        6 |   char *p = strchr (s, 'h');
          |             ^~~~~~

I can modify the file like this:

    $ cat main.c 
    #include <sys/cdefs.h>
    #undef __glibc_const_generic
    #include <string.h>
    int
    main (void)
    {
      const char *s = "hello";
      char *p = strchr (s, 'h');
      return 0;
    }
    $ gcc -std=c99 main.c
    $ gcc -std=c99 -D_GNU_SOURCE main.c

For context, this change has caused myself and the other Gnulib
maintainers some trouble as well. It broke the builds of many GNU
packages that vendor Gnulib (coreutils, m4, hello, and the list goes
on). Patching things to include sys/cdefs.h and undefining
__glibc_const_generic was a workaround we considered, but luckily it
didn't take too much time to make new releases for the affected packages.

Collin

[1] https://lore.kernel.org/git/d3447c19d83c37bf2db84ae0bf75801ef7a36cea.1773115420.git.collin.funk1@gmail.com/