Re: [PATCH 1/2] Move run_hook() from builtin-commit.c into run-command.c (libgit)
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jan 15, 2009, 15:46 UTC
- Message-ID
- <alpine.DEB.1.00.0901151637590.3586@pacific.mpi-cbg.de>
- In-Reply-To
- <1232031618-5243-1-git-send-email-s-beyer@gmx.net>
Hi,
On Thu, 15 Jan 2009, Stephan Beyer wrote:
> Stripping out a libified version seemed better to me than > copy and paste.
Oh, definitely.
Show 8 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.
Show 43 quoted lines
> diff --git a/run-command.c b/run-command.c
> index c90cdc5..602fe85 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -342,3 +342,38 @@ int finish_async(struct async *async)
> #endif
> return ret;
> }
> +
> +int run_hook(const char *index_file, const char *name, ...)
> +{
> + struct child_process hook;
> + const char *argv[10], *env[2];
> + char index[PATH_MAX];
> + va_list args;
> + int i;
> +
> + va_start(args, name);
> + argv[0] = git_path("hooks/%s", name);
> + i = 0;
> + do {
> + if (++i >= ARRAY_SIZE(argv))
> + die("run_hook(): too many arguments");
> + argv[i] = va_arg(args, const char *);
> + } while (argv[i]);
> + va_end(args);
> +
> + if (access(argv[0], X_OK) < 0)
> + return 0;
> +
> + memset(&hook, 0, sizeof(hook));
> + hook.argv = argv;
> + hook.no_stdin = 1;
> + hook.stdout_to_stderr = 1;
> + if (index_file) {
> + snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> + env[0] = index;
> + env[1] = NULL;
> + hook.env = env;
> + }
> +
> + return run_command(&hook);
> +}Lots of improvements possible (I agree; _after_ this patch):
- deuglify the loop,
- use ALLOC_GROW instead of having a fixed size argv,
- use an strbuf for the index file
- checking executability of argv[0] before filling argv,
and possibly others, too.
Ciao, Dscho