On Mon, Oct 10, 2022 at 10:54:23AM -0700, Junio C Hamano wrote: > Taylor Blau <me@xxxxxxxxxxxx> 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. > > 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
