threads / patch / 10405

patch"git help -a" should search all exec_paths and PATH

Subject: [PATCH] "git help -a" should search all exec_paths and PATH

## tl;dr

7 messages between Oct 21, 2007 and Oct 22, 2007. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Scott R Parish· Oct 21, 2007, 21:48 UTC · lore

Currently "git help -a" only searches in the highest priority exec_path, meaning at worst, nothing is listed if the git commands are only available from the PATH. It also makes git slightly less extensible.

To fix this, help.c is modified to search in all the exec_paths and PATH for potential git commands. So that it has access to all the exec_paths, exec_cmd.c now exposes the various paths. "current_exec_path" is renamed as its name is misleading.

Signed-off-by: Scott R Parish <srp@srparish.net>
---
 exec_cmd.c |   26 +++++++++++---
 exec_cmd.h |    3 ++
 git.c      |    2 +-
 help.c     |  103 +++++++++++++++++++++++++++++++++++++++++++-----------------
 4 files changed, 98 insertions(+), 36 deletions(-)
Show changes to 4 files +98 −36

exec_cmd.c, exec_cmd.h, git.c, help.c

diff --git a/exec_cmd.c b/exec_cmd.c
index 374ffc9..2c787a4 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -5,21 +5,35 @@
 
 extern char **environ;
 static const char *builtin_exec_path = GIT_EXEC_PATH;
-static const char *current_exec_path;
+static const char *argv_exec_path;
 
-void git_set_exec_path(const char *exec_path)
+void git_set_argv_exec_path(const char *exec_path)
 {
-	current_exec_path = exec_path;
+	argv_exec_path = exec_path;
 }
 
+const char *git_argv_exec_path(void)
+{
+	return argv_exec_path;
+}
+
+const char *git_builtin_exec_path(void)
+{
+	return builtin_exec_path;
+}
+
+const char *git_env_exec_path(void)
+{
+	return getenv(EXEC_PATH_ENVIRONMENT); 
+}
 
 /* Returns the highest-priority, location to look for git programs. */
 const char *git_exec_path(void)
 {
 	const char *env;
 
-	if (current_exec_path)
-		return current_exec_path;
+	if (argv_exec_path)
+		return argv_exec_path;
 
 	env = getenv(EXEC_PATH_ENVIRONMENT);
 	if (env && *env) {
@@ -34,7 +48,7 @@ int execv_git_cmd(const char **argv)
 {
 	char git_command[PATH_MAX + 1];
 	int i;
-	const char *paths[] = { current_exec_path,
+	const char *paths[] = { argv_exec_path,
 				getenv(EXEC_PATH_ENVIRONMENT),
 				builtin_exec_path,
 				"" };
diff --git a/exec_cmd.h b/exec_cmd.h
index 849a839..315fe83 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -3,6 +3,9 @@
 
 extern void git_set_exec_path(const char *exec_path);
 extern const char* git_exec_path(void);
+extern const char* git_argv_exec_path(void);
+extern const char* git_builtin_exec_path(void);
+extern const char* git_env_exec_path(void);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 extern int execl_git_cmd(const char *cmd, ...);
 
diff --git a/git.c b/git.c
index 853e66c..b67fb17 100644
--- a/git.c
+++ b/git.c
@@ -51,7 +51,7 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 		if (!prefixcmp(cmd, "--exec-path")) {
 			cmd += 11;
 			if (*cmd == '=')
-				git_set_exec_path(cmd + 1);
+				git_set_argv_exec_path(cmd + 1);
 			else {
 				puts(git_exec_path());
 				exit(0);
diff --git a/help.c b/help.c
index b0d2dd4..85b2853 100644
--- a/help.c
+++ b/help.c
@@ -93,37 +93,27 @@ static void pretty_print_string_list(struct cmdname **cmdname, int longest)
 	}
 }
 
-static void list_commands(const char *exec_path, const char *pattern)
+static unsigned int list_commands_in_dir(const char *dir, const char *prefix)
 {
 	unsigned int longest = 0;
-	char path[PATH_MAX];
-	int dirlen;
-	DIR *dir = opendir(exec_path);
+	int start_dir = open(".", O_RDONLY, 0);
+	DIR *dirp = opendir(dir);
 	struct dirent *de;
 
-	if (!dir) {
-		fprintf(stderr, "git: '%s': %s\n", exec_path, strerror(errno));
-		exit(1);
+	if (!dirp || chdir(dir)) {
+		fchdir(start_dir);
+		close(start_dir);
+		return 0;
 	}
 
-	dirlen = strlen(exec_path);
-	if (PATH_MAX - 20 < dirlen) {
-		fprintf(stderr, "git: insanely long exec-path '%s'\n",
-			exec_path);
-		exit(1);
-	}
-
-	memcpy(path, exec_path, dirlen);
-	path[dirlen++] = '/';
-
-	while ((de = readdir(dir)) != NULL) {
+	while ((de = readdir(dirp)) != NULL) {
 		struct stat st;
 		int entlen;
-
-		if (prefixcmp(de->d_name, "git-"))
+			
+		if (prefixcmp(de->d_name, prefix))
 			continue;
-		strcpy(path+dirlen, de->d_name);
-		if (stat(path, &st) || /* stat, not lstat */
+
+		if (stat(de->d_name, &st) || /* stat, not lstat */
 		    !S_ISREG(st.st_mode) ||
 		    !(st.st_mode & S_IXUSR))
 			continue;
@@ -137,12 +127,67 @@ static void list_commands(const char *exec_path, const char *pattern)
 
 		add_cmdname(de->d_name + 4, entlen-4);
 	}
-	closedir(dir);
 
-	printf("git commands available in '%s'\n", exec_path);
-	printf("----------------------------");
-	mput_char('-', strlen(exec_path));
-	putchar('\n');
+	closedir(dirp);
+	fchdir(start_dir);
+	close(start_dir);
+
+	return longest;
+}
+
+static unsigned int list_commands_in_PATH(const char *prefix)
+{
+	unsigned int longest = 0;
+	unsigned int len;
+	const char *env_path = getenv("PATH");
+	char *paths, *path, *colon;
+       
+	if (!env_path)
+		return longest;
+
+	path = paths = xstrdup(env_path);
+
+	while ((char *)1 != path) {
+		if ((colon = strchr(path, ':')))
+			*colon = 0;
+
+		len = list_commands_in_dir(path, prefix);
+		longest = MAX(longest, len);
+
+		path = colon + 1;
+	}
+
+	free(paths);
+	return longest;
+}
+
+static void list_commands(const char *prefix)
+{
+	unsigned int longest = 0;
+	unsigned int len;
+	const char *paths[] = { git_argv_exec_path(),
+				git_env_exec_path(),
+				git_builtin_exec_path(),
+				"" };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(paths); i++) {
+		if (!paths[i])
+			continue;
+
+		if (!*paths[i]) {
+			/* try PATH */
+			len = list_commands_in_PATH(prefix);
+			longest = MAX(longest, len);
+		}
+		else {
+			len = list_commands_in_dir(paths[i], prefix);
+			longest = MAX(longest, len);
+		}
+	}
+
+	printf("available git commands\n");
+	printf("----------------------\n");
 	pretty_print_string_list(cmdname, longest - 4);
 	putchar('\n');
 }
@@ -158,7 +203,7 @@ static void list_common_cmds_help(void)
 
 	puts("The most commonly used git commands are:");
 	for (i = 0; i < ARRAY_SIZE(common_cmds); i++) {
-		printf("   %s   ", common_cmds[i].name);
+		printf("   %s	", common_cmds[i].name);
 		mput_char(' ', longest - strlen(common_cmds[i].name));
 		puts(common_cmds[i].help);
 	}
@@ -210,7 +255,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	else if (!strcmp(help_cmd, "--all") || !strcmp(help_cmd, "-a")) {
 		printf("usage: %s\n\n", git_usage_string);
 		if(exec_path)
-			list_commands(exec_path, "git-*");
+			list_commands("git-");
 		exit(0);
 	}
 
-- 
1.5.3.4.209.g5d1ce-dirty
Johannes Schindelin· Oct 21, 2007, 22:25 UTC · re: Scott R Parish · lore

Re: [PATCH] "git help -a" should search all exec_paths and PATH

Hi,
On Sun, 21 Oct 2007, Scott R Parish wrote:
Show 6 quoted lines
> Currently "git help -a" only searches in the highest priority exec_path, 
> meaning at worst, nothing is listed if the git commands are only 
> available from the PATH. It also makes git slightly less extensible.
> 
> To fix this, help.c is modified to search in all the exec_paths and PATH
> for potential git commands.

With this explanation, I would have expected that you add a loop just like in exec-cmd.c. Not anything more. And certainly not the removal of a sanity check for the length of the path name.

Ciao, Dscho

Scott Parish· Oct 22, 2007, 00:54 UTC · re: Johannes Schindelin · lore

Re: [PATCH] "git help -a" should search all exec_paths and PATH

On Sun, Oct 21, 2007 at 11:25:29PM +0100, Johannes Schindelin wrote:
Show 6 quoted lines
> > To fix this, help.c is modified to search in all the exec_paths and PATH
> > for potential git commands.
> 
> With this explanation, I would have expected that you add a loop just like 
> in exec-cmd.c.  Not anything more.  And certainly not the removal of a 
> sanity check for the length of the path name.

Well, i took a slightly different approach where that sanity check wasn't nessisary. Instead of building up a string of the path of each file, i'm saving the original directory in a file descriptor, and "cd"ing to the exec_path currently being listed. Because of that i can just stat() the names returned by readdir. Much simpler imho

sRp
-- 
Scott Parish
http://srparish.net/
Shawn O. Pearce· Oct 22, 2007, 05:30 UTC · re: Scott R Parish · lore

Re: [PATCH] "git help -a" should search all exec_paths and PATH

Scott R Parish <srp@srparish.net> wrote:
> Currently "git help -a" only searches in the highest priority exec_path,
> meaning at worst, nothing is listed if the git commands are only available
> from the PATH. It also makes git slightly less extensible.
...
Show 11 quoted lines
>  extern char **environ;
>  static const char *builtin_exec_path = GIT_EXEC_PATH;
> -static const char *current_exec_path;
> +static const char *argv_exec_path;
>  
> -void git_set_exec_path(const char *exec_path)
> +void git_set_argv_exec_path(const char *exec_path)
>  {
> -	current_exec_path = exec_path;
> +	argv_exec_path = exec_path;
>  }
I'd rather see a rename isolated from a logic change.  I find
it easier to review.
  
> +const char *git_argv_exec_path(void)
> +const char *git_builtin_exec_path(void)
> +const char *git_env_exec_path(void)

And yet later you then build the same priority array as already used by execv_git_cmd(). Why not just make a function that builds the array for the caller, so both execv_git_cmd() and list_commands() can both use the same array?

> +static unsigned int list_commands_in_dir(const char *dir, const char *prefix)
>  {
> +	int start_dir = open(".", O_RDONLY, 0);
...
> +	if (!dirp || chdir(dir)) {
> +		fchdir(start_dir);

fchdir() isn't as portable as Git currently is. Thus far we have avoided using fchdir(). Requiring it here for something as "simple" as listing help is not a good improvement as it will limit who can run git-help. Why can't you stat the individual entries by joining the paths together?

-- 
Shawn.
Scott Parish· Oct 22, 2007, 06:32 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] "git help -a" should search all exec_paths and PATH

On Mon, Oct 22, 2007 at 01:30:17AM -0400, Shawn O. Pearce wrote:
Show 5 quoted lines
> fchdir() isn't as portable as Git currently is.  Thus far we have
> avoided using fchdir().  Requiring it here for something as "simple"
> as listing help is not a good improvement as it will limit who can
> run git-help.  Why can't you stat the individual entries by joining
> the paths together?

I hadn't realized it wasn't portable, but i do see that there's no POSIX entry in its man page. I was actually looking to use getcwd, but its man page had suggested using this open()/fchdir() method.

Anyway, is there a reason to avoid changing the directory? If not i'm tempted to take the approach that j.sixt suggested--not restoring the cwd since we're exiting anyway. I don't have any good reason to not do the string manipulation, but why do something more complicated then necessary?

sRp
-- 
Scott Parish
http://srparish.net/
Shawn O. Pearce· Oct 22, 2007, 06:39 UTC · re: Scott Parish · lore

Re: [PATCH] "git help -a" should search all exec_paths and PATH

Scott Parish <sRp@srparish.net> wrote:
Show 13 quoted lines
> On Mon, Oct 22, 2007 at 01:30:17AM -0400, Shawn O. Pearce wrote:
> 
> > fchdir() isn't as portable as Git currently is.
> 
> I hadn't realized it wasn't portable, but i do see that there's no POSIX
> entry in its man page. I was actually looking to use getcwd, but its
> man page had suggested using this open()/fchdir() method.
> 
> Anyway, is there a reason to avoid changing the directory? If not
> i'm tempted to take the approach that j.sixt suggested--not restoring
> the cwd since we're exiting anyway. I don't have any good reason
> to not do the string manipulation, but why do something more
> complicated then necessary?

Yea, that was another thought I had. You probably can just chdir(), list, exit, and not worry about going back to the previous directory. And more complicated is always a bad idea. Keep it simple, 'cause us gits like it that way. :-)

-- 
Shawn.
Johannes Sixt· Oct 22, 2007, 06:06 UTC · re: Scott R Parish · lore

Re: [PATCH] "git help -a" should search all exec_paths and PATH

Scott R Parish schrieb:
> +	if (!dirp || chdir(dir)) {
> +		fchdir(start_dir);
/me dislikes this. Windows doesn't have fchdir().

AFAIKS you are only using this to chdir back to where you started, but is this necessary? Aren't we exiting anyway after the command list was printed?

-- Hannes

← back to recent threads