Re: [PATCH 09/11] diffcore-pickaxe: "share" regex error handling code

[Jeff King <peff@peff.net>]

On Thu, Jun 23, 2016 at 06:29:05PM +0200, Nguyễn Thái Ngọc Duy wrote:

> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
> index 7715c13..69c6567 100644
> --- a/diffcore-pickaxe.c
> +++ b/diffcore-pickaxe.c
> @@ -204,20 +204,13 @@ void diffcore_pickaxe(struct diff_options *o)
>  	int opts = o->pickaxe_opts;
>  	regex_t regex, *regexp = NULL;
>  	kwset_t kws = NULL;
> +	int err = 0;
>  
>  	if (opts & (DIFF_PICKAXE_REGEX | DIFF_PICKAXE_KIND_G)) {
> -		int err;
>  		int cflags = REG_EXTENDED | REG_NEWLINE;
>  		if (DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE))
>  			cflags |= REG_ICASE;
>  		err = regcomp(&regex, needle, cflags);
> -		if (err) {
> -			/* The POSIX.2 people are surely sick */
> -			char errbuf[1024];
> -			regerror(err, &regex, errbuf, 1024);
> -			regfree(&regex);
> -			die("invalid regex: %s", errbuf);
> -		}
>  		regexp = &regex;
>  	} else {
>  		kws = kwsalloc(DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE)
> @@ -225,6 +218,13 @@ void diffcore_pickaxe(struct diff_options *o)
>  		kwsincr(kws, needle, strlen(needle));
>  		kwsprep(kws);
>  	}
> +	if (err) {
> +		/* The POSIX.2 people are surely sick */
> +		char errbuf[1024];
> +		regerror(err, &regex, errbuf, 1024);
> +		regfree(&regex);
> +		die("invalid regex: %s", errbuf);
> +	}

Hrm. I wondered what happens if we see an error in the kwset code block,
which did not put anything useful in "regex" at all.

It's OK right now, because "err" is newly promoted to the top of the
function, and so we know that kwset cannot call it. But it seems like
an accident waiting to happen. Calling it "regex_err" or something might
help.

But I also wonder if a function wouldn't be better. You could even roll
it up with regcomp, like:

  static void regcomp_or_die(regex_t *regex, const char *pattern, int flags)
  {
	int err = regcomp(regex, pattern, flags);
	if (err) {
		char buf[1024];
		regerror(err, &regex, buf, sizeof(buf));
		regfree(&regex);
		die("invalid regex: %s", buf);
	}
  }

I think you could also skip the regfree(), since we are about to die. I
also think the error message would probably be better if it mentioned
the text of "pattern" itself (since it might be coming from config, or
you might have provided several patterns, or you might have thought
something was supposed to be a non-regex).

-Peff