| Message ID | 20210216115801.4773-16-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c > index cff46f9f8f7..dd1b5c72332 100644 > --- a/diffcore-pickaxe.c > +++ b/diffcore-pickaxe.c > @@ -132,9 +132,6 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o, > oidset_contains(o->objfind, &p->two->oid)); > } > > - if (!o->pickaxe[0]) > - return 0; So -S"" could pass o->pickaxe a non-NULL pointer, but the string pointed by that pointer could be 0-length. And that is not what we want to see happen. > if (o->flags.allow_textconv) { > textconv_one = get_textconv(o->repo, p->one); > textconv_two = get_textconv(o->repo, p->two); > @@ -230,6 +227,8 @@ void diffcore_pickaxe(struct diff_options *o) > kwset_t kws = NULL; > pickaxe_fn fn; > > + if (opts & ~DIFF_PICKAXE_KIND_OBJFIND && !needle) > + BUG("should have needle under -G or -S"); Here, needle was picked up at the beginning of this function like so: void diffcore_pickaxe(struct diff_options *o) { const char *needle = o->pickaxe; The two checks seem to be protecting from different things. Shouldn't the new BUG() condition be more like if ((opts & ~DIFF_PICKAXE_KIND_OBJFIND) && (!needle || !*needle)) instead? > if (opts & (DIFF_PICKAXE_REGEX | DIFF_PICKAXE_KIND_G)) { > int cflags = REG_EXTENDED | REG_NEWLINE; > if (o->pickaxe_opts & DIFF_PICKAXE_IGNORE_CASE) > diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh > index bcaca7e882c..4b65b89e7a5 100755 > --- a/t/t4209-log-pickaxe.sh > +++ b/t/t4209-log-pickaxe.sh > @@ -56,6 +56,12 @@ test_expect_success setup ' > ' > > test_expect_success 'usage' ' > + test_expect_code 129 git log -S 2>err && > + test_i18ngrep "switch.*requires a value" err && > + > + test_expect_code 129 git log -G 2>err && > + test_i18ngrep "switch.*requires a value" err && > + > test_expect_code 128 git log -Gregex -Sstring 2>err && > test_i18ngrep "mutually exclusive" err &&
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index cff46f9f8f7..dd1b5c72332 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -132,9 +132,6 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o, oidset_contains(o->objfind, &p->two->oid)); } - if (!o->pickaxe[0]) - return 0; - if (o->flags.allow_textconv) { textconv_one = get_textconv(o->repo, p->one); textconv_two = get_textconv(o->repo, p->two); @@ -230,6 +227,8 @@ void diffcore_pickaxe(struct diff_options *o) kwset_t kws = NULL; pickaxe_fn fn; + if (opts & ~DIFF_PICKAXE_KIND_OBJFIND && !needle) + BUG("should have needle under -G or -S"); if (opts & (DIFF_PICKAXE_REGEX | DIFF_PICKAXE_KIND_G)) { int cflags = REG_EXTENDED | REG_NEWLINE; if (o->pickaxe_opts & DIFF_PICKAXE_IGNORE_CASE) diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh index bcaca7e882c..4b65b89e7a5 100755 --- a/t/t4209-log-pickaxe.sh +++ b/t/t4209-log-pickaxe.sh @@ -56,6 +56,12 @@ test_expect_success setup ' ' test_expect_success 'usage' ' + test_expect_code 129 git log -S 2>err && + test_i18ngrep "switch.*requires a value" err && + + test_expect_code 129 git log -G 2>err && + test_i18ngrep "switch.*requires a value" err && + test_expect_code 128 git log -Gregex -Sstring 2>err && test_i18ngrep "mutually exclusive" err &&
Assert early in diffcore_pickaxe() that we've got a needle to work with under -G and -S. This code is redundant to the check -G and -S get from parse-options.c's get_arg(), which I'm adding a test for. This check dates back to e1b161161d (diffcore-pickaxe: fix infinite loop on zero-length needle, 2007-01-25) when "git log -S" could send this code into an infinite loop. It was then later refactored in 8fa4b09fb1 (pickaxe: hoist empty needle check, 2012-10-28) into its current form, but it seemingly wasn't noticed that in the meantime a move to the parse-options.c API in dea007fb4c (diff: parse separate options like -S foo, 2010-08-05) had made it redundant. Let's retain some of the paranoia here with a BUG(), but there's no need to be checking this in the pickaxe_match() inner loop. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- diffcore-pickaxe.c | 5 ++--- t/t4209-log-pickaxe.sh | 6 ++++++ 2 files changed, 8 insertions(+), 3 deletions(-)