git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Sep 10, 2020, 14:16 UTC
Message-ID
<6ed77ee3-4d75-9ef9-0621-13a056e8ca14@gmail.com>
In-Reply-To
<20200909004939.1942347-10-emilyshaffer@google.com>
Hi Emily
On 09/09/2020 01:49, Emily Shaffer wrote:
Show 45 quoted lines
> 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.

Show 86 quoted lines
>   	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.

Show 35 quoted lines
>   	}
>   
>   	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.
Show 123 quoted lines
>   	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.

Show 8 quoted lines
>   	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
Show 14 quoted lines
>   	} 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;
>   }
>   
>
Previous: Emily ShafferNext: Phillip Wood
Message 60 of 69 in “propose config-based hooks”
  1. 0/4 propose config-based hooksEmily Shaffer, May 21, 2020
  2. 1/4 doc: propose hooks managed by the configEmily Shaffer, May 21, 2020
  3. Phillip WoodMay 22, 2020
  4. Emily ShafferJun 9, 2020
  5. 2/4 hook: scaffolding for git-hook subcommandEmily Shaffer, May 21, 2020
  6. 3/4 hook: add list commandEmily Shaffer, May 21, 2020
  7. Phillip WoodMay 22, 2020
  8. Emily ShafferJun 9, 2020
  9. Phillip WoodAug 17, 2020
  10. Johannes SchindelinMay 24, 2020
  11. Emily ShafferMay 27, 2020
  12. 4/4 hook: add --porcelain to list commandEmily Shaffer, May 21, 2020
  13. Johannes SchindelinMay 24, 2020
  14. Johannes SchindelinMay 25, 2020
  15. 0/6 propose config-based hooksEmily Shaffer, Jul 28, 2020
  16. 1/6 doc: propose hooks managed by the configEmily Shaffer, Jul 28, 2020
  17. 2/6 hook: scaffolding for git-hook subcommandEmily Shaffer, Jul 28, 2020
  18. 3/6 hook: add list commandEmily Shaffer, Jul 28, 2020
  19. 5/6 parse-options: parse into argv_arrayEmily Shaffer, Jul 28, 2020
  20. Junio C HamanoJul 29, 2020
  21. Junio C HamanoJul 30, 2020
  22. 4/6 hook: add --porcelain to list commandEmily Shaffer, Jul 28, 2020
  23. 6/6 hook: add 'run' subcommandEmily Shaffer, Jul 28, 2020
  24. 0/9 propose config-based hooksEmily Shaffer, Sep 9, 2020
  25. 1/9 doc: propose hooks managed by the configEmily Shaffer, Sep 9, 2020
  26. Jonathan TanSep 23, 2020
  27. Emily ShafferSep 24, 2020
  28. Ævar Arnfjörð BjarmasonOct 7, 2020
  29. Emily ShafferOct 22, 2020
  30. Ævar Arnfjörð BjarmasonOct 23, 2020
  31. Emily ShafferOct 29, 2020
  32. Ævar Arnfjörð BjarmasonOct 29, 2020
  33. 2/9 hook: scaffolding for git-hook subcommandEmily Shaffer, Sep 9, 2020
  34. Jonathan NiederOct 5, 2020
  35. Emily ShafferOct 6, 2020
  36. 3/9 hook: add list commandEmily Shaffer, Sep 9, 2020
  37. Phillip WoodSep 11, 2020
  38. Emily ShafferSep 11, 2020
  39. Jonathan TanSep 23, 2020
  40. Emily ShafferOct 6, 2020
  41. Martin ÅgrenSep 27, 2020
  42. Emily ShafferOct 6, 2020
  43. Jonathan NiederOct 5, 2020
  44. 4/9 hook: add --porcelain to list commandEmily Shaffer, Sep 9, 2020
  45. Josh SteadmonSep 28, 2020
  46. 5/9 parse-options: parse into strvecEmily Shaffer, Sep 9, 2020
  47. Jonathan NiederOct 5, 2020
  48. Junio C HamanoOct 6, 2020
  49. 6/9 hook: add 'run' subcommandEmily Shaffer, Sep 9, 2020
  50. Phillip WoodSep 11, 2020
  51. Josh SteadmonSep 28, 2020
  52. Jonathan NiederOct 5, 2020
  53. Emily ShafferOct 6, 2020
  54. 7/9 hook: replace run-command.h:find_hookEmily Shaffer, Sep 9, 2020
  55. Junio C HamanoSep 9, 2020
  56. Emily ShafferSep 10, 2020
  57. Jonathan TanSep 23, 2020
  58. Jonathan NiederOct 5, 2020
  59. 9/9 run_commit_hook: take strvec instead of varargsEmily Shaffer, Sep 9, 2020
  60. Phillip WoodSep 10, 2020
  61. Phillip WoodSep 11, 2020
  62. 8/9 commit: use config-based hooksEmily Shaffer, Sep 9, 2020
  63. Phillip WoodSep 10, 2020
  64. Junio C HamanoSep 10, 2020
  65. Jonathan TanSep 23, 2020
  66. Emily ShafferOct 5, 2020
  67. Jonathan NiederOct 5, 2020
  68. Emily ShafferOct 6, 2020
  69. Junio C HamanoSep 9, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.