Junio C Hamano <gitster@xxxxxxxxx> writes:
>> 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
>
>> @@ -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?
Ah, grep.c::compile_grep_patterns() has the answer. We only
populate the .pattern_expression member when we are doing a complex
query and leave it empty otherwise. The .pattern_list member is
used instead as a list of OR'ed patterns in grep.c::match_line()
when .extended is not set.
The !opt->extended guard assumes that opt->pattern_expression exists
only when extended is set, which is correct, but forgets that even
when extended is set, pattern_expression is not necessarily non-NULL.
So I think the right thing to do may be to allow free_pattern_expr()
to take and ignore NULL silently? Ah, that is already what you are
doing in the first hunk. Is this second hunk even necessary?
I wonder how calls to grep.c::match_line() with opt->extended true
and opt->pattern_expression NULL, though. It should die() at the
beginning of match_expr_eval(), which probably is OK, but somehow
feels unsatisfactory.
