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

From: Jeff King <peff@peff.net>
Date: Aug 6, 2014, 21:46 UTC
Message-ID: <20140806214624.GA23878@peff.net>
In-Reply-To: <53DFD63E.8040806@gmail.com>

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

Show 7 quoted lines

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

Show 5 quoted lines

> 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