Re: [PATCH] receive-pack: simplify run_update_post_hook()

From: Jeff King <peff@peff.net>
Date: Mar 17, 2017, 22:23 UTC
Message-ID: <20170317222320.nsg3yk3nopjadr5t@sigill.intra.peff.net>
In-Reply-To: <ac1c0cfc-b1de-37be-23b3-98ef1081ec4a@web.de>

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.

Show 10 quoted lines

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