{"thread":{"id":"45414","subject":"[PATCH] receive-pack: simplify run_update_post_hook()","startedAt":"2017-03-17T22:09:40Z","lastAt":"2017-03-20T03:35:42Z","messageCount":6,"participants":["René Scharfe","Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"314397","messageId":"ac1c0cfc-b1de-37be-23b3-98ef1081ec4a@web.de","threadId":"45414","inReplyTo":null,"subject":"[PATCH] receive-pack: simplify run_update_post_hook()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-17T22:02:13Z","receivedAt":"2017-03-17T22:09:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Instead of counting the arguments to see if there are any and then\nbuilding the full command use a single loop and add the hook command\njust before the first argument. This reduces duplication and overall\ncode size.\n\nSigned-off-by: Rene Scharfe \n---\n builtin/receive-pack.c | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 83492af05f..fb2a090a0c 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1128,25 +1128,22 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n static void run_update_post_hook(struct command *commands)\n {\n \tstruct command *cmd;\n-\tint argc;\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n \tconst char *hook;\n \n \thook = find_hook(\"post-update\");\n-\tfor (argc = 0, cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (cmd->error_string || cmd->did_not_exist)\n-\t\t\tcontinue;\n-\t\targc++;\n-\t}\n-\tif (!argc || !hook)\n+\tif (!hook)\n \t\treturn;\n \n-\targv_array_push(&proc.args, hook);\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (cmd->error_string || cmd->did_not_exist)\n \t\t\tcontinue;\n+\t\tif (!proc.args.argc)\n+\t\t\targv_array_push(&proc.args, hook);\n \t\targv_array_push(&proc.args, cmd->ref_name);\n \t}\n+\tif (!proc.args.argc)\n+\t\treturn;\n \n \tproc.no_stdin = 1;\n \tproc.stdout_to_stderr = 1;\n-- \n2.12.0\n\n"},{"id":"314418","messageId":"20170317222320.nsg3yk3nopjadr5t@sigill.intra.peff.net","threadId":"45414","inReplyTo":"ac1c0cfc-b1de-37be-23b3-98ef1081ec4a@web.de","subject":"Re: [PATCH] receive-pack: simplify run_update_post_hook()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-17T22:23:20Z","receivedAt":"2017-03-17T23:25:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 17, 2017 at 11:02:13PM +0100, René Scharfe wrote:\n\n> Instead of counting the arguments to see if there are any and then\n> building the full command use a single loop and add the hook command\n> just before the first argument. This reduces duplication and overall\n> code size.\n\nYeah, I agree one loop is nicer.\n\n> -\targv_array_push(&proc.args, hook);\n> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n> \t\tif (cmd->error_string || cmd->did_not_exist)\n> \t\t\tcontinue;\n> +\t\tif (!proc.args.argc)\n> +\t\t\targv_array_push(&proc.args, hook);\n> \t\targv_array_push(&proc.args, cmd->ref_name);\n> \t}\n> +\tif (!proc.args.argc)\n> +\t\treturn;\n\nIt looks at first like the result leaks, because you have to realize\nthat the push will modify proc.args.argc. I wonder if:\n\n argv_array_push(&proc.args, hook);\n for (cmd = commands; cmd; cmd = cmd->next) {\n\tif (!cmd->error_string && !cmd->did_not_exist)\n\t\targv_array_push(&proc.args, cmd->ref_name);\n }\n\n if (proc.args.argc == 1) {\n\targv_array_clear(&proc.args);\n\treturn;\n }\n\nwould be more obvious (at the cost of a pointless malloc in the corner\ncase. I can live with it either way.\n\n-Peff\n"},{"id":"314422","messageId":"f61a1089-833b-2883-deab-f97cb77cef96@web.de","threadId":"45414","inReplyTo":"20170317222320.nsg3yk3nopjadr5t@sigill.intra.peff.net","subject":"Re: [PATCH] receive-pack: simplify run_update_post_hook()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-17T23:21:11Z","receivedAt":"2017-03-17T23:30:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.03.2017 um 23:23 schrieb Jeff King:\n> On Fri, Mar 17, 2017 at 11:02:13PM +0100, René Scharfe wrote:\n>\n>> Instead of counting the arguments to see if there are any and then\n>> building the full command use a single loop and add the hook command\n>> just before the first argument. This reduces duplication and overall\n>> code size.\n>\n> Yeah, I agree one loop is nicer.\n>\n>> -\targv_array_push(&proc.args, hook);\n>> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n>> \t\tif (cmd->error_string || cmd->did_not_exist)\n>> \t\t\tcontinue;\n>> +\t\tif (!proc.args.argc)\n>> +\t\t\targv_array_push(&proc.args, hook);\n>> \t\targv_array_push(&proc.args, cmd->ref_name);\n>> \t}\n>> +\tif (!proc.args.argc)\n>> +\t\treturn;\n>\n> It looks at first like the result leaks, because you have to realize\n> that the push will modify proc.args.argc. I wonder if:\n>\n> argv_array_push(&proc.args, hook);\n> for (cmd = commands; cmd; cmd = cmd->next) {\n> \tif (!cmd->error_string && !cmd->did_not_exist)\n> \t\targv_array_push(&proc.args, cmd->ref_name);\n> }\n>\n> if (proc.args.argc == 1) {\n> \targv_array_clear(&proc.args);\n> \treturn;\n> }\n>\n> would be more obvious (at the cost of a pointless malloc in the corner\n> case. I can live with it either way.\n\nSure, that's even simpler. I don't know how often the no-args branch \nwould be taken and if the extra allocation would even matter -- that's \nwhy I tried to avoid it -- but probably the answers are not often and \nnot much. The only test case that hits it is for the deletion of a \nnon-existent ref.\n\nRené\n"},{"id":"314428","messageId":"20170317225040.GQ26789@aiede.mtv.corp.google.com","threadId":"45414","inReplyTo":"ac1c0cfc-b1de-37be-23b3-98ef1081ec4a@web.de","subject":"Re: [PATCH] receive-pack: simplify run_update_post_hook()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-17T22:50:40Z","receivedAt":"2017-03-17T23:59:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"René Scharfe wrote:\n\n> Instead of counting the arguments to see if there are any and then\n> building the full command use a single loop and add the hook command\n> just before the first argument. This reduces duplication and overall\n> code size.\n>\n> Signed-off-by: Rene Scharfe \n> ---\n> builtin/receive-pack.c | 13 +++++--------\n> 1 file changed, 5 insertions(+), 8 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"314508","messageId":"xmqqshma4ihk.fsf@gitster.mtv.corp.google.com","threadId":"45414","inReplyTo":"20170317222320.nsg3yk3nopjadr5t@sigill.intra.peff.net","subject":"Re: [PATCH] receive-pack: simplify run_update_post_hook()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-18T17:12:07Z","receivedAt":"2017-03-18T17:18:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Mar 17, 2017 at 11:02:13PM +0100, René Scharfe wrote:\n>\n>> Instead of counting the arguments to see if there are any and then\n>> building the full command use a single loop and add the hook command\n>> just before the first argument. This reduces duplication and overall\n>> code size.\n>\n> Yeah, I agree one loop is nicer.\n>\n>> -\targv_array_push(&proc.args, hook);\n>> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n>> \t\tif (cmd->error_string || cmd->did_not_exist)\n>> \t\t\tcontinue;\n>> +\t\tif (!proc.args.argc)\n>> +\t\t\targv_array_push(&proc.args, hook);\n>> \t\targv_array_push(&proc.args, cmd->ref_name);\n>> \t}\n>> +\tif (!proc.args.argc)\n>> +\t\treturn;\n>\n> It looks at first like the result leaks, because you have to realize\n> that the push will modify proc.args.argc. \n\nHmph, I needed to read the original twice to imagine how a paranoid\nperson can fear leaks. The return condition says \"if args array is\nempty, just return\" and the thing being empty is an enough indication\nto think nothing is leaking, at least for me.\n\nHaving said that, I'd admit that the \"always push hook and then\nclean up before returning if it turns out there is nothing to call\nthe hook for\" is what I would have wrote if I were doing this, but\nI'm inclined to think that is not because I would have thought of\nboth versions and picked the better one, but because I wouldn't have\nnoticed the \"optimization opportunity\" René spotted here (not that I\nthink an extra alloc would matter).\n\nI'll queue the patch as-is, at least for now.\n\nThanks.\n\n> I wonder if:\n>\n> argv_array_push(&proc.args, hook);\n> for (cmd = commands; cmd; cmd = cmd->next) {\n> \tif (!cmd->error_string && !cmd->did_not_exist)\n> \t\targv_array_push(&proc.args, cmd->ref_name);\n> }\n>\n> if (proc.args.argc == 1) {\n> \targv_array_clear(&proc.args);\n> \treturn;\n> }\n>\n> would be more obvious (at the cost of a pointless malloc in the corner\n> case. I can live with it either way.\n>\n> -Peff\n"},{"id":"314618","messageId":"20170320033530.sl4rbd2lq3qo2kgl@sigill.intra.peff.net","threadId":"45414","inReplyTo":"xmqqshma4ihk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] receive-pack: simplify run_update_post_hook()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T03:35:31Z","receivedAt":"2017-03-20T03:35:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 18, 2017 at 10:12:07AM -0700, Junio C Hamano wrote:\n\n> >> -\targv_array_push(&proc.args, hook);\n> >> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n> >> \t\tif (cmd->error_string || cmd->did_not_exist)\n> >> \t\t\tcontinue;\n> >> +\t\tif (!proc.args.argc)\n> >> +\t\t\targv_array_push(&proc.args, hook);\n> >> \t\targv_array_push(&proc.args, cmd->ref_name);\n> >> \t}\n> >> +\tif (!proc.args.argc)\n> >> +\t\treturn;\n> >\n> > It looks at first like the result leaks, because you have to realize\n> > that the push will modify proc.args.argc. \n> \n> Hmph, I needed to read the original twice to imagine how a paranoid\n> person can fear leaks. The return condition says \"if args array is\n> empty, just return\" and the thing being empty is an enough indication\n> to think nothing is leaking, at least for me.\n\nYeah, I think I just read it funny the first time. I'm OK with it as-is.\n\n-Peff\n"}]}