Re: [PATCH] diff: don't crash with empty argument to -G or -S
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 18, 2025, 18:16 UTC
- Message-ID
- <xmqqbjuzxgn3.fsf@gitster.g>
- In-Reply-To
- <20250217175759.1576684-1-sandals@crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 6 quoted lines
> The pickaxe options, -G and -S, need either a regex or a string to look > through the history for. An empty value isn't very useful since it > would either match everything or nothing, and what's worse, we presently > crash with a BUG like so when the user provides one: > > BUG: diffcore-pickaxe.c:241: should have needle under -G or -S
I agree BUG is unwelcome. I am not sure about the value of forbidding an empty string (I am sure about forbidding NULL, though).
If an empty matches everything, "git log -S" would skip changes that would keep the number of lines, right? For the history of a project that keeps track of source code, such a "feature" would not be useful, but I can see a complaint by somebody who may want to keep track of a "list of things" one-item-per-line, if we had been allowing an empty string. It would be a regression for such a niche user.
Luckily, since we have stopped with a "BUG", we do not have to worry about backward compatibility in this case ;-)
> Since it's not very nice of us to crash and this wouldn't do anything > useful anyway, let's simply inform the user that they must provide a > non-empty argument and exit with an error if they provide an empty one > instead.
So I'd say that it may be a bit premature for us to declare "anything useful", I am perfectly fine with the patch given here. If somebody who wants to maintain a text file, one-item-per-line that keeps track of a list of things to omit commits that do not change the number of items, they can drop "&& !*arg" part, tweak the message and add their own tests, once this fix lands and the dust settles.
Thanks for a quick fix. Will queue.
Show 57 quoted lines
>
> Reported-by: Jared Van Bortel <cebtenzzre@gmail.com>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff.c | 4 ++++
> t/t4209-log-pickaxe.sh | 16 ++++++++++++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/diff.c b/diff.c
> index 019fb893a7..c89c15d98e 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5493,6 +5493,8 @@ static int diff_opt_pickaxe_regex(const struct option *opt,
> BUG_ON_OPT_NEG(unset);
> options->pickaxe = arg;
> options->pickaxe_opts |= DIFF_PICKAXE_KIND_G;
> + if (arg && !*arg)
> + return error(_("-G requires a non-empty argument"));
> return 0;
> }
>
> @@ -5504,6 +5506,8 @@ static int diff_opt_pickaxe_string(const struct option *opt,
> BUG_ON_OPT_NEG(unset);
> options->pickaxe = arg;
> options->pickaxe_opts |= DIFF_PICKAXE_KIND_S;
> + if (arg && !*arg)
> + return error(_("-S requires a non-empty argument"));
> return 0;
> }
>
> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
> index a675ace081..0e2f80a268 100755
> --- a/t/t4209-log-pickaxe.sh
> +++ b/t/t4209-log-pickaxe.sh
> @@ -93,6 +93,22 @@ test_expect_success 'usage: --no-pickaxe-regex' '
> test_cmp expect actual
> '
>
> +test_expect_success 'usage: -G and -S with empty argument' '
> + cat >expect <<-\EOF &&
> + error: -S requires a non-empty argument
> + EOF
> +
> + test_expect_code 129 git log -S "" 2>actual &&
> + test_cmp expect actual &&
> +
> + cat >expect <<-\EOF &&
> + error: -G requires a non-empty argument
> + EOF
> +
> + test_expect_code 129 git log -G "" 2>actual &&
> + test_cmp expect actual
> +'
> +
> test_log expect_initial --grep initial
> test_log expect_nomatch --grep InItial
> test_log_icase expect_initial --grep InItial