Skip to contentSkip to conversation

allow git to use the PATH for finding subcommands and help docs

PATCH In review Scott R Parish 4 participants 10 messages Jun 15, 2016

lore
Share & tools
lore

Scott R Parish

author
#0 lore raw

PATCH 1/1 allow git to use the PATH for finding subcommands and help docs

+ check PATH for git_exec_path after other locations but before builtin
+ prepend MANPATH and PERL5LIB in addition to PATH

Signed-off-by Scott R Parish <[email protected]>
---
 exec_cmd.c |   50 ++++++++++++++++++++++++++++++++++++++-
 git.c      |   76 +++++++++++++++++++++++++++++++++++++++++------------------
 2 files changed, 102 insertions(+), 24 deletions(-)
exec_cmd.c +49 -1
index 9b74ed2..c6ecca9 100644
@@ -13,19 +13,67 @@ void git_set_exec_path(const char *exec_path)
 }
 
 
+/* Return the first path in PATH that git is found in or NULL if not found */
+char *git_path_from_env(void)
+{
+	const char *env_paths = getenv("PATH");
+	const char *git = "/git";
+	int git_len = strlen(git);
+	char *paths, *path, *colon, *git_path;
+	int path_len;
+	struct stat st;
+
+	if (!env_paths)
+		return NULL;
+
+	path_len = strlen(env_paths);
+	path = paths = xmalloc(path_len + 1);
+	memcpy(paths, env_paths, path_len + 1);
+
+	while ((char *)1 != path) {
+		if ((colon = strchr(path, ':')))
+		    *colon = 0;
+
+		path_len = strlen(path);
+		git_path = xmalloc(path_len + git_len + 1);
+		memcpy(git_path, path, path_len);
+		memcpy(git_path + path_len, git, git_len + 1);
+
+		if (!stat(git_path, &st)) { /* found */
+			free(paths);
+			git_path[path_len] = 0;
+			return git_path;
+		}
+
+		free(git_path);
+		path = colon + 1;
+	}
+
+	free(paths);
+	return NULL;
+}
+
+
 /* Returns the highest-priority, location to look for git programs. */
 const char *git_exec_path(void)
 {
-	const char *env;
+	const char *env, *path;
 
 	if (current_exec_path)
 		return current_exec_path;
 
 	env = getenv(EXEC_PATH_ENVIRONMENT);
 	if (env && *env) {
+		current_exec_path = env;
 		return env;
 	}
 
+	if ((path = git_path_from_env())) {
+		current_exec_path = path;
+		return path;
+	}
+
+	current_exec_path = builtin_exec_path;
 	return builtin_exec_path;
 }
git.c +53 -23

101 lines not on this page · raw message

Signature · 1 line
1.5.3.4.206.g58ba4-dirty
#1 lore raw
Quoted 2 lines · Scott R Parish

+ check PATH for git_exec_path after other locations but before builtin
+ prepend MANPATH and PERL5LIB in addition to PATH

This says *what* the patch does, but not *why*. Care to explain?

And then your explanation should go into the commit message.

-- Hannes

author
#2 lore raw

PATCH 1/1 allow git to use the PATH for finding subcommands and help docs

I have a situation where software for a distribution is installed into a fake "prefix" and then moved to one of several potential places to be used by users. Given that the final location isn't static, i can't depend on builtin_exec_path. I'd really like users to be able to get started with git as easily as possible. With the current setup, they would have to create and maintain either an GIT_EXEC_PATH or an alias for including --exec-path, as well as a MANPATH and PERL5LIB. This seem like an unnessisary burden.

I'd like to make it so that git works equally well when it is ran via an absolute path (already partially works), relative path, or from the PATH. (in saying "equally well" i'm including perl commands and help commands)

To do this i've had to make the following changes:

+ check PATH for the location of git
+ the checking of argv[0] was restricted to absolute paths; remove that restriction so it also works when called with a relative path (eg ../../otheruser/usr/bin/git)
+ try to guess and set the env for the typical relative locations for MANPATH and PERL5LIB based off exec_path

Signed-off-by Scott R Parish <[email protected]>
---
 exec_cmd.c |   50 ++++++++++++++++++++++++++++++++++++++-
 git.c      |   76 +++++++++++++++++++++++++++++++++++++++++------------------
 2 files changed, 102 insertions(+), 24 deletions(-)
exec_cmd.c +49 -1
index 9b74ed2..c6ecca9 100644
@@ -13,19 +13,67 @@ void git_set_exec_path(const char *exec_path)
 }
 
 
+/* Return the first path in PATH that git is found in or NULL if not found */
+char *git_path_from_env(void)
+{
+	const char *env_paths = getenv("PATH");
+	const char *git = "/git";
+	int git_len = strlen(git);
+	char *paths, *path, *colon, *git_path;
+	int path_len;
+	struct stat st;
+
+	if (!env_paths)
+		return NULL;
+
+	path_len = strlen(env_paths);
+	path = paths = xmalloc(path_len + 1);
+	memcpy(paths, env_paths, path_len + 1);
+
+	while ((char *)1 != path) {
+		if ((colon = strchr(path, ':')))
+		    *colon = 0;
+
+		path_len = strlen(path);
+		git_path = xmalloc(path_len + git_len + 1);
+		memcpy(git_path, path, path_len);
+		memcpy(git_path + path_len, git, git_len + 1);
+
+		if (!stat(git_path, &st)) { /* found */
+			free(paths);
+			git_path[path_len] = 0;
+			return git_path;
+		}
+
+		free(git_path);
+		path = colon + 1;
+	}
+
+	free(paths);
+	return NULL;
+}
+
+
 /* Returns the highest-priority, location to look for git programs. */
 const char *git_exec_path(void)
 {
-	const char *env;
+	const char *env, *path;
 
 	if (current_exec_path)
 		return current_exec_path;
 
 	env = getenv(EXEC_PATH_ENVIRONMENT);
 	if (env && *env) {
+		current_exec_path = env;
 		return env;
 	}
 
+	if ((path = git_path_from_env())) {
+		current_exec_path = path;
+		return path;
+	}
+
+	current_exec_path = builtin_exec_path;
 	return builtin_exec_path;
 }
git.c +53 -23

101 lines not on this page · raw message

Signature · 1 line
1.5.3.4.206.g58ba4-dirty
#3 lore raw
Quoted 8 lines · Scott Parish

I have a situation where software for a distribution is installed into a fake "prefix" and then moved to one of several potential places to be used by users. Given that the final location isn't static, i can't depend on builtin_exec_path. I'd really like users to be able to get started with git as easily as possible. With the current setup, they would have to create and maintain either an GIT_EXEC_PATH or an alias for including --exec-path, as well as a MANPATH and PERL5LIB. This seem like an unnessisary burden.

Interesting. How does this compare to this 2-patch-series:

http://repo.or.cz/w/git/mingw.git?a=commitdiff;h=e479ea2f911b8c70a269ba59372a4fef90f8907c
http://repo.or.cz/w/git/mingw.git?a=commitdiff;h=00a4ff4f3f8ec7e6b3ac15456f00b22b03f438ae

which I had come up with to accomplish something very similar (on Windows). Your approach looks superior, but I hadn't gone into depths, yet.

-- Hannes

author
#4 lore raw
Quoted 10 lines · Johannes Sixt
Quoted 8 lines · Scott Parish

I have a situation where software for a distribution is installed into a fake "prefix" and then moved to one of several potential places to be used by users. Given that the final location isn't static, i can't depend on builtin_exec_path. I'd really like users to be able to get started with git as easily as possible. With the current setup, they would have to create and maintain either an GIT_EXEC_PATH or an alias for including --exec-path, as well as a MANPATH and PERL5LIB. This seem like an unnessisary burden.

Quoted 8 lines

Interesting. How does this compare to this 2-patch-series:

http://repo.or.cz/w/git/mingw.git?a=commitdiff;h=e479ea2f911b8c70a269ba59372a4fef90f8907c
http://repo.or.cz/w/git/mingw.git?a=commitdiff;h=00a4ff4f3f8ec7e6b3ac15456f00b22b03f438ae

which I had come up with to accomplish something very similar (on Windows). Your approach looks superior, but I hadn't gone into depths, yet.

I know very little about what's available on windows. Looking at your code, it looks like the command isn't passed in in argv[0] and that it contains the windows style path seperators. My code currently assumes that PATH is a colon separated list, and that directories are separated with '/'. How should these assumptions change for windows?

sRp

Signature · 2 lines
Scott Parish
http://srparish.net/
#5 lore raw
Quoted 20 lines · Scott Parish
Quoted 9 lines · Johannes Sixt
Quoted 8 lines · Scott Parish

I have a situation where software for a distribution is installed into a fake "prefix" and then moved to one of several potential places to be used by users. Given that the final location isn't static, i can't depend on builtin_exec_path. I'd really like users to be able to get started with git as easily as possible. With the current setup, they would have to create and maintain either an GIT_EXEC_PATH or an alias for including --exec-path, as well as a MANPATH and PERL5LIB. This seem like an unnessisary burden.

Quoted 8 lines

Interesting. How does this compare to this 2-patch-series:

http://repo.or.cz/w/git/mingw.git?a=commitdiff;h=e479ea2f911b8c70a269ba59372a4fef90f8907c
http://repo.or.cz/w/git/mingw.git?a=commitdiff;h=00a4ff4f3f8ec7e6b3ac15456f00b22b03f438ae

which I had come up with to accomplish something very similar (on Windows). Your approach looks superior, but I hadn't gone into depths, yet.

Quoted 6 lines

I know very little about what's available on windows. Looking at your code, it looks like the command isn't passed in in argv[0] and that it contains the windows style path seperators. My code currently assumes that PATH is a colon separated list, and that directories are separated with '/'. How should these assumptions change for windows?

The question is rather whether my patches would be sufficient to also achieve your requirements. They turn bultin_exec_path into a non-constant that derives exec-path from argv[0] (which on Windows happens to be available in the global _pgmptr). Isn't this enough, or at least the essence of what you need?

(How to get to the value of _pgmptr, ie. argv[0], on non-Windows is a secondary matter.)

-- Hannes

#6 lore raw
In reply to Scott Parish

Hi,

Quoted 1 line · Scott Parish

+ check PATH for the location of git

Okay, but better do it only if the current exec_path did not succeed to find something, to stay as backwards compatible as possible.

Quoted 3 lines

+ the checking of argv[0] was restricted to absolute paths; remove that restriction so it also works when called with a relative path (eg ../../otheruser/usr/bin/git)

This will utterly break down when you try to do things in a subdirectory of your project. git will cd up, and the relative path will no longer be relative.

Quoted 2 lines

+ try to guess and set the env for the typical relative locations for MANPATH and PERL5LIB based off exec_path

Now this is ugly. At least make it a separate patch.

Quoted 29 lines
exec_cmd.c +29 -0
+/* Return the first path in PATH that git is found in or NULL if not found */
+char *git_path_from_env(void)
+{
+	const char *env_paths = getenv("PATH");
+	const char *git = "/git";
+	int git_len = strlen(git);
+	char *paths, *path, *colon, *git_path;
+	int path_len;
+	struct stat st;
+
+	if (!env_paths)
+		return NULL;
+
+	path_len = strlen(env_paths);
+	path = paths = xmalloc(path_len + 1);
+	memcpy(paths, env_paths, path_len + 1);
+
+	while ((char *)1 != path) {
+		if ((colon = strchr(path, ':')))
+		    *colon = 0;
+
+		path_len = strlen(path);
+		git_path = xmalloc(path_len + git_len + 1);
+		memcpy(git_path, path, path_len);
+		memcpy(git_path + path_len, git, git_len + 1);
+
+		if (!stat(git_path, &st)) { /* found */
+			free(paths);
+			git_path[path_len] = 0;

exec_cmd.c void git_set_exec_path(const char *exec_path) · line 54

Quoted 10 lines
exec_cmd.c +10 -0
+			return git_path;
+		}
+
+		free(git_path);
+		path = colon + 1;
+	}
+
+	free(paths);
+	return NULL;
+}

I am convinced that this function will look a lot less ugly when you use strbufs. And I'd call it "find_git_in_path()".

Quoted 13 lines

/* Returns the highest-priority, location to look for git programs. */ const char *git_exec_path(void)
{

exec_cmd.c +2 -1
-	const char *env;
+	const char *env, *path;
 
 	if (current_exec_path)
 		return current_exec_path;
 
 	env = getenv(EXEC_PATH_ENVIRONMENT);
 	if (env && *env) {
+		current_exec_path = env;
 		return env;

exec_cmd.c void git_set_exec_path(const char *exec_path) · line 78

Quoted 10 lines
exec_cmd.c +6 -0
 	}
 
+	if ((path = git_path_from_env())) {
+		current_exec_path = path;
+		return path;
+	}
+
+	current_exec_path = builtin_exec_path;
 	return builtin_exec_path;
 }

As I said, I'd rather have git try with builtin_exec_path first, and only if that fails, search through the PATH, for the _current_ command.

git.c line 9

Quoted 2 lines
git.c +1 -1
-static void prepend_to_path(const char *dir, int len)
+static void prepend_to_env(const char *env, const char *basedir,

I do not like this rename. It makes things more obscure, rather than clearing things up.

Quoted 24 lines
git.c +13 -10
+			   const char *subdir, const char *env_default)
 {
-	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);
+	const char *old = getenv(env);
+	int basedir_len = strlen(basedir);
+	int subdir_len = strlen(subdir);
+	char *new;
+	int old_len;
+	
+	if (!old)
+		old = env_default;
+
+	old_len = strlen(old);
+
+	new = xmalloc(basedir_len + subdir_len + old_len + 1);

git.c line 33

Quoted 10 lines
git.c +10 -0
+	
+	memcpy(new, basedir, basedir_len);
+	memcpy(new + basedir_len, subdir, subdir_len);
+	memcpy(new + basedir_len + subdir_len, old, old_len + 1);
+	
+	if (setenv(env, new, 1))
+		fprintf(stderr, "Setenv failed: %s\n", strerror(errno));
+
+	free(new);
+}

Again, this would be so much more elegant using strbufs.

Quoted 21 lines
git.c +15 -4
-	memcpy(path, dir, len);
-	path[len] = ':';
-	memcpy(path + len + 1, old_path, path_len - len);
+static void prepend_to_envs(const char *dir, int len)
+{
+	char *slash;
+	char *basedir;
+
+	/* basedir is dir with "/bin" stripped off */
+	basedir = xmalloc(len + 1);
+	memcpy(basedir, dir, len + 1);
+	
+	if ((slash = strrchr(basedir, '/'))) {
+		*slash = 0;
+		while (slash == basedir + --len) /* found trailing slash */
+			if ((slash = strrchr(basedir, '/')))
+				*slash = 0;
+	}
 
-	setenv("PATH", path, 1);

git.c line 59

Quoted 10 lines
git.c +7 -1
+	prepend_to_env("PATH", basedir, "/bin:",
+		       "/usr/local/bin:/usr/bin:/bin");
+	prepend_to_env("MANPATH", basedir, "/share/man:",
+		       "/usr/local/share/man:/usr/share/man");
+	prepend_to_env("PERL5LIB", basedir, "/lib/perl5:",
+		       "/usr/lib/perl5");
 
-	free(path);
+	free(basedir);
 }

As I said, this is so controversial it belongs into an own patch.

git.c int main(int argc, const char **argv) · line 447

Quoted 7 lines
git.c +1 -2
@@ -414,8 +444,7 @@ int main(int argc, const char **argv)
 	 */
 	if (slash) {
 		*slash++ = 0;
-		if (*cmd == '/')
-			exec_path = cmd;
+		exec_path = cmd;

As I said, this breaks down. This alone is enough reason to move it to its own patch. And I strongly suggest the use of make_path_absolute() (with an xstrdup()).

git.c int main(int argc, const char **argv) · line 488

Quoted 11 lines
git.c +4 -3
@@ -453,14 +482,15 @@ int main(int argc, const char **argv)
 	/*
 	 * 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.
+	 * environment, 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.

While reading this, I have to wonder why it is not just simpler to try with builtin_exec_path first, and if that fails, just let exec() find the program in the PATH?

Ciao,
Dscho

#7 lore raw
Quoted 3 lines · Johannes Schindelin

While reading this, I have to wonder why it is not just simpler to try with builtin_exec_path first, and if that fails, just let exec() find the program in the PATH?

Why not try the directory where the git executable is, too ?

Mike

#8 lore raw

Hi,

Quoted 6 lines · Mike Hommey
Quoted 3 lines · Johannes Schindelin

While reading this, I have to wonder why it is not just simpler to try with builtin_exec_path first, and if that fails, just let exec() find the program in the PATH?

Why not try the directory where the git executable is, too ?

I commented on that. If the git command was not specified with an absolute path, then make it absolute (and only if not even a relative path was specified, ignore this altogether since git is in the PATH).

I was a bit terse on the issue I have to admit, though.

Ciao,
Dscho

author
#9 lore raw
In reply to Johannes Schindelin
Quoted 3 lines · Johannes Schindelin

While reading this, I have to wonder why it is not just simpler to try with builtin_exec_path first, and if that fails, just let exec() find the program in the PATH?

I think you're right; that is a much better way to do this. I've rewritten this as two patches i'll post shortly. I have mixed feelings about the MANPATH/PERL5LIB hack, so i'm leaving it out for now.

sRp

Signature · 2 lines
Scott Parish
http://srparish.net/