Re: [PATCH 1/2] Move run_hook() from builtin-commit.c into run-command.c (libgit)

From: Stephan Beyer <s-beyer@gmx.net>
Date: Jan 16, 2009, 17:25 UTC
Message-ID: <20090116172521.GD28177@leksak.fem-net>
In-Reply-To: <alpine.DEB.1.00.0901151637590.3586@pacific.mpi-cbg.de>

Hi,

Show 12 quoted lines

> > -	ret = start_command(&hook);
> > -	if (ret) {
> > -		warning("Could not spawn %s", argv[0]);
> > -		return ret;
> > -	}
> > -	ret = finish_command(&hook);
> > -	if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
> > -		warning("%s exited due to uncaught signal", argv[0]);
> 
> What are the side effects of replacing this with "ret = 
> run_command(&hook);"?  This has to be discussed and defended in the commit 
> message.

This is a very good point. The consequences are that two warnings are missing, but these are warnings that are useful enough to be included for all those hooks, imho.

Thanks!

> Lots of improvements possible (I agree; _after_ this patch):

[...]

> - use ALLOC_GROW instead of having a fixed size argv,

Agreed.

> - use an strbuf for the index file

Is that useful in some way? Currently it would only adds code to generate strbufs at the caller side. And in the case the caller has a strbuf for the index file, it can simply use the .buf member.

> - checking executability of argv[0] before filling argv,

Agreed.

Patch series v2 will follow.

Thanks,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F