Re: [PATCH v5 03/10] test-regex: expose full regcomp() to the command line

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 29, 2016, 05:31 UTC
Message-ID: <20160129053143.GA17334@flurp.local>
In-Reply-To: <1453982183-24124-4-git-send-email-pclouds@gmail.com>

On Thu, Jan 28, 2016 at 06:56:16PM +0700, Nguyễn Thái Ngọc Duy wrote:

Show 24 quoted lines

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/test-regex.c b/test-regex.c
> @@ -1,19 +1,63 @@
>  int main(int argc, char **argv)
>  {
> -	char *pat = "[^={} \t]+";
> -	char *str = "={}\nfred";
> +	const char *pat;
> +	const char *str;
> +	int flags = 0;
>  	regex_t r;
>  	regmatch_t m[1];
>  
> -	if (regcomp(&r, pat, REG_EXTENDED | REG_NEWLINE))
> +	if (argc == 1) {
> +		/* special case, bug check */
> +		pat = "[^={} \t]+";
> +		str = "={}\nfred";
> +		flags = REG_EXTENDED | REG_NEWLINE;
> +	} else {
> +		argv++;
> +		pat = *argv++;
> +		str = *argv++;

I realize that this is just a test program, but it might be a good idea to insert:

    if (argc < 3)
        die("usage: ...");

prior to the *argv++ dereferences to give a controlled failure rather than an outright crash when an incorrect number of arguments is given.

More below...

Show 28 quoted lines

> +		while (*argv) {
> +			struct reg_flag *rf;
> +			for (rf = reg_flags; rf->name; rf++)
> +				if (!strcmp(*argv, rf->name)) {
> +					flags |= rf->flag;
> +					break;
> +				}
> +			if (!rf->name)
> +				die("do not recognize %s", *argv);
> +			argv++;
> +		}
> +		git_setup_gettext();
> +	}
> +
> +	if (regcomp(&r, pat, flags))
>  		die("failed regcomp() for pattern '%s'", pat);
> -	if (regexec(&r, str, 1, m, 0))
> -		die("no match of pattern '%s' to string '%s'", pat, str);
> +	if (regexec(&r, str, 1, m, 0)) {
> +		if (argc == 1)
> +			die("no match of pattern '%s' to string '%s'", pat, str);
> +		return 1;
> +	}
>  
>  	/* http://sourceware.org/bugzilla/show_bug.cgi?id=3957  */
> -	if (m[0].rm_so == 3) /* matches '\n' when it should not */
> +	if (argc == 1 && m[0].rm_so == 3) /* matches '\n' when it should not */
>  		die("regex bug confirmed: re-build git with NO_REGEX=1");

Again, I realize that this is just a test program, but sprinkling this 'argc == 1' special case throughout the code makes it unnecessarily difficult to follow. Some alternatives:

1. Rename the existing test-regex to test-regex-bug (or
   test-regex-bugs), and then name the new general purpose program
   test-regex.

2. Drop the special case altogether and have the program emit the
   matched text on stdout (in addition to the exit code indicating
   success/failure). Most callers will care only about the exit
   status, but the one special case in t0070 which wants to check for
   the glibc bug can do so itself:

    test_expect_success 'check for a bug in the regex routines' '
        # if this test fails, re-build git with NO_REGEX=1
        printf "fred" >expect &&
        test-regex "[^={} \t]+" "={}\nfred" EXTENDED NEWLINE >actual &&
        test_cmp expect actual
    '

   Of course, that doesn't actually work because "\n" in the 'str'
   argument isn't really a newline, so test-regex would have to do a
   bit of preprocessing of 'str' first (which might be as simple as
   calling unquote_c_style() or something).

3. [less desirable] Move the 'argc == 1' special case to its own
   function, which will result in a bit of duplicated code, but the
   result should at least be easier to follow.

>  	exit(0);
> -- 
> 2.7.0.288.g1d8ad15