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 Mon, Feb 02, 2026 at 09:19:01PM -0800, Collin Funk wrote:
>
>> Unsure if this should be tagged [RFC], but this patch clears up lots
>> of warning spam with glibc 2.43 because of a change mentioned in the
>> commit message.
>
> Thanks for the heads-up. I can reproduce here by installing glibc 2.43
> via "apt install -t experimental libc6" on my debian unstable machine.
>
>> I plan to handle the rest of them and try to organize the changes by
>> subsystem, for lack of a better term. But I figured it was best to
>> submit just this one for review first.
>
> Wow, there's...a lot of spots. Looks like ~65 of them based on my hacky
> first-pass. Many of them are quite obvious "s/char/const char/" fixes in
> variable declarations, that should have been const all along. I think
> those can all go together in one patch, as the compiler can verify that
> we never try to write to the result.

Yep, it is quite noisy.

And that plan makes sense to me. I'll create a seperate patch handling
the obvious 's/char/const char/' conversions that make sense regardless
of this glibc change.

> And then, yeah, I'd do the tricky ones system by system. Some of the
> ones that do write to the resulting pointers are rather nasty, and seem
> to fall into one of two camps:
>
>   1. Some function interface takes a const pointer, even though we try
>      to write to it under the hood (after laundering it through strchr()
>      or similar). I think it would be worth refactoring these interfaces
>      when we can, though some of them are pretty questionable. For
>      instance, all of the rev-parse/revision.c "dotdot" parsing works on
>      a "const char *arg". Surely we feed this from command line options
>      in some cases? I guess argv is guaranteed to be writable by the
>      standard, though we tend to treat is as const everywhere.

Yes, I see. I think the "arg" there is from the command line or from a
buffer read using fgets() in get_object_list from
builtin/pack-objects.c, so it is safe to write to there.

It's also called like this though:

    handle_revision_arg("HEAD", &revs, 0, 0);

We can't write to the string "HEAD", but it doesn't have a "dotdot" so
we don't. It could probably be cleaned up a bit.

FYI, that code would also be made much clearer if not all of the
declarations were at the top of the function. I guess it just hasn't
been touched in a long while.

>   2. We know we have a non-const pointer, but it is passed through a
>      const pointer that is used as an out-parameter to a function like
>      skip_prefix(). For instance, in http.c's redact_sensitive_header()
>      we have something like this:
>
>         const char *sensitive_header;
> 	if (skip_iprefix(header->buf, "Cookie:", &sensitive_header)) {
> 		const char *cookie = sensitive_header;
> 		char *semicolon = strchr(cookie, "; ");
> 		*semicolon = 0;
> 		...
>
>      Our header->buf here is a strbuf, so we know we are working with a
>      non-const buffer. We launder away constness with the strchr()
>      assignment to "semicolon", which glibc now complains about. We
>      should make "cookie" non-const, which is easy. But now we'll get a
>      complaint about assigning the const "sensitive_header" to it. And
>      that one should _also_ be non-const, because it comes from
>      header->buf. But switching it will cause the compiler to complain
>      about passing it to skip_iprefix().
>
>      So we have the problem in reverse (instead of laundering a const
>      string to a non-const, we've accidentally added constness where it
>      is not needed). If we drop the const from skip_iprefix(), then that
>      has fallout in all the other spots that do pass in a const haystack
>      parameter.
>
>      I don't know what the right solution is here. I guess the best we
>      can do is probably adding casts with comments like "this is OK
>      because it comes from...". But I'm not sure if we are better to
>      cast away the constness in one spot, or to make all of the
>      variables non-const and cast the out-parameter to skip_iprefix().

Makes sense, I'll try to handle the non-obviously ones separately.

>
>>  #ifndef find_last_dir_sep
>> -static inline char *git_find_last_dir_sep(const char *path)
>> +static inline const char *git_find_last_dir_sep(const char *path)
>>  {
>>  	return strrchr(path, '/');
>>  }
>
> This kind of recreates that reverse problem again, though: any caller
> who really does have a non-const "path" will get "const" added back into
> it. And that leads to casts like...

I figured it was okay since only one place casted the qualifier away.
But I agree it is probably worth cleaning that up later.

>>  static int chop_last_dir(char **remoteurl, int is_relative)
>>  {
>> -	char *rfind = find_last_dir_sep(*remoteurl);
>> +	char *rfind = (char *) find_last_dir_sep(*remoteurl);
>>  	if (rfind) {
>>  		*rfind = '\0';
>>  		return 0;
>
> ...this one. Can we implement it as a macro? That lets the compiler do
> the right thing, because we do not declare any type then. It used to be
> a macro, but switched in bf7283465b (turn path macros into inline
> function, 2014-08-16). There's also a level of macro indirection; on
> Windows this expands to win32_find_last_dir_sep(), which of course casts
> away the constness manually. ;)
>
> I also wonder if we could do some gcc/glibc-specific magic to get the
> best of both worlds. That is, could we get the same "the return value is
> const if the input parameter was" type-checking that is happening with
> strchr()?
>
> 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.

> Yuck. What a mess. I do think that fixing these warnings will improve
> most of the call-sites I looked at, but some of them get a bit hairy.

Thanks to C23. :)

Collin

[1] https://en.cppreference.com/w/c/language/generic.html