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

Re: [PATCH v1 03/19] rebase -i: reword executes pre-commit hook on interim commit

From
Fabian Ruch <bafain@gmail.com>
Date
Aug 4, 2014, 18:51 UTC
Message-ID
<53DFD63E.8040806@gmail.com>
In-Reply-To
<20140801234708.GA20040@peff.net>
Hi,
Jeff King writes:
Show 50 quoted lines
> On Tue, Jul 29, 2014 at 01:18:03AM +0200, Fabian Ruch wrote:
> 
>> Specify the git-commit option `--no-verify` to disable the pre-commit
>> hook when editing the log message. Because `--no-verify` also skips
>> the commit-msg hook, execute the hook from within
>> git-rebase--interactive after the commit is created. Fortunately, the
>> commit message is still available in `$GIT_DIR/COMMIT_EDITMSG` after
>> git-commit terminates. Caveat: In case the commit-msg hook finds the
>> new log message ill-formatted, the user is only notified of the
>> failed commit-msg hook but the log message is used for the commit
>> anyway. git-commit ought to offer more fine-grained control over
>> which hooks are executed.
> 
> Thanks for a nice explanation of the tradeoff. Have you looked at adding
> an option to git-commit? We already have --no-post-rewrite. I think you
> would just need:
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 5ed6036..f7af220 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -102,6 +102,7 @@ static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
>  static int no_post_rewrite, allow_empty_message;
>  static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
>  static char *sign_commit;
> +static int no_pre_commit;
>  
>  /*
>   * The default commit message cleanup mode will remove the lines
> @@ -661,7 +662,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 && !no_pre_commit &&
> +	    run_commit_hook(use_editor, index_file, "pre-commit", NULL))
>  		return 0;
>  
>  	if (squash_message) {
> @@ -1604,6 +1606,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  			 N_("terminate entries with NUL")),
>  		OPT_BOOL(0, "amend", &amend, N_("amend previous commit")),
>  		OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
> +		OPT_BOOL(0, "no-pre-commit", &no_pre_commit, N_("bypass pre-commit hook")),
>  		{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, N_("mode"), N_("show untracked files, optional modes: all, normal, no. (Default: all)"), PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
>  		/* end commit contents options */
>  
> 
> though I would also not be opposed to some more uniform hook selection
> mechanism (e.g., "--no-verify=pre-commit" or something).

While the --no-verify= mechanism doesn't add a new option to the git-commit interface but lets one refine the --no-verify option, users might find it weird to have an argument to name disabled hooks but then not be able to disable all hooks that way.

To have that uniform hook selection without duplicating code, we might want to have something like --bypass-hook= right away. This would cover --no-post-rewrite as well and add support for disabling prepare-commit-msg and post-commit, whose execution cannot be controlled via the git-commit interface at the moment.

Since the hook selection wouldn't have to change, the options parsing code seems to be simpler (--bypass-hook= would have to support several occurrences with different arguments which could be implemented as an OPT_CALLBACK?) and git-commit decided to have --no- options for hook selection so far, I would lean towards your patch from above.

I know you didn't say anything otherwise, I just wanted to expand a little. You find an amended version of your patch below that documents --no-verify as a synonym for --no-pre-commit and --no-commit-msg, and adds tests.

Show 76 quoted lines
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 5e2221c..813aa78 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -98,12 +98,27 @@ static char *edit_message, *use_message;
>  static char *fixup_message, *squash_message;
>  static int all, also, interactive, patch_interactive, only, amend, signoff;
>  static int edit_flag = -1; /* unspecified */
> -static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
> +static int quiet, verbose, allow_empty, dry_run, renew_authorship;
>  static int no_post_rewrite, allow_empty_message;
>  static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
>  static char *sign_commit;
>  
>  /*
> + * The verify variable is interpreted as a bitmap of enabled commit
> + * verification hooks according to the legend below.
> + *
> + * By default, the pre-commit and commit-msg hooks are enabled. This
> + * is represented by both the PRE_COMMIT and COMMIT_MSG bits being
> + * set.
> + *
> + * The bitmap is changed through the command line options
> + * --no-verify, --no-pre-commit and --no-commit-msg.
> + */
> +#define PRE_COMMIT (1<<0)
> +#define COMMIT_MSG (1<<1)
> +static int verify = PRE_COMMIT | COMMIT_MSG;
> +
> +/*
>   * The default commit message cleanup mode will remove the lines
>   * beginning with # (shell comments) and leading and trailing
>   * whitespaces (empty lines or containing only whitespaces)
> @@ -667,7 +682,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 (verify & PRE_COMMIT &&
> +	    run_commit_hook(use_editor, index_file, "pre-commit", NULL))
>  		return 0;
>  
>  	if (squash_message) {
> @@ -968,7 +984,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		}
>  	}
>  
> -	if (!no_verify &&
> +	if (verify & COMMIT_MSG &&
>  	    run_commit_hook(use_editor, index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
>  		return 0;
>  	}
> @@ -1597,7 +1613,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL(0, "interactive", &interactive, N_("interactively add files")),
>  		OPT_BOOL('p', "patch", &patch_interactive, N_("interactively add changes")),
>  		OPT_BOOL('o', "only", &only, N_("commit only specified files")),
> -		OPT_BOOL('n', "no-verify", &no_verify, N_("bypass pre-commit hook")),
> +		OPT_NEGBIT('n', "no-verify", &verify,
> +			   N_("synonym for --no-pre-commit --no-commit-msg"),
> +			   PRE_COMMIT | COMMIT_MSG),
>  		OPT_BOOL(0, "dry-run", &dry_run, N_("show what would be committed")),
>  		OPT_SET_INT(0, "short", &status_format, N_("show status concisely"),
>  			    STATUS_FORMAT_SHORT),
> @@ -1610,6 +1628,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL('z', "null", &s.null_termination,
>  			 N_("terminate entries with NUL")),
>  		OPT_BOOL(0, "amend", &amend, N_("amend previous commit")),
> +		OPT_NEGBIT(0, "no-pre-commit", &verify,
> +			   N_("bypass pre-commit hook"),
> +			   PRE_COMMIT),
> +		OPT_NEGBIT(0, "no-commit-msg", &verify,
> +			   N_("bypass commit-msg hook"),
> +			   COMMIT_MSG),
>  		OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
>  		{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, N_("mode"), N_("show untracked files, optional modes: all, normal, no. (Default: all)"), PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
>  		/* end commit contents options */

Since all of the hook options are motivated by internal usage from git-rebase, perhaps they should be configured as PARSE_OPT_HIDDEN. Any thoughts on this?

    Fabian
Previous: Jeff KingNext: Jeff King
Message 53 of 148 in “rebase -i: Implement `reword` and `squash` in terms of `do_pick`”
  1. 0/7 rebase -i: Implement `reword` and `squash` in terms of `do_pick`Fabian Ruch, Jun 19, 2014
  2. 00/19 Enable options --signoff, --reset-author for pick, rewordFabian Ruch, Jul 2, 2014
  3. 01/19 rebase -i: Failed reword prints redundant error messageFabian Ruch, Jul 2, 2014
  4. Junio C HamanoJul 8, 2014
  5. Andrew WongJul 10, 2014
  6. Fabian RuchJul 10, 2014
  7. Andrew WongJul 10, 2014
  8. 02/19 rebase -i: reword complains about empty commit despite --keep-emptyFabian Ruch, Jul 2, 2014
  9. Junio C HamanoJul 8, 2014
  10. Fabian RuchJul 9, 2014
  11. 03/19 rebase -i: reword executes pre-commit hook on interim commitFabian Ruch, Jul 2, 2014
  12. Junio C HamanoJul 8, 2014
  13. Fabian RuchJul 13, 2014
  14. 04/19 rebase -i: Teach do_pick the option --editFabian Ruch, Jul 2, 2014
  15. 05/19 rebase -i: Implement reword in terms of do_pickFabian Ruch, Jul 2, 2014
  16. Matthieu MoyAug 4, 2014
  17. Fabian RuchAug 4, 2014
  18. 06/19 rebase -i: Stop on root commits with empty log messagesFabian Ruch, Jul 2, 2014
  19. Junio C HamanoJul 8, 2014
  20. Fabian RuchJul 10, 2014
  21. Junio C HamanoJul 10, 2014
  22. Junio C HamanoJul 10, 2014
  23. 07/19 rebase -i: The replay of root commits is not shown with --verboseFabian Ruch, Jul 2, 2014
  24. Junio C HamanoJul 8, 2014
  25. Fabian RuchJul 11, 2014
  26. Chris WebbJul 15, 2014
  27. 08/19 rebase -i: Root commits are replayed with an unnecessary optionFabian Ruch, Jul 2, 2014
  28. Junio C HamanoJul 8, 2014
  29. Fabian RuchJul 18, 2014
  30. Junio C HamanoJul 18, 2014
  31. Fabian RuchJul 19, 2014
  32. 09/19 rebase -i: Commit only once when rewriting picksFabian Ruch, Jul 2, 2014
  33. 10/19 rebase -i: Do not die in do_pickFabian Ruch, Jul 2, 2014
  34. 11/19 rebase -i: Teach do_pick the option --amendFabian Ruch, Jul 2, 2014
  35. 12/19 rebase -i: Teach do_pick the option --fileFabian Ruch, Jul 2, 2014
  36. 13/19 rebase -i: Prepare for squash in terms of do_pick --amendFabian Ruch, Jul 2, 2014
  37. 14/19 rebase -i: Implement squash in terms of do_pickFabian Ruch, Jul 2, 2014
  38. 15/19 rebase -i: Explicitly distinguish replay commands and exec tasksFabian Ruch, Jul 2, 2014
  39. Junio C HamanoJul 10, 2014
  40. 16/19 rebase -i: Parse to-do list command line optionsFabian Ruch, Jul 2, 2014
  41. 17/19 rebase -i: Teach do_pick the option --reset-authorFabian Ruch, Jul 2, 2014
  42. 18/19 rebase -i: Teach do_pick the option --signoffFabian Ruch, Jul 2, 2014
  43. 19/19 rebase -i: Enable options --signoff, --reset-author for pick, rewordFabian Ruch, Jul 2, 2014
  44. Michael HaggertyJul 3, 2014
  45. Junio C HamanoJul 8, 2014
  46. Fabian RuchJul 9, 2014
  47. Thomas RastJul 18, 2014
  48. 00/19 Enable options --signoff, --reset-author for pick, rewordFabian Ruch, Jul 28, 2014
  49. 01/19 rebase -i: failed reword prints redundant error messageFabian Ruch, Jul 28, 2014
  50. 02/19 rebase -i: allow rewording an empty commit without complaintsFabian Ruch, Jul 28, 2014
  51. 03/19 rebase -i: reword executes pre-commit hook on interim commitFabian Ruch, Jul 28, 2014
  52. Jeff KingAug 1, 2014
  53. Fabian RuchAug 4, 2014
  54. Jeff KingAug 6, 2014
  55. 04/19 rebase -i: teach do_pick the option --editFabian Ruch, Jul 28, 2014
  56. 05/19 rebase -i: implement reword in terms of do_pickFabian Ruch, Jul 28, 2014
  57. 06/19 rebase -i: allow replaying commits with empty log messagesFabian Ruch, Jul 28, 2014
  58. 07/19 rebase -i: log the replay of root commitsFabian Ruch, Jul 28, 2014
  59. Jeff KingAug 2, 2014
  60. Fabian RuchAug 4, 2014
  61. Jeff KingAug 6, 2014
  62. 08/19 rebase -i: root commits are replayed with an unnecessary optionFabian Ruch, Jul 28, 2014
  63. Jeff KingAug 2, 2014
  64. Fabian RuchAug 4, 2014
  65. 09/19 rebase -i: commit only once when rewriting picksFabian Ruch, Jul 28, 2014
  66. Jeff KingAug 2, 2014
  67. Fabian RuchAug 7, 2014
  68. 10/19 rebase -i: do not die in do_pickFabian Ruch, Jul 28, 2014
  69. 11/19 rebase -i: teach do_pick the option --amendFabian Ruch, Jul 28, 2014
  70. 12/19 rebase -i: teach do_pick the option --fileFabian Ruch, Jul 28, 2014
  71. 13/19 rebase -i: prepare for squash in terms of do_pick --amendFabian Ruch, Jul 28, 2014
  72. 14/19 rebase -i: implement squash in terms of do_pickFabian Ruch, Jul 28, 2014
  73. 15/19 rebase -i: explicitly distinguish replay commands and exec tasksFabian Ruch, Jul 28, 2014
  74. 16/19 rebase -i: parse to-do list command line optionsFabian Ruch, Jul 28, 2014
  75. 17/19 rebase -i: teach do_pick the option --reset-authorFabian Ruch, Jul 28, 2014
  76. 18/19 rebase -i: teach do_pick the option --signoffFabian Ruch, Jul 28, 2014
  77. 19/19 rebase -i: enable options --signoff, --reset-author for pick, rewordFabian Ruch, Jul 28, 2014
  78. Jeff KingAug 2, 2014
  79. Fabian RuchAug 4, 2014
  80. 00/23 Enable options --signoff, --reset-author for pick, rewordFabian Ruch, Aug 6, 2014
  81. 01/23 rebase -i: allow replaying commits with empty log messagesFabian Ruch, Aug 6, 2014
  82. 02/23 rebase -i: allow squashing empty commits without complaintsFabian Ruch, Aug 6, 2014
  83. Peter KreftingAug 7, 2014
  84. Eric SunshineAug 7, 2014
  85. Fabian RuchAug 11, 2014
  86. Phil HordAug 13, 2014
  87. 03/23 rebase -i: allow rewording empty commits without complaintsFabian Ruch, Aug 6, 2014
  88. 04/23 rebase -i: hide interactive command messages in verbose modeFabian Ruch, Aug 6, 2014
  89. Thomas RastAug 8, 2014
  90. Fabian RuchAug 11, 2014
  91. Thomas RastAug 11, 2014
  92. 05/23 rebase -i: failed reword prints redundant error messageFabian Ruch, Aug 6, 2014
  93. 06/23 commit: allow disabling pre-commit and commit-msg separatelyFabian Ruch, Aug 6, 2014
  94. 07/23 rebase -i: squash skips commit-msg hookFabian Ruch, Aug 6, 2014
  95. 08/23 rebase -i: reword executes pre-commit hook on interim commitFabian Ruch, Aug 6, 2014
  96. Thomas RastAug 8, 2014
  97. Fabian RuchAug 11, 2014
  98. Thomas RastAug 11, 2014
  99. 09/23 rebase -i: teach do_pick the option --editFabian Ruch, Aug 6, 2014
  100. 10/23 rebase -i: implement reword in terms of do_pickFabian Ruch, Aug 6, 2014
  101. 11/23 rebase -i: log the replay of root commitsFabian Ruch, Aug 6, 2014
  102. 12/23 rebase -i: root commits are replayed with an unnecessary optionFabian Ruch, Aug 6, 2014
  103. 13/23 rebase -i: commit only once when rewriting picksFabian Ruch, Aug 6, 2014
  104. 14/23 rebase -i: do not die in do_pickFabian Ruch, Aug 6, 2014
  105. 15/23 rebase -i: teach do_pick the option --amendFabian Ruch, Aug 6, 2014
  106. 16/23 rebase -i: teach do_pick the option --fileFabian Ruch, Aug 6, 2014
  107. 17/23 rebase -i: prepare for squash in terms of do_pick --amendFabian Ruch, Aug 6, 2014
  108. 18/23 rebase -i: implement squash in terms of do_pickFabian Ruch, Aug 6, 2014
  109. 19/23 rebase -i: explicitly distinguish replay commands and exec tasksFabian Ruch, Aug 6, 2014
  110. 20/23 rebase -i: parse to-do list command line optionsFabian Ruch, Aug 6, 2014
  111. Thomas RastAug 8, 2014
  112. Fabian RuchAug 11, 2014
  113. 21/23 rebase -i: teach do_pick the option --reset-authorFabian Ruch, Aug 6, 2014
  114. 22/23 rebase -i: teach do_pick the option --signoffFabian Ruch, Aug 6, 2014
  115. 23/23 rebase -i: enable options --signoff, --reset-author for pick, rewordFabian Ruch, Aug 6, 2014
  116. Thomas RastAug 8, 2014
  117. Fabian RuchAug 12, 2014
  118. Michael HaggertyAug 13, 2014
  119. Fabian RuchAug 14, 2014
  120. Fabian RuchSep 21, 2014
  121. 00/27 Enable options --signoff, --reset-author for pick, reword, editFabian Ruch, Aug 18, 2014
  122. 01/27 rebase -i: allow replaying commits with empty log messagesFabian Ruch, Aug 18, 2014
  123. 02/27 rebase -i: allow squashing empty commits without complaintsFabian Ruch, Aug 18, 2014
  124. 03/27 rebase -i: allow rewording empty commits without complaintsFabian Ruch, Aug 18, 2014
  125. 04/27 fake_editor: leave standard output unchangedFabian Ruch, Aug 18, 2014
  126. 05/27 rebase -i: hide interactive command messages in verbose modeFabian Ruch, Aug 18, 2014
  127. 06/27 rebase -i: discard redundant message when rewording failsFabian Ruch, Aug 18, 2014
  128. 07/27 commit: allow disabling pre-commit and commit-msg separatelyFabian Ruch, Aug 18, 2014
  129. 08/27 rebase -i: verify squash messages using commit-msgFabian Ruch, Aug 18, 2014
  130. 09/27 rebase -i: do not verify reworded patches using pre-commitFabian Ruch, Aug 18, 2014
  131. 10/27 rebase -i: teach do_pick the option --editFabian Ruch, Aug 18, 2014
  132. 11/27 rebase -i: implement reword in terms of do_pickFabian Ruch, Aug 18, 2014
  133. 12/27 rebase -i: log the replay of root commitsFabian Ruch, Aug 18, 2014
  134. 13/27 rebase -i: do not use -C when --no-edit is sufficientFabian Ruch, Aug 18, 2014
  135. 14/27 rebase -i: commit only once when rewriting picksFabian Ruch, Aug 18, 2014
  136. 15/27 rebase -i: do not die in do_pickFabian Ruch, Aug 18, 2014
  137. 16/27 rebase -i: teach do_pick the option --amendFabian Ruch, Aug 18, 2014
  138. 17/27 rebase -i: teach do_pick the option --fileFabian Ruch, Aug 18, 2014
  139. 18/27 rebase -i: remove no-op do_with_author git commit --amendFabian Ruch, Aug 18, 2014
  140. 19/27 rebase -i: prepare for squash in terms of do_pick --amendFabian Ruch, Aug 18, 2014
  141. 20/27 rebase -i: implement squash in terms of do_pickFabian Ruch, Aug 18, 2014
  142. 21/27 rebase -i: explicitly distinguish replay commands and exec tasksFabian Ruch, Aug 18, 2014
  143. 22/27 rebase -i: parse to-do list command line optionsFabian Ruch, Aug 18, 2014
  144. 23/27 rebase -i: teach do_pick the option --reset-authorFabian Ruch, Aug 18, 2014
  145. 24/27 rebase -i: teach do_pick the option --signoffFabian Ruch, Aug 18, 2014
  146. 25/27 rebase -i: do not overwrite user author informationFabian Ruch, Aug 18, 2014
  147. 26/27 rebase -i: refuse to commit when resuming with updated headFabian Ruch, Aug 18, 2014
  148. 27/27 rebase -i: enable --signoff, --reset-author for pick, reword, editFabian Ruch, Aug 18, 2014

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.