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

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

Taylor Blau <me@ttaylorr.com> writes:

> diff --git a/grep.c b/grep.c
> index 52a894c989..bcc6e63365 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -752,6 +752,9 @@ void compile_grep_patterns(struct grep_opt *opt)
>  
>  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?

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

Thanks.