{"thread":{"id":"41449","subject":"[PATCH] git-compat-util.h: move extension stripping from handle_builtin()","startedAt":"2016-02-20T08:10:58Z","lastAt":"2016-02-20T12:35:47Z","messageCount":3,"participants":["Alexander Kuleshov","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"278720","messageId":"1455955858-30081-1-git-send-email-kuleshovmail@gmail.com","threadId":"41449","inReplyTo":null,"subject":"[PATCH] git-compat-util.h: move extension stripping from handle_builtin()","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2016-02-20T08:10:58Z","receivedAt":"2016-02-20T08:10:58Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"The handle_builtin() starts from striping of command extension if\nSTRIP_EXTENSION is enabled. As this is an OS dependent, let's move\nto the git-compat-util.h as all similar functions to do handle_builtin()\nmore cleaner.\n---\n git-compat-util.h | 18 ++++++++++++++++++\n git.c             | 13 +------------\n 2 files changed, 19 insertions(+), 12 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 658d03b..57f2fda 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -323,6 +323,24 @@ extern char *gitbasename(char *);\n \n #ifndef STRIP_EXTENSION\n #define STRIP_EXTENSION \"\"\n+static inline const char *strip_extension(const char **argv)\n+{\n+\treturn argv[0];\n+}\n+#else\n+static inline const char *strip_extension(const char **argv)\n+{\n+\tstatic const char ext[] = STRIP_EXTENSION;\n+\tint ext_len = strlen(argv[0]) - strlen(ext);\n+\n+\tif (ext_len > 0 && !strcmp(argv[0] + ext_len, ext)) {\n+\t\tchar *argv0 = xstrdup(argv[0]);\n+\t\targv[0] = argv0;\n+\t\targv0[ext_len] = '\\0';\n+\t}\n+\n+\treturn argv[0];\n+}\n #endif\n \n #ifndef has_dos_drive_prefix\ndiff --git a/git.c b/git.c\nindex 8751ef0..a4d2a46 100644\n--- a/git.c\n+++ b/git.c\n@@ -506,19 +506,8 @@ int is_builtin(const char *s)\n \n static void handle_builtin(int argc, const char **argv)\n {\n-\tconst char *cmd = argv[0];\n-\tint i;\n-\tstatic const char ext[] = STRIP_EXTENSION;\n \tstruct cmd_struct *builtin;\n-\n-\tif (sizeof(ext) > 1) {\n-\t\ti = strlen(argv[0]) - strlen(ext);\n-\t\tif (i > 0 && !strcmp(argv[0] + i, ext)) {\n-\t\t\tchar *argv0 = xstrdup(argv[0]);\n-\t\t\targv[0] = cmd = argv0;\n-\t\t\targv0[i] = '\\0';\n-\t\t}\n-\t}\n+\tconst char *cmd = strip_extension(argv);\n \n \t/* Turn \"git cmd --help\" into \"git help cmd\" */\n \tif (argc > 1 && !strcmp(argv[1], \"--help\")) {\n-- \n2.4.4.764.gf6c74eb.dirty\n"},{"id":"278724","messageId":"20160220084804.GB17171@sigill.intra.peff.net","threadId":"41449","inReplyTo":"1455955858-30081-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: [PATCH] git-compat-util.h: move extension stripping from handle_builtin()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-20T08:48:04Z","receivedAt":"2016-02-20T08:48:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 20, 2016 at 02:10:58PM +0600, Alexander Kuleshov wrote:\n\n> The handle_builtin() starts from striping of command extension if\n> STRIP_EXTENSION is enabled. As this is an OS dependent, let's move\n> to the git-compat-util.h as all similar functions to do handle_builtin()\n> more cleaner.\n\nI'm not convinced that moving the functions inline into git-compat-util\nis actually cleaner. We've expanded the interface that is visible to the\nwhole code base, warts at all.\n\nOne wart I see is that the caller cannot know whether the return value\nwas newly allocated or not, and therefore cannot free it, creating a\npotential memory leak. Another is that the return value is not really\nnecessary at all; we always munge argv[0].\n\nDoes any other part of the code actually care about this\nextension-stripping?\n\nPerhaps instead, could we do this:\n\n>  git-compat-util.h | 18 ++++++++++++++++++\n>  git.c             | 13 +------------\n>  2 files changed, 19 insertions(+), 12 deletions(-)\n> \n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 658d03b..57f2fda 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -323,6 +323,24 @@ extern char *gitbasename(char *);\n>  \n>  #ifndef STRIP_EXTENSION\n>  #define STRIP_EXTENSION \"\"\n> +static inline const char *strip_extension(const char **argv)\n> +{\n> +\treturn argv[0];\n> +}\n> +#else\n> +static inline const char *strip_extension(const char **argv)\n> +{\n> +\tstatic const char ext[] = STRIP_EXTENSION;\n> +\tint ext_len = strlen(argv[0]) - strlen(ext);\n> +\n> +\tif (ext_len > 0 && !strcmp(argv[0] + ext_len, ext)) {\n> +\t\tchar *argv0 = xstrdup(argv[0]);\n> +\t\targv[0] = argv0;\n> +\t\targv0[ext_len] = '\\0';\n> +\t}\n> +\n> +\treturn argv[0];\n> +}\n>  #endif\n\nIf we drop this default-to-empty value of STRIP_EXTENSION entirely, then\nwe can do our #ifdef local to git.c, where it does not bother anybody\nelse. Like:\n\n  #ifdef STRIP_EXTENSION\n  static void strip_extension(const char **argv)\n  {\n\t/* Do the thing */\n  }\n  #else\n  #define strip_extension(x)\n  #endif\n\n(Note that I also simplified the return value).\n\nIn the case that we do have STRIP_EXTENSION, I don't think we need to\nhandle the empty-string case. It would be a regression for somebody\npassing -DSTRIP_EXTENSION=\"\", but I don't think that's worth worrying\nabout. That macro is defined totally internally.\n\nI suspect you could also use strip_suffix here. So something like:\n\n  size_t len;\n\n  if (strip_suffix(str, STRIP_EXTENSION, &len))\n\targv[0] = xmemdupz(argv[0], len);\n\nwould probably work, but that's totally untested.\n\n-Peff\n"},{"id":"278731","messageId":"20160220123547.GB1389@localhost","threadId":"41449","inReplyTo":"20160220084804.GB17171@sigill.intra.peff.net","subject":"Re: [PATCH] git-compat-util.h: move extension stripping from handle_builtin()","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2016-02-20T12:35:47Z","receivedAt":"2016-02-20T12:35:47Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Hello Jeff,\n\nOn 02-20-16, Jeff King wrote:\n> On Sat, Feb 20, 2016 at 02:10:58PM +0600, Alexander Kuleshov wrote:\n> \n> I'm not convinced that moving the functions inline into git-compat-util\n> is actually cleaner. We've expanded the interface that is visible to the\n> whole code base, warts at all.\n> \n> One wart I see is that the caller cannot know whether the return value\n> was newly allocated or not, and therefore cannot free it, creating a\n> potential memory leak. Another is that the return value is not really\n> necessary at all; we always munge argv[0].\n> \n> Does any other part of the code actually care about this\n> extension-stripping?\n\nNope, only this one.\n\n> \n> Perhaps instead, could we do this:\n> If we drop this default-to-empty value of STRIP_EXTENSION entirely, then\n> we can do our #ifdef local to git.c, where it does not bother anybody\n> else. Like:\n> \n>   #ifdef STRIP_EXTENSION\n>   static void strip_extension(const char **argv)\n>   {\n> \t/* Do the thing */\n>   }\n>   #else\n>   #define strip_extension(x)\n>   #endif\n> \n> (Note that I also simplified the return value).\n> \n> In the case that we do have STRIP_EXTENSION, I don't think we need to\n> handle the empty-string case. It would be a regression for somebody\n> passing -DSTRIP_EXTENSION=\"\", but I don't think that's worth worrying\n> about. That macro is defined totally internally.\n> \n> I suspect you could also use strip_suffix here. So something like:\n> \n>   size_t len;\n> \n>   if (strip_suffix(str, STRIP_EXTENSION, &len))\n> \targv[0] = xmemdupz(argv[0], len);\n> \n> would probably work, but that's totally untested.\n\nGood suggestion. I will try to do it and test.\n\nThank you.\n"}]}