{"thread":{"id":"9674","subject":"[RFC] remarks about custom diff driver","startedAt":"2007-08-27T10:36:34Z","lastAt":"2007-08-27T23:29:11Z","messageCount":3,"participants":["Matthieu Moy","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"51679","messageId":"vpq8x7x5knh.fsf@bauges.imag.fr","threadId":"9674","inReplyTo":null,"subject":"[RFC] remarks about custom diff driver","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2007-08-27T10:36:34Z","receivedAt":"2007-08-27T10:36:34Z","isPatch":false,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Hi,\n\nThe following is a reflexion about Git's custom diff drivers, thought\nafter setting up a custom diff driver for OpenDocument files.\n\n  http://www-verimag.imag.fr/~moy/opendocument/\n\nIndeed, my solution has a few drawbacks: it doesn't take advantage of\ngit's diff engine. --color, --color-words do not work. I could use\ngit-diff within my script, but I can't get the argument passed on the\ncommand-line, so I can hardcode --color for example, but not be\nconsistant with the rest of the diff (in case \"git diff\" shows the\ndiff between two text files and two OpenDocument files).\n\nAlso, git-diff doesn't have a -L option, so using git-diff would mean\nhaving some a/tmp/oodiff.xzy.1 in the output, which is ugly.\n\nIndeed, what I would have needed is a custum text converter. In my\ncase, I would have said something like\n\n# ~/.gitconfig\n[textconverter \"odt2txt\"]\n\tcommand=odt2txt\n\nThen, in .gitattributes\n\n*.ods textconverter=odt2txt\n*.odp textconverter=odt2txt\n*.odt textconverter=odt2txt\n\nAnd git-diff could apply an algorithm like\n\nif custom-diff-driver-specified\nthen\n    custom-diff-command $1 .. $7\nelif custom-textconverter-specified\n    echo \"Using textconverter $textconverter\"\n    custom-textconverter-command $file1 > $tmpfile1\n    custom-textconverter-command $file2 > $tmpfile2\n    use git engine on $tmpfile1 and $tmpfile2\nelse\n    use git engine on $file1 and $file2\nfi\n\nThis way, someone specifying a textconverter would automatically\nbenefit from all the facilities that git has for text files (all the\ngood stuff of git-diff, --color, --color-words, correct diff label\nwithout bothering with -L option of diff, ...). One could imagine also\nhave \"git-blame\" work for these files, ...\n\nOne drawback: making this too much transparent, users may want to use\n\"git apply\", or just \"patch\" on the generated diffs. So,\ngit-format-patch shouldn't use it, and that's why my pseudo-code above\ndisplays a message \"Using textconverter ...\".\n\nAny opinion? Do you think that's overkill? Anyone else have a\nparticuliar experience with custom diff engine?\n\n-- \nMatthieu\n"},{"id":"51680","messageId":"20070827104529.GA25685@coredump.intra.peff.net","threadId":"9674","inReplyTo":"vpq8x7x5knh.fsf@bauges.imag.fr","subject":"Re: [RFC] remarks about custom diff driver","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-08-27T10:45:29Z","receivedAt":"2007-08-27T10:45:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 27, 2007 at 12:36:34PM +0200, Matthieu Moy wrote:\n\n> Indeed, what I would have needed is a custum text converter. In my\n> case, I would have said something like\n> \n> [...]\n>\n> Any opinion? Do you think that's overkill? Anyone else have a\n> particuliar experience with custom diff engine?\n\nInterestingly enough, I tried using a custom diff for the first time\nearlier today and came to the exact same conclusion (I was diff'ing\nphotos that were tagged with exif keywords). I suspect a large number of\ndiff engines will simply want to convert content into a canonical text\nformat.\n\nI wonder if you could simply use a \"diff-filter\" attribute that would\nclean and smudge in the same way as \"filter\", except it would only do so\nwhen creating or applying diffs. In most cases, you wouldn't have a\n\"clean\" component (since the conversion to text is lossy).\n\n-Peff\n"},{"id":"51767","messageId":"vpqabsc8sl4.fsf@bauges.imag.fr","threadId":"9674","inReplyTo":"vpq8x7x5knh.fsf@bauges.imag.fr","subject":"Re: [RFC][PATCH] remarks about custom diff driver","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2007-08-27T23:29:11Z","receivedAt":"2007-08-27T23:29:11Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n\n> Indeed, what I would have needed is a custum text converter. In my\n> case, I would have said something like\n>\n> # ~/.gitconfig\n> [textconv \"odt2txt\"]\n> \tcommand=odt2txt\n>\n> Then, in .gitattributes\n>\n> *.ods textconv=odt2txt\n> *.odp textconv=odt2txt\n> *.odt textconv=odt2txt\n\nI started implementing something like this. The code is rather ugly\n(I'm not so fluent in C, and not familiar with the git codebase ...),\nbut it gives an idea, and seems to work.\n\nIf you're interested in having a look (but I'll have to cleanup the\ncode before asking for a detailed review), here's the patch.\n\nIn the particular case of OpenDocument files, that makes a few tens of\nlines of C in git itself (once and for all), but it makes it totally\ntrivial for the user to configure git to get all of \"git diff\". Sounds\npromising to me.\n\n\n>From 7e1bbb5fc6dc5c0adb8cad5b875d57ee17247f4a Mon Sep 17 00:00:00 2001\nFrom: Matthieu Moy <Matthieu.Moy@imag.fr>\nDate: Tue, 28 Aug 2007 00:46:07 +0200\nSubject: [PATCH] Prototype for a textconverter filter in git-diff.\n\nThat feature is similar to the custom diff driver, but the user only has\nto provide a text filter (a command to convert a file into a plain-text\nrepresentation). Git takes care of diffing, mode change, ...\n\nIn particular, with\n\n[textconv \"odt2txt\"]\n          command=odt2txt\n\n*.ods textconv=odt2txt\n*.odp textconv=odt2txt\n*.odt textconv=odt2txt\n\nOne can use \"git diff\" on OpenOffice files (including \"git diff --color\"\nand friends).\n\nThis could be extended so that \"git blame\" can use it also.\n---\n diff.c |  193 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n 1 files changed, 171 insertions(+), 22 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex a7e7671..ad99f27 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -50,11 +50,12 @@ static int parse_diff_color_slot(const char *var, int ofs)\n \tdie(\"bad config variable '%s'\", var);\n }\n \n-static struct ll_diff_driver {\n+static struct ll_cmd_driver {\n \tconst char *name;\n-\tstruct ll_diff_driver *next;\n+\tstruct ll_cmd_driver *next;\n \tchar *cmd;\n-} *user_diff, **user_diff_tail;\n+} *user_diff, **user_diff_tail,\n+\t*user_textconv, **user_textconv_tail;\n \n static void read_config_if_needed(void)\n {\n@@ -70,29 +71,30 @@ static void read_config_if_needed(void)\n  * this in a bit convoluted way to allow low level diff driver\n  * called \"color\".\n  */\n-static int parse_lldiff_command(const char *var, const char *ep, const char *value)\n+static int parse_ll_command(const char *var, const char *name,\n+\t\t\t    const char *ep, const char *value,\n+\t\t\t    struct ll_cmd_driver **user_driver,\n+\t\t\t    struct ll_cmd_driver ***user_driver_tail)\n {\n-\tconst char *name;\n \tint namelen;\n-\tstruct ll_diff_driver *drv;\n+\tstruct ll_cmd_driver *drv;\n \n-\tname = var + 5;\n \tnamelen = ep - name;\n-\tfor (drv = user_diff; drv; drv = drv->next)\n+\tfor (drv = *user_driver; drv; drv = drv->next)\n \t\tif (!strncmp(drv->name, name, namelen) && !drv->name[namelen])\n \t\t\tbreak;\n \tif (!drv) {\n \t\tchar *namebuf;\n-\t\tdrv = xcalloc(1, sizeof(struct ll_diff_driver));\n+\t\tdrv = xcalloc(1, sizeof(struct ll_cmd_driver));\n \t\tnamebuf = xmalloc(namelen + 1);\n \t\tmemcpy(namebuf, name, namelen);\n \t\tnamebuf[namelen] = 0;\n \t\tdrv->name = namebuf;\n \t\tdrv->next = NULL;\n-\t\tif (!user_diff_tail)\n-\t\t\tuser_diff_tail = &user_diff;\n-\t\t*user_diff_tail = drv;\n-\t\tuser_diff_tail = &(drv->next);\n+\t\tif (!*user_driver_tail)\n+\t\t\t*user_driver_tail = &*user_driver;\n+\t\t**user_driver_tail = drv;\n+\t\t*user_driver_tail = &(drv->next);\n \t}\n \n \tif (!value)\n@@ -101,6 +103,38 @@ static int parse_lldiff_command(const char *var, const char *ep, const char *val\n \treturn 0;\n }\n \n+static void setup_textconv_attr_check(struct git_attr_check *check)\n+{\n+\tstatic struct git_attr *attr_textconv;\n+\n+\tif (!attr_textconv) {\n+\t\tattr_textconv = git_attr(\"textconv\", 8);\n+\t}\n+\tcheck[0].attr = attr_textconv;\n+}\n+\n+static const char *external_textconv_attr(const char *name)\n+{\n+\tstruct git_attr_check attr_textconv_check;\n+\n+\tsetup_textconv_attr_check(&attr_textconv_check);\n+\tif (!git_checkattr(name, 1, &attr_textconv_check)) {\n+\t\tconst char *value = attr_textconv_check.value;\n+\t\tif (!ATTR_TRUE(value) &&\n+\t\t    !ATTR_FALSE(value) &&\n+\t\t    !ATTR_UNSET(value)) {\n+\t\t\tstruct ll_cmd_driver *drv;\n+\n+\t\t\tread_config_if_needed();\n+\t\t\tfor (drv = user_textconv; drv; drv = drv->next) {\n+\t\t\t\tif (!strcmp(drv->name, value))\n+\t\t\t\t\treturn drv->cmd;\n+\t\t\t}\n+\t\t}\n+\t}\n+\treturn NULL;\n+}\n+\n /*\n  * 'diff.<what>.funcname' attribute can be specified in the configuration\n  * to define a customized regexp to find the beginning of a function to\n@@ -171,11 +205,21 @@ int git_diff_ui_config(const char *var, const char *value)\n \n \t\tif (ep != var + 4) {\n \t\t\tif (!strcmp(ep, \".command\"))\n-\t\t\t\treturn parse_lldiff_command(var, ep, value);\n+\t\t\t\treturn parse_ll_command(var, var + 5, ep, value,\n+\t\t\t\t\t\t\t&user_diff, &user_diff_tail);\n \t\t\tif (!strcmp(ep, \".funcname\"))\n \t\t\t\treturn parse_funcname_pattern(var, ep, value);\n \t\t}\n \t}\n+\tif (!prefixcmp(var, \"textconv.\")) {\n+\t\tconst char *ep = strrchr(var, '.');\n+\n+\t\tif (ep != var + 8) {\n+\t\t\tif (!strcmp(ep, \".command\"))\n+\t\t\t\treturn parse_ll_command(var, var + 9, ep, value,\n+\t\t\t\t\t\t\t&user_textconv, &user_textconv_tail);\n+\t\t}\n+\t}\n \tif (!prefixcmp(var, \"diff.color.\") || !prefixcmp(var, \"color.diff.\")) {\n \t\tint slot = parse_diff_color_slot(var, 11);\n \t\tcolor_parse(value, var, diff_colors[slot]);\n@@ -244,6 +288,13 @@ static struct diff_tempfile {\n \tchar tmp_path[PATH_MAX];\n } diff_temp[2];\n \n+/* Forward declarations */\n+static void prepare_temp_file(const char *name,\n+\t\t\t      struct diff_tempfile *temp,\n+\t\t\t      struct diff_filespec *one);\n+static int spawn_prog(const char *pgm, const char **arg);\n+/* End forward declarations */\n+\n static int count_lines(const char *data, int size)\n {\n \tint count, ch, completely_empty = 1, nl_just_seen = 0;\n@@ -1260,6 +1311,72 @@ static const char *diff_funcname_pattern(struct diff_filespec *one)\n \treturn NULL;\n }\n \n+static int spawn_prog_to_buf(const char *pgm, const char **arg, char **buf, unsigned long * size)\n+{\n+\tpid_t pid;\n+\tint status;\n+\tint fd[2];\n+\n+#define INCREMENT 1024\n+\tssize_t got;\n+\n+\tfflush(NULL);\n+\tif (pipe(fd) < 0)\n+\t\tdie(\"unable to create pipe\");\n+\tpid = fork();\n+\tif (pid < 0)\n+\t\tdie(\"unable to fork\");\n+\tif (!pid) {\n+\t\tdup2(fd[1], 1);\n+\t\tclose(fd[0]);\n+\t\tclose(fd[1]);\n+\t\texecvp(pgm, (char *const*) arg);\n+\t\texit(255);\n+\t}\n+\tclose(fd[1]);\n+\n+\t*size = 0;\n+\t*buf = NULL;\n+\twhile (1) {\n+\t\t*buf = xrealloc(*buf, *size + INCREMENT);\n+\t\tgot = xread(fd[0], *buf + *size, INCREMENT);\n+\t\tif (!got)\n+\t\t\tbreak; /* EOF */\n+\t\tif (got < 0)\n+\t\t\treturn error(\"error while reading from stdin %s\",\n+\t\t\t\t     strerror(errno));\n+\t\t*size += got;\n+\t}\n+\n+\twhile (waitpid(pid, &status, 0) < 0) {\n+\t\tif (errno == EINTR)\n+\t\t\tcontinue;\n+\t\treturn -1;\n+\t}\n+\n+#if 0\n+\t/*\n+\t * copied from the external diff spawn_prog. Is it needed\n+\t * here, too ?\n+\t */\n+\n+\t/* Earlier we did not check the exit status because\n+\t * diff exits non-zero if files are different, and\n+\t * we are not interested in knowing that.  It was a\n+\t * mistake which made it harder to quit a diff-*\n+\t * session that uses the git-apply-patch-script as\n+\t * the GIT_EXTERNAL_DIFF.  A custom GIT_EXTERNAL_DIFF\n+\t * should also exit non-zero only when it wants to\n+\t * abort the entire diff-* session.\n+\t */\n+\tif (WIFEXITED(status) && !WEXITSTATUS(status))\n+\t\treturn 0;\n+\treturn -1;\n+#endif\n+}\n+\n+\n+\n static void builtin_diff(const char *name_a,\n \t\t\t const char *name_b,\n \t\t\t struct diff_filespec *one,\n@@ -1314,6 +1431,38 @@ static void builtin_diff(const char *name_a,\n \tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n \t\tdie(\"unable to read files to diff\");\n \n+\tconst char * textconv = external_textconv_attr(name_a);\n+\tconst char * textconvb = external_textconv_attr(name_b);\n+\n+\t/*\n+\t * Only use the textconv driver is it is set, and is the same\n+\t * for source and destination file.\n+\t */\n+\tif (textconv && textconvb && !strcmp(textconv, textconvb)) {\n+\t\tconst char * spawn_arg[3];\n+\t\tconst char ** arg = &spawn_arg[0];\n+\t\tchar *bufa, *bufb;\n+\t\tunsigned long sizea, sizeb;\n+\t\tstruct diff_tempfile *temp = diff_temp;\n+\t\tprepare_temp_file(name_a, &temp[0], one);\n+\t\t*arg++ = textconv;\n+\t\t*arg++ = temp[0].name;\n+\t\t*arg++ = NULL;\n+\t\tspawn_prog_to_buf(textconv, spawn_arg, &(mf1.ptr), &(mf1.size));\n+\t\tone->data = mf1.ptr;\n+\t\tone->size = mf1.size;\n+\t\tone->should_free = 1;\n+\t\tone->should_munmap = 1;\n+\n+\t\tprepare_temp_file(name_b, &temp[1], two);\n+\t\tspawn_arg[1] = temp[1].name;\n+\t\tspawn_prog_to_buf(textconv, spawn_arg, &(mf2.ptr), &(mf2.size));\n+\t\ttwo->data = mf2.ptr;\n+\t\ttwo->size = mf2.size;\n+\t\ttwo->should_free = 1;\n+\t\ttwo->should_munmap = 1;\n+\t}\n+\n \tif (!o->text &&\n \t    (diff_filespec_is_binary(one) || diff_filespec_is_binary(two))) {\n \t\t/* Quite common confusing case */\n@@ -1890,7 +2039,7 @@ static const char *external_diff_attr(const char *name)\n \t\tif (!ATTR_TRUE(value) &&\n \t\t    !ATTR_FALSE(value) &&\n \t\t    !ATTR_UNSET(value)) {\n-\t\t\tstruct ll_diff_driver *drv;\n+\t\t\tstruct ll_cmd_driver *drv;\n \n \t\t\tread_config_if_needed();\n \t\t\tfor (drv = user_diff; drv; drv = drv->next)\n@@ -2853,14 +3002,14 @@ struct patch_id_t {\n static int remove_space(char *line, int len)\n {\n \tint i;\n-        char *dst = line;\n-        unsigned char c;\n+\tchar *dst = line;\n+\tunsigned char c;\n \n-        for (i = 0; i < len; i++)\n-                if (!isspace((c = line[i])))\n-                        *dst++ = c;\n+\tfor (i = 0; i < len; i++)\n+\t\tif (!isspace((c = line[i])))\n+\t\t\t*dst++ = c;\n \n-        return dst - line;\n+\treturn dst - line;\n }\n \n static void patch_id_consume(void *priv, char *line, unsigned long len)\n-- \n1.5.3.rc6.21.g153d-dirty\n\n\n\n-- \nMatthieu\n"}]}