Re: [PATCH] git: handle aliases defined in $GIT_DIR/config

[Junio C Hamano <junkio@cox.net>]

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 ;-).

> +#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.

> +	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.

> +			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.

> +		}
> +	}
> +
> +	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.

> @@ -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.

>  		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.