{"thread":{"id":"51329","subject":"[RFC PATCH] userdiff: ship built-in driver config file","startedAt":"2019-06-17T16:55:24Z","lastAt":"2019-06-20T07:41:18Z","messageCount":12,"participants":["Boxuan Li","Johannes Sixt","Jeff King","Junio C Hamano","LI, BO XUAN"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"377357","messageId":"20190617165450.81916-1-liboxuan@connect.hku.hk","threadId":"51329","inReplyTo":null,"subject":"[RFC PATCH] userdiff: ship built-in driver config file","fromName":"Boxuan Li","fromEmail":"liboxuan@connect.hku.hk","sentAt":"2019-06-17T16:54:50Z","receivedAt":"2019-06-17T16:55:24Z","isPatch":true,"sender":{"key":"liboxuan@connect.hku.hk","avatar":"https://avatars.githubusercontent.com/u/25746010?v=4"},"body":"The userdiff.c has been rewritten to avoid hard-coded built-in\ndriver patterns. Now we ship\n$(sharedir)/git-core/templates/userdiff that can be read using\ngit_config_from_file() interface, using a very narrow callback\nfunction that understands only diff.*.xfuncname,\ndiff.*.wordregex, and diff.*.regIcase.\n\nSigned-off-by: Boxuan Li <liboxuan@connect.hku.hk>\n---\nA few notes and questions:\n1. In [diff \"tex\"] section, \\x80 and \\xff cannot be parsed by git config parser.\nI have no idea why this is happening. I changed them to \\\\x80 and \\\\xff as a workaround, which\nresulted in t4034 failure (See https://travis-ci.org/li-boxuan/git/jobs/546729906#L4679).\n2. I am not sure how and where I can free the memory allocated to \"builtin_drivers\".\n3. When I run `git format-patch HEAD~1`, core dump happens occasionally. Seems\nno test case caught this problem. Till now, I have no luck finding out the reason.\n\nAny hint or review would be appreciated.\n---\n templates/this--userdiff | 164 ++++++++++++++++++++++\n userdiff.c               | 284 +++++++++++++++------------------------\n 2 files changed, 275 insertions(+), 173 deletions(-)\n create mode 100644 templates/this--userdiff\n\ndiff --git a/templates/this--userdiff b/templates/this--userdiff\nnew file mode 100644\nindex 0000000000..85114a7229\n--- /dev/null\n+++ b/templates/this--userdiff\n@@ -0,0 +1,164 @@\n+[diff \"ada\"]\n+\txfuncname = \"!^(.*[ \\t])?(is[ \\t]+new|renames|is[ \\t]+separate)([ \\t].*)?$\\n\"\n+\txfuncname = \"!^[ \\t]*with[ \\t].*$\\n\"\n+\txfuncname = \"^[ \\t]*((procedure|function)[ \\t]+.*)$\\n\"\n+\txfuncname = \"^[ \\t]*((package|protected|task)[ \\t]+.*)$\"\n+\twordRegex = \"[a-zA-Z][a-zA-Z0-9_]*\"\n+\twordRegex = \"|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?\"\n+\twordRegex = \"|=>|\\\\.\\\\.|\\\\*\\\\*|:=|/=|>=|<=|<<|>>|<>\"\n+\tregIcase = true\n+\n+[diff \"fortran\"]\n+\txfuncname = \"!^([C*]|[ \\t]*!)\\n\"\n+\txfuncname = \"!^[ \\t]*MODULE[ \\t]+PROCEDURE[ \\t]\\n\"\n+\txfuncname = \"^[ \\t]*((END[ \\t]+)?(PROGRAM|MODULE|BLOCK[ \\t]+DATA\"\n+\txfuncname = \"|([^'\\\" \\t]+[ \\t]+)*(SUBROUTINE|FUNCTION))[ \\t]+[A-Z].*)$\"\n+\twordRegex = \"[a-zA-Z][a-zA-Z0-9_]*\"\n+\twordRegex = \"|\\\\.([Ee][Qq]|[Nn][Ee]|[Gg][TtEe]|[Ll][TtEe]|[Tt][Rr][Uu][Ee]|[Ff][Aa][Ll][Ss][Ee]|[Aa][Nn][Dd]|[Oo][Rr]|[Nn]?[Ee][Qq][Vv]|[Nn][Oo][Tt])\\\\.\"\n+\t; numbers and format statements like 2E14.4, or ES12.6, 9X.\n+\t; Don't worry about format statements without leading digits since\n+\t; they would have been matched above as a variable anyway.\n+\twordRegex = \"|[-+]?[0-9.]+([AaIiDdEeFfLlTtXx][Ss]?[-+]?[0-9.]*)?(_[a-zA-Z0-9][a-zA-Z0-9_]*)?\"\n+\twordRegex = \"|//|\\\\*\\\\*|::|[/<>=]=\"\n+\tregIcase = true\n+\n+[diff \"fountain\"]\n+\txfuncname = \"^((\\\\.[^.]|(int|ext|est|int\\\\.?/ext|i/e)[. ]).*)$\"\n+\twordRegex = \"[^ \\t-]+\"\n+\tregIcase = true\n+\n+[diff \"golang\"]\n+\t; Functions\n+\txfuncname = \"^[ \\t]*(func[ \\t]*.*(\\\\{[ \\t]*)?)\\n\"\n+\t; Structs and interfaces\n+\txfuncname = \"^[ \\t]*(type[ \\t].*(struct|interface)[ \\t]*(\\\\{[ \\t]*)?)\"\n+\twordRegex = \"[a-zA-Z_][a-zA-Z0-9_]*\"\n+\twordRegex = \"|[-+0-9.eE]+i?|0[xX]?[0-9a-fA-F]+i?\"\n+\twordRegex = \"|[-+*/<>%&^|=!:]=|--|\\\\+\\\\+|<<=?|>>=?|&\\\\^=?|&&|\\\\|\\\\||<-|\\\\.{3}\"\n+\n+[diff \"html\"]\n+\txfuncname = \"^[ \\t]*(<[Hh][1-6]([ \\t].*)?>.*)$\"\n+\twordRegex = \"[^<>= \\t]+\"\n+\n+[diff \"java\"]\n+\txfuncname = \"!^[ \\t]*(catch|do|for|if|instanceof|new|return|switch|throw|while)\\n\"\n+\txfuncname = \"^[ \\t]*(([A-Za-z_][A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)$\"\n+\twordRegex = \"[a-zA-Z_][a-zA-Z0-9_]*\"\n+\twordRegex = \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n+\twordRegex = \"|[-+*/<>%&^|=!]=\"\n+\twordRegex = \"|--|\\\\+\\\\+|<<=?|>>>?=?|&&|\\\\|\\\\|\"\n+\n+[diff \"matlab\"]\n+\txfuncname = \"^[[:space:]]*((classdef|function)[[:space:]].*)$|^%%[[:space:]].*$\"\n+\twordRegex = \"[a-zA-Z_][a-zA-Z0-9_]*|[-+0-9.e]+|[=~<>]=|\\\\.[*/\\\\^']|\\\\|\\\\||&&\"\n+\n+[diff \"objc\"]\n+\t; Negate C statements that can look like functions\n+\txfuncname = \"!^[ \\t]*(do|for|if|else|return|switch|while)\\n\"\n+\t; Objective-C methods\n+\txfuncname = \"^[ \\t]*([-+][ \\t]*\\\\([ \\t]*[A-Za-z_][A-Za-z_0-9* \\t]*\\\\)[ \\t]*[A-Za-z_].*)$\\n\"\n+\t; C functions\n+\txfuncname = \"^[ \\t]*(([A-Za-z_][A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)$\\n\"\n+\t; Objective-C class/protocol definitions\n+\txfuncname = \"^(@(implementation|interface|protocol)[ \\t].*)$\"\n+\twordRegex = \"[a-zA-Z_][a-zA-Z0-9_]*\"\n+\twordRegex = \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n+\twordRegex = \"|[-+*/<>%&^|=!]=|--|\\\\+\\\\+|<<=?|>>=?|&&|\\\\|\\\\||::|->\"\n+\n+[diff \"pascal\"]\n+\txfuncname = \"^(((class[ \\t]+)?(procedure|function)|constructor|destructor|interface|\"\n+\txfuncname = \"implementation|initialization|finalization)[ \\t]*.*)$\"\n+\txfuncname = \"\\n\"\n+\txfuncname = \"^(.*=[ \\t]*(class|record).*)$\"\n+\twordRegex = \"[a-zA-Z_][a-zA-Z0-9_]*\"\n+\twordRegex = \"|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+\"\n+\twordRegex = \"|<>|<=|>=|:=|\\\\.\\\\.\"\n+\n+[diff \"perl\"]\n+\txfuncname = \"^package .*\\n\"\n+\txfuncname = \"^sub [[:alnum:]_':]+[ \\t]*\"\n+\t\txfuncname = \"(\\\\([^)]*\\\\)[ \\t]*)?\" ; prototype\n+\t\t; Attributes.  A regex can't count nested parentheses,\n+\t\t; so just slurp up whatever we see, taking care not\n+\t\t; to accept lines like \"sub foo; # defined elsewhere\".\n+\t\t;\n+\t\t; An attribute could contain a semicolon, but at that\n+\t\t; point it seems reasonable enough to give up.\n+\t\txfuncname = \"(:[^;#]*)?\"\n+\t\txfuncname = \"(\\\\{[ \\t]*)?\" ; brace can come here or on the next line\n+\t\txfuncname = \"(#.*)?$\\n\" ; comment\n+\txfuncname = \"^(BEGIN|END|INIT|CHECK|UNITCHECK|AUTOLOAD|DESTROY)[ \\t]*\"\n+\t\txfuncname = \"(\\\\{[ \\t]*)?\" ; brace can come here or on the next line\n+\t\txfuncname = \"(#.*)?$\\n\"\n+\txfuncname = \"^=head[0-9] .*\" ; POD\n+\twordRegex = \"[[:alpha:]_'][[:alnum:]_']*\"\n+\twordRegex = \"|0[xb]?[0-9a-fA-F_]*\"\n+\t; taking care not to interpret 3..5 as (3.)(.5)\n+\twordRegex = \"|[0-9a-fA-F_]+(\\\\.[0-9a-fA-F_]+)?([eE][-+]?[0-9_]+)?\"\n+\twordRegex = \"|=>|-[rwxoRWXOezsfdlpSugkbctTBMAC>]|~~|::\"\n+\twordRegex = \"|&&=|\\\\|\\\\|=|//=|\\\\*\\\\*=\"\n+\twordRegex = \"|&&|\\\\|\\\\||//|\\\\+\\\\+|--|\\\\*\\\\*|\\\\.\\\\.\\\\.?\"\n+\twordRegex = \"|[-+*/%.^&<>=!|]=\"\n+\twordRegex = \"|=~|!~\"\n+\twordRegex = \"|<<|<>|<=>|>>\"\n+\n+[diff \"php\"]\n+\txfuncname = \"^[\\t ]*(((public|protected|private|static)[\\t ]+)*function.*)$\\n\"\n+\txfuncname = \"^[\\t ]*((((final|abstract)[\\t ]+)?class|interface|trait).*)$\"\n+\twordRegex = \"[a-zA-Z_][a-zA-Z0-9_]*\"\n+\twordRegex = \"|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+\"\n+\twordRegex = \"|[-+*/<>%&^|=!.]=|--|\\\\+\\\\+|<<=?|>>=?|===|&&|\\\\|\\\\||::|->\"\n+\n+[diff \"python\"]\n+\txfuncname = \"^[ \\t]*((class|def)[ \\t].*)$\"\n+\twordRegex = \"[a-zA-Z_][a-zA-Z0-9_]*\"\n+\twordRegex = \"|[-+0-9.e]+[jJlL]?|0[xX]?[0-9a-fA-F]+[lL]?\"\n+\twordRegex = \"|[-+*/<>%&^|=!]=|//=?|<<=?|>>=?|\\\\*\\\\*=?\"\n+\n+[diff \"ruby\"]\n+\txfuncname = \"^[ \\t]*((class|module|def)[ \\t].*)$\"\n+\twordRegex = \"(@|@@|\\\\$)?[a-zA-Z_][a-zA-Z0-9_]*\"\n+\twordRegex = \"|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+|\\\\?(\\\\\\\\C-)?(\\\\\\\\M-)?.\"\n+\twordRegex = \"|//=?|[-+*/<>%&^|=!]=|<<=?|>>=?|===|\\\\.{1,3}|::|[!=]~\"\n+\n+[diff \"bibtex\"]\n+\txfuncname = \"(@[a-zA-Z]{1,}[ \\t]*\\\\{{0,1}[ \\t]*[^ \\t\\\"@',\\\\#}{~%]*).*$\"\n+\twordRegex = \"[={}\\\"]|[^={}\\\" \\t]+\"\n+\n+[diff \"tex\"]\n+\txfuncname = \"^(\\\\\\\\((sub)*section|chapter|part)\\\\*{0,1}\\\\{.*)$\"\n+\twordRegex = \"\\\\\\\\[a-zA-Z@]+|\\\\\\\\.|[a-zA-Z0-9\\\\x80-\\\\xff]+\"\n+\n+[diff \"cpp\"]\n+\t; Jump targets or access declarations\n+\txfuncname = \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:[[:space:]]*($|/[/*])\\n\"\n+\t; functions/methods, variables, and compounds at top level\n+\txfuncname = \"^((::[[:space:]]*)?[A-Za-z_].*)$\"\n+\twordRegex = \"[a-zA-Z_][a-zA-Z0-9_]*\"\n+\twordRegex = \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lLuU]*\"\n+\twordRegex = \"|[-+*/<>%&^|=!]=|--|\\\\+\\\\+|<<=?|>>=?|&&|\\\\|\\\\||::|->\\\\*?|\\\\.\\\\*\"\n+\n+[diff \"csharp\"]\n+\t; Keywords\n+\txfuncname = \"!^[ \\t]*(do|while|for|if|else|instanceof|new|return|switch|case|throw|catch|using)\\n\"\n+\t; Methods and constructors\n+\txfuncname = \"^[ \\t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe|async)[ \\t]+)*[][<>@.~_[:alnum:]]+[ \\t]+[<>@._[:alnum:]]+[ \\t]*\\\\(.*\\\\))[ \\t]*$\\n\"\n+\t; Properties\n+\txfuncname = \"^[ \\t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe)[ \\t]+)*[][<>@.~_[:alnum:]]+[ \\t]+[@._[:alnum:]]+)[ \\t]*$\\n\"\n+\t; Type definitions\n+\txfuncname = \"^[ \\t]*(((static|public|internal|private|protected|new|unsafe|sealed|abstract|partial)[ \\t]+)*(class|enum|interface|struct)[ \\t]+.*)$\\n\"\n+\t; Namespace\n+\txfuncname = \"^[ \\t]*(namespace[ \\t]+.*)$\"\n+\twordRegex = \"[a-zA-Z_][a-zA-Z0-9_]*\"\n+\twordRegex = \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n+\twordRegex = \"|[-+*/<>%&^|=!]=|--|\\\\+\\\\+|<<=?|>>=?|&&|\\\\|\\\\||::|->\"\n+\n+[diff \"css\"]\n+\txfuncname = \"![:;][[:space:]]*$\\n\"\n+\txfuncname = \"^[_a-z0-9].*$\"\n+\t; This regex comes from W3C CSS specs. Should theoretically also\n+\t; allow ISO 10646 characters U+00A0 and higher,\n+\t; but they are not handled in this regex.\n+\twordRegex = \"-?[_a-zA-Z][-_a-zA-Z0-9]*\" ; identifiers\n+\twordRegex = \"|-?[0-9]+|\\\\#[0-9a-fA-F]+\" ; numbers\n+\tregIcase = true\ndiff --git a/userdiff.c b/userdiff.c\nindex 3a78fbf504..3e7052e13c 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -2,178 +2,121 @@\n #include \"config.h\"\n #include \"userdiff.h\"\n #include \"attr.h\"\n+#include \"exec-cmd.h\"\n+#include \"repository.h\"\n \n static struct userdiff_driver *drivers;\n static int ndrivers;\n static int drivers_alloc;\n+static struct config_set gm_config;\n+static int config_init;\n+struct userdiff_driver *builtin_drivers;\n+static int builtin_drivers_size;\n \n-#define PATTERNS(name, pattern, word_regex)\t\t\t\\\n-\t{ name, NULL, -1, { pattern, REG_EXTENDED },\t\t\\\n-\t  word_regex \"|[^[:space:]]|[\\xc0-\\xff][\\x80-\\xbf]+\" }\n-#define IPATTERN(name, pattern, word_regex)\t\t\t\\\n-\t{ name, NULL, -1, { pattern, REG_EXTENDED | REG_ICASE }, \\\n-\t  word_regex \"|[^[:space:]]|[\\xc0-\\xff][\\x80-\\xbf]+\" }\n-static struct userdiff_driver builtin_drivers[] = {\n-IPATTERN(\"ada\",\n-\t \"!^(.*[ \\t])?(is[ \\t]+new|renames|is[ \\t]+separate)([ \\t].*)?$\\n\"\n-\t \"!^[ \\t]*with[ \\t].*$\\n\"\n-\t \"^[ \\t]*((procedure|function)[ \\t]+.*)$\\n\"\n-\t \"^[ \\t]*((package|protected|task)[ \\t]+.*)$\",\n-\t /* -- */\n-\t \"[a-zA-Z][a-zA-Z0-9_]*\"\n-\t \"|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?\"\n-\t \"|=>|\\\\.\\\\.|\\\\*\\\\*|:=|/=|>=|<=|<<|>>|<>\"),\n-IPATTERN(\"fortran\",\n-\t \"!^([C*]|[ \\t]*!)\\n\"\n-\t \"!^[ \\t]*MODULE[ \\t]+PROCEDURE[ \\t]\\n\"\n-\t \"^[ \\t]*((END[ \\t]+)?(PROGRAM|MODULE|BLOCK[ \\t]+DATA\"\n-\t\t\"|([^'\\\" \\t]+[ \\t]+)*(SUBROUTINE|FUNCTION))[ \\t]+[A-Z].*)$\",\n-\t /* -- */\n-\t \"[a-zA-Z][a-zA-Z0-9_]*\"\n-\t \"|\\\\.([Ee][Qq]|[Nn][Ee]|[Gg][TtEe]|[Ll][TtEe]|[Tt][Rr][Uu][Ee]|[Ff][Aa][Ll][Ss][Ee]|[Aa][Nn][Dd]|[Oo][Rr]|[Nn]?[Ee][Qq][Vv]|[Nn][Oo][Tt])\\\\.\"\n-\t /* numbers and format statements like 2E14.4, or ES12.6, 9X.\n-\t  * Don't worry about format statements without leading digits since\n-\t  * they would have been matched above as a variable anyway. */\n-\t \"|[-+]?[0-9.]+([AaIiDdEeFfLlTtXx][Ss]?[-+]?[0-9.]*)?(_[a-zA-Z0-9][a-zA-Z0-9_]*)?\"\n-\t \"|//|\\\\*\\\\*|::|[/<>=]=\"),\n-IPATTERN(\"fountain\", \"^((\\\\.[^.]|(int|ext|est|int\\\\.?/ext|i/e)[. ]).*)$\",\n-\t \"[^ \\t-]+\"),\n-PATTERNS(\"golang\",\n-\t /* Functions */\n-\t \"^[ \\t]*(func[ \\t]*.*(\\\\{[ \\t]*)?)\\n\"\n-\t /* Structs and interfaces */\n-\t \"^[ \\t]*(type[ \\t].*(struct|interface)[ \\t]*(\\\\{[ \\t]*)?)\",\n-\t /* -- */\n-\t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n-\t \"|[-+0-9.eE]+i?|0[xX]?[0-9a-fA-F]+i?\"\n-\t \"|[-+*/<>%&^|=!:]=|--|\\\\+\\\\+|<<=?|>>=?|&\\\\^=?|&&|\\\\|\\\\||<-|\\\\.{3}\"),\n-PATTERNS(\"html\", \"^[ \\t]*(<[Hh][1-6]([ \\t].*)?>.*)$\",\n-\t \"[^<>= \\t]+\"),\n-PATTERNS(\"java\",\n-\t \"!^[ \\t]*(catch|do|for|if|instanceof|new|return|switch|throw|while)\\n\"\n-\t \"^[ \\t]*(([A-Za-z_][A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)$\",\n-\t /* -- */\n-\t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n-\t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n-\t \"|[-+*/<>%&^|=!]=\"\n-\t \"|--|\\\\+\\\\+|<<=?|>>>?=?|&&|\\\\|\\\\|\"),\n-PATTERNS(\"matlab\",\n-\t \"^[[:space:]]*((classdef|function)[[:space:]].*)$|^%%[[:space:]].*$\",\n-\t \"[a-zA-Z_][a-zA-Z0-9_]*|[-+0-9.e]+|[=~<>]=|\\\\.[*/\\\\^']|\\\\|\\\\||&&\"),\n-PATTERNS(\"objc\",\n-\t /* Negate C statements that can look like functions */\n-\t \"!^[ \\t]*(do|for|if|else|return|switch|while)\\n\"\n-\t /* Objective-C methods */\n-\t \"^[ \\t]*([-+][ \\t]*\\\\([ \\t]*[A-Za-z_][A-Za-z_0-9* \\t]*\\\\)[ \\t]*[A-Za-z_].*)$\\n\"\n-\t /* C functions */\n-\t \"^[ \\t]*(([A-Za-z_][A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)$\\n\"\n-\t /* Objective-C class/protocol definitions */\n-\t \"^(@(implementation|interface|protocol)[ \\t].*)$\",\n-\t /* -- */\n-\t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n-\t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n-\t \"|[-+*/<>%&^|=!]=|--|\\\\+\\\\+|<<=?|>>=?|&&|\\\\|\\\\||::|->\"),\n-PATTERNS(\"pascal\",\n-\t \"^(((class[ \\t]+)?(procedure|function)|constructor|destructor|interface|\"\n-\t\t\"implementation|initialization|finalization)[ \\t]*.*)$\"\n-\t \"\\n\"\n-\t \"^(.*=[ \\t]*(class|record).*)$\",\n-\t /* -- */\n-\t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n-\t \"|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+\"\n-\t \"|<>|<=|>=|:=|\\\\.\\\\.\"),\n-PATTERNS(\"perl\",\n-\t \"^package .*\\n\"\n-\t \"^sub [[:alnum:]_':]+[ \\t]*\"\n-\t\t\"(\\\\([^)]*\\\\)[ \\t]*)?\" /* prototype */\n-\t\t/*\n-\t\t * Attributes.  A regex can't count nested parentheses,\n-\t\t * so just slurp up whatever we see, taking care not\n-\t\t * to accept lines like \"sub foo; # defined elsewhere\".\n-\t\t *\n-\t\t * An attribute could contain a semicolon, but at that\n-\t\t * point it seems reasonable enough to give up.\n-\t\t */\n-\t\t\"(:[^;#]*)?\"\n-\t\t\"(\\\\{[ \\t]*)?\" /* brace can come here or on the next line */\n-\t\t\"(#.*)?$\\n\" /* comment */\n-\t \"^(BEGIN|END|INIT|CHECK|UNITCHECK|AUTOLOAD|DESTROY)[ \\t]*\"\n-\t\t\"(\\\\{[ \\t]*)?\" /* brace can come here or on the next line */\n-\t\t\"(#.*)?$\\n\"\n-\t \"^=head[0-9] .*\",\t/* POD */\n-\t /* -- */\n-\t \"[[:alpha:]_'][[:alnum:]_']*\"\n-\t \"|0[xb]?[0-9a-fA-F_]*\"\n-\t /* taking care not to interpret 3..5 as (3.)(.5) */\n-\t \"|[0-9a-fA-F_]+(\\\\.[0-9a-fA-F_]+)?([eE][-+]?[0-9_]+)?\"\n-\t \"|=>|-[rwxoRWXOezsfdlpSugkbctTBMAC>]|~~|::\"\n-\t \"|&&=|\\\\|\\\\|=|//=|\\\\*\\\\*=\"\n-\t \"|&&|\\\\|\\\\||//|\\\\+\\\\+|--|\\\\*\\\\*|\\\\.\\\\.\\\\.?\"\n-\t \"|[-+*/%.^&<>=!|]=\"\n-\t \"|=~|!~\"\n-\t \"|<<|<>|<=>|>>\"),\n-PATTERNS(\"php\",\n-\t \"^[\\t ]*(((public|protected|private|static)[\\t ]+)*function.*)$\\n\"\n-\t \"^[\\t ]*((((final|abstract)[\\t ]+)?class|interface|trait).*)$\",\n-\t /* -- */\n-\t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n-\t \"|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+\"\n-\t \"|[-+*/<>%&^|=!.]=|--|\\\\+\\\\+|<<=?|>>=?|===|&&|\\\\|\\\\||::|->\"),\n-PATTERNS(\"python\", \"^[ \\t]*((class|def)[ \\t].*)$\",\n-\t /* -- */\n-\t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n-\t \"|[-+0-9.e]+[jJlL]?|0[xX]?[0-9a-fA-F]+[lL]?\"\n-\t \"|[-+*/<>%&^|=!]=|//=?|<<=?|>>=?|\\\\*\\\\*=?\"),\n-\t /* -- */\n-PATTERNS(\"ruby\", \"^[ \\t]*((class|module|def)[ \\t].*)$\",\n-\t /* -- */\n-\t \"(@|@@|\\\\$)?[a-zA-Z_][a-zA-Z0-9_]*\"\n-\t \"|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+|\\\\?(\\\\\\\\C-)?(\\\\\\\\M-)?.\"\n-\t \"|//=?|[-+*/<>%&^|=!]=|<<=?|>>=?|===|\\\\.{1,3}|::|[!=]~\"),\n-PATTERNS(\"bibtex\", \"(@[a-zA-Z]{1,}[ \\t]*\\\\{{0,1}[ \\t]*[^ \\t\\\"@',\\\\#}{~%]*).*$\",\n-\t \"[={}\\\"]|[^={}\\\" \\t]+\"),\n-PATTERNS(\"tex\", \"^(\\\\\\\\((sub)*section|chapter|part)\\\\*{0,1}\\\\{.*)$\",\n-\t \"\\\\\\\\[a-zA-Z@]+|\\\\\\\\.|[a-zA-Z0-9\\x80-\\xff]+\"),\n-PATTERNS(\"cpp\",\n-\t /* Jump targets or access declarations */\n-\t \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:[[:space:]]*($|/[/*])\\n\"\n-\t /* functions/methods, variables, and compounds at top level */\n-\t \"^((::[[:space:]]*)?[A-Za-z_].*)$\",\n-\t /* -- */\n-\t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n-\t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lLuU]*\"\n-\t \"|[-+*/<>%&^|=!]=|--|\\\\+\\\\+|<<=?|>>=?|&&|\\\\|\\\\||::|->\\\\*?|\\\\.\\\\*\"),\n-PATTERNS(\"csharp\",\n-\t /* Keywords */\n-\t \"!^[ \\t]*(do|while|for|if|else|instanceof|new|return|switch|case|throw|catch|using)\\n\"\n-\t /* Methods and constructors */\n-\t \"^[ \\t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe|async)[ \\t]+)*[][<>@.~_[:alnum:]]+[ \\t]+[<>@._[:alnum:]]+[ \\t]*\\\\(.*\\\\))[ \\t]*$\\n\"\n-\t /* Properties */\n-\t \"^[ \\t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe)[ \\t]+)*[][<>@.~_[:alnum:]]+[ \\t]+[@._[:alnum:]]+)[ \\t]*$\\n\"\n-\t /* Type definitions */\n-\t \"^[ \\t]*(((static|public|internal|private|protected|new|unsafe|sealed|abstract|partial)[ \\t]+)*(class|enum|interface|struct)[ \\t]+.*)$\\n\"\n-\t /* Namespace */\n-\t \"^[ \\t]*(namespace[ \\t]+.*)$\",\n-\t /* -- */\n-\t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n-\t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n-\t \"|[-+*/<>%&^|=!]=|--|\\\\+\\\\+|<<=?|>>=?|&&|\\\\|\\\\||::|->\"),\n-IPATTERN(\"css\",\n-\t \"![:;][[:space:]]*$\\n\"\n-\t \"^[_a-z0-9].*$\",\n-\t /* -- */\n-\t /*\n-\t  * This regex comes from W3C CSS specs. Should theoretically also\n-\t  * allow ISO 10646 characters U+00A0 and higher,\n-\t  * but they are not handled in this regex.\n-\t  */\n-\t \"-?[_a-zA-Z][-_a-zA-Z0-9]*\" /* identifiers */\n-\t \"|-?[0-9]+|\\\\#[0-9a-fA-F]+\" /* numbers */\n-),\n-{ \"default\", NULL, -1, { NULL, 0 } },\n-};\n-#undef PATTERNS\n-#undef IPATTERN\n+static int userdiff_config_init(void)\n+{\n+\tint ret = -1;\n+\tif (!config_init) {\n+\t\tgit_configset_init(&gm_config);\n+\t\tif (the_repository && the_repository->gitdir)\n+\t\t\tret = git_configset_add_file(&gm_config, git_pathdup(\"userdiff\"));\n+\n+\t\t// if .git/userdiff does not exist, set config_init to be -1\n+\t\tif (ret == 0)\n+\t\t\tconfig_init = 1;\n+\t\telse\n+\t\t\tconfig_init = -1;\n+\n+\t\tbuiltin_drivers = (struct userdiff_driver *) malloc(sizeof(struct userdiff_driver));\n+\t\t*builtin_drivers = (struct userdiff_driver) { \"default\", NULL, -1, { NULL, 0 } };\n+\t\tbuiltin_drivers_size = 1;\n+\t}\n+\treturn 0;\n+}\n+\n+static char* join_strings(const struct string_list *strings)\n+{\n+\tchar* str;\n+\tint i, len, length = 0;\n+\tif (!strings)\n+\t\treturn NULL;\n+\n+\tfor (i = 0; i < strings->nr; i++)\n+\t\tlength += strlen(strings->items[i].string);\n+\n+\tstr = (char *) malloc(length + 1);\n+\tlength = 0;\n+\n+\tfor (i = 0; i < strings->nr; i++) {\n+\t\tlen = strlen(strings->items[i].string);\n+\t\tmemcpy(str + length, strings->items[i].string, len);\n+\t\tlength += len;\n+\t}\n+\tstr[length] = '\\0';\n+\treturn str;\n+}\n+\n+static struct userdiff_driver *userdiff_find_builtin_by_namelen(const char *k, int len)\n+{\n+\tint i, key_length, word_regex_size, ret, reg_icase, cflags;\n+\tchar *xfuncname_key, *word_regex_key, *ipattern_key;\n+\tchar *xfuncname_value, *word_regex_value, *word_regex, *name;\n+\tstruct userdiff_driver *builtin_driver;\n+\tchar word_regex_extra[] = \"|[^[:space:]]|[\\xc0-\\xff][\\x80-\\xbf]+\";\n+\tuserdiff_config_init();\n+\tname = (char *) malloc(len + 1);\n+\tmemcpy(name, k, len);\n+\tname[len] = '\\0';\n+\n+\t// look up builtin_driver\n+\tfor (i = 0; i < builtin_drivers_size; i++) {\n+\t\tstruct userdiff_driver *drv = builtin_drivers + i;\n+\t\tif (!strncmp(drv->name, name, len) && !drv->name[len])\n+\t\t\treturn drv;\n+\t}\n+\n+\t// if .git/userdiff does not exist and name is not \"default\", return NULL\n+\tif (config_init == -1) {\n+\t\treturn NULL;\n+\t}\n+\n+\t// load xfuncname and wordRegex from userdiff config file\n+\tkey_length = len + 16;\n+\txfuncname_key = (char *) malloc(key_length);\n+\tword_regex_key = (char *) malloc(key_length);\n+\tipattern_key = (char *) malloc(key_length - 1);\n+\tsnprintf(xfuncname_key, key_length, \"diff.%s.xfuncname\", name);\n+\tsnprintf(word_regex_key, key_length, \"diff.%s.wordRegex\", name);\n+\tsnprintf(ipattern_key, key_length - 1, \"diff.%s.regIcase\", name);\n+\n+\txfuncname_value = join_strings(git_configset_get_value_multi(&gm_config, xfuncname_key));\n+\tword_regex_value = join_strings(git_configset_get_value_multi(&gm_config, word_regex_key));\n+\n+\tret = git_configset_get_bool(&gm_config, ipattern_key, &reg_icase);\n+\t// if \"regIcase\" is not found, do not use REG_ICASE flag\n+\tif (ret == 1)\n+\t\treg_icase = 0;\n+\tcflags = reg_icase ? REG_EXTENDED | REG_ICASE : REG_EXTENDED;\n+\n+\tfree(xfuncname_key);\n+\tfree(word_regex_key);\n+\tfree(ipattern_key);\n+\n+\tif (!xfuncname_value || !word_regex_value)\n+\t\treturn NULL;\n+\n+\tword_regex_size = strlen(word_regex_value) + strlen(word_regex_extra) + 1;\n+\tword_regex = (char *) malloc(word_regex_size);\n+\tsnprintf(word_regex, word_regex_size,\n+\t\t\t\"%s%s\", word_regex_value, word_regex_extra);\n+\n+\tbuiltin_drivers_size++;\n+\tbuiltin_drivers = realloc(builtin_drivers, builtin_drivers_size * sizeof(struct userdiff_driver));\n+\tbuiltin_driver = builtin_drivers + builtin_drivers_size - 1;\n+\t*builtin_driver = (struct userdiff_driver) {\n+\t\t\tname, NULL, -1, { xfuncname_value, cflags }, word_regex };\n+\treturn builtin_driver;\n+}\n \n static struct userdiff_driver driver_true = {\n \t\"diff=true\",\n@@ -197,12 +140,7 @@ static struct userdiff_driver *userdiff_find_by_namelen(const char *k, int len)\n \t\tif (!strncmp(drv->name, k, len) && !drv->name[len])\n \t\t\treturn drv;\n \t}\n-\tfor (i = 0; i < ARRAY_SIZE(builtin_drivers); i++) {\n-\t\tstruct userdiff_driver *drv = builtin_drivers + i;\n-\t\tif (!strncmp(drv->name, k, len) && !drv->name[len])\n-\t\t\treturn drv;\n-\t}\n-\treturn NULL;\n+\treturn userdiff_find_builtin_by_namelen(k, len);\n }\n \n static int parse_funcname(struct userdiff_funcname *f, const char *k,\n-- \n2.21.0 (Apple Git-120)\n\n"},{"id":"377458","messageId":"488f215a-cb32-db3f-4a45-0062f0f0a0a9@kdbg.org","threadId":"51329","inReplyTo":"20190617165450.81916-1-liboxuan@connect.hku.hk","subject":"Re: [RFC PATCH] userdiff: ship built-in driver config file","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-06-18T20:32:47Z","receivedAt":"2019-06-18T20:32:53Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.06.19 um 18:54 schrieb Boxuan Li:\n> The userdiff.c has been rewritten to avoid hard-coded built-in\n> driver patterns. Now we ship\n> $(sharedir)/git-core/templates/userdiff that can be read using\n> git_config_from_file() interface, using a very narrow callback\n> function that understands only diff.*.xfuncname,\n> diff.*.wordregex, and diff.*.regIcase.\n> \n> Signed-off-by: Boxuan Li <liboxuan@connect.hku.hk>\n> ---\n> A few notes and questions:\n> 1. In [diff \"tex\"] section, \\x80 and \\xff cannot be parsed by git config parser.\n> I have no idea why this is happening. I changed them to \\\\x80 and \\\\xff as a workaround, which\n> resulted in t4034 failure (See https://travis-ci.org/li-boxuan/git/jobs/546729906#L4679).\n\nI guess, the idea is to catch bytes of UTF-8 encoded characters as\nregular words.\n\nThe problem is to write such bytes literally into a git-config file and\nstill keep the file editable in a portable. Perhaps it is necessary to\ndeclare the file as CP1252 encoded via .gitattributes, write that part\nof the regexp as [a-zA-Z0-9€-þ], and hope that your text editor writes\nthe file acutally as CP1252. ISO8859-1 does not work because \\x80 is not\noccupied.\n\n> 2. I am not sure how and where I can free the memory allocated to \"builtin_drivers\".\n> 3. When I run `git format-patch HEAD~1`, core dump happens occasionally. Seems\n> no test case caught this problem. Till now, I have no luck finding out the reason.\n\nI admit that haven't tested the driver beyond running t4018 and t4034.\n\n> \n> Any hint or review would be appreciated.\n> ---\n>  templates/this--userdiff | 164 ++++++++++++++++++++++\n>  userdiff.c               | 284 +++++++++++++++------------------------\n>  2 files changed, 275 insertions(+), 173 deletions(-)\n>  create mode 100644 templates/this--userdiff\n> \n> diff --git a/templates/this--userdiff b/templates/this--userdiff\n> new file mode 100644\n> index 0000000000..85114a7229\n> --- /dev/null\n> +++ b/templates/this--userdiff\n\nWhy place this file in .git? To have per-repository diff drivers, we can\nalready specify them via 'git config'. This file should be installed in\nthe system.\n\n> @@ -0,0 +1,164 @@\n> +[diff \"ada\"]\n\netc... Please be aware that there are a few changes in 'next' that\naffect this patch, in particular, the matlab pattern and new rust patterns.\n\n> diff --git a/userdiff.c b/userdiff.c\n> index 3a78fbf504..3e7052e13c 100644\n> --- a/userdiff.c\n> +++ b/userdiff.c\n\n>  static struct userdiff_driver *drivers;\n>  static int ndrivers;\n>  static int drivers_alloc;\n> +static struct config_set gm_config;\n> +static int config_init;\n> +struct userdiff_driver *builtin_drivers;\n> +static int builtin_drivers_size;\n\nWhy do you not merge the builtin drivers with the other drivers? If\nthere is a reason to separate the two classes, please follow the\nexisting pattern to use ALLOC_GROW to reallocate the array.\n\n> +static int userdiff_config_init(void)\n> +{\n> +\tint ret = -1;\n> +\tif (!config_init) {\n\nPlease make this an early return to reduce the indentation of the\nsubsequent code.\n\n> +\t\tgit_configset_init(&gm_config);\n> +\t\tif (the_repository && the_repository->gitdir)\n> +\t\t\tret = git_configset_add_file(&gm_config, git_pathdup(\"userdiff\"));\n> +\n> +\t\t// if .git/userdiff does not exist, set config_init to be -1\n\nPlease do not use C++ style comments.\n\n> +\t\tif (ret == 0)\n> +\t\t\tconfig_init = 1;\n> +\t\telse\n> +\t\t\tconfig_init = -1;\n\nAfter having done the initialization, it should be irrelevant whether\nthe driver list was not found. So, config_init = 1; should be the only\nrelevant case. Am I missing something?\n\n> +\n> +\t\tbuiltin_drivers = (struct userdiff_driver *) malloc(sizeof(struct userdiff_driver));\n\nPlease do not use a cast here. It is unnecessary.\nPlease use xmalloc, which checks for an allocation failure.\nI'm not going to repeat this for all other occurrences.\n\n> +\t\t*builtin_drivers = (struct userdiff_driver) { \"default\", NULL, -1, { NULL, 0 } };\n\nI don't think we use this modern (GNU?) form of struct constants\nanywhere already.\n\n> +\t\tbuiltin_drivers_size = 1;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +static char* join_strings(const struct string_list *strings)\n> +{\n> +\tchar* str;\n> +\tint i, len, length = 0;\n> +\tif (!strings)\n> +\t\treturn NULL;\n> +\n> +\tfor (i = 0; i < strings->nr; i++)\n> +\t\tlength += strlen(strings->items[i].string);\n> +\n> +\tstr = (char *) malloc(length + 1);\n> +\tlength = 0;\n> +\n> +\tfor (i = 0; i < strings->nr; i++) {\n> +\t\tlen = strlen(strings->items[i].string);\n> +\t\tmemcpy(str + length, strings->items[i].string, len);\n> +\t\tlength += len;\n> +\t}\n> +\tstr[length] = '\\0';\n> +\treturn str;\n> +}\n\nIf you use the strbuf API instead of raw strings and\nfor_each_string_list_item, I'm sure you can boil this down to just a\nhandful of lines.\n\n> +\n> +static struct userdiff_driver *userdiff_find_builtin_by_namelen(const char *k, int len)\n> +{\n> +\tint i, key_length, word_regex_size, ret, reg_icase, cflags;\n> +\tchar *xfuncname_key, *word_regex_key, *ipattern_key;\n> +\tchar *xfuncname_value, *word_regex_value, *word_regex, *name;\n> +\tstruct userdiff_driver *builtin_driver;\n> +\tchar word_regex_extra[] = \"|[^[:space:]]|[\\xc0-\\xff][\\x80-\\xbf]+\";\n\nAha! Have a look at 664d44ee7fb1 (\"userdiff: simplify word-diff\nsafeguard\", 2011-01-11). Perhaps the special part in the TeX pattern\nshould just be removed (in a preparatory patch). It would change the\nmeaning because it would treat runs of digits and letters as separate\nwords, but I don't think that will hurt.\n\n> +\tuserdiff_config_init();\n> +\tname = (char *) malloc(len + 1);\n> +\tmemcpy(name, k, len);\n> +\tname[len] = '\\0';\n\nxmemdupz?\n\n> +\n> +\t// look up builtin_driver\n> +\tfor (i = 0; i < builtin_drivers_size; i++) {\n> +\t\tstruct userdiff_driver *drv = builtin_drivers + i;\n> +\t\tif (!strncmp(drv->name, name, len) && !drv->name[len])\n> +\t\t\treturn drv;\n> +\t}\n> +\n> +\t// if .git/userdiff does not exist and name is not \"default\", return NULL\n> +\tif (config_init == -1) {\n> +\t\treturn NULL;\n> +\t}\n\nI wonder why you look up a driver before initialization.\n\n> +\n> +\t// load xfuncname and wordRegex from userdiff config file\n> +\tkey_length = len + 16;\n> +\txfuncname_key = (char *) malloc(key_length);\n> +\tword_regex_key = (char *) malloc(key_length);\n> +\tipattern_key = (char *) malloc(key_length - 1);\n> +\tsnprintf(xfuncname_key, key_length, \"diff.%s.xfuncname\", name);\n> +\tsnprintf(word_regex_key, key_length, \"diff.%s.wordRegex\", name);\n> +\tsnprintf(ipattern_key, key_length - 1, \"diff.%s.regIcase\", name);\n> +\n> +\txfuncname_value = join_strings(git_configset_get_value_multi(&gm_config, xfuncname_key));\n> +\tword_regex_value = join_strings(git_configset_get_value_multi(&gm_config, word_regex_key));\n\nI'm not familiar with the git_config API. Can't comment on what this is\nall about.\n\n> +\n> +\tret = git_configset_get_bool(&gm_config, ipattern_key, &reg_icase);\n> +\t// if \"regIcase\" is not found, do not use REG_ICASE flag\n> +\tif (ret == 1)\n> +\t\treg_icase = 0;\n> +\tcflags = reg_icase ? REG_EXTENDED | REG_ICASE : REG_EXTENDED;\n> +\n> +\tfree(xfuncname_key);\n> +\tfree(word_regex_key);\n> +\tfree(ipattern_key);\n> +\n> +\tif (!xfuncname_value || !word_regex_value)\n> +\t\treturn NULL;\n> +\n> +\tword_regex_size = strlen(word_regex_value) + strlen(word_regex_extra) + 1;\n> +\tword_regex = (char *) malloc(word_regex_size);\n> +\tsnprintf(word_regex, word_regex_size,\n> +\t\t\t\"%s%s\", word_regex_value, word_regex_extra);\n> +\n> +\tbuiltin_drivers_size++;\n> +\tbuiltin_drivers = realloc(builtin_drivers, builtin_drivers_size * sizeof(struct userdiff_driver));\n\nThis is where you should use ALLOC_GROW.\n\n> +\tbuiltin_driver = builtin_drivers + builtin_drivers_size - 1;\n> +\t*builtin_driver = (struct userdiff_driver) {\n> +\t\t\tname, NULL, -1, { xfuncname_value, cflags }, word_regex };\n> +\treturn builtin_driver;\n> +}\n\nSo, after having read through the whole patch, I understand that you are\nusing the builtin_drivers just as cache.\n\nThat is not how I initially thought it would work. IMO, you should just\nslurp in all of the builtin drivers and stash them away once during\ninitialization. Then it is not necessary to parse the file more than once.\n\n>  \n>  static struct userdiff_driver driver_true = {\n>  \t\"diff=true\",\n> @@ -197,12 +140,7 @@ static struct userdiff_driver *userdiff_find_by_namelen(const char *k, int len)\n>  \t\tif (!strncmp(drv->name, k, len) && !drv->name[len])\n>  \t\t\treturn drv;\n>  \t}\n> -\tfor (i = 0; i < ARRAY_SIZE(builtin_drivers); i++) {\n> -\t\tstruct userdiff_driver *drv = builtin_drivers + i;\n> -\t\tif (!strncmp(drv->name, k, len) && !drv->name[len])\n> -\t\t\treturn drv;\n> -\t}\n> -\treturn NULL;\n> +\treturn userdiff_find_builtin_by_namelen(k, len);\n>  }\n\nI hate functions with this layout:\n\nfun()\n{\n    loop {\n       stuff;\n    }\n    something_else();\n}\n\nThe preferred layout is, IMO:\n\ndo_stuff()\n{\n    loop {\n       stuff;\n    }\n}\nfun()\n{\n     do_stuff();\n     something_else();\n}\n\nOr (less preferable) expand something_else() in the function.\n\nIn this case, the goal could be:\n\nstatic struct userdiff_driver *userdiff_find_by_namelen1(...)\n{\n\t...lookup loop comes here...\n}\n\nstatic struct userdiff_driver *userdiff_find_by_namelen(const char *k,\nint len)\n{\n\tstruct userdiff_driver *drv;\n\tdrv = userdiff_find_by_namelen1(drivers, drivers_alloc);\n\tif (drv)\n\t\treturn drv;\n\tif (!config_init)\n\t\tuserdiff_config_init();\n\treturn userdiff_find_by_namelen1(builtin_drivers, builtin_drivers_size);\n}\n\nNeedless to say that userdiff_config_init() should parse the file and\nstash away all the drivers it finds.\n\n-- Hannes\n"},{"id":"377459","messageId":"1856d6bc-2cc5-6759-0ed9-8e4d46b6cc47@kdbg.org","threadId":"51329","inReplyTo":"488f215a-cb32-db3f-4a45-0062f0f0a0a9@kdbg.org","subject":"Re: [RFC PATCH] userdiff: ship built-in driver config file","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-06-18T20:50:21Z","receivedAt":"2019-06-18T20:50:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 18.06.19 um 22:32 schrieb Johannes Sixt:\n> Am 17.06.19 um 18:54 schrieb Boxuan Li:\n>> The userdiff.c has been rewritten to avoid hard-coded built-in\n>> driver patterns. Now we ship\n>> $(sharedir)/git-core/templates/userdiff that can be read using\n>> git_config_from_file() interface, using a very narrow callback\n>> function that understands only diff.*.xfuncname,\n>> diff.*.wordregex, and diff.*.regIcase.\n\nI forgot to thank you for working on this. It is much appreciated.\n\n>>\n>> Signed-off-by: Boxuan Li <liboxuan@connect.hku.hk>\n>> ---\n>> A few notes and questions:\n>> 1. In [diff \"tex\"] section, \\x80 and \\xff cannot be parsed by git config parser.\n>> I have no idea why this is happening. I changed them to \\\\x80 and \\\\xff as a workaround, which\n>> resulted in t4034 failure (See https://travis-ci.org/li-boxuan/git/jobs/546729906#L4679).\n> \n> I guess, the idea is...\n\nI think you noticed that I read the patch top-to-bottom and understood\nthe gist of it only near the end.\n\n-- Hannes\n"},{"id":"377481","messageId":"20190619034929.GA515@sigill.intra.peff.net","threadId":"51329","inReplyTo":"488f215a-cb32-db3f-4a45-0062f0f0a0a9@kdbg.org","subject":"Re: [RFC PATCH] userdiff: ship built-in driver config file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-19T03:49:29Z","receivedAt":"2019-06-19T03:51:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 18, 2019 at 10:32:47PM +0200, Johannes Sixt wrote:\n\n> Am 17.06.19 um 18:54 schrieb Boxuan Li:\n> > The userdiff.c has been rewritten to avoid hard-coded built-in\n> > driver patterns. Now we ship\n> > $(sharedir)/git-core/templates/userdiff that can be read using\n> > git_config_from_file() interface, using a very narrow callback\n> > function that understands only diff.*.xfuncname,\n> > diff.*.wordregex, and diff.*.regIcase.\n> > \n> > Signed-off-by: Boxuan Li <liboxuan@connect.hku.hk>\n> > ---\n> > A few notes and questions:\n> > 1. In [diff \"tex\"] section, \\x80 and \\xff cannot be parsed by git config parser.\n> > I have no idea why this is happening. I changed them to \\\\x80 and \\\\xff as a workaround, which\n> > resulted in t4034 failure (See https://travis-ci.org/li-boxuan/git/jobs/546729906#L4679).\n> \n> I guess, the idea is to catch bytes of UTF-8 encoded characters as\n> regular words.\n> \n> The problem is to write such bytes literally into a git-config file and\n> still keep the file editable in a portable. Perhaps it is necessary to\n> declare the file as CP1252 encoded via .gitattributes, write that part\n> of the regexp as [a-zA-Z0-9€-þ], and hope that your text editor writes\n> the file acutally as CP1252. ISO8859-1 does not work because \\x80 is not\n> occupied.\n\nWe don't allow octal or hex escapes in config values, though we do allow\ncommon C ones like \"\\n\". Maybe we should support them.\n\nI didn't check whether we actually want the raw bytes here ourselves,\nthough, or are happy making sure the backslashed forms make it into the\nregex parser (it _seems_ like the latter would be what we want, but that\ndoes mean escaping the backslashes so they make it through the config\nparser literally).\n\n> > diff --git a/templates/this--userdiff b/templates/this--userdiff\n> > new file mode 100644\n> > index 0000000000..85114a7229\n> > --- /dev/null\n> > +++ b/templates/this--userdiff\n> \n> Why place this file in .git? To have per-repository diff drivers, we can\n> already specify them via 'git config'. This file should be installed in\n> the system.\n\nI think it _could_ actually just be part of the system /etc/gitconfig,\nthough it is kind of big, and Git has a tendency to parse the config\nmore than necessary. I wonder if would add a noticeable slowdown.\n\n-Peff\n"},{"id":"377482","messageId":"20190619035857.GB515@sigill.intra.peff.net","threadId":"51329","inReplyTo":"20190617165450.81916-1-liboxuan@connect.hku.hk","subject":"Re: [RFC PATCH] userdiff: ship built-in driver config file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-19T03:58:58Z","receivedAt":"2019-06-19T03:59:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 18, 2019 at 12:54:50AM +0800, Boxuan Li wrote:\n\n> A few notes and questions:\n> 1. In [diff \"tex\"] section, \\x80 and \\xff cannot be parsed by git config parser.\n> I have no idea why this is happening. I changed them to \\\\x80 and \\\\xff as a workaround, which\n> resulted in t4034 failure (See https://travis-ci.org/li-boxuan/git/jobs/546729906#L4679).\n> 2. I am not sure how and where I can free the memory allocated to \"builtin_drivers\".\n> 3. When I run `git format-patch HEAD~1`, core dump happens occasionally. Seems\n> no test case caught this problem. Till now, I have no luck finding out the reason.\n\nI couldn't replicate it with a simple test, but perhaps running under\nvalgrind or \"make SANITIZE=address\" would help?\n\n> diff --git a/templates/this--userdiff b/templates/this--userdiff\n> new file mode 100644\n> index 0000000000..85114a7229\n> --- /dev/null\n> +++ b/templates/this--userdiff\n> @@ -0,0 +1,164 @@\n> +[diff \"ada\"]\n> +\txfuncname = \"!^(.*[ \\t])?(is[ \\t]+new|renames|is[ \\t]+separate)([ \\t].*)?$\\n\"\n> +\txfuncname = \"!^[ \\t]*with[ \\t].*$\\n\"\n> +\txfuncname = \"^[ \\t]*((procedure|function)[ \\t]+.*)$\\n\"\n> +\txfuncname = \"^[ \\t]*((package|protected|task)[ \\t]+.*)$\"\n\nWhile having separate lines that get joined here does make the result\neasier to read, I think it creates some confusion. diff.*.xfuncname in a\nregular config file _doesn't_ behave this way (it's the usual\nlast-one-wins, so we expect a single string). You've handled this\nspecially in your code to read this file, but it's confusing because\nthis test otherwise looks exactly like a config file. And thus somebody\nmight be tempted to copy it to their config file and modify it, but it\nwould not do what they expected.\n\nI don't recall how well our config parser copes with embedded newlines\nin values.  I.e., if it would be possible to write:\n\n  [diff \"foo\"]\n  xfuncname = \"the pattern starts here...\n  and continues through newlines!\"\n\nI think it doesn't work, but perhaps it would be a nice feature to add\nit. It would make the format slightly more complex, though (and make\ndiagnosing a missing double-quote much harder). I dunno.\n\n-Peff\n"},{"id":"377486","messageId":"1da78555-96eb-1206-679b-527911f6af29@kdbg.org","threadId":"51329","inReplyTo":"20190619034929.GA515@sigill.intra.peff.net","subject":"Re: [RFC PATCH] userdiff: ship built-in driver config file","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-06-19T06:30:25Z","receivedAt":"2019-06-19T06:30:29Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 19.06.19 um 05:49 schrieb Jeff King:\n> On Tue, Jun 18, 2019 at 10:32:47PM +0200, Johannes Sixt wrote:\n> \n>> Am 17.06.19 um 18:54 schrieb Boxuan Li:\n>>> diff --git a/templates/this--userdiff b/templates/this--userdiff\n>>> new file mode 100644\n>>> index 0000000000..85114a7229\n>>> --- /dev/null\n>>> +++ b/templates/this--userdiff\n>>\n>> Why place this file in .git? To have per-repository diff drivers, we can\n>> already specify them via 'git config'. This file should be installed in\n>> the system.\n> \n> I think it _could_ actually just be part of the system /etc/gitconfig,\n> though it is kind of big, and Git has a tendency to parse the config\n> more than necessary. I wonder if would add a noticeable slowdown.\n\nBut /etc/gitconfig would be the wrong place, because it would not be\nupdated when a new version ships with new patterns.\n\nI would suggest to install the file as $prefix/share/git-core/userdiff\nalthough the name \"userdiff\" sounds like an accident. How about\n.../filetypes?\n\n-- Hannes\n"},{"id":"377527","messageId":"xmqqfto5r5nr.fsf@gitster-ct.c.googlers.com","threadId":"51329","inReplyTo":"20190619034929.GA515@sigill.intra.peff.net","subject":"Re: [RFC PATCH] userdiff: ship built-in driver config file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-19T14:50:00Z","receivedAt":"2019-06-19T14:50:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think it _could_ actually just be part of the system /etc/gitconfig,\n> though it is kind of big, and Git has a tendency to parse the config\n> more than necessary. I wonder if would add a noticeable slowdown.\n\nYeah, that was what I was wondering too when somebody made a casual\nmention of the \"why do we want to pile these settings hardcoded in\nthe code for each and every language?\", which I think led to this\npatch.  I was actually imagining that this would be totally outside\nthe normal config mechanism (there is no strong reason to even share\nthe syntax, but there is no need to come up with a different syntax,\neither), treated like other fixed data files (like *.mo files), and\nis read lazily only when diff driver needs to be instantiated.\n\nI also agree with you that changing how multiply-defined variables\nare handled with the patch is a no-go, if we are going to share the\nparser.\n\nThanks for a patch and a review.\n\n"},{"id":"377528","messageId":"xmqqblytr525.fsf@gitster-ct.c.googlers.com","threadId":"51329","inReplyTo":"1da78555-96eb-1206-679b-527911f6af29@kdbg.org","subject":"Re: [RFC PATCH] userdiff: ship built-in driver config file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-19T15:02:58Z","receivedAt":"2019-06-19T15:03:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> But /etc/gitconfig would be the wrong place, because it would not be\n> updated when a new version ships with new patterns.\n>\n> I would suggest to install the file as $prefix/share/git-core/userdiff\n> although the name \"userdiff\" sounds like an accident. How about\n> .../filetypes?\n\nI am not sure about the filename, but I do agree with your choice of\n\"somewhere under $prefix/share/\".\n\nThanks.\n"},{"id":"377537","messageId":"CALM0=-=4b4j1brRN=Jnq4N+f+_vJXrL-JBAOxaLw0FgugNqX5w@mail.gmail.com","threadId":"51329","inReplyTo":"20190619035857.GB515@sigill.intra.peff.net","subject":"Re: [RFC PATCH] userdiff: ship built-in driver config file","fromName":"LI, BO XUAN","fromEmail":"liboxuan@connect.hku.hk","sentAt":"2019-06-19T15:32:19Z","receivedAt":"2019-06-19T15:33:02Z","isPatch":true,"sender":{"key":"liboxuan@connect.hku.hk","avatar":"https://avatars.githubusercontent.com/u/25746010?v=4"},"body":"On Wed, Jun 19, 2019 at 11:58 AM Jeff King <peff@peff.net> wrote:\n>\n> While having separate lines that get joined here does make the result\n> easier to read, I think it creates some confusion. diff.*.xfuncname in a\n> regular config file _doesn't_ behave this way (it's the usual\n> last-one-wins, so we expect a single string). You've handled this\n> specially in your code to read this file, but it's confusing because\n> this test otherwise looks exactly like a config file. And thus somebody\n> might be tempted to copy it to their config file and modify it, but it\n> would not do what they expected.\n>\n> I don't recall how well our config parser copes with embedded newlines\n> in values.  I.e., if it would be possible to write:\n>\n>   [diff \"foo\"]\n>   xfuncname = \"the pattern starts here...\n>   and continues through newlines!\"\n>\n\nIf I recall correctly, the above version wouldn't work, but the\nfollowing version would:\n\n[diff \"foo\"]\n    xfuncname = \"The pattern starts here...\"\n\"and continues here! But the indentation looks ugly,\"\n\"and we lose the ability to add comments inline (within pattern)\"\n\nActually, at the very beginning, I was imaging some syntax like this:\n\n[diff \"foo\"]\n    xfuncname = \"The pattern starts here...\"\n    ; using '+=' will continue the pattern above\n    xfuncname += \"and continues here!\"\n    ; a '=' symbol will start a new pattern\n    xfuncname = \"This is another pattern..\"\n    xfuncname += \"and remember, last one always wins\"\n\nThe existing config parser does not support \"+=\" though, which is a\nnice feature to have in my opinion. Maybe there is a reason?\n\nBy the way, thanks for all the reviews! Especially when I found more\nlines of reviews than lines of my code.\n\nBest regards,\nBoxuan\n"},{"id":"377547","messageId":"20190619183931.GA28145@sigill.intra.peff.net","threadId":"51329","inReplyTo":"1da78555-96eb-1206-679b-527911f6af29@kdbg.org","subject":"Re: [RFC PATCH] userdiff: ship built-in driver config file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-19T18:39:32Z","receivedAt":"2019-06-19T18:39:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 19, 2019 at 08:30:25AM +0200, Johannes Sixt wrote:\n\n> >> Why place this file in .git? To have per-repository diff drivers, we can\n> >> already specify them via 'git config'. This file should be installed in\n> >> the system.\n> > \n> > I think it _could_ actually just be part of the system /etc/gitconfig,\n> > though it is kind of big, and Git has a tendency to parse the config\n> > more than necessary. I wonder if would add a noticeable slowdown.\n> \n> But /etc/gitconfig would be the wrong place, because it would not be\n> updated when a new version ships with new patterns.\n\nI was thinking it would be, but I guess there is a merging problem if\nthe admin has made their own changes.\n\n> I would suggest to install the file as $prefix/share/git-core/userdiff\n> although the name \"userdiff\" sounds like an accident. How about\n> .../filetypes?\n\nDoes it need to be specific to userdiff or filetypes? Could this be a\ngeneric fourth level of config: repo, user, system, builtin? We\neffectively already have that, except the \"builtin\" ones are truly baked\ninto the binary, which means they are not visible. So right now you\ncannot say \"git config diff.tex.xfuncname\" and get any useful\ninformation, even though we clearly are going to respect it.\n\nOn the other hand, this would make \"git config --list\" quite a bit\nlonger. And any solution that involves putting it into the generic\nconfig paths may suffer from the bloating/slowdown problem I mentioned.\n\nBut without that, I have to wonder what problem we are really solving.\nNow it's baked into the binary. Later it will be baked into the\ndistribution, but we still don't want anybody to touch it because their\nchanges will be overwritten. I guess it's a little easier for somebody\nto find .../share/git-core/userdiff and use it as a template than it is\nto find the definitions in the source. But it's not exactly easy.\n\n-Peff\n"},{"id":"377548","messageId":"20190619184214.GB28145@sigill.intra.peff.net","threadId":"51329","inReplyTo":"CALM0=-=4b4j1brRN=Jnq4N+f+_vJXrL-JBAOxaLw0FgugNqX5w@mail.gmail.com","subject":"Re: [RFC PATCH] userdiff: ship built-in driver config file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-19T18:42:14Z","receivedAt":"2019-06-19T18:42:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 19, 2019 at 11:32:19PM +0800, LI, BO XUAN wrote:\n\n> >   [diff \"foo\"]\n> >   xfuncname = \"the pattern starts here...\n> >   and continues through newlines!\"\n> >\n> \n> If I recall correctly, the above version wouldn't work, but the\n> following version would:\n> \n> [diff \"foo\"]\n>     xfuncname = \"The pattern starts here...\"\n> \"and continues here! But the indentation looks ugly,\"\n> \"and we lose the ability to add comments inline (within pattern)\"\n\nI don't think that works for our config files either (it does in C, of\ncourse).\n\n> Actually, at the very beginning, I was imaging some syntax like this:\n> \n> [diff \"foo\"]\n>     xfuncname = \"The pattern starts here...\"\n>     ; using '+=' will continue the pattern above\n>     xfuncname += \"and continues here!\"\n>     ; a '=' symbol will start a new pattern\n>     xfuncname = \"This is another pattern..\"\n>     xfuncname += \"and remember, last one always wins\"\n> \n> The existing config parser does not support \"+=\" though, which is a\n> nice feature to have in my opinion. Maybe there is a reason?\n\nThere's no particular reason that feature isn't there, but in general\nwe've been very hesitant to add syntactic changes to the config files,\nsince there are many parsers in the wild. In general, today's files are\ncompatible with ones from 2005.\n\n-Peff\n"},{"id":"377617","messageId":"1db4a03e-5bc2-673d-6d89-bf654b785129@kdbg.org","threadId":"51329","inReplyTo":"20190619183931.GA28145@sigill.intra.peff.net","subject":"Re: [RFC PATCH] userdiff: ship built-in driver config file","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-06-20T07:41:13Z","receivedAt":"2019-06-20T07:41:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 19.06.19 um 20:39 schrieb Jeff King:\n> But without that, I have to wonder what problem we are really solving.\n\nYou have a point here.\n\n> Now it's baked into the binary. Later it will be baked into the\n> distribution, but we still don't want anybody to touch it because their\n> changes will be overwritten.\n\nHaving the patterns outside the binary reduces our mental barrier to\naccept additional patterns or slightly modified patterns for new file\ntypes, like we had recently for octave that are just a small extension\nto the matlab patterns.\n\nBut we are not solving a technical problem or a problem our users have,\nAFAICS.\n\n> I guess it's a little easier for somebody\n> to find .../share/git-core/userdiff and use it as a template than it is\n> to find the definitions in the source. But it's not exactly easy.\n\nHaving a slightly easiser to discover database may count as an advantage. ;)\n\n-- Hannes\n"}]}