Re: [PATCH 2/2] grep.c: tolerate NULL grep_expr in free_pattern_expr()

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 10, 2022, 18:10 UTC
Message-ID: <Y0RgIGpxXXilh7YM@nand.local>
In-Reply-To: <xmqqy1tn36pc.fsf@gitster.g>

On Mon, Oct 10, 2022 at 10:54:23AM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> Taylor Blau <me@ttaylorr.com> writes:
> > @@ -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?

It's not "does not want to" be freed. As best I can tell, we conflate `opt->extended` with "there is something in `opt->pattern_expression`". So checking whether or not `opt->extended` is non-zero isn't "keep this around because I'm going to use it later", but instead "there is nothing to free, don't bother calling `free_pattern_expr()`".

A more direct way of saying the latter would have been to replace the if-statement with `if (opt->pattern_expression)`.

I hinted at this in the commit message, but I will make it more direct to avoid future readers' confusion.

Show 24 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.

Good to know. I had considered it good practice, even for a small fix, as a way to show your work and prove that you had a legitimately broken test case demonstrating the bug. But if it creates an extra hassle, I don't mind squashing it down.

I can send a squashed version of these two patches, but let's see if there are any other comments, first.

Thanks, Taylor