From: Torsten Bögershausen <tboegi@web.de>
Date: Wed, 19 Mar 2014 07:32:24 GMT
Subject: Re: [PATCH 4/7] commit: fix patch hunk editing with "commit -p -m"
Message-ID: <53294808.8030106@web.de>
In-Reply-To: <1395136856-17225-4-git-send-email-benoit.pierre@gmail.com>

On 03/18/2014 11:00 AM, Benoit Pierre wrote:
> Don't change git environment: move the GIT_EDITOR=":" override to the
> hook command subprocess, like it's already done for GIT_INDEX_FILE.
>
> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
> ---
>   builtin/checkout.c      |  8 ++++----
>   builtin/clone.c         |  4 ++--
>   builtin/commit.c        | 35 ++++++++++++++++++++++++++++-------
>   builtin/gc.c            |  2 +-
>   builtin/merge.c         |  6 +++---
>   commit.h                |  3 +++
>   run-command.c           | 44 ++++++++++++++++++++++++++++++++------------
>   run-command.h           |  6 +++++-
>   t/t7514-commit-patch.sh |  4 ++--
>   9 files changed, 80 insertions(+), 32 deletions(-)
>
[]
> index 3914d9c..75abc47 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -760,13 +760,11 @@ char *find_hook(const char *name)
>   	return path;
>   }
>
> -int run_hook(const char *index_file, const char *name, ...)
> +int run_hook_ve(const char *const *env, const char *name, va_list args)
>   {
>   	struct child_process hook;
>   	struct argv_array argv = ARGV_ARRAY_INIT;
> -	const char *p, *env[2];
> -	char index[PATH_MAX];
(Please see below)
> -	va_list args;
> +	const char *p;
>   	int ret;
>
>   	p = find_hook(name);
> @@ -775,23 +773,45 @@ int run_hook(const char *index_file, const char *name, ...)
>
>   	argv_array_push(&argv, p);
>
> -	va_start(args, name);
>   	while ((p = va_arg(args, const char *)))
>   		argv_array_push(&argv, p);
> -	va_end(args);
>
>   	memset(&hook, 0, sizeof(hook));
>   	hook.argv = argv.argv;
> +	hook.env = env;
>   	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;
> -	}
>
>   	ret = run_command(&hook);
>   	argv_array_clear(&argv);
>   	return ret;
>   }
> +
> +int run_hook_le(const char *const *env, const char *name, ...)
> +{
> +	va_list args;
> +	int ret;
> +
> +	va_start(args, name);
> +	ret = run_hook_ve(env, name, args);
> +	va_end(args);
> +
> +	return ret;
> +}
> +
> +int run_hook_with_custom_index(const char *index_file, const char *name, ...)
> +{
> +	const char *hook_env[3] =  { NULL };
> +	char index[PATH_MAX];
Sorry being late with the review.

Recently some effort has been put to replace the
  "PATH_MAX/snprintf() combo" with code using strbuf.

So I think for new developed code it could make sense to avoid PATH_MAX 
from the start.
To my knowledge mkpathdup() from path.c can be used,
(or are there better ways ?)
and the whole function will look like below (not tested)

> +	va_list args;
> +	int ret;
> +
> +	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> +	hook_env[0] = index;
> +
> +	va_start(args, name);
> +	ret = run_hook_ve(hook_env, name, args);
> +	va_end(args);
> +
> +	return ret;
> +}

int run_hook_with_custom_index(const char *index_file, const char *name, 
...)
{
	const char *hook_env[3] =  { NULL };
	char index = mkpathdup("GIT_INDEX_FILE=%s", index_file);
	va_list args;
	int ret;

	hook_env[0] = index;

	va_start(args, name);
	ret = run_hook_ve(hook_env, name, args);
	va_end(args);

	free(index);
	return ret;
}




> diff --git a/run-command.h b/run-command.h
> index 6b985af..88460f9 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -47,7 +47,11 @@ int run_command(struct child_process *);
>
>   extern char *find_hook(const char *name);
>   LAST_ARG_MUST_BE_NULL
> -extern int run_hook(const char *index_file, const char *name, ...);
> +extern int run_hook_le(const char *const *env, const char *name, ...);
> +extern int run_hook_ve(const char *const *env, const char *name, va_list args);
> +
> +LAST_ARG_MUST_BE_NULL
> +extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
>
>   #define RUN_COMMAND_NO_STDIN 1
>   #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
> diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
> index 41dd37a..998a210 100755
> --- a/t/t7514-commit-patch.sh
> +++ b/t/t7514-commit-patch.sh
> @@ -15,7 +15,7 @@ test_expect_success 'setup (initial)' '
>   	git commit -m commit1
>   '
>
> -test_expect_failure 'edit hunk "commit -p -m message"' '
> +test_expect_success 'edit hunk "commit -p -m message"' '
>   	test_when_finished "rm -f editor_was_started" &&
>   	rm -f editor_was_started &&
>   	echo more >>file &&
> @@ -23,7 +23,7 @@ test_expect_failure 'edit hunk "commit -p -m message"' '
>   	test -r editor_was_started
>   '
>
> -test_expect_failure 'edit hunk "commit --dry-run -p -m message"' '
> +test_expect_success 'edit hunk "commit --dry-run -p -m message"' '
>   	test_when_finished "rm -f editor_was_started" &&
>   	rm -f editor_was_started &&
>   	echo more >>file &&
>