Re: [RFC PATCH] userdiff: ship built-in driver config file

[Jeff King <peff@peff.net>]

On Tue, Jun 18, 2019 at 12:54:50AM +0800, Boxuan Li wrote:

> A few notes and questions:
> 1. In [diff "tex"] section, \x80 and \xff cannot be parsed by git config parser.
> I have no idea why this is happening. I changed them to \\x80 and \\xff as a workaround, which
> resulted in t4034 failure (See https://travis-ci.org/li-boxuan/git/jobs/546729906#L4679).
> 2. I am not sure how and where I can free the memory allocated to "builtin_drivers".
> 3. When I run `git format-patch HEAD~1`, core dump happens occasionally. Seems
> no test case caught this problem. Till now, I have no luck finding out the reason.

I couldn't replicate it with a simple test, but perhaps running under
valgrind or "make SANITIZE=address" would help?

> diff --git a/templates/this--userdiff b/templates/this--userdiff
> new file mode 100644
> index 0000000000..85114a7229
> --- /dev/null
> +++ b/templates/this--userdiff
> @@ -0,0 +1,164 @@
> +[diff "ada"]
> +	xfuncname = "!^(.*[ \t])?(is[ \t]+new|renames|is[ \t]+separate)([ \t].*)?$\n"
> +	xfuncname = "!^[ \t]*with[ \t].*$\n"
> +	xfuncname = "^[ \t]*((procedure|function)[ \t]+.*)$\n"
> +	xfuncname = "^[ \t]*((package|protected|task)[ \t]+.*)$"

While having separate lines that get joined here does make the result
easier to read, I think it creates some confusion. diff.*.xfuncname in a
regular config file _doesn't_ behave this way (it's the usual
last-one-wins, so we expect a single string). You've handled this
specially in your code to read this file, but it's confusing because
this test otherwise looks exactly like a config file. And thus somebody
might be tempted to copy it to their config file and modify it, but it
would not do what they expected.

I don't recall how well our config parser copes with embedded newlines
in values.  I.e., if it would be possible to write:

  [diff "foo"]
  xfuncname = "the pattern starts here...
  and continues through newlines!"

I think it doesn't work, but perhaps it would be a nice feature to add
it. It would make the format slightly more complex, though (and make
diagnosing a missing double-quote much harder). I dunno.

-Peff