Re: [PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early
Hi Ævar,
On Mon, 19 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Nov 14 2018, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin
> >
> > It is a good idea to error out early upon seeing, say, `-Cbad`, rather
> > than starting the rebase only to have the `--am` backend complain later.
> >
> > Let's do this.
> >
> > The only options accepting parameters which we pass through to `git am`
> > (which may, or may not, forward them to `git apply`) are `-C` and
> > `--whitespace`. The other options we pass through do not accept
> > parameters, so we do not have to validate them here.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > builtin/rebase.c | 12 +++-
> > t/t3406-rebase-message.sh | 7 +++
> > 2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 96ffa80b71..571cf899d5 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const
> > char *prefix)
> > }
> >
> > for (i = 0; i < options.git_am_opts.argc; i++) {
> > - const char *option = options.git_am_opts.argv[i];
> > + const char *option = options.git_am_opts.argv[i], *p;
> > if (!strcmp(option, "--committer-date-is-author-date") ||
> > !strcmp(option, "--ignore-date") ||
> > !strcmp(option, "--whitespace=fix") ||
> > !strcmp(option, "--whitespace=strip"))
> > options.flags |= REBASE_FORCE;
> > + else if (skip_prefix(option, "-C", )) {
> > + while (*p)
> > + if (!isdigit(*(p++)))
> > + die(_("switch `C' expects a "
> > + "numerical value"));
> > + } else if (skip_prefix(option, "--whitespace=", )) {
> > + if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
> > + strcmp(p, "error") && strcmp(p, "error-all"))
> > + die("Invalid whitespace option: '%s'", p);
> > + }
> > }
> >
> > if (!(options.flags & REBASE_NO_QUIET))
> > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> > index 0392e36d23..2c79eed4fe 100755
> > --- a/t/t3406-rebase-message.sh
> > +++ b/t/t3406-rebase-message.sh
> > @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid
> > ref' '
> > test_i18ngrep "invalid-ref" err
> > '
> >
> > +test_expect_success 'error out early upon -C or --whitespace=' '
> > + test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
> > + test_i18ngrep "numerical value" err &&
> > + test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> > + test_i18ngrep "Invalid whitespace option" err
> > +'
> > +
>
> The combination of this gitster/js/rebase-am-options and my
> gitster/ab/rebase-in-c-escape-hatch breaks tests under
> GIT_TEST_REBASE_USE_BUILTIN=false for obvious reasons. The C version is
> now more strict.
Maybe you can concoct a prereq for this test? Something like
test_lazy_prereq BUILTIN_REBASE '
test true = "${GIT_TEST_REBASE_USE_BUILTIN:-true}"
'
Ciao,
Dscho
Re: [PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early
On Wed, Nov 14 2018, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin
>
> It is a good idea to error out early upon seeing, say, `-Cbad`, rather
> than starting the rebase only to have the `--am` backend complain later.
>
> Let's do this.
>
> The only options accepting parameters which we pass through to `git am`
> (which may, or may not, forward them to `git apply`) are `-C` and
> `--whitespace`. The other options we pass through do not accept
> parameters, so we do not have to validate them here.
>
> Signed-off-by: Johannes Schindelin
> ---
> builtin/rebase.c | 12 +++-
> t/t3406-rebase-message.sh | 7 +++
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 96ffa80b71..571cf899d5 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const
> char *prefix)
> }
>
> for (i = 0; i < options.git_am_opts.argc; i++) {
> - const char *option = options.git_am_opts.argv[i];
> + const char *option = options.git_am_opts.argv[i], *p;
> if (!strcmp(option, "--committer-date-is-author-date") ||
> !strcmp(option, "--ignore-date") ||
> !strcmp(option, "--whitespace=fix") ||
> !strcmp(option, "--whitespace=strip"))
> options.flags |= REBASE_FORCE;
> + else if (skip_prefix(option, "-C", )) {
> + while (*p)
> + if (!isdigit(*(p++)))
> + die(_("switch `C' expects a "
> + "numerical value"));
> + } else if (skip_prefix(option, "--whitespace=", )) {
> + if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
> + strcmp(p, "error") && strcmp(p, "error-all"))
> + die("Invalid whitespace option: '%s'", p);
> + }
> }
>
> if (!(options.flags & REBASE_NO_QUIET))
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 0392e36d23..2c79eed4fe 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid
> ref' '
> test_i18ngrep "invalid-ref" err
> '
>
> +test_expect_success 'error out early upon -C or --whitespace=' '
> + test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
> + test_i18ngrep "numerical value" err &&
> + test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> + test_i18ngrep "Invalid whitespace option" err
> +'
> +
The combination of this gitster/js/rebase-am-options and my
gitster/ab/rebase-in-c-escape-hatch breaks tests under
GIT_TEST_REBASE_USE_BUILTIN=false for obvious reasons. The C version is
now more strict.
Re: [PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early
Hi Phillip,
On Wed, 14 Nov 2018, Phillip Wood wrote:
> Thanks for doing this, I think this patch is good.
Thanks!
> I've not checked the first patch as I think it is the same as before
> judging from the covering letter.
Indeed, that's what the range-diff said. Sorry for not stating this
explicitly.
Thank you for your review,
Dscho
>
> Best Wishes
>
> Phillip
>
> On 14/11/2018 16:25, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin
> >
> > It is a good idea to error out early upon seeing, say, `-Cbad`, rather
> > than starting the rebase only to have the `--am` backend complain later.
> >
> > Let's do this.
> >
> > The only options accepting parameters which we pass through to `git am`
> > (which may, or may not, forward them to `git apply`) are `-C` and
> > `--whitespace`. The other options we pass through do not accept
> > parameters, so we do not have to validate them here.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > builtin/rebase.c | 12 +++-
> > t/t3406-rebase-message.sh | 7 +++
> > 2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 96ffa80b71..571cf899d5 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv,
> > const char *prefix)
> >}
> >
> > for (i = 0; i < options.git_am_opts.argc; i++) {
> > - const char *option = options.git_am_opts.argv[i];
> > + const char *option = options.git_am_opts.argv[i], *p;
> > if (!strcmp(option, "--committer-date-is-author-date") ||
> > !strcmp(option, "--ignore-date") ||
> > !strcmp(option, "--whitespace=fix") ||
> > !strcmp(option, "--whitespace=strip"))
> > options.flags |= REBASE_FORCE;
> > + else if (skip_prefix(option, "-C", )) {
> > + while (*p)
> > + if (!isdigit(*(p++)))
> > + die(_("switch `C' expects a "
> > + "numerical value"));
> > + } else if (skip_prefix(option, "--whitespace=", )) {
> > + if (*p && strcmp(p, "warn") && strcmp(p, "nowarn")
> > &&
> > + strcmp(p, "error") && strcmp(p, "error-all"))
> > + die("Invalid whitespace option: '%s'", p);
> > + }
> >}
> >
> > if (!(options.flags & REBASE_NO_QUIET))
> > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> > index 0392e36d23..2c79eed4fe 100755
> > --- a/t/t3406-rebase-message.sh
> > +++ b/t/t3406-rebase-message.sh
> > @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the
> > invalid ref' '
> > test_i18ngrep "invalid-ref" err
> > '
> >
> > +test_expect_success 'error out early upon -C or --whitespace='
> > '
> > + test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
> > + test_i18ngrep "numerical value" err &&
> > + test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> > + test_i18ngrep "Invalid whitespace option" err
> > +'
> > +
> > test_done
> >
>
>
Re: [PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early
Hi Johannes
Thanks for doing this, I think this patch is good. I've not checked the
first patch as I think it is the same as before judging from the
covering letter.
Best Wishes
Phillip
On 14/11/2018 16:25, Johannes Schindelin via GitGitGadget wrote:
From: Johannes Schindelin
It is a good idea to error out early upon seeing, say, `-Cbad`, rather
than starting the rebase only to have the `--am` backend complain later.
Let's do this.
The only options accepting parameters which we pass through to `git am`
(which may, or may not, forward them to `git apply`) are `-C` and
`--whitespace`. The other options we pass through do not accept
parameters, so we do not have to validate them here.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 12 +++-
t/t3406-rebase-message.sh | 7 +++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 96ffa80b71..571cf899d5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
}
for (i = 0; i < options.git_am_opts.argc; i++) {
- const char *option = options.git_am_opts.argv[i];
+ const char *option = options.git_am_opts.argv[i], *p;
if (!strcmp(option, "--committer-date-is-author-date") ||
!strcmp(option, "--ignore-date") ||
!strcmp(option, "--whitespace=fix") ||
!strcmp(option, "--whitespace=strip"))
options.flags |= REBASE_FORCE;
+ else if (skip_prefix(option, "-C", )) {
+ while (*p)
+ if (!isdigit(*(p++)))
+ die(_("switch `C' expects a "
+ "numerical value"));
+ } else if (skip_prefix(option, "--whitespace=", )) {
+ if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
+ strcmp(p, "error") && strcmp(p, "error-all"))
+ die("Invalid whitespace option: '%s'", p);
+ }
}
if (!(options.flags & REBASE_NO_QUIET))
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 0392e36d23..2c79eed4fe 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
test_i18ngrep "invalid-ref" err
'
+test_expect_success 'error out early upon -C or --whitespace=' '
+ test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
+ test_i18ngrep "numerical value" err &&
+ test_must_fail git rebase --whitespace=bad HEAD 2>err &&
+ test_i18ngrep "Invalid whitespace option" err
+'
+
test_done
[PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early
From: Johannes Schindelin
It is a good idea to error out early upon seeing, say, `-Cbad`, rather
than starting the rebase only to have the `--am` backend complain later.
Let's do this.
The only options accepting parameters which we pass through to `git am`
(which may, or may not, forward them to `git apply`) are `-C` and
`--whitespace`. The other options we pass through do not accept
parameters, so we do not have to validate them here.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 12 +++-
t/t3406-rebase-message.sh | 7 +++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 96ffa80b71..571cf899d5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
}
for (i = 0; i < options.git_am_opts.argc; i++) {
- const char *option = options.git_am_opts.argv[i];
+ const char *option = options.git_am_opts.argv[i], *p;
if (!strcmp(option, "--committer-date-is-author-date") ||
!strcmp(option, "--ignore-date") ||
!strcmp(option, "--whitespace=fix") ||
!strcmp(option, "--whitespace=strip"))
options.flags |= REBASE_FORCE;
+ else if (skip_prefix(option, "-C", )) {
+ while (*p)
+ if (!isdigit(*(p++)))
+ die(_("switch `C' expects a "
+ "numerical value"));
+ } else if (skip_prefix(option, "--whitespace=", )) {
+ if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
+ strcmp(p, "error") && strcmp(p, "error-all"))
+ die("Invalid whitespace option: '%s'", p);
+ }
}
if (!(options.flags & REBASE_NO_QUIET))
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 0392e36d23..2c79eed4fe 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
test_i18ngrep "invalid-ref" err
'
+test_expect_success 'error out early upon -C or --whitespace=' '
+ test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
+ test_i18ngrep "numerical value" err &&
+ test_must_fail git rebase --whitespace=bad HEAD 2>err &&
+ test_i18ngrep "Invalid whitespace option" err
+'
+
test_done
--
gitgitgadget
