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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Oct 22, 2007, 05:30 UTC
Message-ID: <20071022053016.GN14735@spearce.org>
In-Reply-To: <20071021214846.GI16291@srparish.net>

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.