Re: [PATCH 03/11] builtin rebase: handle the pre-rebase hook (and add --no-verify)

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 27, 2018, 12:15 UTC
Message-ID: <nycvar.QRO.7.76.6.1808271412140.73@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqwot0fydp.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Wed, 8 Aug 2018, Junio C Hamano wrote:

Show 12 quoted lines

> Pratik Karki <predatoramigo@gmail.com> writes:
> 
> > This commit converts the equivalent part of the shell script
> > `git-legacy-rebase.sh` to run the pre-rebase hook (unless disabled), and
> > to interrupt the rebase with error if the hook fails.
> >
> > Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> > ---
> 
> Introduction of upstream_arg in this step looked a bit
> surprising, but the hook invocation is the only thing that uses it,
> so it is understandable.

Yep, that's literally all that `upstream_arg` is used for:

$ git grep upstream_arg v2.19.0-rc0 v2.19.0-rc0:git-rebase.sh: upstream_arg="$upstream_name" v2.19.0-rc0:git-rebase.sh: upstream_arg=--root v2.19.0-rc0:git-rebase.sh:run_pre_rebase_hook "$upstream_arg" "$@"

> "rebase: handle the re-rebase hook and --no-verify" would have been
> sufficient, without "add" or parentheses.

Fixed.

Show 14 quoted lines

> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 38c496dd10..b79f9b0a9f 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -317,6 +319,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >  		OPT_STRING(0, "onto", &options.onto_name,
> >  			   N_("revision"),
> >  			   N_("rebase onto given branch instead of upstream")),
> > +		OPT_BOOL(0, "no-verify", &ok_to_skip_pre_rebase,
> > +			 N_("allow pre-rebase hook to run")),
> 
> Do we need to catch "--no-no-verify" ourselves with NONEG bit, or is
> this sufficient to tell parse_options() machinery to take care of
> it?

I just issued

	$ ./git rebase --verify --no-no-verify --xyz

and it showed

	error: unknown option `xyz'
	[... usage ...]

I vaguely remembered that the parse_options() machinery special-cases "no-" prefixes, and my test seems to confirm it.

Holler if you want a more in-depth analysis.

Ciao, Dscho