Re: [PATCH] git-compat-util.h: move extension stripping from handle_builtin()

[Alexander Kuleshov <kuleshovmail@gmail.com>]

Hello Jeff,

On 02-20-16, Jeff King wrote:
> On Sat, Feb 20, 2016 at 02:10:58PM +0600, Alexander Kuleshov wrote:
> 
> I'm not convinced that moving the functions inline into git-compat-util
> is actually cleaner. We've expanded the interface that is visible to the
> whole code base, warts at all.
> 
> One wart I see is that the caller cannot know whether the return value
> was newly allocated or not, and therefore cannot free it, creating a
> potential memory leak. Another is that the return value is not really
> necessary at all; we always munge argv[0].
> 
> Does any other part of the code actually care about this
> extension-stripping?

Nope, only this one.

> 
> Perhaps instead, could we do this:
> If we drop this default-to-empty value of STRIP_EXTENSION entirely, then
> we can do our #ifdef local to git.c, where it does not bother anybody
> else. Like:
> 
>   #ifdef STRIP_EXTENSION
>   static void strip_extension(const char **argv)
>   {
> 	/* Do the thing */
>   }
>   #else
>   #define strip_extension(x)
>   #endif
> 
> (Note that I also simplified the return value).
> 
> In the case that we do have STRIP_EXTENSION, I don't think we need to
> handle the empty-string case. It would be a regression for somebody
> passing -DSTRIP_EXTENSION="", but I don't think that's worth worrying
> about. That macro is defined totally internally.
> 
> I suspect you could also use strip_suffix here. So something like:
> 
>   size_t len;
> 
>   if (strip_suffix(str, STRIP_EXTENSION, &len))
> 	argv[0] = xmemdupz(argv[0], len);
> 
> would probably work, but that's totally untested.

Good suggestion. I will try to do it and test.

Thank you.