[PATCH 1/7] "git" calls help_unknown_cmd(""); "git help" and "git help -a" return 0

[PATCH 1/7] "git" calls help_unknown_cmd(""); "git help" and "git help -a" return 0

[Scott R Parish]

Signed-off-by: Scott R Parish <srp@srparish.net>
---
 git.c  |    5 ++---
 help.c |    4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/git.c b/git.c
index 853e66c..e1c99e3 100644
--- a/git.c
+++ b/git.c
@@ -445,9 +445,8 @@ int main(int argc, const char **argv)
 		if (!prefixcmp(argv[0], "--"))
 			argv[0] += 2;
 	} else {
-		/* Default command: "help" */
-		argv[0] = "help";
-		argc = 1;
+		/* The user didn't specify a command; give them help */
+		help_unknown_cmd("");
 	}
 	cmd = argv[0];
 
diff --git a/help.c b/help.c
index 1cd33ec..b0d2dd4 100644
--- a/help.c
+++ b/help.c
@@ -204,14 +204,14 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	if (!help_cmd) {
 		printf("usage: %s\n\n", git_usage_string);
 		list_common_cmds_help();
-		exit(1);
+		exit(0);
 	}
 
 	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-*");
-		exit(1);
+		exit(0);
 	}
 
 	else
-- 
gitgui.0.8.4.11176.gd9205-dirty

[PATCH 2/7] s/pattern/prefix/ in help's list_commands

[Scott R Parish]

list_commands() currently accepts and ignores a "pattern" argument,
and then hard codes a prefix as well as some magic numbers. This
renames the arg from pattern to prefix and uses that instead of the
hardcoded stuff.

Signed-off-by: Scott R Parish <srp@srparish.net>
---
 help.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/help.c b/help.c
index b0d2dd4..950f62d 100644
--- a/help.c
+++ b/help.c
@@ -93,11 +93,12 @@ static void pretty_print_string_list(struct cmdname **cmdname, int longest)
 	}
 }
 
-static void list_commands(const char *exec_path, const char *pattern)
+static void list_commands(const char *exec_path, const char *prefix)
 {
 	unsigned int longest = 0;
 	char path[PATH_MAX];
 	int dirlen;
+	int prefix_len = strlen(prefix);
 	DIR *dir = opendir(exec_path);
 	struct dirent *de;
 
@@ -120,7 +121,7 @@ static void list_commands(const char *exec_path, const char *pattern)
 		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 */
@@ -128,14 +129,14 @@ static void list_commands(const char *exec_path, const char *pattern)
 		    !(st.st_mode & S_IXUSR))
 			continue;
 
-		entlen = strlen(de->d_name);
+		entlen = strlen(de->d_name) - prefix_len;
 		if (has_extension(de->d_name, ".exe"))
 			entlen -= 4;
 
 		if (longest < entlen)
 			longest = entlen;
 
-		add_cmdname(de->d_name + 4, entlen-4);
+		add_cmdname(de->d_name + prefix_len, entlen);
 	}
 	closedir(dir);
 
@@ -143,7 +144,7 @@ static void list_commands(const char *exec_path, const char *pattern)
 	printf("----------------------------");
 	mput_char('-', strlen(exec_path));
 	putchar('\n');
-	pretty_print_string_list(cmdname, longest - 4);
+	pretty_print_string_list(cmdname, longest);
 	putchar('\n');
 }
 
@@ -210,7 +211,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(exec_path, "git-");
 		exit(0);
 	}
 
-- 
gitgui.0.8.4.11176.gd9205-dirty

[PATCH 3/7] "current_exec_path" is a misleading name, use "argv_exec_path" Signed-off-by: Scott R Parish <srp@srparish.net>

[Scott R Parish]

---
 exec_cmd.c |   12 ++++++------
 exec_cmd.h |    2 +-
 git.c      |    2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 9b74ed2..8b681d0 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -5,11 +5,11 @@
 
 extern char **environ;
 static const char *builtin_exec_path = GIT_EXEC_PATH;
-static const char *current_exec_path;
+static const char *argv_exec_path = 0;
 
-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;
 }
 
 
@@ -18,8 +18,8 @@ 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 +34,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..da99287 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -1,7 +1,7 @@
 #ifndef GIT_EXEC_CMD_H
 #define GIT_EXEC_CMD_H
 
-extern void git_set_exec_path(const char *exec_path);
+extern void git_set_argv_exec_path(const char *exec_path);
 extern const char* git_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 e1c99e3..f659338 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);
-- 
gitgui.0.8.4.11176.gd9205-dirty

[PATCH 4/7] use only the PATH for exec'ing git commands

[Scott R Parish]

We need to correctly set up PATH for non-c based git commands. Since we
already do this, we can just use that PATH and execvp, instead of looping
over the paths with execve.

This patch adds a setup_path() function to exec_cmd.c, which sets
the PATH order correctly for our search order. execv_git_cmd() is
stripped down to setting up argv and calling execvp(). git.c's main()
only only needs to call setup_path().

Signed-off-by: Scott R Parish <srp@srparish.net>
---
 exec_cmd.c |  121 ++++++++++++++++++++++++++----------------------------------
 exec_cmd.h |    1 +
 git.c      |   43 +++------------------
 3 files changed, 60 insertions(+), 105 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 8b681d0..c228dbf 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -29,85 +29,68 @@ const char *git_exec_path(void)
 	return builtin_exec_path;
 }
 
+static void add_path(struct strbuf *out, const char *path)
+{
+	if (path && strlen(path)) {
+		if (is_absolute_path(path))
+			strbuf_addstr(out, path);
+		else
+			strbuf_addstr(out, make_absolute_path(path));
+
+		strbuf_addch(out, ':');
+	}
+}
+
+void setup_path(const char *cmd_path)
+{
+	const char *old_path = getenv("PATH");
+	struct strbuf new_path;
+
+	strbuf_init(&new_path, 0);
+
+	add_path(&new_path, argv_exec_path);
+	add_path(&new_path, getenv(EXEC_PATH_ENVIRONMENT));
+	add_path(&new_path, builtin_exec_path);
+	add_path(&new_path, cmd_path);
+
+	if (old_path)
+		strbuf_addstr(&new_path, old_path);
+	else
+		strbuf_addstr(&new_path, "/usr/local/bin:/usr/bin:/bin");
+
+	setenv("PATH", new_path.buf, 1);
+
+	strbuf_release(&new_path);
+}
 
 int execv_git_cmd(const char **argv)
 {
-	char git_command[PATH_MAX + 1];
-	int i;
-	const char *paths[] = { argv_exec_path,
-				getenv(EXEC_PATH_ENVIRONMENT),
-				builtin_exec_path };
-
-	for (i = 0; i < ARRAY_SIZE(paths); ++i) {
-		size_t len;
-		int rc;
-		const char *exec_dir = paths[i];
-		const char *tmp;
-
-		if (!exec_dir || !*exec_dir) continue;
-
-		if (*exec_dir != '/') {
-			if (!getcwd(git_command, sizeof(git_command))) {
-				fprintf(stderr, "git: cannot determine "
-					"current directory: %s\n",
-					strerror(errno));
-				break;
-			}
-			len = strlen(git_command);
-
-			/* Trivial cleanup */
-			while (!prefixcmp(exec_dir, "./")) {
-				exec_dir += 2;
-				while (*exec_dir == '/')
-					exec_dir++;
-			}
-
-			rc = snprintf(git_command + len,
-				      sizeof(git_command) - len, "/%s",
-				      exec_dir);
-			if (rc < 0 || rc >= sizeof(git_command) - len) {
-				fprintf(stderr, "git: command name given "
-					"is too long.\n");
-				break;
-			}
-		} else {
-			if (strlen(exec_dir) + 1 > sizeof(git_command)) {
-				fprintf(stderr, "git: command name given "
-					"is too long.\n");
-				break;
-			}
-			strcpy(git_command, exec_dir);
-		}
-
-		len = strlen(git_command);
-		rc = snprintf(git_command + len, sizeof(git_command) - len,
-			      "/git-%s", argv[0]);
-		if (rc < 0 || rc >= sizeof(git_command) - len) {
-			fprintf(stderr,
-				"git: command name given is too long.\n");
-			break;
-		}
+	struct strbuf cmd;
+	const char *tmp;
 
-		/* argv[0] must be the git command, but the argv array
-		 * belongs to the caller, and my be reused in
-		 * subsequent loop iterations. Save argv[0] and
-		 * restore it on error.
-		 */
+	strbuf_init(&cmd, 0);
+	strbuf_addf(&cmd, "git-%s", argv[0]);
 
-		tmp = argv[0];
-		argv[0] = git_command;
+	/* argv[0] must be the git command, but the argv array
+	 * belongs to the caller, and my be reused in
+	 * subsequent loop iterations. Save argv[0] and
+	 * restore it on error.
+	 */
+	tmp = argv[0];
+	argv[0] = cmd.buf;
 
-		trace_argv_printf(argv, -1, "trace: exec:");
+	trace_argv_printf(argv, -1, "trace: exec:");
 
-		/* execve() can only ever return if it fails */
-		execve(git_command, (char **)argv, environ);
+	/* execvp() can only ever return if it fails */
+	execvp(cmd.buf, (char **)argv);
 
-		trace_printf("trace: exec failed: %s\n", strerror(errno));
+	trace_printf("trace: exec failed: %s\n", strerror(errno));
 
-		argv[0] = tmp;
-	}
-	return -1;
+	argv[0] = tmp;
 
+	strbuf_release(&cmd);
+
+	return -1;
 }
 
 
diff --git a/exec_cmd.h b/exec_cmd.h
index da99287..a892355 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -3,6 +3,7 @@
 
 extern void git_set_argv_exec_path(const char *exec_path);
 extern const char* git_exec_path(void);
+extern void setup_path(const char *);
 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 f659338..a639e42 100644
--- a/git.c
+++ b/git.c
@@ -6,28 +6,6 @@
 const char git_usage_string[] =
 	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
 
-static void prepend_to_path(const char *dir, int len)
-{
-	const char *old_path = getenv("PATH");
-	char *path;
-	int path_len = len;
-
-	if (!old_path)
-		old_path = "/usr/local/bin:/usr/bin:/bin";
-
-	path_len = len + strlen(old_path) + 1;
-
-	path = xmalloc(path_len + 1);
-
-	memcpy(path, dir, len);
-	path[len] = ':';
-	memcpy(path + len + 1, old_path, path_len - len);
-
-	setenv("PATH", path, 1);
-
-	free(path);
-}
-
 static int handle_options(const char*** argv, int* argc, int* envchanged)
 {
 	int handled = 0;
@@ -403,7 +381,7 @@ int main(int argc, const char **argv)
 {
 	const char *cmd = argv[0] ? argv[0] : "git-help";
 	char *slash = strrchr(cmd, '/');
-	const char *exec_path = NULL;
+	const char *cmd_path = NULL;
 	int done_alias = 0;
 
 	/*
@@ -413,10 +391,7 @@ int main(int argc, const char **argv)
 	 */
 	if (slash) {
 		*slash++ = 0;
-		if (*cmd == '/')
-			exec_path = cmd;
-		else
-			exec_path = xstrdup(make_absolute_path(cmd));
+		cmd_path = cmd;
 		cmd = slash;
 	}
 
@@ -451,16 +426,12 @@ int main(int argc, const char **argv)
 	cmd = argv[0];
 
 	/*
-	 * We execute external git command via execv_git_cmd(),
-	 * which looks at "--exec-path" option, GIT_EXEC_PATH
-	 * environment, and $(gitexecdir) in Makefile while built,
-	 * in this order.  For scripted commands, we prepend
-	 * the value of the exec_path variable to the PATH.
+	 * We use PATH to find git commands, but we prepend some higher
+	 * precidence paths: the "--exec-path" option, the GIT_EXEC_PATH
+	 * environment, and the $(gitexecdir) from the Makefile at build
+	 * time.
 	 */
-	if (exec_path)
-		prepend_to_path(exec_path, strlen(exec_path));
-	exec_path = git_exec_path();
-	prepend_to_path(exec_path, strlen(exec_path));
+	setup_path(cmd_path);
 
 	while (1) {
 		/* See if it's an internal command */
-- 
gitgui.0.8.4.11176.gd9205-dirty

[PATCH 5/7] chdir() into list_commands() dir instead of building paths for stat()

[Scott R Parish]

Signed-off-by: Scott R Parish <srp@srparish.net>
---
 help.c |   18 +++---------------
 1 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/help.c b/help.c
index 950f62d..906f8f6 100644
--- a/help.c
+++ b/help.c
@@ -96,35 +96,23 @@ static void pretty_print_string_list(struct cmdname **cmdname, int longest)
 static void list_commands(const char *exec_path, const char *prefix)
 {
 	unsigned int longest = 0;
-	char path[PATH_MAX];
-	int dirlen;
 	int prefix_len = strlen(prefix);
 	DIR *dir = opendir(exec_path);
 	struct dirent *de;
 
-	if (!dir) {
+	if (!dir || chdir(exec_path)) {
 		fprintf(stderr, "git: '%s': %s\n", exec_path, strerror(errno));
 		exit(1);
 	}
 
-	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) {
 		struct stat st;
 		int entlen;
 
 		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;
-- 
gitgui.0.8.4.11176.gd9205-dirty

[PATCH 6/7] walk PATH to generate list of commands for "help -a"

[Scott R Parish]

Signed-off-by: Scott R Parish <srp@srparish.net>
---
 help.c |   69 ++++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/help.c b/help.c
index 906f8f6..3f8b4aa 100644
--- a/help.c
+++ b/help.c
@@ -64,6 +64,19 @@ static int cmdname_compare(const void *a_, const void *b_)
 	return strcmp(a->name, b->name);
 }
 
+static void uniq(struct cmdname **cmdname)
+{
+	int i, j;
+
+	for (i = j = 1; i < cmdname_cnt; i++) {
+		if (strcmp(cmdname[i]->name, cmdname[i-1]->name)) {
+			cmdname[j++] = cmdname[i];
+		}
+	}
+
+	cmdname_cnt = j;
+}
+
 static void pretty_print_string_list(struct cmdname **cmdname, int longest)
 {
 	int cols = 1, rows;
@@ -71,12 +84,13 @@ static void pretty_print_string_list(struct cmdname **cmdname, int longest)
 	int max_cols = term_columns() - 1; /* don't print *on* the edge */
 	int i, j;
 
+	qsort(cmdname, cmdname_cnt, sizeof(*cmdname), cmdname_compare);
+	uniq(cmdname);
+
 	if (space < max_cols)
 		cols = max_cols / space;
 	rows = (cmdname_cnt + cols - 1) / cols;
 
-	qsort(cmdname, cmdname_cnt, sizeof(*cmdname), cmdname_compare);
-
 	for (i = 0; i < rows; i++) {
 		printf("  ");
 
@@ -93,19 +107,17 @@ static void pretty_print_string_list(struct cmdname **cmdname, int longest)
 	}
 }
 
-static void list_commands(const char *exec_path, const char *prefix)
+static unsigned int list_commands_in_dir(const char *dir, const char *prefix)
 {
 	unsigned int longest = 0;
 	int prefix_len = strlen(prefix);
-	DIR *dir = opendir(exec_path);
+	DIR *dirp = opendir(dir);
 	struct dirent *de;
 
-	if (!dir || chdir(exec_path)) {
-		fprintf(stderr, "git: '%s': %s\n", exec_path, strerror(errno));
-		exit(1);
-	}
+	if (!dirp || chdir(dir))
+		return 0;
 
-	while ((de = readdir(dir)) != NULL) {
+	while ((de = readdir(dirp)) != NULL) {
 		struct stat st;
 		int entlen;
 
@@ -126,12 +138,37 @@ static void list_commands(const char *exec_path, const char *prefix)
 
 		add_cmdname(de->d_name + prefix_len, entlen);
 	}
-	closedir(dir);
+	closedir(dirp);
 
-	printf("git commands available in '%s'\n", exec_path);
-	printf("----------------------------");
-	mput_char('-', strlen(exec_path));
-	putchar('\n');
+	return longest;
+}
+
+static void list_commands(const char *prefix)
+{
+	unsigned int longest = 0;
+	unsigned int len;
+	const char *env_path = getenv("PATH");
+	char *paths, *path, *colon;
+
+	if (!env_path) {
+		fprintf(stderr, "PATH not set\n");
+		exit(1);
+	}
+
+	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);
+
+	printf("available git commands\n");
+	printf("----------------------\n");
 	pretty_print_string_list(cmdname, longest);
 	putchar('\n');
 }
@@ -188,7 +225,6 @@ int cmd_version(int argc, const char **argv, const char *prefix)
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
 	const char *help_cmd = argc > 1 ? argv[1] : NULL;
-	const char *exec_path = git_exec_path();
 
 	if (!help_cmd) {
 		printf("usage: %s\n\n", git_usage_string);
@@ -198,8 +234,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);
 	}
 
-- 
gitgui.0.8.4.11176.gd9205-dirty

[PATCH 7/7] shell should call setup_path() instead of manually setting up its path

[Scott R Parish]

Signed-off-by: Scott R Parish <srp@srparish.net>
---
 shell.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/shell.c b/shell.c
index cfe372b..9826109 100644
--- a/shell.c
+++ b/shell.c
@@ -24,17 +24,11 @@ static int do_cvs_cmd(const char *me, char *arg)
 	const char *cvsserver_argv[3] = {
 		"cvsserver", "server", NULL
 	};
-	const char *oldpath = getenv("PATH");
-	struct strbuf newpath = STRBUF_INIT;
 
 	if (!arg || strcmp(arg, "server"))
 		die("git-cvsserver only handles server: %s", arg);
 
-	strbuf_addstr(&newpath, git_exec_path());
-	strbuf_addch(&newpath, ':');
-	strbuf_addstr(&newpath, oldpath);
-
-	setenv("PATH", strbuf_detach(&newpath, NULL), 1);
+	setup_path(NULL);
 
 	return execv_git_cmd(cvsserver_argv);
 }
-- 
gitgui.0.8.4.11176.gd9205-dirty

Re: [PATCH 1/7] "git" calls help_unknown_cmd(""); "git help" and "git help -a" return 0

[Junio C Hamano]

Scott R Parish <srp@srparish.net> writes:

> Signed-off-by: Scott R Parish <srp@srparish.net>
> ---
>  git.c  |    5 ++---
>  help.c |    4 ++--
>  2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/git.c b/git.c
> index 853e66c..e1c99e3 100644
> --- a/git.c
> +++ b/git.c
> @@ -445,9 +445,8 @@ int main(int argc, const char **argv)
>  		if (!prefixcmp(argv[0], "--"))
>  			argv[0] += 2;
>  	} else {
> -		/* Default command: "help" */
> -		argv[0] = "help";
> -		argc = 1;
> +		/* The user didn't specify a command; give them help */
> +		help_unknown_cmd("");

Sorry, but I fail to see why this is an improvement.

If you type "git<Enter>", before this patch we call cmd_help()
without help_cmd, which gives the usage string and lists common
commands, and exits with 1.

With this patch, you get "git: '' is not a git-command",
followed by list of common commands, and then the invocation
exits with 1.

I think "git help" should exit with 0.  The user asked for help,
we give help, everything is going as expected and there is no
reason to exit with non-zero.

However, with the current implementation, these changes to
help.c also make "git<Enter>" to exit with 0 after it gives
help, which is not so nice (both "cvs" and "svn" without
parameter seem to exit with 1 and it sort of makes sense,
although I do not think it matters much).  As a few datapoints,
"cvs -H" exits with 1 and "svn help" exits with 0.

So in short,

 - "git" should retain the current behaviour (both output and
   exit code).

 - "git help" should retain the current output but probably
   should exit with 0.

 - Ditto for "git help -a".

Re: [PATCH 2/7] s/pattern/prefix/ in help's list_commands

[Junio C Hamano]

Scott R Parish <srp@srparish.net> writes:

> list_commands() currently accepts and ignores a "pattern" argument,
> and then hard codes a prefix as well as some magic numbers.

Correct observation.

Personally, I find this static function should not pretend to be
as flexible --- it is to list git subcommands anyway (and it
even knows about ".exe"), so rather than renaming the pattern
and using it, it might be simpler and cleaner to just drop the
parameter and be done with it.

Re: [PATCH 6/7] walk PATH to generate list of commands for "help -a"

[Junio C Hamano]

Scott R Parish <srp@srparish.net> writes:

> Signed-off-by: Scott R Parish <srp@srparish.net>

Rationale?

There are two cases execv_git_cmd() runs "git-that" from a non
standard place, if we take your [PATCH 4/7].

 - If there is a directory that contains a location that used to
   hold an old installation of git-* commands (some of which may
   have been removed in the latest git) and if the user has that
   directory on PATH, we would run obsolete git subcommand from
   there.

 - If the user has a custom command "git-that" in $HOME/bin/
   that is outside GIT_EXEC_PATH, the new subcommand "that" can
   be used as if it is part of the official git.  This is an
   improvement [PATCH 4/7] would bring in.  We allow this
   already for scripts anyway, and the patch is merely making
   the behaviour of the execv_git_cmd() consistent with it.

It may be nicer if the user can somehow tell from the output if
each of the command is from the standard set (i.e. on
GIT_EXEC_PATH or built-in), or from a non standard place (either
custom command as intended, or an unintended obsolete leftover).

Re: [PATCH 1/7] "git" calls help_unknown_cmd(""); "git help" and "git help -a" return 0

[Scott Parish]

On Wed, Oct 24, 2007 at 09:40:55PM -0700, Junio C Hamano wrote:

> Sorry, but I fail to see why this is an improvement.
> 
> However, with the current implementation, these changes to
> help.c also make "git<Enter>" to exit with 0 after it gives
> help, which is not so nice (both "cvs" and "svn" without
> parameter seem to exit with 1 and it sort of makes sense,
> although I do not think it matters much).  As a few datapoints,
> "cvs -H" exits with 1 and "svn help" exits with 0.

That's strange because when i run that patch on my system:

  % ./git; echo $?
  git: '' is not a git-command
 
  <list of common commands>
  1
  % ./git help; echo $?
  <list of common commands>
  0
  % ./git help -a; echo $?
  <list of all commands>
  0

> So in short,
> 
>  - "git" should retain the current behaviour (both output and
>    exit code).
> 
>  - "git help" should retain the current output but probably
>    should exit with 0.
> 
>  - Ditto for "git help -a".

That's what i was hoping this patch did. I'm not entirely sure how
its wrong as it seems to work for me.

Regarding "git: '' is not a git-command" the way i was seeing that
is that git is usually only called with commands, and '' isn't a
valid command, hence the reason to exit 1, the help is just a nice
user experience.

sRp

-- 
Scott Parish
http://srparish.net/

Re: [PATCH 2/7] s/pattern/prefix/ in help's list_commands

[Scott Parish]

On Wed, Oct 24, 2007 at 09:41:30PM -0700, Junio C Hamano wrote:

> Scott R Parish <srp@srparish.net> writes:
> 
> > list_commands() currently accepts and ignores a "pattern" argument,
> > and then hard codes a prefix as well as some magic numbers.
> 
> Correct observation.
> 
> Personally, I find this static function should not pretend to be
> as flexible --- it is to list git subcommands anyway (and it
> even knows about ".exe"), so rather than renaming the pattern
> and using it, it might be simpler and cleaner to just drop the
> parameter and be done with it.

Nice, i like that even better

sRp

-- 
Scott Parish
http://srparish.net/

Re: [PATCH 6/7] walk PATH to generate list of commands for "help -a"

[Scott Parish]

On Wed, Oct 24, 2007 at 09:42:42PM -0700, Junio C Hamano wrote:

> Scott R Parish <srp@srparish.net> writes:
> 
> > Signed-off-by: Scott R Parish <srp@srparish.net>
> 
> Rationale?

Well, the ultimate reason that i've been working on all of this is
i'd like to push git as a viable development tool where i work. To
give an effective idea, lets say that shared tools get placed on
nfs servers, which can be mounted to different paths depending on
which nfs server is up or down or which system is the nfs client.

I have no control over each users PATH nor things like MANPATH or
GIT_EXEC_PATH and have no way of compiling in a path ahead of time,
but i would like to provide the easiest user experiance possible,
meaning that whether they have git in their PATH, or whether they
are using an absolute or relative path to it, it just works, hopefully
including "git help" and "git help -a".

Should i be putting all that in my commit messages?

> There are two cases execv_git_cmd() runs "git-that" from a non
> standard place, if we take your [PATCH 4/7].
> 
>  - If there is a directory that contains a location that used to
>    hold an old installation of git-* commands (some of which may
>    have been removed in the latest git) and if the user has that
>    directory on PATH, we would run obsolete git subcommand from
>    there.

I could see that as being problematic. I suppose there are ways
around that (have "git" pass to "git-cmd" an argument of what version
it is) but none that i really like.

>  - If the user has a custom command "git-that" in $HOME/bin/
>    that is outside GIT_EXEC_PATH, the new subcommand "that" can
>    be used as if it is part of the official git.  This is an
>    improvement [PATCH 4/7] would bring in.  We allow this
>    already for scripts anyway, and the patch is merely making
>    the behaviour of the execv_git_cmd() consistent with it.
> 
> It may be nicer if the user can somehow tell from the output if
> each of the command is from the standard set (i.e. on
> GIT_EXEC_PATH or built-in), or from a non standard place (either
> custom command as intended, or an unintended obsolete leftover).

What if git marked commands that weren't found in the location where
it thinks that it is running from?

sRp

-- 
Scott Parish
http://srparish.net/

Re: [PATCH 6/7] walk PATH to generate list of commands for "help -a"

[Junio C Hamano]

Scott Parish <sRp@srparish.net> writes:

> On Wed, Oct 24, 2007 at 09:42:42PM -0700, Junio C Hamano wrote:
>
>> Scott R Parish <srp@srparish.net> writes:
>> 
>> > Signed-off-by: Scott R Parish <srp@srparish.net>
>> 
>> Rationale?
>
> Well, the ultimate reason that i've been working on all of this is
> i'd like to push git as a viable development tool where i work. To
> give an effective idea, lets say that shared tools get placed on
> nfs servers, which can be mounted to different paths depending on
> which nfs server is up or down or which system is the nfs client.

It sounds to me that your nfs client systems might find what
people usually expect in /usr/local/bin not there but on
/mnt/random47/bin depending on the system, without a reasonable
system administration effort that places stable symlinks to give
end users a consistent view of the world regardless from which
client, which sounds insane.  I personally do not think we
should support lazy system administrators by making git unsafe.

But using PATH as a fallback is what we already do for scripts,
and that is good enough to deal with such an installation.

> Should i be putting all that in my commit messages?

Even in a well behaved installation, where everything is found
where they should be (iow, GIT_EXEC_PATH), this is needed
because 4/7 lets you run a custom "git that" command from PATH
and this 6/7 is to teach "help -a" about it.  I think at least
that much needs to be said in the commit message.

>> There are two cases execv_git_cmd() runs "git-that" from a non
>> standard place, if we take your [PATCH 4/7].
>> 
>>  - If there is a directory that contains a location that used to
>>    hold an old installation of git-* commands (some of which may
>>    have been removed in the latest git) and if the user has that
>>    directory on PATH, we would run obsolete git subcommand from
>>    there.
>
> I could see that as being problematic. I suppose there are ways
> around that (have "git" pass to "git-cmd" an argument of what version
> it is) but none that i really like.

As I said, this is making git a bit less safe from unintended
leftover executables, but the scripts already work that way and
your 4/7 merely makes the C level in line with that behaviour.
I do not think it is too much of a problem anyway.

>> It may be nicer if the user can somehow tell from the output if
>> each of the command is from the standard set (i.e. on
>> GIT_EXEC_PATH or built-in), or from a non standard place (either
>> custom command as intended, or an unintended obsolete leftover).
>
> What if git marked commands that weren't found in the location where
> it thinks that it is running from?

Currently "git help -a" says "available in $where" at the top.
Perhaps make a separate list that is listed as "available from
elsewhere" and show the ones that are on PATH but not masked by
the ones on GIT_EXEC_PATH?

    git commands available in '/home/junio/git-next/bin'
    ----------------------------------------------------
      add                 gui                 rebase--interactive
      add--interactive    hash-object         receive-pack
      ...

    git commands available from elsewhere on your $PATH
    ----------------------------------------------------
      frotz               nitfol

[PATCH 2/7] remove unused/unneeded "pattern" argument of list_commands

[Scott R Parish]

list_commands() currently accepts and ignores a "pattern" argument,
and then hard codes a prefix as well as some magic numbers. This
hardcodes the prefix inside of the function and removes the magic
numbers.

Signed-off-by: Scott R Parish <srp@srparish.net>
---
 help.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/help.c b/help.c
index b0d2dd4..d6dfdff 100644
--- a/help.c
+++ b/help.c
@@ -93,10 +93,12 @@ static void pretty_print_string_list(struct cmdname **cmdname, int longest)
 	}
 }
 
-static void list_commands(const char *exec_path, const char *pattern)
+static void list_commands(const char *exec_path)
 {
 	unsigned int longest = 0;
 	char path[PATH_MAX];
+	const char *prefix = "git-";
+	int prefix_len = strlen(prefix);
 	int dirlen;
 	DIR *dir = opendir(exec_path);
 	struct dirent *de;
@@ -120,7 +122,7 @@ static void list_commands(const char *exec_path, const char *pattern)
 		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 */
@@ -128,14 +130,14 @@ static void list_commands(const char *exec_path, const char *pattern)
 		    !(st.st_mode & S_IXUSR))
 			continue;
 
-		entlen = strlen(de->d_name);
+		entlen = strlen(de->d_name) - prefix_len;
 		if (has_extension(de->d_name, ".exe"))
 			entlen -= 4;
 
 		if (longest < entlen)
 			longest = entlen;
 
-		add_cmdname(de->d_name + 4, entlen-4);
+		add_cmdname(de->d_name + prefix_len, entlen);
 	}
 	closedir(dir);
 
@@ -143,7 +145,7 @@ static void list_commands(const char *exec_path, const char *pattern)
 	printf("----------------------------");
 	mput_char('-', strlen(exec_path));
 	putchar('\n');
-	pretty_print_string_list(cmdname, longest - 4);
+	pretty_print_string_list(cmdname, longest);
 	putchar('\n');
 }
 
@@ -210,7 +212,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(exec_path);
 		exit(0);
 	}
 
-- 
gitgui.0.8.4.11178.g9a1bf-dirty

[PATCH 5/7] chdir() into list_commands() dir instead of building paths for stat()

[Scott R Parish]

Signed-off-by: Scott R Parish <srp@srparish.net>
---
 help.c |   18 +++---------------
 1 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/help.c b/help.c
index d6dfdff..322ddaa 100644
--- a/help.c
+++ b/help.c
@@ -96,36 +96,24 @@ static void pretty_print_string_list(struct cmdname **cmdname, int longest)
 static void list_commands(const char *exec_path)
 {
 	unsigned int longest = 0;
-	char path[PATH_MAX];
 	const char *prefix = "git-";
 	int prefix_len = strlen(prefix);
-	int dirlen;
 	DIR *dir = opendir(exec_path);
 	struct dirent *de;
 
-	if (!dir) {
+	if (!dir || chdir(exec_path)) {
 		fprintf(stderr, "git: '%s': %s\n", exec_path, strerror(errno));
 		exit(1);
 	}
 
-	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) {
 		struct stat st;
 		int entlen;
 
 		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;
-- 
gitgui.0.8.4.11178.g9a1bf-dirty

Re: [PATCH 6/7] walk PATH to generate list of commands for "help -a"

[Scott Parish]

On Wed, Oct 24, 2007 at 10:33:32PM -0700, Junio C Hamano wrote:

> > Well, the ultimate reason that i've been working on all of this is
> > i'd like to push git as a viable development tool where i work. To
> > give an effective idea, lets say that shared tools get placed on
> > nfs servers, which can be mounted to different paths depending on
> > which nfs server is up or down or which system is the nfs client.
> 
> It sounds to me that your nfs client systems might find what
> people usually expect in /usr/local/bin not there but on
> /mnt/random47/bin depending on the system, without a reasonable
> system administration effort that places stable symlinks to give
> end users a consistent view of the world regardless from which
> client, which sounds insane.  I personally do not think we
> should support lazy system administrators by making git unsafe.

Well, the exact details are completely fictitious, made up to
illustrate the situation without breaking confidential agreements.
I'm not sure i completely agree with the design, but there are good
reasons for it, and at this point i have little or no control over
it.

> >> It may be nicer if the user can somehow tell from the output if
> >> each of the command is from the standard set (i.e. on
> >> GIT_EXEC_PATH or built-in), or from a non standard place (either
> >> custom command as intended, or an unintended obsolete leftover).
> >
> > What if git marked commands that weren't found in the location where
> > it thinks that it is running from?
> 
> Currently "git help -a" says "available in $where" at the top.
> Perhaps make a separate list that is listed as "available from
> elsewhere" and show the ones that are on PATH but not masked by
> the ones on GIT_EXEC_PATH?
> 
>     git commands available in '/home/junio/git-next/bin'
>     ----------------------------------------------------
>       add                 gui                 rebase--interactive
>       add--interactive    hash-object         receive-pack
>       ...
> 
>     git commands available from elsewhere on your $PATH
>     ----------------------------------------------------
>       frotz               nitfol

Nice! I'll try doing that, probably won't have time to finish until
later tomorrow.

sRp

-- 
Scott Parish
http://srparish.net/

Re: [PATCH 1/7] "git" calls help_unknown_cmd(""); "git help" and "git help -a" return 0

[Junio C Hamano]

Scott Parish <sRp@srparish.net> writes:

> That's what i was hoping this patch did. I'm not entirely sure how
> its wrong as it seems to work for me.

I misread the patch.  My mistake.

> Regarding "git: '' is not a git-command" the way i was seeing that
> is that git is usually only called with commands, and '' isn't a
> valid command, hence the reason to exit 1, the help is just a nice
> user experience.

But think who would type "git<Enter>".  They are either people
who (1) do not even know that "git" alone is not useful and that
it always wants a subcommand, or (2) know "git<Enter>" is the
same as "git help" and wants to get the "common command list"
quickly.  Technically, "'' is not a git-command" is correct, but
the message does not help either audience, does it?

Re: [PATCH 1/7] "git" calls help_unknown_cmd(""); "git help" and "git help -a" return 0

[Scott Parish]

On Fri, Oct 26, 2007 at 04:03:48PM -0700, Junio C Hamano wrote:

> > Regarding "git: '' is not a git-command" the way i was seeing that
> > is that git is usually only called with commands, and '' isn't a
> > valid command, hence the reason to exit 1, the help is just a nice
> > user experience.
> 
> But think who would type "git<Enter>".  They are either people
> who (1) do not even know that "git" alone is not useful and that
> it always wants a subcommand, or (2) know "git<Enter>" is the
> same as "git help" and wants to get the "common command list"
> quickly.  Technically, "'' is not a git-command" is correct, but
> the message does not help either audience, does it?

Fair enough, i'll drop that in the updated patch set i'm about ready
to send. Incidentally i was also missing the "usage" string.

sRp

-- 
Scott Parish
http://srparish.net/

[PATCH 6/7] walk $PATH to generate list of commands for "help -a"

[Scott R Parish]

Git had previously been using the $PATH for scripts--a previous
patch moved exec'ed commands to also use the $PATH. For consistancy
"help -a" should also use the $PATH.

We walk all the paths in $PATH collecting the names of "git-*"
commands. To help distinguish between the main git commands
and commands picked up elsewhere (probably extensions) we
print them seperately. The main commands are the ones that
are found in the first directory in $PATH that contains the
"git" binary.

Signed-off-by: Scott R Parish <srp@srparish.net>
---
 help.c |  158 ++++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 125 insertions(+), 33 deletions(-)

diff --git a/help.c b/help.c
index ce3d795..ee4fce0 100644
--- a/help.c
+++ b/help.c
@@ -37,24 +37,29 @@ static inline void mput_char(char c, unsigned int num)
 		putchar(c);
 }
 
-static struct cmdname {
-	size_t len;
-	char name[1];
-} **cmdname;
-static int cmdname_alloc, cmdname_cnt;
-
-static void add_cmdname(const char *name, int len)
+static struct cmdnames {
+	int alloc;
+	int cnt;
+	char *dir;
+	struct cmdname {
+		size_t len;
+		char name[1];
+	} **names;
+} main_cmds, other_cmds;
+
+static void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
 	struct cmdname *ent;
-	if (cmdname_alloc <= cmdname_cnt) {
-		cmdname_alloc = cmdname_alloc + 200;
-		cmdname = xrealloc(cmdname, cmdname_alloc * sizeof(*cmdname));
+	if (cmds->alloc <= cmds->cnt) {
+		cmds->alloc = cmds->alloc + 200;
+		cmds->names = xrealloc(cmds->names,
+				       cmds->alloc * sizeof(*cmds->names));
 	}
 	ent = xmalloc(sizeof(*ent) + len);
 	ent->len = len;
 	memcpy(ent->name, name, len);
 	ent->name[len] = 0;
-	cmdname[cmdname_cnt++] = ent;
+	cmds->names[cmds->cnt++] = ent;
 }
 
 static int cmdname_compare(const void *a_, const void *b_)
@@ -64,7 +69,42 @@ static int cmdname_compare(const void *a_, const void *b_)
 	return strcmp(a->name, b->name);
 }
 
-static void pretty_print_string_list(struct cmdname **cmdname, int longest)
+static void uniq(struct cmdnames *cmds)
+{
+	int i, j;
+
+	if (!cmds->cnt)
+		return;
+
+	for (i = j = 1; i < cmds->cnt; i++) {
+		if (strcmp(cmds->names[i]->name, cmds->names[i-1]->name)) {
+			cmds->names[j++] = cmds->names[i];
+		}
+	}
+
+	cmds->cnt = j;
+}
+
+static void subtract_cmds(struct cmdnames *a, struct cmdnames *b) {
+	int ai, aj, bi;
+
+	ai = aj = bi = 0;
+	while (ai < a->cnt && bi < b->cnt) {
+		if (0 > strcmp(a->names[ai]->name, b->names[bi]->name))
+			a->names[aj++] = a->names[ai++];
+		else if (0 > strcmp(a->names[ai]->name, b->names[bi]->name))
+			bi++;
+		else
+			ai++, bi++;
+	}
+
+	while (ai < a->cnt)
+		a->names[aj++] = a->names[ai++];
+
+	a->cnt = aj;
+}
+
+static void pretty_print_string_list(struct cmdnames *cmds, int longest)
 {
 	int cols = 1, rows;
 	int space = longest + 1; /* min 1 SP between words */
@@ -73,9 +113,7 @@ static void pretty_print_string_list(struct cmdname **cmdname, int longest)
 
 	if (space < max_cols)
 		cols = max_cols / space;
-	rows = (cmdname_cnt + cols - 1) / cols;
-
-	qsort(cmdname, cmdname_cnt, sizeof(*cmdname), cmdname_compare);
+	rows = (cmds->cnt + cols - 1) / cols;
 
 	for (i = 0; i < rows; i++) {
 		printf("  ");
@@ -83,31 +121,39 @@ static void pretty_print_string_list(struct cmdname **cmdname, int longest)
 		for (j = 0; j < cols; j++) {
 			int n = j * rows + i;
 			int size = space;
-			if (n >= cmdname_cnt)
+			if (n >= cmds->cnt)
 				break;
-			if (j == cols-1 || n + rows >= cmdname_cnt)
+			if (j == cols-1 || n + rows >= cmds->cnt)
 				size = 1;
-			printf("%-*s", size, cmdname[n]->name);
+			printf("%-*s", size, cmds->names[n]->name);
 		}
 		putchar('\n');
 	}
 }
 
-static void list_commands(const char *exec_path)
+static unsigned int list_commands_in_dir(const char *dir)
 {
 	unsigned int longest = 0;
 	const char *prefix = "git-";
 	int prefix_len = strlen(prefix);
-	DIR *dir = opendir(exec_path);
+	DIR *dirp = opendir(dir);
+	struct cmdnames *cmds;
 	struct dirent *de;
+	struct stat st;
 
-	if (!dir || chdir(exec_path)) {
-		fprintf(stderr, "git: '%s': %s\n", exec_path, strerror(errno));
-		exit(1);
+	if (!dirp || chdir(dir))
+		return 0;
+
+	if (!main_cmds.cnt &&
+	    !stat("git", &st) &&
+	    S_ISREG(st.st_mode) && (st.st_mode & S_IXUSR)) {
+		cmds = &main_cmds;
+		cmds->dir = xstrdup(dir);
 	}
+	else
+		cmds = &other_cmds;
 
-	while ((de = readdir(dir)) != NULL) {
-		struct stat st;
+	while ((de = readdir(dirp)) != NULL) {
 		int entlen;
 
 		if (prefixcmp(de->d_name, prefix))
@@ -122,18 +168,66 @@ static void list_commands(const char *exec_path)
 		if (has_extension(de->d_name, ".exe"))
 			entlen -= 4;
 
+		if (has_extension(de->d_name, ".perl") ||
+		    has_extension(de->d_name, ".sh"))
+			continue;
+
 		if (longest < entlen)
 			longest = entlen;
 
-		add_cmdname(de->d_name + prefix_len, entlen);
+		add_cmdname(cmds, de->d_name + prefix_len, entlen);
+	}
+	closedir(dirp);
+
+	return longest;
+}
+
+static void list_commands()
+{
+	unsigned int longest = 0;
+	unsigned int len;
+	const char *env_path = getenv("PATH");
+	char *paths, *path, *colon;
+
+	if (!env_path) {
+		fprintf(stderr, "PATH not set\n");
+		exit(1);
 	}
-	closedir(dir);
 
-	printf("git commands available in '%s'\n", exec_path);
+	path = paths = xstrdup(env_path);
+	while ((char *)1 != path) {
+		if ((colon = strchr(path, ':')))
+			*colon = 0;
+
+		len = list_commands_in_dir(path);
+		longest = MAX(longest, len);
+
+		path = colon + 1;
+	}
+	free(paths);
+
+	qsort(main_cmds.names, main_cmds.cnt,
+	      sizeof(*main_cmds.names), cmdname_compare);
+	uniq(&main_cmds);
+
+	qsort(other_cmds.names, other_cmds.cnt,
+	      sizeof(*other_cmds.names), cmdname_compare);
+	uniq(&other_cmds);
+	subtract_cmds(&other_cmds, &main_cmds);
+
+	printf("available git commands in '%s'\n", main_cmds.dir);
 	printf("----------------------------");
-	mput_char('-', strlen(exec_path));
+	mput_char('-', strlen(main_cmds.dir));
+	putchar('\n');
+	pretty_print_string_list(&main_cmds, longest);
 	putchar('\n');
-	pretty_print_string_list(cmdname, longest);
+
+	if (!other_cmds.cnt)
+		return;
+
+	printf("git commands available from elsewhere on your $PATH\n");
+	printf("---------------------------------------------------\n");
+	pretty_print_string_list(&other_cmds, longest);
 	putchar('\n');
 }
 
@@ -189,7 +283,6 @@ int cmd_version(int argc, const char **argv, const char *prefix)
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
 	const char *help_cmd = argc > 1 ? argv[1] : NULL;
-	const char *exec_path = git_exec_path();
 
 	if (!help_cmd) {
 		printf("usage: %s\n\n", git_usage_string);
@@ -199,8 +292,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);
+		list_commands();
 		exit(0);
 	}
 
-- 
gitgui.0.8.4.11178.g9a1bf-dirty

Re: [PATCH 6/7] walk $PATH to generate list of commands for "help -a"

[Junio C Hamano]

Scott R Parish <srp@srparish.net> writes:

> Git had previously been using the $PATH for scripts--a previous
> patch moved exec'ed commands to also use the $PATH. For consistancy
> "help -a" should also use the $PATH.

s/consistancy/consistency/

> We walk all the paths in $PATH collecting the names of "git-*"
> commands. To help distinguish between the main git commands
> and commands picked up elsewhere (probably extensions) we
> print them seperately. The main commands are the ones that
> are found in the first directory in $PATH that contains the
> "git" binary.

This is not right.  $(gitexecdir) in Makefile is designed to
allow distros to move git-* commands out of the primary user
$PATH directories and install only "git" wrapper in /usr/bin.
"Use the directory 'git' is in" rule breaks this.

The "main commands" should be the first of argv_exec_path,
EXEC_PATH_ENVIRONMENT or builtin_exec_path.

> diff --git a/help.c b/help.c
> index ce3d795..ee4fce0 100644
> --- a/help.c
> +++ b/help.c
> @@ -64,7 +69,42 @@ static int cmdname_compare(const void *a_, const void *b_)
> ...
> +static void subtract_cmds(struct cmdnames *a, struct cmdnames *b) {
> +	int ai, aj, bi;
> +
> +	ai = aj = bi = 0;
> +	while (ai < a->cnt && bi < b->cnt) {
> +		if (0 > strcmp(a->names[ai]->name, b->names[bi]->name))
> +			a->names[aj++] = a->names[ai++];
> +		else if (0 > strcmp(a->names[ai]->name, b->names[bi]->name))
> +			bi++;
> +		else
> +			ai++, bi++;

In general, xxxcmp(a, b) is designed to return the same sign as
"a - b" (subtract b from a, using an appropriate definition of
"subtract" in the domain of a and b).  It is a good habit to
write:

	strcmp(a, b) < 0	strcmp(a, b) > 0

because these give the same sign as

	a < b			a > b

and makes your program easier to read.

> @@ -122,18 +168,66 @@ static void list_commands(const char *exec_path)
>  		if (has_extension(de->d_name, ".exe"))
>  			entlen -= 4;
>  
> +		if (has_extension(de->d_name, ".perl") ||
> +		    has_extension(de->d_name, ".sh"))
> +			continue;
> +

This needs a good justification.

If you have "." on PATH, and you run ./git in a freshly built
source directory, "git relink.perl" would try to run
./git-relink.perl.

I do not think excluding these is necessary nor is a good idea.

> +static void list_commands()
> +{

ANSI.  "static void list_commands(void)".

> +	path = paths = xstrdup(env_path);
> +	while ((char *)1 != path) {
> +		if ((colon = strchr(path, ':')))
> +			*colon = 0;
> +
> +		len = list_commands_in_dir(path);
> +		longest = MAX(longest, len);
> +
> +		path = colon + 1;
> +	}

I know that on modern architectures bit representation of
(char*) NULL is the same as integer 0 of the same size as a
pointer, and adding 1 to it would yield (char *)1, but the above
feels _dirty_.

	while (1) {
        	...
                if (!colon)
	                break;
		path = colon + 1;
	}

Re: [PATCH 6/7] walk $PATH to generate list of commands for "help -a"

[Scott Parish]

On Sat, Oct 27, 2007 at 11:18:02PM -0700, Junio C Hamano wrote:

> > We walk all the paths in $PATH collecting the names of "git-*"
> > commands. To help distinguish between the main git commands
> > and commands picked up elsewhere (probably extensions) we
> > print them seperately. The main commands are the ones that
> > are found in the first directory in $PATH that contains the
> > "git" binary.
> 
> This is not right.  $(gitexecdir) in Makefile is designed to
> allow distros to move git-* commands out of the primary user
> $PATH directories and install only "git" wrapper in /usr/bin.
> "Use the directory 'git' is in" rule breaks this.
> 
> The "main commands" should be the first of argv_exec_path,
> EXEC_PATH_ENVIRONMENT or builtin_exec_path.

This is after we've already prepended the above three paths (if
they're specified) to $PATH, so yes, generally they should be in
one of those directories, but more generally, it will be in one of
the directories in $PATH.

Its not clear to me what exactly you're looking for me to change,
just the wording i'm using in my comment? Or are you refering to
the approach?

When i email the changes, should i keep emailing the whole
patch series, or just the few patches that have changed?

Thanks
sRp

-- 
Scott Parish
http://srparish.net/

Re: [PATCH 6/7] walk $PATH to generate list of commands for "help -a"

[Junio C Hamano]

Scott Parish <sRp@srparish.net> writes:

> On Sat, Oct 27, 2007 at 11:18:02PM -0700, Junio C Hamano wrote:
>
>> > We walk all the paths in $PATH collecting the names of "git-*"
>> > commands. To help distinguish between the main git commands
>> > and commands picked up elsewhere (probably extensions) we
>> > print them seperately. The main commands are the ones that
>> > are found in the first directory in $PATH that contains the
>> > "git" binary.
>> ...
> Its not clear to me what exactly you're looking for me to change,
> just the wording i'm using in my comment? Or are you refering to
> the approach?

"git" binary will be found as /usr/bin/git while git-foo will be
found as /usr/libexec/git/git-foo in such an installation that
takes advantage of $(gitexecdir).  And /usr/libexec/git/git will
not exist.  Using existence of /usr/bin/git (I am referring to
your 'first directory on $PATH that contains the "git" binary'
above) as the cue for the location of "main commands" is wrong.

> When i email the changes, should i keep emailing the whole
> patch series, or just the few patches that have changed?

It's up to you.  Obviously, if you are replacing 2 patches out
of 100-patch series, you would not want to resend the whole
thing, though ;-)

Re: [PATCH 6/7] walk $PATH to generate list of commands for "help -a"

[Scott Parish]

On Sun, Oct 28, 2007 at 03:07:14AM -0700, Junio C Hamano wrote:

> Scott Parish <sRp@srparish.net> writes:
> 
> > On Sat, Oct 27, 2007 at 11:18:02PM -0700, Junio C Hamano wrote:
> >
> >> > We walk all the paths in $PATH collecting the names of "git-*"
> >> > commands. To help distinguish between the main git commands
> >> > and commands picked up elsewhere (probably extensions) we
> >> > print them seperately. The main commands are the ones that
> >> > are found in the first directory in $PATH that contains the
> >> > "git" binary.
> >> ...
> > Its not clear to me what exactly you're looking for me to change,
> > just the wording i'm using in my comment? Or are you refering to
> > the approach?
> 
> "git" binary will be found as /usr/bin/git while git-foo will be
> found as /usr/libexec/git/git-foo in such an installation that
> takes advantage of $(gitexecdir).  And /usr/libexec/git/git will
> not exist.  Using existence of /usr/bin/git (I am referring to
> your 'first directory on $PATH that contains the "git" binary'
> above) as the cue for the location of "main commands" is wrong.

Thanks for the clarification, that would be a problem. I've modified
the patch to list the main commands from git_exec_path(); i have
mixed feelings, but curious what you think.

sRp

-- 
Scott Parish
http://srparish.net/