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

From: Jeff King <peff@peff.net>
Date: Jun 23, 2016, 19:16 UTC
Message-ID: <20160623191614.GA1841@sigill.intra.peff.net>
In-Reply-To: <20160623162907.23295-10-pclouds@gmail.com>

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

Show 37 quoted lines

> 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