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

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

[Alexander Kuleshov]

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.
---
 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
 
 #ifndef has_dos_drive_prefix
diff --git a/git.c b/git.c
index 8751ef0..a4d2a46 100644
--- a/git.c
+++ b/git.c
@@ -506,19 +506,8 @@ int is_builtin(const char *s)
 
 static void handle_builtin(int argc, const char **argv)
 {
-	const char *cmd = argv[0];
-	int i;
-	static const char ext[] = STRIP_EXTENSION;
 	struct cmd_struct *builtin;
-
-	if (sizeof(ext) > 1) {
-		i = strlen(argv[0]) - strlen(ext);
-		if (i > 0 && !strcmp(argv[0] + i, ext)) {
-			char *argv0 = xstrdup(argv[0]);
-			argv[0] = cmd = argv0;
-			argv0[i] = '\0';
-		}
-	}
+	const char *cmd = strip_extension(argv);
 
 	/* Turn "git cmd --help" into "git help cmd" */
 	if (argc > 1 && !strcmp(argv[1], "--help")) {
-- 
2.4.4.764.gf6c74eb.dirty

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

[Jeff King]

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

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

[Alexander Kuleshov]

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.