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

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> 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.