Re: [PATCH v2] grep.c: remove "extended" in favor of "pattern_expression", fix segfault

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 11, 2022, 15:46 UTC
Message-ID: <xmqqczayweg4.fsf@gitster.g>
In-Reply-To: <patch-v2-1.1-6ad7627706f-20221011T094715Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 33 quoted lines

>  grep.c         | 15 +++++++--------
>  grep.h         |  1 -
>  t/t4202-log.sh |  9 +++++++++
>  3 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index 52a894c9890..06eed694936 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -708,6 +708,7 @@ void compile_grep_patterns(struct grep_opt *opt)
>  {
>  	struct grep_pat *p;
>  	struct grep_expr *header_expr = prep_header_patterns(opt);
> +	int extended = 0;
>  
>  	for (p = opt->pattern_list; p; p = p->next) {
>  		switch (p->token) {
> @@ -717,14 +718,14 @@ void compile_grep_patterns(struct grep_opt *opt)
>  			compile_regexp(p, opt);
>  			break;
>  		default:
> -			opt->extended = 1;
> +			extended = 1;
>  			break;
>  		}
>  	}
>  
>  	if (opt->all_match || opt->no_body_match || header_expr)
> -		opt->extended = 1;
> -	else if (!opt->extended)
> +		extended = 1;
> +	else if (!extended)
>  		return;

Nice. I like this change to make "!!opt->pattern_expression" the authoritative source of truth for opt->extended by getting rid of the latter. This function did need to have a handy way to tell "do we need to populate pattern_expression?" while going over the list and also use of some features forced us to do so no matter how simple the patterns on the list are, but after doing so and populating the pattern_expression, there was no reason to keep it around by having it as a member in the opt structure.

Show 36 quoted lines

> @@ -790,7 +791,7 @@ void free_grep_patterns(struct grep_opt *opt)
>  		free(p);
>  	}
>  
> -	if (!opt->extended)
> +	if (!opt->pattern_expression)
>  		return;
>  	free_pattern_expr(opt->pattern_expression);
>  }
> @@ -971,8 +972,6 @@ static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x,
>  {
>  	int h = 0;
>  
> -	if (!x)
> -		die("Not a valid grep expression");
>  	switch (x->node) {
>  	case GREP_NODE_TRUE:
>  		h = 1;
> @@ -1052,7 +1051,7 @@ static int match_line(struct grep_opt *opt,
>  	struct grep_pat *p;
>  	int hit = 0;
>  
> -	if (opt->extended)
> +	if (opt->pattern_expression)
>  		return match_expr(opt, bol, eol, ctx, col, icol,
>  				  collect_hits);
>  
> @@ -1370,7 +1369,7 @@ static int should_lookahead(struct grep_opt *opt)
>  {
>  	struct grep_pat *p;
>  
> -	if (opt->extended)
> +	if (opt->pattern_expression)
>  		return 0; /* punt for too complex stuff */
>  	if (opt->invert)
>  		return 0;

And the necessary change for users is surprisingly small.

Show 28 quoted lines

> diff --git a/grep.h b/grep.h
> index bdcadce61b8..6075f997e68 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -151,7 +151,6 @@ struct grep_opt {
>  #define GREP_BINARY_TEXT	2
>  	int binary;
>  	int allow_textconv;
> -	int extended;
>  	int use_reflog_filter;
>  	int relative;
>  	int pathname;
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index cc15cb4ff62..2ce2b41174d 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -249,6 +249,15 @@ test_expect_success 'log --grep' '
>  	test_cmp expect actual
>  '
>  
> +for noop_opt in --invert-grep --all-match
> +do
> +	test_expect_success "log $noop_opt without --grep is a NOOP" '
> +		git log >expect &&
> +		git log $noop_opt >actual &&
> +		test_cmp expect actual
> +	'
> +done

OK.

Thanks, will queue.