Re: [PATCH 6/7] walk $PATH to generate list of commands for "help -a"
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 28, 2007, 06:18 UTC
- Message-ID
- <7vsl3vzrs5.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <1193474215-6728-6-git-send-email-srp@srparish.net>
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/
Show 6 quoted lines
> 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.
Show 17 quoted lines
> 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.
Show 8 quoted lines
> @@ -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)".
Show 10 quoted lines
> + 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;
}