diff mbox series

[v4,9/9] run_commit_hook: take strvec instead of varargs

Message ID 20200909004939.1942347-10-emilyshaffer@google.com
State New
Headers show
Series propose config-based hooks | expand

Commit Message

Emily Shaffer Sept. 9, 2020, 12:49 a.m. UTC
Taking varargs in run_commit_hook() led to some bizarre patterns, like
callers using two string variables (which may or may not be filled) to
express different argument lists for the commit hooks. Because
run_commit_hook() no longer needs to call a variadic function for the
hook run itself, we can use strvec to make the calling code more
conventional.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 builtin/commit.c | 46 +++++++++++++++++++++++-----------------------
 builtin/merge.c  | 20 ++++++++++++++++----
 commit.c         | 13 ++-----------
 commit.h         |  5 +++--
 sequencer.c      | 15 ++++++++-------
 5 files changed, 52 insertions(+), 47 deletions(-)

Comments

Phillip Wood Sept. 10, 2020, 2:16 p.m. UTC | #1
Hi Emily

On 09/09/2020 01:49, Emily Shaffer wrote:
> Taking varargs in run_commit_hook() led to some bizarre patterns, like
> callers using two string variables (which may or may not be filled) to
> express different argument lists for the commit hooks. Because
> run_commit_hook() no longer needs to call a variadic function for the
> hook run itself, we can use strvec to make the calling code more
> conventional.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>   builtin/commit.c | 46 +++++++++++++++++++++++-----------------------
>   builtin/merge.c  | 20 ++++++++++++++++----
>   commit.c         | 13 ++-----------
>   commit.h         |  5 +++--
>   sequencer.c      | 15 ++++++++-------
>   5 files changed, 52 insertions(+), 47 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index a19c6478eb..f029d4f5ac 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -691,8 +691,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>   	struct strbuf committer_ident = STRBUF_INIT;
>   	int committable;
>   	struct strbuf sb = STRBUF_INIT;
> -	const char *hook_arg1 = NULL;
> -	const char *hook_arg2 = NULL;
> +	struct strvec hook_args = STRVEC_INIT;
>   	int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
>   	int old_display_comment_prefix;
>   	int merge_contains_scissors = 0;
> @@ -700,7 +699,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>   	/* This checks and barfs if author is badly specified */
>   	determine_author_info(author_ident);
>   
> -	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
> +	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit",
> +					  &hook_args))
>   		return 0;
>   
>   	if (squash_message) {
> @@ -722,27 +722,28 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>   		}
>   	}
>   
> +	strvec_push(&hook_args, git_path_commit_editmsg());

This is a long way from the call where we use hook_args. With the 
variadic interface it is clear by looking at the call to 
run_commit_hook() what the first argument is and that is always the same.

>   	if (have_option_m && !fixup_message) {
>   		strbuf_addbuf(&sb, &message);
> -		hook_arg1 = "message";
> +		strvec_push(&hook_args, "message");
>   	} else if (logfile && !strcmp(logfile, "-")) {
>   		if (isatty(0))
>   			fprintf(stderr, _("(reading log message from standard input)\n"));
>   		if (strbuf_read(&sb, 0, 0) < 0)
>   			die_errno(_("could not read log from standard input"));
> -		hook_arg1 = "message";
> +		strvec_push(&hook_args, "message");
>   	} else if (logfile) {
>   		if (strbuf_read_file(&sb, logfile, 0) < 0)
>   			die_errno(_("could not read log file '%s'"),
>   				  logfile);
> -		hook_arg1 = "message";
> +		strvec_push(&hook_args, "message");
>   	} else if (use_message) {
>   		char *buffer;
>   		buffer = strstr(use_message_buffer, "\n\n");
>   		if (buffer)
>   			strbuf_addstr(&sb, skip_blank_lines(buffer + 2));
> -		hook_arg1 = "commit";
> -		hook_arg2 = use_message;
> +		strvec_pushl(&hook_args, "commit", use_message, NULL);
>   	} else if (fixup_message) {
>   		struct pretty_print_context ctx = {0};
>   		struct commit *commit;
> @@ -754,7 +755,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>   				      &sb, &ctx);
>   		if (have_option_m)
>   			strbuf_addbuf(&sb, &message);
> -		hook_arg1 = "message";
> +		strvec_push(&hook_args, "message");
>   	} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
>   		size_t merge_msg_start;
>   
> @@ -765,9 +766,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>   		if (!stat(git_path_squash_msg(the_repository), &statbuf)) {
>   			if (strbuf_read_file(&sb, git_path_squash_msg(the_repository), 0) < 0)
>   				die_errno(_("could not read SQUASH_MSG"));
> -			hook_arg1 = "squash";
> +			strvec_push(&hook_args, "squash");
>   		} else
> -			hook_arg1 = "merge";
> +			strvec_push(&hook_args, "merge");
>   
>   		merge_msg_start = sb.len;
>   		if (strbuf_read_file(&sb, git_path_merge_msg(the_repository), 0) < 0)
> @@ -781,11 +782,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>   	} else if (!stat(git_path_squash_msg(the_repository), &statbuf)) {
>   		if (strbuf_read_file(&sb, git_path_squash_msg(the_repository), 0) < 0)
>   			die_errno(_("could not read SQUASH_MSG"));
> -		hook_arg1 = "squash";
> +		strvec_push(&hook_args, "squash");
>   	} else if (template_file) {
>   		if (strbuf_read_file(&sb, template_file, 0) < 0)
>   			die_errno(_("could not read '%s'"), template_file);
> -		hook_arg1 = "template";
> +		strvec_push(&hook_args, "template");
>   		clean_message_contents = 0;
>   	}
>   
> @@ -794,11 +795,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>   	 * just set the argument(s) to the prepare-commit-msg hook.
>   	 */
>   	else if (whence == FROM_MERGE)
> -		hook_arg1 = "merge";
> -	else if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) {
> -		hook_arg1 = "commit";
> -		hook_arg2 = "CHERRY_PICK_HEAD";
> -	}
> +		strvec_push(&hook_args, "merge");
> +	else if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK)
> +		strvec_pushl(&hook_args, "commit", "CHERRY_PICK_HEAD", NULL);
>   
>   	if (squash_message) {
>   		/*
> @@ -806,8 +805,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>   		 * then we're possibly hijacking other commit log options.
>   		 * Reset the hook args to tell the real story.
>   		 */
> -		hook_arg1 = "message";
> -		hook_arg2 = "";
> +		strvec_clear(&hook_args);
> +		strvec_pushl(&hook_args, git_path_commit_editmsg(), "message", NULL);

It's a shame we have to clear the strvec and remember to re-add 
git_path_commit_editmsg() here.

>   	}
>   
>   	s->fp = fopen_for_writing(git_path_commit_editmsg());
> @@ -1001,8 +1000,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>   		return 0;
>   	}
>   
> -	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
> -			    git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))
> +	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg", &hook_args))
>   		return 0;
>   
>   	if (use_editor) {
> @@ -1017,8 +1015,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>   		strvec_clear(&env);
>   	}
>   
> +	strvec_clear(&hook_args);
> +	strvec_push(&hook_args, git_path_commit_editmsg());
>   	if (!no_verify &&
> -	    run_commit_hook(use_editor, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) {
> +	    run_commit_hook(use_editor, index_file, "commit-msg", &hook_args)) {
>   		return 0;
>   	}
>   
> diff --git a/builtin/merge.c b/builtin/merge.c
> index c1a9d0083d..863c9039a3 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -821,10 +821,14 @@ static void write_merge_heads(struct commit_list *);
>   static void prepare_to_commit(struct commit_list *remoteheads)
>   {
>   	struct strbuf msg = STRBUF_INIT;
> +	struct strvec hook_args = STRVEC_INIT;
> +	struct strbuf hook_name = STRBUF_INIT;

As far as I can see hook_name is never used except to free it at the end.

>   	const char *index_file = get_index_file();
>   
> -	if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
> +	if (!no_verify && run_commit_hook(0 < option_edit, index_file,
> +					  "pre-merge-commit", &hook_args))
>   		abort_commit(remoteheads, NULL);
> +
>   	/*
>   	 * Re-read the index as pre-merge-commit hook could have updated it,
>   	 * and write it out as a tree.  We must do this before we invoke
> @@ -832,6 +836,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>   	 */
>   	if (hook_exists("pre-merge-commit"))
>   		discard_cache();
> +
>   	read_cache_from(index_file);
>   	strbuf_addbuf(&msg, &merge_msg);
>   	if (squash)
> @@ -851,17 +856,22 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>   		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
>   	write_merge_heads(remoteheads);
>   	write_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);
> +
> +	strvec_clear(&hook_args);
> +	strvec_pushl(&hook_args, git_path_merge_msg(the_repository), "merge", NULL);
>   	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
> -			    git_path_merge_msg(the_repository), "merge", NULL))
> +			    &hook_args))
>   		abort_commit(remoteheads, NULL);
> +
>   	if (0 < option_edit) {
>   		if (launch_editor(git_path_merge_msg(the_repository), NULL, NULL))
>   			abort_commit(remoteheads, NULL);
>   	}
>   
> +	strvec_clear(&hook_args);
> +	strvec_push(&hook_args, git_path_merge_msg(the_repository));
>   	if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
> -					  "commit-msg",
> -					  git_path_merge_msg(the_repository), NULL))
> +					  "commit-msg", &hook_args))
>   		abort_commit(remoteheads, NULL);
>   
>   	read_merge_msg(&msg);
> @@ -871,6 +881,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>   	strbuf_release(&merge_msg);
>   	strbuf_addbuf(&merge_msg, &msg);
>   	strbuf_release(&msg);
> +	strbuf_release(&hook_name);
> +	strvec_clear(&hook_args);
>   }
>   
>   static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
> diff --git a/commit.c b/commit.c
> index c7a243e848..726407152c 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1629,12 +1629,9 @@ size_t ignore_non_trailer(const char *buf, size_t len)
>   }
>   
>   int run_commit_hook(int editor_is_used, const char *index_file,
> -		    const char *name, ...)
> +		    const char *name, struct strvec *args)
>   {
>   	struct strvec hook_env = STRVEC_INIT;
> -	va_list args;
> -	const char *arg;
> -	struct strvec hook_args = STRVEC_INIT;
>   	struct strbuf hook_name = STRBUF_INIT;
>   	int ret;
>   
> @@ -1648,14 +1645,8 @@ int run_commit_hook(int editor_is_used, const char *index_file,
>   	if (!editor_is_used)
>   		strvec_push(&hook_env, "GIT_EDITOR=:");
>   
> -	va_start(args, name);
> -	while ((arg = va_arg(args, const char *)))
> -		strvec_push(&hook_args, arg);
> -	va_end(args);
> -
> -	ret = run_hooks(hook_env.v, &hook_name, &hook_args);
> +	ret = run_hooks(hook_env.v, &hook_name, args);
>   	strvec_clear(&hook_env);
> -	strvec_clear(&hook_args);
>   	strbuf_release(&hook_name);
>   
>   	return ret;
> diff --git a/commit.h b/commit.h
> index e901538909..978da3c3e0 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -9,6 +9,7 @@
>   #include "string-list.h"
>   #include "pretty.h"
>   #include "commit-slab.h"
> +#include "strvec.h"
>   
>   #define COMMIT_NOT_FROM_GRAPH 0xFFFFFFFF
>   #define GENERATION_NUMBER_INFINITY 0xFFFFFFFF
> @@ -353,7 +354,7 @@ void verify_merge_signature(struct commit *commit, int verbose,
>   int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);
>   int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);
>   
> -LAST_ARG_MUST_BE_NULL
> -int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
> +int run_commit_hook(int editor_is_used, const char *index_file,
> +		    const char *name, struct strvec *args);
>   
>   #endif /* COMMIT_H */
> diff --git a/sequencer.c b/sequencer.c
> index cc3f8fa88e..5dd4b134d6 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1124,22 +1124,23 @@ static int run_prepare_commit_msg_hook(struct repository *r,
>   				       const char *commit)
>   {
>   	int ret = 0;
> -	const char *name, *arg1 = NULL, *arg2 = NULL;
> +	struct strvec args = STRVEC_INIT;
> +	const char *name = git_path_commit_editmsg();
>   
> -	name = git_path_commit_editmsg();
> +	strvec_push(&args, name);

I think you could drop name altogether and just pass 
git_path_commit_editmsg() instead.

>   	if (write_message(msg->buf, msg->len, name, 0))
>   		return -1;
>   
>   	if (commit) {
> -		arg1 = "commit";
> -		arg2 = commit;
> +		strvec_push(&args, "commit");
> +		strvec_push(&args, commit);

Complete nit pick but the other conversions all used strvec_pushl() 
rather than two strvec_push() calls.

I don't have a strong opinion about these changes (though I'm not 
particularly enthusiastic). Having to push the arguments in order is not 
particularly convenient and the use of strvec_pushl() means we are 
replacing a small number of variadic calls to run_commit_hook() with a 
larger number of calls to a different variadic interface.

Best Wishes

Phillip

>   	} else {
> -		arg1 = "message";
> +		strvec_push(&args, "message");
>   	}
> -	if (run_commit_hook(0, r->index_file, "prepare-commit-msg", name,
> -			    arg1, arg2, NULL))
> +	if (run_commit_hook(0, r->index_file, "prepare-commit-msg", &args))
>   		ret = error(_("'prepare-commit-msg' hook failed"));
>   
> +	strvec_clear(&args);
>   	return ret;
>   }
>   
>
Phillip Wood Sept. 11, 2020, 1:20 p.m. UTC | #2
On 10/09/2020 15:16, Phillip Wood wrote:
> Hi Emily
> 
> On 09/09/2020 01:49, Emily Shaffer wrote:
>> Taking varargs in run_commit_hook() led to some bizarre patterns, like
>> callers using two string variables (which may or may not be filled) to
>> express different argument lists for the commit hooks. Because
>> run_commit_hook() no longer needs to call a variadic function for the
>> hook run itself, we can use strvec to make the calling code more
>> conventional.
>>
>> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
>> ---
>>   builtin/commit.c | 46 +++++++++++++++++++++++-----------------------
>>   builtin/merge.c  | 20 ++++++++++++++++----
>>   commit.c         | 13 ++-----------
>>   commit.h         |  5 +++--
>>   sequencer.c      | 15 ++++++++-------
>>   5 files changed, 52 insertions(+), 47 deletions(-)
>>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index a19c6478eb..f029d4f5ac 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -691,8 +691,7 @@ static int prepare_to_commit(const char 
>> *index_file, const char *prefix,
>>       struct strbuf committer_ident = STRBUF_INIT;
>>       int committable;
>>       struct strbuf sb = STRBUF_INIT;
>> -    const char *hook_arg1 = NULL;
>> -    const char *hook_arg2 = NULL;
>> +    struct strvec hook_args = STRVEC_INIT;
>>       int clean_message_contents = (cleanup_mode != 
>> COMMIT_MSG_CLEANUP_NONE);
>>       int old_display_comment_prefix;
>>       int merge_contains_scissors = 0;
>> @@ -700,7 +699,8 @@ static int prepare_to_commit(const char 
>> *index_file, const char *prefix,
>>       /* This checks and barfs if author is badly specified */
>>       determine_author_info(author_ident);
>> -    if (!no_verify && run_commit_hook(use_editor, index_file, 
>> "pre-commit", NULL))
>> +    if (!no_verify && run_commit_hook(use_editor, index_file, 
>> "pre-commit",
>> +                      &hook_args))
>>           return 0;
>>       if (squash_message) {
>> @@ -722,27 +722,28 @@ static int prepare_to_commit(const char 
>> *index_file, const char *prefix,
>>           }
>>       }
>> +    strvec_push(&hook_args, git_path_commit_editmsg());
> 
> This is a long way from the call where we use hook_args. With the 
> variadic interface it is clear by looking at the call to 
> run_commit_hook() what the first argument is and that is always the same.
> 
>>       if (have_option_m && !fixup_message) {
>>           strbuf_addbuf(&sb, &message);
>> -        hook_arg1 = "message";
>> +        strvec_push(&hook_args, "message");
>>       } else if (logfile && !strcmp(logfile, "-")) {
>>           if (isatty(0))
>>               fprintf(stderr, _("(reading log message from standard 
>> input)\n"));
>>           if (strbuf_read(&sb, 0, 0) < 0)
>>               die_errno(_("could not read log from standard input"));
>> -        hook_arg1 = "message";
>> +        strvec_push(&hook_args, "message");
>>       } else if (logfile) {
>>           if (strbuf_read_file(&sb, logfile, 0) < 0)
>>               die_errno(_("could not read log file '%s'"),
>>                     logfile);
>> -        hook_arg1 = "message";
>> +        strvec_push(&hook_args, "message");
>>       } else if (use_message) {
>>           char *buffer;
>>           buffer = strstr(use_message_buffer, "\n\n");
>>           if (buffer)
>>               strbuf_addstr(&sb, skip_blank_lines(buffer + 2));
>> -        hook_arg1 = "commit";
>> -        hook_arg2 = use_message;
>> +        strvec_pushl(&hook_args, "commit", use_message, NULL);
>>       } else if (fixup_message) {
>>           struct pretty_print_context ctx = {0};
>>           struct commit *commit;
>> @@ -754,7 +755,7 @@ static int prepare_to_commit(const char 
>> *index_file, const char *prefix,
>>                         &sb, &ctx);
>>           if (have_option_m)
>>               strbuf_addbuf(&sb, &message);
>> -        hook_arg1 = "message";
>> +        strvec_push(&hook_args, "message");
>>       } else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
>>           size_t merge_msg_start;
>> @@ -765,9 +766,9 @@ static int prepare_to_commit(const char 
>> *index_file, const char *prefix,
>>           if (!stat(git_path_squash_msg(the_repository), &statbuf)) {
>>               if (strbuf_read_file(&sb, 
>> git_path_squash_msg(the_repository), 0) < 0)
>>                   die_errno(_("could not read SQUASH_MSG"));
>> -            hook_arg1 = "squash";
>> +            strvec_push(&hook_args, "squash");
>>           } else
>> -            hook_arg1 = "merge";
>> +            strvec_push(&hook_args, "merge");
>>           merge_msg_start = sb.len;
>>           if (strbuf_read_file(&sb, 
>> git_path_merge_msg(the_repository), 0) < 0)
>> @@ -781,11 +782,11 @@ static int prepare_to_commit(const char 
>> *index_file, const char *prefix,
>>       } else if (!stat(git_path_squash_msg(the_repository), &statbuf)) {
>>           if (strbuf_read_file(&sb, 
>> git_path_squash_msg(the_repository), 0) < 0)
>>               die_errno(_("could not read SQUASH_MSG"));
>> -        hook_arg1 = "squash";
>> +        strvec_push(&hook_args, "squash");
>>       } else if (template_file) {
>>           if (strbuf_read_file(&sb, template_file, 0) < 0)
>>               die_errno(_("could not read '%s'"), template_file);
>> -        hook_arg1 = "template";
>> +        strvec_push(&hook_args, "template");
>>           clean_message_contents = 0;
>>       }
>> @@ -794,11 +795,9 @@ static int prepare_to_commit(const char 
>> *index_file, const char *prefix,
>>        * just set the argument(s) to the prepare-commit-msg hook.
>>        */
>>       else if (whence == FROM_MERGE)
>> -        hook_arg1 = "merge";
>> -    else if (is_from_cherry_pick(whence) || whence == 
>> FROM_REBASE_PICK) {
>> -        hook_arg1 = "commit";
>> -        hook_arg2 = "CHERRY_PICK_HEAD";
>> -    }
>> +        strvec_push(&hook_args, "merge");
>> +    else if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK)
>> +        strvec_pushl(&hook_args, "commit", "CHERRY_PICK_HEAD", NULL);
>>       if (squash_message) {
>>           /*
>> @@ -806,8 +805,8 @@ static int prepare_to_commit(const char 
>> *index_file, const char *prefix,
>>            * then we're possibly hijacking other commit log options.
>>            * Reset the hook args to tell the real story.
>>            */
>> -        hook_arg1 = "message";
>> -        hook_arg2 = "";
>> +        strvec_clear(&hook_args);
>> +        strvec_pushl(&hook_args, git_path_commit_editmsg(), 
>> "message", NULL);
> 
> It's a shame we have to clear the strvec and remember to re-add 
> git_path_commit_editmsg() here.
> 
>>       }
>>       s->fp = fopen_for_writing(git_path_commit_editmsg());
>> @@ -1001,8 +1000,7 @@ static int prepare_to_commit(const char 
>> *index_file, const char *prefix,
>>           return 0;
>>       }
>> -    if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
>> -                git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))
>> +    if (run_commit_hook(use_editor, index_file, "prepare-commit-msg", 
>> &hook_args))
>>           return 0;
>>       if (use_editor) {
>> @@ -1017,8 +1015,10 @@ static int prepare_to_commit(const char 
>> *index_file, const char *prefix,
>>           strvec_clear(&env);
>>       }
>> +    strvec_clear(&hook_args);
>> +    strvec_push(&hook_args, git_path_commit_editmsg());
>>       if (!no_verify &&
>> -        run_commit_hook(use_editor, index_file, "commit-msg", 
>> git_path_commit_editmsg(), NULL)) {
>> +        run_commit_hook(use_editor, index_file, "commit-msg", 
>> &hook_args)) {
>>           return 0;
>>       }
 >[...]
> 
> I don't have a strong opinion about these changes (though I'm not 
> particularly enthusiastic). Having to push the arguments in order is not 
> particularly convenient and the use of strvec_pushl() means we are 
> replacing a small number of variadic calls to run_commit_hook() with a 
> larger number of calls to a different variadic interface.

On reflection I think it is the conversion in builtin/commit.c rather 
than the change in the API that makes me uncomfortable. If it kept 
`hook_arg1` and `hook_arg2` and just did

strvec_push(&hook_args, git_path_commit_editmsg())\
strvec_push(&hook_args, hook_arg1);
if (hook_arg2)
	strvec_push(&hook_args, hook_arg2);
run_commit_hook(..., &hook_args);

It would keep the fixed first argument near the call to 
run_commit_hook() and avoid the problem of having to clear hook_args in 
the hunk at line 806.

Thank you for adding the last couple of patches that show an example 
conversion, it is really helpful to see how the API would be used.

Best Wishes

Phillip

> Best Wishes
> 
> Phillip
> 
>>       } else {
>> -        arg1 = "message";
>> +        strvec_push(&args, "message");
>>       }
>> -    if (run_commit_hook(0, r->index_file, "prepare-commit-msg", name,
>> -                arg1, arg2, NULL))
>> +    if (run_commit_hook(0, r->index_file, "prepare-commit-msg", &args))
>>           ret = error(_("'prepare-commit-msg' hook failed"));
>> +    strvec_clear(&args);
>>       return ret;
>>   }
>>
diff mbox series

Patch

diff --git a/builtin/commit.c b/builtin/commit.c
index a19c6478eb..f029d4f5ac 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -691,8 +691,7 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 	struct strbuf committer_ident = STRBUF_INIT;
 	int committable;
 	struct strbuf sb = STRBUF_INIT;
-	const char *hook_arg1 = NULL;
-	const char *hook_arg2 = NULL;
+	struct strvec hook_args = STRVEC_INIT;
 	int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
 	int old_display_comment_prefix;
 	int merge_contains_scissors = 0;
@@ -700,7 +699,8 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
 
-	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
+	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit",
+					  &hook_args))
 		return 0;
 
 	if (squash_message) {
@@ -722,27 +722,28 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 		}
 	}
 
+	strvec_push(&hook_args, git_path_commit_editmsg());
+
 	if (have_option_m && !fixup_message) {
 		strbuf_addbuf(&sb, &message);
-		hook_arg1 = "message";
+		strvec_push(&hook_args, "message");
 	} else if (logfile && !strcmp(logfile, "-")) {
 		if (isatty(0))
 			fprintf(stderr, _("(reading log message from standard input)\n"));
 		if (strbuf_read(&sb, 0, 0) < 0)
 			die_errno(_("could not read log from standard input"));
-		hook_arg1 = "message";
+		strvec_push(&hook_args, "message");
 	} else if (logfile) {
 		if (strbuf_read_file(&sb, logfile, 0) < 0)
 			die_errno(_("could not read log file '%s'"),
 				  logfile);
-		hook_arg1 = "message";
+		strvec_push(&hook_args, "message");
 	} else if (use_message) {
 		char *buffer;
 		buffer = strstr(use_message_buffer, "\n\n");
 		if (buffer)
 			strbuf_addstr(&sb, skip_blank_lines(buffer + 2));
-		hook_arg1 = "commit";
-		hook_arg2 = use_message;
+		strvec_pushl(&hook_args, "commit", use_message, NULL);
 	} else if (fixup_message) {
 		struct pretty_print_context ctx = {0};
 		struct commit *commit;
@@ -754,7 +755,7 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 				      &sb, &ctx);
 		if (have_option_m)
 			strbuf_addbuf(&sb, &message);
-		hook_arg1 = "message";
+		strvec_push(&hook_args, "message");
 	} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
 		size_t merge_msg_start;
 
@@ -765,9 +766,9 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 		if (!stat(git_path_squash_msg(the_repository), &statbuf)) {
 			if (strbuf_read_file(&sb, git_path_squash_msg(the_repository), 0) < 0)
 				die_errno(_("could not read SQUASH_MSG"));
-			hook_arg1 = "squash";
+			strvec_push(&hook_args, "squash");
 		} else
-			hook_arg1 = "merge";
+			strvec_push(&hook_args, "merge");
 
 		merge_msg_start = sb.len;
 		if (strbuf_read_file(&sb, git_path_merge_msg(the_repository), 0) < 0)
@@ -781,11 +782,11 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 	} else if (!stat(git_path_squash_msg(the_repository), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path_squash_msg(the_repository), 0) < 0)
 			die_errno(_("could not read SQUASH_MSG"));
-		hook_arg1 = "squash";
+		strvec_push(&hook_args, "squash");
 	} else if (template_file) {
 		if (strbuf_read_file(&sb, template_file, 0) < 0)
 			die_errno(_("could not read '%s'"), template_file);
-		hook_arg1 = "template";
+		strvec_push(&hook_args, "template");
 		clean_message_contents = 0;
 	}
 
@@ -794,11 +795,9 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 	 * just set the argument(s) to the prepare-commit-msg hook.
 	 */
 	else if (whence == FROM_MERGE)
-		hook_arg1 = "merge";
-	else if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) {
-		hook_arg1 = "commit";
-		hook_arg2 = "CHERRY_PICK_HEAD";
-	}
+		strvec_push(&hook_args, "merge");
+	else if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK)
+		strvec_pushl(&hook_args, "commit", "CHERRY_PICK_HEAD", NULL);
 
 	if (squash_message) {
 		/*
@@ -806,8 +805,8 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 		 * then we're possibly hijacking other commit log options.
 		 * Reset the hook args to tell the real story.
 		 */
-		hook_arg1 = "message";
-		hook_arg2 = "";
+		strvec_clear(&hook_args);
+		strvec_pushl(&hook_args, git_path_commit_editmsg(), "message", NULL);
 	}
 
 	s->fp = fopen_for_writing(git_path_commit_editmsg());
@@ -1001,8 +1000,7 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
-			    git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))
+	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg", &hook_args))
 		return 0;
 
 	if (use_editor) {
@@ -1017,8 +1015,10 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 		strvec_clear(&env);
 	}
 
+	strvec_clear(&hook_args);
+	strvec_push(&hook_args, git_path_commit_editmsg());
 	if (!no_verify &&
-	    run_commit_hook(use_editor, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) {
+	    run_commit_hook(use_editor, index_file, "commit-msg", &hook_args)) {
 		return 0;
 	}
 
diff --git a/builtin/merge.c b/builtin/merge.c
index c1a9d0083d..863c9039a3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -821,10 +821,14 @@  static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
+	struct strvec hook_args = STRVEC_INIT;
+	struct strbuf hook_name = STRBUF_INIT;
 	const char *index_file = get_index_file();
 
-	if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+	if (!no_verify && run_commit_hook(0 < option_edit, index_file,
+					  "pre-merge-commit", &hook_args))
 		abort_commit(remoteheads, NULL);
+
 	/*
 	 * Re-read the index as pre-merge-commit hook could have updated it,
 	 * and write it out as a tree.  We must do this before we invoke
@@ -832,6 +836,7 @@  static void prepare_to_commit(struct commit_list *remoteheads)
 	 */
 	if (hook_exists("pre-merge-commit"))
 		discard_cache();
+
 	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
 	if (squash)
@@ -851,17 +856,22 @@  static void prepare_to_commit(struct commit_list *remoteheads)
 		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
 	write_merge_heads(remoteheads);
 	write_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);
+
+	strvec_clear(&hook_args);
+	strvec_pushl(&hook_args, git_path_merge_msg(the_repository), "merge", NULL);
 	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
-			    git_path_merge_msg(the_repository), "merge", NULL))
+			    &hook_args))
 		abort_commit(remoteheads, NULL);
+
 	if (0 < option_edit) {
 		if (launch_editor(git_path_merge_msg(the_repository), NULL, NULL))
 			abort_commit(remoteheads, NULL);
 	}
 
+	strvec_clear(&hook_args);
+	strvec_push(&hook_args, git_path_merge_msg(the_repository));
 	if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
-					  "commit-msg",
-					  git_path_merge_msg(the_repository), NULL))
+					  "commit-msg", &hook_args))
 		abort_commit(remoteheads, NULL);
 
 	read_merge_msg(&msg);
@@ -871,6 +881,8 @@  static void prepare_to_commit(struct commit_list *remoteheads)
 	strbuf_release(&merge_msg);
 	strbuf_addbuf(&merge_msg, &msg);
 	strbuf_release(&msg);
+	strbuf_release(&hook_name);
+	strvec_clear(&hook_args);
 }
 
 static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
diff --git a/commit.c b/commit.c
index c7a243e848..726407152c 100644
--- a/commit.c
+++ b/commit.c
@@ -1629,12 +1629,9 @@  size_t ignore_non_trailer(const char *buf, size_t len)
 }
 
 int run_commit_hook(int editor_is_used, const char *index_file,
-		    const char *name, ...)
+		    const char *name, struct strvec *args)
 {
 	struct strvec hook_env = STRVEC_INIT;
-	va_list args;
-	const char *arg;
-	struct strvec hook_args = STRVEC_INIT;
 	struct strbuf hook_name = STRBUF_INIT;
 	int ret;
 
@@ -1648,14 +1645,8 @@  int run_commit_hook(int editor_is_used, const char *index_file,
 	if (!editor_is_used)
 		strvec_push(&hook_env, "GIT_EDITOR=:");
 
-	va_start(args, name);
-	while ((arg = va_arg(args, const char *)))
-		strvec_push(&hook_args, arg);
-	va_end(args);
-
-	ret = run_hooks(hook_env.v, &hook_name, &hook_args);
+	ret = run_hooks(hook_env.v, &hook_name, args);
 	strvec_clear(&hook_env);
-	strvec_clear(&hook_args);
 	strbuf_release(&hook_name);
 
 	return ret;
diff --git a/commit.h b/commit.h
index e901538909..978da3c3e0 100644
--- a/commit.h
+++ b/commit.h
@@ -9,6 +9,7 @@ 
 #include "string-list.h"
 #include "pretty.h"
 #include "commit-slab.h"
+#include "strvec.h"
 
 #define COMMIT_NOT_FROM_GRAPH 0xFFFFFFFF
 #define GENERATION_NUMBER_INFINITY 0xFFFFFFFF
@@ -353,7 +354,7 @@  void verify_merge_signature(struct commit *commit, int verbose,
 int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);
 int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);
 
-LAST_ARG_MUST_BE_NULL
-int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
+int run_commit_hook(int editor_is_used, const char *index_file,
+		    const char *name, struct strvec *args);
 
 #endif /* COMMIT_H */
diff --git a/sequencer.c b/sequencer.c
index cc3f8fa88e..5dd4b134d6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1124,22 +1124,23 @@  static int run_prepare_commit_msg_hook(struct repository *r,
 				       const char *commit)
 {
 	int ret = 0;
-	const char *name, *arg1 = NULL, *arg2 = NULL;
+	struct strvec args = STRVEC_INIT;
+	const char *name = git_path_commit_editmsg();
 
-	name = git_path_commit_editmsg();
+	strvec_push(&args, name);
 	if (write_message(msg->buf, msg->len, name, 0))
 		return -1;
 
 	if (commit) {
-		arg1 = "commit";
-		arg2 = commit;
+		strvec_push(&args, "commit");
+		strvec_push(&args, commit);
 	} else {
-		arg1 = "message";
+		strvec_push(&args, "message");
 	}
-	if (run_commit_hook(0, r->index_file, "prepare-commit-msg", name,
-			    arg1, arg2, NULL))
+	if (run_commit_hook(0, r->index_file, "prepare-commit-msg", &args))
 		ret = error(_("'prepare-commit-msg' hook failed"));
 
+	strvec_clear(&args);
 	return ret;
 }