{"thread":{"id":"10405","subject":"[PATCH] \"git help -a\" should search all exec_paths and PATH","startedAt":"2007-10-21T21:48:46Z","lastAt":"2007-10-22T06:39:33Z","messageCount":7,"participants":["Scott R Parish","Johannes Schindelin","Scott Parish","Shawn O. Pearce","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"56780","messageId":"20071021214846.GI16291@srparish.net","threadId":"10405","inReplyTo":null,"subject":"[PATCH] \"git help -a\" should search all exec_paths and PATH","fromName":"Scott R Parish","fromEmail":"srp@srparish.net","sentAt":"2007-10-21T21:48:46Z","receivedAt":"2007-10-21T21:48:46Z","isPatch":true,"sender":{"key":"srp@srparish.net","avatar":"https://gravatar.com/avatar/870e5b6fc4f710cf4db5684bd9af7f2cee5734b3dab3209b13e00cf64f6c9f0e?d=mp&s=160"},"body":"Currently \"git help -a\" only searches in the highest priority exec_path,\nmeaning at worst, nothing is listed if the git commands are only available\nfrom the PATH. It also makes git slightly less extensible.\n\nTo fix this, help.c is modified to search in all the exec_paths and PATH\nfor potential git commands. So that it has access to all the exec_paths,\nexec_cmd.c now exposes the various paths. \"current_exec_path\" is renamed\nas its name is misleading.\n\nSigned-off-by: Scott R Parish <srp@srparish.net>\n---\n exec_cmd.c |   26 +++++++++++---\n exec_cmd.h |    3 ++\n git.c      |    2 +-\n help.c     |  103 +++++++++++++++++++++++++++++++++++++++++++-----------------\n 4 files changed, 98 insertions(+), 36 deletions(-)\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 374ffc9..2c787a4 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -5,21 +5,35 @@\n \n extern char **environ;\n static const char *builtin_exec_path = GIT_EXEC_PATH;\n-static const char *current_exec_path;\n+static const char *argv_exec_path;\n \n-void git_set_exec_path(const char *exec_path)\n+void git_set_argv_exec_path(const char *exec_path)\n {\n-\tcurrent_exec_path = exec_path;\n+\targv_exec_path = exec_path;\n }\n \n+const char *git_argv_exec_path(void)\n+{\n+\treturn argv_exec_path;\n+}\n+\n+const char *git_builtin_exec_path(void)\n+{\n+\treturn builtin_exec_path;\n+}\n+\n+const char *git_env_exec_path(void)\n+{\n+\treturn getenv(EXEC_PATH_ENVIRONMENT); \n+}\n \n /* Returns the highest-priority, location to look for git programs. */\n const char *git_exec_path(void)\n {\n \tconst char *env;\n \n-\tif (current_exec_path)\n-\t\treturn current_exec_path;\n+\tif (argv_exec_path)\n+\t\treturn argv_exec_path;\n \n \tenv = getenv(EXEC_PATH_ENVIRONMENT);\n \tif (env && *env) {\n@@ -34,7 +48,7 @@ int execv_git_cmd(const char **argv)\n {\n \tchar git_command[PATH_MAX + 1];\n \tint i;\n-\tconst char *paths[] = { current_exec_path,\n+\tconst char *paths[] = { argv_exec_path,\n \t\t\t\tgetenv(EXEC_PATH_ENVIRONMENT),\n \t\t\t\tbuiltin_exec_path,\n \t\t\t\t\"\" };\ndiff --git a/exec_cmd.h b/exec_cmd.h\nindex 849a839..315fe83 100644\n--- a/exec_cmd.h\n+++ b/exec_cmd.h\n@@ -3,6 +3,9 @@\n \n extern void git_set_exec_path(const char *exec_path);\n extern const char* git_exec_path(void);\n+extern const char* git_argv_exec_path(void);\n+extern const char* git_builtin_exec_path(void);\n+extern const char* git_env_exec_path(void);\n extern int execv_git_cmd(const char **argv); /* NULL terminated */\n extern int execl_git_cmd(const char *cmd, ...);\n \ndiff --git a/git.c b/git.c\nindex 853e66c..b67fb17 100644\n--- a/git.c\n+++ b/git.c\n@@ -51,7 +51,7 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)\n \t\tif (!prefixcmp(cmd, \"--exec-path\")) {\n \t\t\tcmd += 11;\n \t\t\tif (*cmd == '=')\n-\t\t\t\tgit_set_exec_path(cmd + 1);\n+\t\t\t\tgit_set_argv_exec_path(cmd + 1);\n \t\t\telse {\n \t\t\t\tputs(git_exec_path());\n \t\t\t\texit(0);\ndiff --git a/help.c b/help.c\nindex b0d2dd4..85b2853 100644\n--- a/help.c\n+++ b/help.c\n@@ -93,37 +93,27 @@ static void pretty_print_string_list(struct cmdname **cmdname, int longest)\n \t}\n }\n \n-static void list_commands(const char *exec_path, const char *pattern)\n+static unsigned int list_commands_in_dir(const char *dir, const char *prefix)\n {\n \tunsigned int longest = 0;\n-\tchar path[PATH_MAX];\n-\tint dirlen;\n-\tDIR *dir = opendir(exec_path);\n+\tint start_dir = open(\".\", O_RDONLY, 0);\n+\tDIR *dirp = opendir(dir);\n \tstruct dirent *de;\n \n-\tif (!dir) {\n-\t\tfprintf(stderr, \"git: '%s': %s\\n\", exec_path, strerror(errno));\n-\t\texit(1);\n+\tif (!dirp || chdir(dir)) {\n+\t\tfchdir(start_dir);\n+\t\tclose(start_dir);\n+\t\treturn 0;\n \t}\n \n-\tdirlen = strlen(exec_path);\n-\tif (PATH_MAX - 20 < dirlen) {\n-\t\tfprintf(stderr, \"git: insanely long exec-path '%s'\\n\",\n-\t\t\texec_path);\n-\t\texit(1);\n-\t}\n-\n-\tmemcpy(path, exec_path, dirlen);\n-\tpath[dirlen++] = '/';\n-\n-\twhile ((de = readdir(dir)) != NULL) {\n+\twhile ((de = readdir(dirp)) != NULL) {\n \t\tstruct stat st;\n \t\tint entlen;\n-\n-\t\tif (prefixcmp(de->d_name, \"git-\"))\n+\t\t\t\n+\t\tif (prefixcmp(de->d_name, prefix))\n \t\t\tcontinue;\n-\t\tstrcpy(path+dirlen, de->d_name);\n-\t\tif (stat(path, &st) || /* stat, not lstat */\n+\n+\t\tif (stat(de->d_name, &st) || /* stat, not lstat */\n \t\t    !S_ISREG(st.st_mode) ||\n \t\t    !(st.st_mode & S_IXUSR))\n \t\t\tcontinue;\n@@ -137,12 +127,67 @@ static void list_commands(const char *exec_path, const char *pattern)\n \n \t\tadd_cmdname(de->d_name + 4, entlen-4);\n \t}\n-\tclosedir(dir);\n \n-\tprintf(\"git commands available in '%s'\\n\", exec_path);\n-\tprintf(\"----------------------------\");\n-\tmput_char('-', strlen(exec_path));\n-\tputchar('\\n');\n+\tclosedir(dirp);\n+\tfchdir(start_dir);\n+\tclose(start_dir);\n+\n+\treturn longest;\n+}\n+\n+static unsigned int list_commands_in_PATH(const char *prefix)\n+{\n+\tunsigned int longest = 0;\n+\tunsigned int len;\n+\tconst char *env_path = getenv(\"PATH\");\n+\tchar *paths, *path, *colon;\n+       \n+\tif (!env_path)\n+\t\treturn longest;\n+\n+\tpath = paths = xstrdup(env_path);\n+\n+\twhile ((char *)1 != path) {\n+\t\tif ((colon = strchr(path, ':')))\n+\t\t\t*colon = 0;\n+\n+\t\tlen = list_commands_in_dir(path, prefix);\n+\t\tlongest = MAX(longest, len);\n+\n+\t\tpath = colon + 1;\n+\t}\n+\n+\tfree(paths);\n+\treturn longest;\n+}\n+\n+static void list_commands(const char *prefix)\n+{\n+\tunsigned int longest = 0;\n+\tunsigned int len;\n+\tconst char *paths[] = { git_argv_exec_path(),\n+\t\t\t\tgit_env_exec_path(),\n+\t\t\t\tgit_builtin_exec_path(),\n+\t\t\t\t\"\" };\n+\tint i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(paths); i++) {\n+\t\tif (!paths[i])\n+\t\t\tcontinue;\n+\n+\t\tif (!*paths[i]) {\n+\t\t\t/* try PATH */\n+\t\t\tlen = list_commands_in_PATH(prefix);\n+\t\t\tlongest = MAX(longest, len);\n+\t\t}\n+\t\telse {\n+\t\t\tlen = list_commands_in_dir(paths[i], prefix);\n+\t\t\tlongest = MAX(longest, len);\n+\t\t}\n+\t}\n+\n+\tprintf(\"available git commands\\n\");\n+\tprintf(\"----------------------\\n\");\n \tpretty_print_string_list(cmdname, longest - 4);\n \tputchar('\\n');\n }\n@@ -158,7 +203,7 @@ static void list_common_cmds_help(void)\n \n \tputs(\"The most commonly used git commands are:\");\n \tfor (i = 0; i < ARRAY_SIZE(common_cmds); i++) {\n-\t\tprintf(\"   %s   \", common_cmds[i].name);\n+\t\tprintf(\"   %s\t\", common_cmds[i].name);\n \t\tmput_char(' ', longest - strlen(common_cmds[i].name));\n \t\tputs(common_cmds[i].help);\n \t}\n@@ -210,7 +255,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \telse if (!strcmp(help_cmd, \"--all\") || !strcmp(help_cmd, \"-a\")) {\n \t\tprintf(\"usage: %s\\n\\n\", git_usage_string);\n \t\tif(exec_path)\n-\t\t\tlist_commands(exec_path, \"git-*\");\n+\t\t\tlist_commands(\"git-\");\n \t\texit(0);\n \t}\n \n-- \n1.5.3.4.209.g5d1ce-dirty\n"},{"id":"56787","messageId":"Pine.LNX.4.64.0710212323580.25221@racer.site","threadId":"10405","inReplyTo":"20071021214846.GI16291@srparish.net","subject":"Re: [PATCH] \"git help -a\" should search all exec_paths and PATH","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-10-21T22:25:29Z","receivedAt":"2007-10-21T22:25:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 21 Oct 2007, Scott R Parish wrote:\n\n> Currently \"git help -a\" only searches in the highest priority exec_path, \n> meaning at worst, nothing is listed if the git commands are only \n> available from the PATH. It also makes git slightly less extensible.\n> \n> To fix this, help.c is modified to search in all the exec_paths and PATH\n> for potential git commands.\n\nWith this explanation, I would have expected that you add a loop just like \nin exec-cmd.c.  Not anything more.  And certainly not the removal of a \nsanity check for the length of the path name.\n\nCiao,\nDscho\n"},{"id":"56794","messageId":"20071022005429.GK16291@srparish.net","threadId":"10405","inReplyTo":"Pine.LNX.4.64.0710212323580.25221@racer.site","subject":"Re: [PATCH] \"git help -a\" should search all exec_paths and PATH","fromName":"Scott Parish","fromEmail":"srp@srparish.net","sentAt":"2007-10-22T00:54:30Z","receivedAt":"2007-10-22T00:54:30Z","isPatch":true,"sender":{"key":"srp@srparish.net","avatar":"https://gravatar.com/avatar/870e5b6fc4f710cf4db5684bd9af7f2cee5734b3dab3209b13e00cf64f6c9f0e?d=mp&s=160"},"body":"On Sun, Oct 21, 2007 at 11:25:29PM +0100, Johannes Schindelin wrote:\n\n> > To fix this, help.c is modified to search in all the exec_paths and PATH\n> > for potential git commands.\n> \n> With this explanation, I would have expected that you add a loop just like \n> in exec-cmd.c.  Not anything more.  And certainly not the removal of a \n> sanity check for the length of the path name.\n\nWell, i took a slightly different approach where that sanity check\nwasn't nessisary. Instead of building up a string of the path of\neach file, i'm saving the original directory in a file descriptor,\nand \"cd\"ing to the exec_path currently being listed. Because of\nthat i can just stat() the names returned by readdir. Much simpler\nimho\n\nsRp\n\n-- \nScott Parish\nhttp://srparish.net/\n"},{"id":"56804","messageId":"20071022053016.GN14735@spearce.org","threadId":"10405","inReplyTo":"20071021214846.GI16291@srparish.net","subject":"Re: [PATCH] \"git help -a\" should search all exec_paths and PATH","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-10-22T05:30:17Z","receivedAt":"2007-10-22T05:30:17Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Scott R Parish <srp@srparish.net> wrote:\n> Currently \"git help -a\" only searches in the highest priority exec_path,\n> meaning at worst, nothing is listed if the git commands are only available\n> from the PATH. It also makes git slightly less extensible.\n...\n>  extern char **environ;\n>  static const char *builtin_exec_path = GIT_EXEC_PATH;\n> -static const char *current_exec_path;\n> +static const char *argv_exec_path;\n>  \n> -void git_set_exec_path(const char *exec_path)\n> +void git_set_argv_exec_path(const char *exec_path)\n>  {\n> -\tcurrent_exec_path = exec_path;\n> +\targv_exec_path = exec_path;\n>  }\n\nI'd rather see a rename isolated from a logic change.  I find\nit easier to review.\n  \n> +const char *git_argv_exec_path(void)\n> +const char *git_builtin_exec_path(void)\n> +const char *git_env_exec_path(void)\n\nAnd yet later you then build the same priority array as already used\nby execv_git_cmd().  Why not just make a function that builds the\narray for the caller, so both execv_git_cmd() and list_commands()\ncan both use the same array?\n\n> +static unsigned int list_commands_in_dir(const char *dir, const char *prefix)\n>  {\n> +\tint start_dir = open(\".\", O_RDONLY, 0);\n...\n> +\tif (!dirp || chdir(dir)) {\n> +\t\tfchdir(start_dir);\n\nfchdir() isn't as portable as Git currently is.  Thus far we have\navoided using fchdir().  Requiring it here for something as \"simple\"\nas listing help is not a good improvement as it will limit who can\nrun git-help.  Why can't you stat the individual entries by joining\nthe paths together?\n\n-- \nShawn.\n"},{"id":"56820","messageId":"471C3DD4.90505@viscovery.net","threadId":"10405","inReplyTo":"20071021214846.GI16291@srparish.net","subject":"Re: [PATCH] \"git help -a\" should search all exec_paths and PATH","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-10-22T06:06:12Z","receivedAt":"2007-10-22T06:06:12Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Scott R Parish schrieb:\n> +\tif (!dirp || chdir(dir)) {\n> +\t\tfchdir(start_dir);\n\n/me dislikes this. Windows doesn't have fchdir().\n\nAFAIKS you are only using this to chdir back to where you started, but is \nthis necessary? Aren't we exiting anyway after the command list was printed?\n\n-- Hannes\n"},{"id":"56823","messageId":"20071022063201.GN16291@srparish.net","threadId":"10405","inReplyTo":"20071022053016.GN14735@spearce.org","subject":"Re: [PATCH] \"git help -a\" should search all exec_paths and PATH","fromName":"Scott Parish","fromEmail":"srp@srparish.net","sentAt":"2007-10-22T06:32:01Z","receivedAt":"2007-10-22T06:32:01Z","isPatch":true,"sender":{"key":"srp@srparish.net","avatar":"https://gravatar.com/avatar/870e5b6fc4f710cf4db5684bd9af7f2cee5734b3dab3209b13e00cf64f6c9f0e?d=mp&s=160"},"body":"On Mon, Oct 22, 2007 at 01:30:17AM -0400, Shawn O. Pearce wrote:\n\n> fchdir() isn't as portable as Git currently is.  Thus far we have\n> avoided using fchdir().  Requiring it here for something as \"simple\"\n> as listing help is not a good improvement as it will limit who can\n> run git-help.  Why can't you stat the individual entries by joining\n> the paths together?\n\nI hadn't realized it wasn't portable, but i do see that there's no POSIX\nentry in its man page. I was actually looking to use getcwd, but its\nman page had suggested using this open()/fchdir() method.\n\nAnyway, is there a reason to avoid changing the directory? If not\ni'm tempted to take the approach that j.sixt suggested--not restoring\nthe cwd since we're exiting anyway. I don't have any good reason\nto not do the string manipulation, but why do something more\ncomplicated then necessary?\n\nsRp\n\n-- \nScott Parish\nhttp://srparish.net/\n"},{"id":"56826","messageId":"20071022063933.GU14735@spearce.org","threadId":"10405","inReplyTo":"20071022063201.GN16291@srparish.net","subject":"Re: [PATCH] \"git help -a\" should search all exec_paths and PATH","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-10-22T06:39:33Z","receivedAt":"2007-10-22T06:39:33Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Scott Parish <sRp@srparish.net> wrote:\n> On Mon, Oct 22, 2007 at 01:30:17AM -0400, Shawn O. Pearce wrote:\n> \n> > fchdir() isn't as portable as Git currently is.\n> \n> I hadn't realized it wasn't portable, but i do see that there's no POSIX\n> entry in its man page. I was actually looking to use getcwd, but its\n> man page had suggested using this open()/fchdir() method.\n> \n> Anyway, is there a reason to avoid changing the directory? If not\n> i'm tempted to take the approach that j.sixt suggested--not restoring\n> the cwd since we're exiting anyway. I don't have any good reason\n> to not do the string manipulation, but why do something more\n> complicated then necessary?\n\nYea, that was another thought I had.  You probably can just chdir(),\nlist, exit, and not worry about going back to the previous directory.\nAnd more complicated is always a bad idea.  Keep it simple, 'cause\nus gits like it that way.  :-)\n\n-- \nShawn.\n"}]}