Re: [PATCH 2/2] grep.c: tolerate NULL grep_expr in free_pattern_expr()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 10, 2022, 17:54 UTC
- Message-ID
- <xmqqy1tn36pc.fsf@gitster.g>
- In-Reply-To
- <7e094882c2a71894416089f894557a9eae07e8f8.1665423686.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
Show 14 quoted lines
> diff --git a/grep.c b/grep.c
> index 52a894c989..bcc6e63365 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -752,6 +752,9 @@ void compile_grep_patterns(struct grep_opt *opt)
>
> static void free_pattern_expr(struct grep_expr *x)
> {
> + if (!x)
> + return;
> +
> switch (x->node) {
> case GREP_NODE_TRUE:
> case GREP_NODE_ATOM:This hunk makes sense, but
Show 8 quoted lines
> @@ -790,8 +793,6 @@ void free_grep_patterns(struct grep_opt *opt) > free(p); > } > > - if (!opt->extended) > - return; > free_pattern_expr(opt->pattern_expression); > }
I do not know about this one. We used to avoid freeing, even when the .pattern_expression member is set, as long as the .extended bit is not set. Now we unconditionally try to free it even when the bit says it does not want to. Why?
Show 13 quoted lines
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh > index e3ec5f5661..44f7ef0ea2 100755 > --- a/t/t4202-log.sh > +++ b/t/t4202-log.sh > @@ -297,7 +297,7 @@ test_expect_success 'log --invert-grep --grep -i' ' > fi > ' > > -test_expect_failure 'log --invert-grep (no --grep)' ' > +test_expect_success 'log --invert-grep (no --grep)' ' > git log --pretty="tformat:%s" >expect && > git log --invert-grep --pretty="tformat:%s" >actual && > test_cmp expect actual
Especially for something this small, doing the "failing test first and then fix with flipping the test to success" is very much unwelcome. For whoever gets curious (me included when accepting posted patch), it is easy to revert only the part of the commit outside t/ tentatively to see how the original code breaks. Keeping the fix and protection of the fix together will avoid mistakes. In this case, the whole test fits inside the post context of the patch, but in general, this "flip failure to success" will hide the body of the test that changes behaviour while reviewing the patch text, which is another downside.
Thanks.