Re: [PATCH] git: handle aliases defined in $GIT_DIR/config
- From
Junio C Hamano <junkio@cox.net>
- Date
- Jun 4, 2006, 20:24 UTC
- Message-ID
- <7v3bekacts.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <Pine.LNX.4.63.0606042047160.1598@wbgn013.biozentrum.uni-wuerzburg.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> For me, short cuts have to be easy to type, so they never > include digits, and they are never case sensitive, so I do not > need any fancy config stuff...
Fair enough, and the spirit is the same as what Pasky suggested earlier, I think.
However, I am not sure about some parts of the code. I started mucking with it myself, but realized it is far easier for me to just let the original submitter, especially the capable one like you, do a bit more work ;-).
Show 5 quoted lines
> +#define MAX_ALIAS_ARGS 32
> +
> +static int handle_alias(int *argcp, const char **argv, char **envp)
> +{
> + int i, i2, j = 0;Please name them src, dst and cnt.
Show 13 quoted lines
> + char *new_argv[MAX_ALIAS_ARGS];
> +
> + alias_command = argv[0];
> + git_config(git_alias_config);
> + if (!alias_string)
> + return 0;
> +
> + /* split alias_string */
> + new_argv[j++] = alias_string;
> + for (i = i2 = 0; alias_string[i]; i++, i2++) {
> + if (isspace(alias_string[i])) {
> + alias_string[i2] = 0;
> + while (alias_string[++i] && isspace(alias_string[i]));Please make empty loops easier to read by saying:
while (alias_string[++src] && isspace(alias_string[src])) ; /* skip */
> + new_argv[j++] = alias_string + i; > + i2 = i;
Do we need to reset dst here? I suspect starting the next parsed string immediately after the terminating NUL might be cleaner.
Show 6 quoted lines
> + if (j >= MAX_ALIAS_ARGS)
> + die("too many args in alias %s",
> + alias_command);
> + } else {
> + if (alias_string[i] == '\\')
> + i++;Barf when the config line ends with a lone backslash, perhaps? Since the configuration file parser uses backslash for quoting itself, the user would need to have double backslashes, I suspect. We might want to support single/double quote pairs in this parser as well. I would further suggest separating this "split single string into a pair of (argc, argv)" into a helper function, so we can reuse it in other parts of the system later.
> + if (i != i2) > + alias_string[i2] = alias_string[i];
Doing this unconditionally is probably more readable (i.e. lose the if condition) -- this is not performance critical part of the system.
Show 10 quoted lines
> + }
> + }
> +
> + if (j < 1)
> + die("empty alias: %s", alias_command);
> +
> + /* insert after command name */
> + if (j > 1)
> + memmove(argv + j, argv + 1, (*argcp - 1) * sizeof(char*));
> + memcpy(argv, new_argv, j * sizeof(char*));Who guarantees the original argv array main() received is big enough to hold (j-1) additional pointers, I wonder? I think you would need to allocate a new array, and muck with both argc and argv of the caller by passing the pointers to them, not just argc.
Show 5 quoted lines
> @@ -121,6 +176,7 @@ int main(int argc, const char **argv, ch
> if (!strncmp(cmd, "git-", 4)) {
> cmd += 4;
> argv[0] = cmd;
> + handle_alias(&argc, argv, envp);Hence probably "handle_alias(&argc, &argv, envp)" is needed here.
Show 9 quoted lines
> handle_internal_command(argc, argv, envp);
> die("cannot handle %s internally", cmd);
> }
> @@ -178,6 +234,8 @@ int main(int argc, const char **argv, ch
> exec_path = git_exec_path();
> prepend_to_path(exec_path, strlen(exec_path));
>
> + handle_alias(&argc, argv, envp);
> +... and here.