On Sat, Feb 20, 2016 at 02:10:58PM +0600, Alexander Kuleshov wrote:
> The handle_builtin() starts from striping of command extension if
> STRIP_EXTENSION is enabled. As this is an OS dependent, let's move
> to the git-compat-util.h as all similar functions to do handle_builtin()
> more cleaner.
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?
Perhaps instead, could we do this:
> git-compat-util.h | 18 ++++++++++++++++++
> git.c | 13 +------------
> 2 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 658d03b..57f2fda 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -323,6 +323,24 @@ extern char *gitbasename(char *);
>
> #ifndef STRIP_EXTENSION
> #define STRIP_EXTENSION ""
> +static inline const char *strip_extension(const char **argv)
> +{
> + return argv[0];
> +}
> +#else
> +static inline const char *strip_extension(const char **argv)
> +{
> + static const char ext[] = STRIP_EXTENSION;
> + int ext_len = strlen(argv[0]) - strlen(ext);
> +
> + if (ext_len > 0 && !strcmp(argv[0] + ext_len, ext)) {
> + char *argv0 = xstrdup(argv[0]);
> + argv[0] = argv0;
> + argv0[ext_len] = '\0';
> + }
> +
> + return argv[0];
> +}
> #endif
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.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
