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

[Junio C Hamano <gitster@pobox.com>]

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;
	}