On 29/01/16 05:31, Eric Sunshine wrote:
> On Thu, Jan 28, 2016 at 06:56:16PM +0700, Nguyễn Thái Ngọc Duy wrote:
>> Signed-off-by: Nguyễn Thái Ngọc Duy <[email protected]>
>> ---
>> 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...
>
>> + 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.
I completely agree!
> 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.
I think this is the most desirable (and was going to be my first
suggestion); the duplication is minimal and it makes the code _much_
easier to follow. [I suppose separate test programs (ie. point 1) would
be my second choice.]
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
