Re: [PATCH 03/11] builtin rebase: handle the pre-rebase hook (and add --no-verify)
Hi Junio,
On Wed, 8 Aug 2018, Junio C Hamano wrote:
> Pratik Karki 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
> > ---
>
> 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.
> > 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
Re: [PATCH 03/11] builtin rebase: handle the pre-rebase hook (and add --no-verify)
Pratik Karki 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
> ---
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.
"rebase: handle the re-rebase hook and --no-verify" would have been
sufficient, without "add" or parentheses.
> builtin/rebase.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 38c496dd10..b79f9b0a9f 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -70,6 +70,7 @@ struct rebase_options {
> const char *state_dir;
> struct commit *upstream;
> const char *upstream_name;
> + const char *upstream_arg;
> char *head_name;
> struct object_id orig_head;
> struct commit *onto;
> @@ -310,6 +311,7 @@ int cmd_rebase(int argc, const char **argv, const char
> *prefix)
> };
> const char *branch_name;
> int ret, flags;
> + int ok_to_skip_pre_rebase = 0;
> struct strbuf msg = STRBUF_INIT;
> struct strbuf revisions = STRBUF_INIT;
> struct object_id merge_base;
> @@ -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?
> OPT_END(),
> };
>
> @@ -382,6 +386,7 @@ int cmd_rebase(int argc, const char **argv, const char
> *prefix)
> options.upstream = peel_committish(options.upstream_name);
> if (!options.upstream)
> die(_("invalid upstream '%s'"), options.upstream_name);
> + options.upstream_arg = options.upstream_name;
> } else
> die("TODO: upstream for --root");
>
> @@ -430,6 +435,12 @@ int cmd_rebase(int argc, const char **argv, const char
> *prefix)
> die(_("Could not resolve HEAD to a revision"));
> }
>
> + /* If a hook exists, give it a chance to interrupt*/
> + if (!ok_to_skip_pre_rebase &&
> + run_hook_le(NULL, "pre-rebase", options.upstream_arg,
> + argc ? argv[0] : NULL, NULL))
> + die(_("The pre-rebase hook refused to rebase."));
> +
> strbuf_addf(&msg, "rebase: checkout %s", options.onto_name);
> if (reset_head(&options.onto->object.oid, "checkout", NULL, 1))
> die(_("Could not detach HEAD"));
[PATCH 03/11] builtin rebase: handle the pre-rebase hook (and add --no-verify)
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
---
builtin/rebase.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 38c496dd10..b79f9b0a9f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -70,6 +70,7 @@ struct rebase_options {
const char *state_dir;
struct commit *upstream;
const char *upstream_name;
+ const char *upstream_arg;
char *head_name;
struct object_id orig_head;
struct commit *onto;
@@ -310,6 +311,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
};
const char *branch_name;
int ret, flags;
+ int ok_to_skip_pre_rebase = 0;
struct strbuf msg = STRBUF_INIT;
struct strbuf revisions = STRBUF_INIT;
struct object_id merge_base;
@@ -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")),
OPT_END(),
};
@@ -382,6 +386,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
options.upstream = peel_committish(options.upstream_name);
if (!options.upstream)
die(_("invalid upstream '%s'"), options.upstream_name);
+ options.upstream_arg = options.upstream_name;
} else
die("TODO: upstream for --root");
@@ -430,6 +435,12 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
die(_("Could not resolve HEAD to a revision"));
}
+ /* If a hook exists, give it a chance to interrupt*/
+ if (!ok_to_skip_pre_rebase &&
+ run_hook_le(NULL, "pre-rebase", options.upstream_arg,
+ argc ? argv[0] : NULL, NULL))
+ die(_("The pre-rebase hook refused to rebase."));
+
strbuf_addf(&msg, "rebase: checkout %s", options.onto_name);
if (reset_head(&options.onto->object.oid, "checkout", NULL, 1))
die(_("Could not detach HEAD"));
--
2.18.0
