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, 18:11 UTC
Message-ID: <xmqqsfjv35xi.fsf@gitster.g>
In-Reply-To: <xmqqy1tn36pc.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 24 quoted lines

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