Jeff King <peff@xxxxxxxx> writes:
> On Fri, Mar 17, 2017 at 11:02:13PM +0100, René Scharfe wrote:
>
>> Instead of counting the arguments to see if there are any and then
>> building the full command use a single loop and add the hook command
>> just before the first argument. This reduces duplication and overall
>> code size.
>
> Yeah, I agree one loop is nicer.
>
>> - argv_array_push(&proc.args, hook);
>> for (cmd = commands; cmd; cmd = cmd->next) {
>> if (cmd->error_string || cmd->did_not_exist)
>> continue;
>> + if (!proc.args.argc)
>> + argv_array_push(&proc.args, hook);
>> argv_array_push(&proc.args, cmd->ref_name);
>> }
>> + if (!proc.args.argc)
>> + return;
>
> It looks at first like the result leaks, because you have to realize
> that the push will modify proc.args.argc.
Hmph, I needed to read the original twice to imagine how a paranoid
person can fear leaks. The return condition says "if args array is
empty, just return" and the thing being empty is an enough indication
to think nothing is leaking, at least for me.
Having said that, I'd admit that the "always push hook and then
clean up before returning if it turns out there is nothing to call
the hook for" is what I would have wrote if I were doing this, but
I'm inclined to think that is not because I would have thought of
both versions and picked the better one, but because I wouldn't have
noticed the "optimization opportunity" René spotted here (not that I
think an extra alloc would matter).
I'll queue the patch as-is, at least for now.
Thanks.
> I wonder if:
>
> argv_array_push(&proc.args, hook);
> for (cmd = commands; cmd; cmd = cmd->next) {
> if (!cmd->error_string && !cmd->did_not_exist)
> argv_array_push(&proc.args, cmd->ref_name);
> }
>
> if (proc.args.argc == 1) {
> argv_array_clear(&proc.args);
> return;
> }
>
> would be more obvious (at the cost of a pointless malloc in the corner
> case. I can live with it either way.
>
> -Peff