Re: [PATCH v2 15/22] pickaxe: assert that we must have a needle under -G or -S
From: Junio C Hamano <hidden>
Date: 2021-03-30 23:50:45
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted hunk
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.
quoted hunk
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?
quoted hunk
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 &&