Web lists-archives.com

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




Am 17.03.2017 um 23:23 schrieb Jeff King:
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. 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.

Sure, that's even simpler. I don't know how often the no-args branch would be taken and if the extra allocation would even matter -- that's why I tried to avoid it -- but probably the answers are not often and not much. The only test case that hits it is for the deletion of a non-existent ref.

René