On Thu, Jan 6, 2011 at 8:41 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Erik Faye-Lund wrote:
>
>> --- a/git.c
>> +++ b/git.c
>> @@ -177,19 +177,20 @@ static int handle_alias(int *argcp, const char ***argv)
> [...]
>> - trace_printf("trace: alias to shell cmd: %s => %s\n",
>> - alias_command, alias_string + 1);
>
> Replaced by
>
> trace: run_command: ...
>
> (followed by "trace: exec: ..." on non-Windows (execv_shell_cmd)).
> Ok.
>
>> - ret = system(alias_string + 1);
>> +
>> + /* build alias_argv */
>> + alias_argv = malloc(sizeof(char *) * *argcp + 1);
>
> This seems to be missing parentheses, so valgrind will complain
> except on 8-bit systems. ;-)
>
> What if malloc fails?
>
2x whoops :)
>> + alias_argv[0] = alias_string + 1;
>> + for (i = 1; i < *argcp; ++i)
>> + alias_argv[i] = (*argv)[i];
>> + alias_argv[*argcp] = NULL;
>
> Nit: all these *argcp are noisy.
>
Yes. Fetching argc once is cleaner, thanks.
>> +
>> + ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
>> +
>> if (ret >= 0 && WIFEXITED(ret) &&
>
> The return value from run_command and from system do not mean
> the same thing.
>
Yet another "whoops" :)
>> die("Failed to run '%s' when expanding alias '%s'",
>> alias_string + 1, alias_command);
>
> run_command already prints an error message, but this one still
> seems useful since it mentions the alias.
>
> Except as noted above,
> Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
>
Thanks, I agree with all your comments. But why did you remove the "/*
build alias_argv */"-comment? :)
v2 coming up!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html