| Message ID | 20250217175759.1576684-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a620046b29d3a9b8a0337c0396441c26ac84ebe9 |
| Headers | show |
| Series | diff: don't crash with empty argument to -G or -S | expand |
On Mon, Feb 17, 2025 at 9:58 AM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > 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 > > 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. Makes sense. > 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; > } Simple fix, as expected. > 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 > +' Looks good to me.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > 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. > > 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
On 2025-02-18 at 18:16:32, Junio C Hamano wrote: > 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. I actually just ran a `git grep -e ''` to see what it does, and it does indeed match every line, so presumably `git log -G` would do so as well. I do see your argument that this could be useful for a limited number of use cases, but as someone who often keeps track of lists of things in text files and therefore could be a target for that feature, I still feel like this would be very much a corner case. > Luckily, since we have stopped with a "BUG", we do not have to worry > about backward compatibility in this case ;-) I agree. The good news is that we haven't broken anyone's workflow, unless their workflow involves trying to trigger bugs. > 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. Exactly. If there's one thing I've learned, it's that there are lots of users who will try new things, and I'm sure we'll get a report here or elsewhere that they'd like to add this feature if there's actually interest. Fortunately, I expect that it shouldn't be too hard to add such a feature.
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
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 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. 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(+)