git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] receive-pack: simplify run_update_post_hook()
@ 2017-03-17 22:02 René Scharfe
  2017-03-17 22:23 ` Jeff King
  2017-03-17 22:50 ` Jonathan Nieder
  0 siblings, 2 replies; 6+ messages in thread
From: René Scharfe @ 2017-03-17 22:02 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Jeff King

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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/receive-pack.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 83492af05f..fb2a090a0c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1128,25 +1128,22 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 static void run_update_post_hook(struct command *commands)
 {
 	struct command *cmd;
-	int argc;
 	struct child_process proc = CHILD_PROCESS_INIT;
 	const char *hook;
 
 	hook = find_hook("post-update");
-	for (argc = 0, cmd = commands; cmd; cmd = cmd->next) {
-		if (cmd->error_string || cmd->did_not_exist)
-			continue;
-		argc++;
-	}
-	if (!argc || !hook)
+	if (!hook)
 		return;
 
-	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;
 
 	proc.no_stdin = 1;
 	proc.stdout_to_stderr = 1;
-- 
2.12.0


^ permalink raw reply	[flat|threaded] 6+ messages in thread

* Re: [PATCH] receive-pack: simplify run_update_post_hook()
  2017-03-17 22:02 [PATCH] receive-pack: simplify run_update_post_hook() René Scharfe
@ 2017-03-17 22:23 ` Jeff King
  2017-03-17 23:21   ` René Scharfe
  2017-03-18 17:12   ` Junio C Hamano
  2017-03-17 22:50 ` Jonathan Nieder
  1 sibling, 2 replies; 6+ messages in thread
From: Jeff King @ 2017-03-17 22:23 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git List, Junio C Hamano

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.

-Peff

^ permalink raw reply	[flat|threaded] 6+ messages in thread

* Re: [PATCH] receive-pack: simplify run_update_post_hook()
  2017-03-17 22:02 [PATCH] receive-pack: simplify run_update_post_hook() René Scharfe
  2017-03-17 22:23 ` Jeff King
@ 2017-03-17 22:50 ` Jonathan Nieder
  1 sibling, 0 replies; 6+ messages in thread
From: Jonathan Nieder @ 2017-03-17 22:50 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git List, Junio C Hamano, Jeff King

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.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  builtin/receive-pack.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

^ permalink raw reply	[flat|threaded] 6+ messages in thread

* Re: [PATCH] receive-pack: simplify run_update_post_hook()
  2017-03-17 22:23 ` Jeff King
@ 2017-03-17 23:21   ` René Scharfe
  2017-03-18 17:12   ` Junio C Hamano
  1 sibling, 0 replies; 6+ messages in thread
From: René Scharfe @ 2017-03-17 23:21 UTC (permalink / raw)
  To: Jeff King; +Cc: Git List, Junio C Hamano

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é

^ permalink raw reply	[flat|threaded] 6+ messages in thread

* Re: [PATCH] receive-pack: simplify run_update_post_hook()
  2017-03-17 22:23 ` Jeff King
  2017-03-17 23:21   ` René Scharfe
@ 2017-03-18 17:12   ` Junio C Hamano
  2017-03-20  3:35     ` Jeff King
  1 sibling, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2017-03-18 17:12 UTC (permalink / raw)
  To: Jeff King; +Cc: René Scharfe, Git List

Jeff King <peff@peff.net> 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

^ permalink raw reply	[flat|threaded] 6+ messages in thread

* Re: [PATCH] receive-pack: simplify run_update_post_hook()
  2017-03-18 17:12   ` Junio C Hamano
@ 2017-03-20  3:35     ` Jeff King
  0 siblings, 0 replies; 6+ messages in thread
From: Jeff King @ 2017-03-20  3:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: René Scharfe, Git List

On Sat, Mar 18, 2017 at 10:12:07AM -0700, Junio C Hamano wrote:

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

Yeah, I think I just read it funny the first time. I'm OK with it as-is.

-Peff

^ permalink raw reply	[flat|threaded] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-17 22:02 [PATCH] receive-pack: simplify run_update_post_hook() René Scharfe
2017-03-17 22:23 ` Jeff King
2017-03-17 23:21   ` René Scharfe
2017-03-18 17:12   ` Junio C Hamano
2017-03-20  3:35     ` Jeff King
2017-03-17 22:50 ` Jonathan Nieder

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox