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

[Jeff King]

On Mon, Aug 04, 2014 at 08:51:42PM +0200, Fabian Ruch wrote:

  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.

Right, I think that is only worth doing if we add it uniformly for all
hooks. I wonder if it should be a git option, not one to specific
commands. Like:

  git --disable-hook=pre-commit commit ...

and then the run_hook code can just respect the disabled list.

I think a name like --disable-hook or your --bypass-hook is
better than --no-verify here, too. Not all hooks are about verifying
(I also think disable is better than bypass for that reason, too).

 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.

You'd probably implement --disable-hook with OPT_STRING_LIST (or just do
it by hand if it's the git.c option parser). And then the
--no-post-rewrite arguments remain for historical compatibility, and can
be implemented as synonyms for --disable-hook=post-rewrite, etc.

I think people have also asked for the ability to override hooks, too,
though I do not remember the exact details.  Instead of --disable-hook,
we could have an option for setting specific hooks (and setting them to
nothing to disable would just be one possibility).

This is getting bigger in scope, though. I was trying not to derail you
too much from your GSoC project, but see if we could just fix this one
hacky corner easily.

 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?

I could go either way. Just because they are motivated by git-rebase
does not mean other callers might not find them useful (after all, git
commands are often meant to be scripted). As long as we promise to
support them in future versions as we do with normal options, I do not
think there is any problem in advertising them.

That being said, git commit -h is already getting pretty long. It
might be worth cutting some seldom-used options from that list just to
make it more palatable to normal users.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Fabian Ruch]

Hi,

Jeff King writes:
 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.

 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 (10)
 +#define COMMIT_MSG (11)
 +static int verify = PRE_COMMIT | COMMIT_MSG;
 +
 +/*
   * The default commit message cleanup mode will remove the lines
   * 

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

[Jeff King]

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).

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Fabian Ruch]

The to-do list command `reword` replays a commit like `pick` but lets
the user also edit the commit's log message. This happens in two
steps. Firstly, the named commit is cherry-picked. Secondly, the
commit created in the first step is amended using an unchanged index
to edit the log message. The pre-commit hook is meant to verify the
changes introduced by a commit (for instance, catching inserted
trailing white space). Since the committed tree is not changed
between the two steps and we do not verify rebased patches, do not
execute the pre-commit hook in the second step.

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.

Teach `test_commit` the `--no-verify` option and add test.

Signed-off-by: Fabian Ruch baf...@gmail.com
---
 git-rebase--interactive.sh| 17 +
 t/t3404-rebase-interactive.sh | 38 ++
 t/test-lib-functions.sh   |  6 +-
 3 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 689400e..b50770d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -504,10 +504,19 @@ do_next () {
 
mark_action_done
do_pick $sha1 $rest
-   git commit --allow-empty --amend --no-post-rewrite 
${gpg_sign_opt:+$gpg_sign_opt} || {
-   warn Could not amend commit after successfully picking 
$sha1... $rest
-   exit_with_patch $sha1 1
-   }
+   # TODO: Work around the fact that git-commit lets us
+   # disable either both the pre-commit and the commit-msg
+   # hook or none. Disable the pre-commit hook because the
+   # tree is left unchanged but run the commit-msg hook
+   # from here because the log message is altered.
+   git commit --allow-empty --amend --no-post-rewrite -n 
${gpg_sign_opt:+$gpg_sign_opt} 
+   if test -x $GIT_DIR/hooks/commit-msg
+   then
+   $GIT_DIR/hooks/commit-msg 
$GIT_DIR/COMMIT_EDITMSG
+   fi || {
+   warn Could not amend commit after successfully 
picking $sha1... $rest
+   exit_with_patch $sha1 1
+   }
record_in_rewritten $sha1
;;
edit|e)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9931143..2da4b9c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -577,6 +577,44 @@ test_expect_success 'rebase a commit violating pre-commit' 
'
 
 '
 
+test_expect_success 'reword a commit violating pre-commit' '
+   test_when_finished rm -r .git/hooks 
+   mkdir -p .git/hooks 
+   PRE_COMMIT=.git/hooks/pre-commit 
+   cat $PRE_COMMIT EOF
+#!/bin/sh
+echo running pre-commit: exit 1
+exit 1
+EOF
+   chmod +x $PRE_COMMIT 
+   test_must_fail test_commit pre-commit-violated 
+   test_commit --no-verify pre-commit-violated 
+   test_when_finished reset_rebase 
+   set_fake_editor 
+   FAKE_LINES=pick 1 git rebase -i HEAD^ 
+   FAKE_LINES=pick 1 git rebase -i --no-ff HEAD^ 
+   FAKE_LINES=reword 1 git rebase -i HEAD^
+'
+
+test_expect_success 'reword a commit violating commit-msg' '
+   test_when_finished rm -r .git/hooks 
+   mkdir -p .git/hooks 
+   COMMIT_MSG=.git/hooks/commit-msg 
+   cat $COMMIT_MSG EOF
+#!/bin/sh
+echo running commit-msg: exit 1
+exit 1
+EOF
+   chmod +x $COMMIT_MSG 
+   test_must_fail test_commit commit-msg-violated 
+   test_commit --no-verify commit-msg-violated 
+   test_when_finished reset_rebase 
+   set_fake_editor 
+   FAKE_LINES=pick 1 git rebase -i HEAD^ 
+   FAKE_LINES=pick 1 git rebase -i --no-ff HEAD^ 
+   test_must_fail env FAKE_LINES=reword 1 git rebase -i HEAD^
+'
+
 test_expect_success 'rebase with a file named HEAD in worktree' '
 
rm -fr .git/hooks 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0377d3e..db65653 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -155,6 +155,7 @@ test_pause () {
 test_commit () {
notick= 
signoff= 
+   noverify= 
while test $# != 0
do
case $1 in
@@ -164,6 +165,9 @@ test_commit () {