From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Thu, 06 Jan 2011 19:52:04 GMT
Subject: Re: [PATCH/RFC] alias: use run_command api to execute aliases
Message-ID: <AANLkTiniRYH2jz69vhuSCzPMmAg4KQ5nowd7NqrmMYVw@mail.gmail.com>
In-Reply-To: <20110106194101.GA14750@burratino>

On Thu, Jan 6, 2011 at 8:41 PM, Jonathan Nieder <jrnieder@gmail.com> 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@gmail.com>
>

Thanks, I agree with all your comments. But why did you remove the "/*
build alias_argv */"-comment? :)

v2 coming up!