{"thread":{"id":"15693","subject":"Implementation of a \"textconv\" filter for easy custom diff.","startedAt":"2008-09-28T02:06:53Z","lastAt":"2008-10-27T08:23:55Z","messageCount":85,"participants":["Matthieu Moy","Jeff King","Johannes Sixt","Jakub Narebski","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"91761","messageId":"1222567618-22156-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"15693","inReplyTo":null,"subject":"Implementation of a \"textconv\" filter for easy custom diff.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-09-28T02:06:53Z","receivedAt":"2008-09-28T02:06:53Z","isPatch":false,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"\nThis patch serie give an implementation of a \"textconv\" filter for\n\"git diff\", that allows one to diff anything that can easily be\nconverted to text with just a few lines in ~/.gitconfig and\n.gitattributes.\n\nThe really cool thing in comparison with GIT_EXTERNAL_DIFF is that one\ngets all the cool things of \"git diff\" (colors & cie) for free.\n\nI had already posted a prototype of this a long time back[1], but I\nfinally got time to clean up the code a bit.\n\nhttp://article.gmane.org/gmane.comp.version-control.git/56896\n"},{"id":"91766","messageId":"1222567618-22156-2-git-send-email-Matthieu.Moy@imag.fr","threadId":"15693","inReplyTo":"1222567618-22156-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH] Facility to have multiple kinds of drivers for diff.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-09-28T02:06:54Z","receivedAt":"2008-09-28T02:06:54Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"We now have an array attr_cmd_specs describing the possible kinds of\ndrivers.\n---\n diff.c | 63 +++++++++++++++++++++++++++++++++++++++++----------------------\n 1 files changed, 41 insertions(+), 22 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 099ce3f..1c8dd19 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -56,11 +56,29 @@ static int parse_diff_color_slot(const char *var, int ofs)\n \tdie(\"bad config variable '%s'\", var);\n }\n \n-static struct ll_cmd_driver {\n+struct ll_cmd_driver {\n \tconst char *name;\n \tstruct ll_cmd_driver *next;\n \tconst char *cmd;\n-} *user_diff, **user_diff_tail;\n+};\n+\n+struct attr_cmd_spec {\n+\tchar * name;\n+\tstruct git_attr * attr_cmd;\n+\tstruct ll_cmd_driver *head;\n+\tstruct ll_cmd_driver **tail;\n+};\n+\n+enum driver_indices {\n+\tDIFF_DRIVER=0,\n+\tTEXTCONV_DRIVER=1,\n+\tDRIVER_COUNT,\n+};\n+\n+static struct attr_cmd_spec attr_cmd_specs[DRIVER_COUNT] = {\n+\t{\"diff\", NULL},\n+\t{\"textconv\", NULL},\n+};\n \n /*\n * Currently there is only \"diff..command\" variable;\n@@ -68,24 +86,24 @@ static struct ll_cmd_driver {\n * this in a bit convoluted way to allow low level diff driver\n * called \"color\".\n */\n-static int parse_ll_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 int driver)\n {\n-\tconst char *name;\n \tint namelen;\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 = attr_cmd_specs[driver].head; drv; drv = drv->next)\n \t\tif (!strncmp(drv->name, name, namelen) && !drv->name[namelen])\n \t\t\tbreak;\n \tif (!drv) {\n \t\tdrv = xcalloc(1, sizeof(struct ll_cmd_driver));\n \t\tdrv->name = xmemdupz(name, namelen);\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 (!attr_cmd_specs[driver].tail)\n+\t\t\tattr_cmd_specs[driver].tail = &attr_cmd_specs[driver].head;\n+\t\t*attr_cmd_specs[driver].tail = drv;\n+\t\tattr_cmd_specs[driver].tail = &(drv->next);\n \t}\n \n \treturn git_config_string(&(drv->cmd), var, value);\n@@ -161,7 +179,7 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\tconst char *ep = strrchr(var, '.');\n \n \t\tif (ep != var + 4 && !strcmp(ep, \".command\"))\n-\t\t\treturn parse_ll_command(var, ep, value);\n+\t\t\treturn parse_ll_command(var, var + 5, ep, value, DIFF_DRIVER);\n \t}\n \n \treturn git_diff_basic_config(var, value, cb);\n@@ -1340,14 +1358,14 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two)\n \temit_binary_diff_body(file, two, one);\n }\n \n-static void setup_diff_attr_check(struct git_attr_check *check)\n+/* driver_index must be among the values of \"enum driver_indices\" */\n+static void setup_cmd_attr_check(struct git_attr_check *check, int driver_index)\n {\n-\tstatic struct git_attr *attr_diff;\n-\n-\tif (!attr_diff) {\n-\t\tattr_diff = git_attr(\"diff\", 4);\n+\tstruct attr_cmd_spec * spec = &attr_cmd_specs[driver_index];\n+\tif (!spec->attr_cmd) {\n+\t\tspec->attr_cmd = git_attr(spec->name, strlen(spec->name));\n \t}\n-\tcheck[0].attr = attr_diff;\n+\tcheck[0].attr = spec->attr_cmd;\n }\n \n static void diff_filespec_check_attr(struct diff_filespec *one)\n@@ -1358,7 +1376,7 @@ static void diff_filespec_check_attr(struct diff_filespec *one)\n \tif (one->checked_attr)\n \t\treturn;\n \n-\tsetup_cmd_attr_check(&attr_cmd_check);\n+\tsetup_cmd_attr_check(&attr_cmd_check, DIFF_DRIVER);\n \tone->is_binary = 0;\n \tone->funcname_pattern_ident = NULL;\n \n@@ -2092,14 +2110,14 @@ static void run_external_diff(const char *pgm,\n \t}\n }\n \n-static const char *external_cmd_attr(const char *name)\n+static const char *external_cmd_attr(const char *name, int driver_index)\n {\n \tstruct git_attr_check attr_cmd_check;\n \n \tif (!name)\n \t\treturn NULL;\n \n-\tsetup_diff_attr_check(&attr_cmd_check);\n+\tsetup_cmd_attr_check(&attr_cmd_check, driver_index);\n \tif (!git_checkattr(name, 1, &attr_cmd_check)) {\n \t\tconst char *value = attr_cmd_check.value;\n \t\tif (!ATTR_TRUE(value) &&\n@@ -2107,7 +2125,8 @@ static const char *external_cmd_attr(const char *name)\n \t\t !ATTR_UNSET(value)) {\n \t\t\tstruct ll_cmd_driver *drv;\n \n-\t\t\tfor (drv = user_diff; drv; drv = drv->next)\n+\t\t\tfor (drv = attr_cmd_specs[driver_index].head;\n+\t\t\t drv; drv = drv->next)\n \t\t\t\tif (!strcmp(drv->name, value))\n \t\t\t\t\treturn drv->cmd;\n \t\t}\n@@ -2128,7 +2147,7 @@ static void run_diff_cmd(const char *pgm,\n \tif (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))\n \t\tpgm = NULL;\n \telse {\n-\t\tconst char *cmd = external_cmd_attr(attr_path);\n+\t\tconst char *cmd = external_cmd_attr(attr_path, DIFF_DRIVER);\n \t\tif (cmd)\n \t\t\tpgm = cmd;\n \t}\n-- \n1.6.0.2.312.g1ef81a\n"},{"id":"91762","messageId":"1222567618-22156-3-git-send-email-Matthieu.Moy@imag.fr","threadId":"15693","inReplyTo":"1222567618-22156-2-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH] Implement run_command_to_buf (spawn a process and reads its stdout)","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-09-28T02:06:55Z","receivedAt":"2008-09-28T02:06:55Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"---\n diff.c | 24 ++++++++++++++++++++++++\n 1 files changed, 24 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 1c8dd19..6917981 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -260,6 +260,10 @@ static struct diff_tempfile {\n \tchar tmp_path[PATH_MAX];\n } diff_temp[2];\n \n+/* Forward declarations */\n+static int run_command_to_buf(const char **argv, char **buf, size_t * size);\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@@ -1487,6 +1491,26 @@ void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const\n \t\toptions->b_prefix = b;\n }\n \n+static int run_command_to_buf(const char **argv, char **buf, size_t * size)\n+{\n+\tstruct child_process cmd;\n+\tstruct strbuf buffer = STRBUF_INIT;\n+\tmemset(&cmd, 0, sizeof(cmd));\n+\tcmd.argv = argv;\n+\tcmd.out = -1;\n+\n+\tif (start_command(&cmd))\n+\t\treturn -1;\n+\tstrbuf_read(&buffer, cmd.out, 1024);\n+\tclose(cmd.out);\n+\n+\tif (finish_command(&cmd))\n+\t\treturn -1;\n+\n+\t*buf = strbuf_detach(&buffer, size);\n+\treturn 0;\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-- \n1.6.0.2.312.g1ef81a\n"},{"id":"91763","messageId":"1222567618-22156-4-git-send-email-Matthieu.Moy@imag.fr","threadId":"15693","inReplyTo":"1222567618-22156-3-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH] Implement a textconv filter for \"git diff\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-09-28T02:06:56Z","receivedAt":"2008-09-28T02:06:56Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"That 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\nFor example, 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 | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 68 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 6917981..7cbc42b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -181,6 +181,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\tif (ep != var + 4 && !strcmp(ep, \".command\"))\n \t\t\treturn parse_ll_command(var, var + 5, ep, value, DIFF_DRIVER);\n \t}\n+\tif (!prefixcmp(var, \"textconv.\")) {\n+\t\tconst char *ep = strrchr(var, '.');\n+\n+\t\tif (ep != var + 8 && !strcmp(ep, \".command\"))\n+\t\t\treturn parse_ll_command(var, var + 9, ep, value, TEXTCONV_DRIVER);\n+\t}\n \n \treturn git_diff_basic_config(var, value, cb);\n }\n@@ -262,6 +268,10 @@ static struct diff_tempfile {\n \n /* Forward declarations */\n static int run_command_to_buf(const char **argv, char **buf, size_t * size);\n+static const char *external_cmd_attr(const char *name, int driver_index);\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 /* End forward declarations */\n \n static int count_lines(const char *data, int size)\n@@ -1511,6 +1521,52 @@ static int run_command_to_buf(const char **argv, char **buf, size_t * size)\n \treturn 0;\n }\n \n+static int textconv_two_files(const char *textconv,\n+\t\t\t const char *name_a,\n+\t\t\t const char *name_b,\n+\t\t\t mmfile_t *mf1,\n+\t\t\t mmfile_t *mf2,\n+\t\t\t struct diff_filespec *one,\n+\t\t\t struct diff_filespec *two)\n+{\n+\tconst char *spawn_arg[3];\n+\tconst char **arg = &spawn_arg[0];\n+\tstruct diff_tempfile *temp = diff_temp;\n+\tchar *buf_a, *buf_b;\n+\tsize_t size_a, size_b;\n+\tprepare_temp_file(name_a, &temp[0], one);\n+\t*arg++ = textconv;\n+\t*arg++ = temp[0].name;\n+\t*arg++ = NULL;\n+\t/*\n+\t * Run both commands before touching arguments to make sure we\n+\t * do all or nothing.\n+\t */\n+\tif(run_command_to_buf(spawn_arg, &buf_a, &size_a))\n+\t\treturn -1;\n+\n+\tprepare_temp_file(name_b, &temp[1], two);\n+\tspawn_arg[1] = temp[1].name;\n+\tif(run_command_to_buf(spawn_arg, &buf_b, &size_b))\n+\t\treturn -1;\n+\n+\tmf1->ptr = buf_a;\n+\tmf1->size = (long)size_a;\n+\tone->data = mf1->ptr;\n+\tone->size = mf1->size;\n+\tone->should_free = 1;\n+\tone->should_munmap = 1;\n+\n+\tmf2->ptr = buf_b;\n+\tmf2->size = (long)size_b;\n+\ttwo->data = mf2->ptr;\n+\ttwo->size = mf2->size;\n+\ttwo->should_free = 1;\n+\ttwo->should_munmap = 1;\n+\treturn 0;\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@@ -1574,6 +1630,18 @@ 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_cmd_attr(name_a, TEXTCONV_DRIVER);\n+\tconst char * textconvb = external_cmd_attr(name_b, TEXTCONV_DRIVER);\n+\n+\t/*\n+\t * Only use the textconv driver if 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\tif(textconv_two_files(textconv, name_a, name_b, &mf1, &mf2, one, two))\n+\t\t\tfprintf(stderr, \"warning: textconv filter failed, \"\n+\t\t\t\t\"falling back to plain diff\\n\");\n+\n \tif (!DIFF_OPT_TST(o, TEXT) &&\n \t (diff_filespec_is_binary(one) || diff_filespec_is_binary(two))) {\n \t\t/* Quite common confusing case */\n-- \n1.6.0.2.312.g1ef81a\n"},{"id":"91765","messageId":"1222567618-22156-5-git-send-email-Matthieu.Moy@imag.fr","threadId":"15693","inReplyTo":"1222567618-22156-4-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH] Document the textconv filter.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-09-28T02:06:57Z","receivedAt":"2008-09-28T02:06:57Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"---\n Documentation/gitattributes.txt | 43 +++++++++++++++++++++++++++++++++++++++\n 1 files changed, 43 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex e848c94..c4f2b8f 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -325,6 +325,49 @@ patterns are available:\n \n - `tex` suitable for source code for LaTeX documents.\n \n+Converting files to text before a diff\n+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n+\n+The attribute `textconv` affects 'git diff' in a way similar to the\n+`diff` attribute, but with `textconv`, the user provides only a way to\n+convert the file into text, and git takes care of doing the diff as\n+usual (i.e. other options of 'git diff' such as '--color' remain\n+available).\n+\n+The value of `textconv` must be a string, which is the textconv\n+driver.\n+\n+To tell git to use the `exif` filter for jpeg images, use:\n+\n+----------------------------------------------------------------\n+*.jpg textconv=exif\n+----------------------------------------------------------------\n+\n+Defining a custom textconv driver\n+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n+\n+The definition of the `textconv` driver is done in `gitconfig`. To\n+define a driver `exif`, add this to your `$GIT_DIR/config` file (or\n+`$HOME/.gitconfig` file):\n+\n+----------------------------------------------------------------\n+[textconv \"exif\"]\n+\tcommand = exiftags\n+----------------------------------------------------------------\n+\n+Git will call the command specified in `command` with the file to\n+convert as only argument. The program should write the text on its\n+standard output.\n+\n+Examples of useful filters include:\n+\n+----------------------------------------------------------------\n+[textconv \"odt2txt\"]\n+\tcommand = odt2txt\n+[textconv \"word\"]\n+\tcommand = catdoc\n+----------------------------------------------------------------\n+\n \n Performing a three-way merge\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n-- \n1.6.0.2.312.g1ef81a\n"},{"id":"91764","messageId":"1222567618-22156-6-git-send-email-Matthieu.Moy@imag.fr","threadId":"15693","inReplyTo":"1222567618-22156-5-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH] Add a basic test for the textconv filter.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-09-28T02:06:58Z","receivedAt":"2008-09-28T02:06:58Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"---\n t/t4020-diff-external.sh | 7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex dfe3fbc..cf99912 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -99,6 +99,13 @@ test_expect_success 'no diff with -diff' '\n \tgit diff | grep Binary\n '\n \n+test_expect_success 'textconv attribute' '\n+\tgit config textconv.echo.command echo\n+\techo >.gitattributes \"file textconv=echo\"\n+\n+\tgit diff | grep \"^+file$\"\n+'\n+\n echo NULZbetweenZwords | perl -pe 'y/Z/\\000/' > file\n \n test_expect_success 'force diff with \"diff\"' '\n-- \n1.6.0.2.312.g1ef81a\n"},{"id":"91770","messageId":"20080928041040.GA24214@coredump.intra.peff.net","threadId":"15693","inReplyTo":"1222567618-22156-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: Implementation of a \"textconv\" filter for easy custom diff.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-09-28T04:10:40Z","receivedAt":"2008-09-28T04:10:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 28, 2008 at 04:06:53AM +0200, Matthieu Moy wrote:\n\n> This patch serie give an implementation of a \"textconv\" filter for\n> \"git diff\", that allows one to diff anything that can easily be\n> converted to text with just a few lines in ~/.gitconfig and\n> .gitattributes.\n> \n> The really cool thing in comparison with GIT_EXTERNAL_DIFF is that one\n> gets all the cool things of \"git diff\" (colors & cie) for free.\n\nNeat. I started on something like this quite a while ago, and have been\nmeaning to clean it up for some time (and I somehow missed your other\nprototype, too). I agree that it matches my goals much better: the\nfilters are easier to write, and you get the benefit of a nice colorized\ndiff (or even --color-words!).\n\nWe have one major difference in our approaches. In yours, there is a\nnew \"textconv\" attribute that can be used. In mine, I subtly changed the\nmeaning of the \"diff=foo\" attribute to be \"use the diff driver named by\ndiff.foo.*\", and you would set diff.foo.textconv to your command. This\nis a bit simpler to implement, and it provides a better path forward for\ndefining sets of diff tweaks.\n\nFor example, one of the limitations of the current syntax is that you\ncan't say \"Choose automatically whether this is binary or text, but if\nit is text, use this hunk header.\" But with my scheme it is easy to do:\n\n in attributes:\n file diff=foo\n\n in config:\n [diff \"foo\"]\n xfuncname = \"some regex\"\n binary = auto\n\nIs there a particular reason you chose the route you did?\n\n-Peff\n"},{"id":"91771","messageId":"20080928041526.GB24214@coredump.intra.peff.net","threadId":"15693","inReplyTo":"1222567618-22156-4-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH] Implement a textconv filter for \"git diff\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-09-28T04:15:26Z","receivedAt":"2008-09-28T04:15:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 28, 2008 at 04:06:56AM +0200, Matthieu Moy wrote:\n\n> +static int textconv_two_files(const char *textconv,\n> +\t\t\t const char *name_a,\n> +\t\t\t const char *name_b,\n> +\t\t\t mmfile_t *mf1,\n> +\t\t\t mmfile_t *mf2,\n> +\t\t\t struct diff_filespec *one,\n> +\t\t\t struct diff_filespec *two)\n> +{\n\nMust we always be textconv'ing two files? What if I am comparing\n\"v1.5:foo.odf\" to \"foo.txt\" in the working tree?\n\nIn my implementation, I textconv one at a time. I just did so from\nfill_mmfile, so all of diff automagically just sees the converted text.\n\n-Peff\n"},{"id":"91783","messageId":"vpqd4io1tla.fsf@bauges.imag.fr","threadId":"15693","inReplyTo":"20080928041040.GA24214@coredump.intra.peff.net","subject":"Re: Implementation of a \"textconv\" filter for easy custom diff.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-09-28T09:57:05Z","receivedAt":"2008-09-28T09:57:05Z","isPatch":false,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Jeff King writes:\n\n> Neat. I started on something like this quite a while ago,\n\nDid you publish/send it anywhere?\n\n> We have one major difference in our approaches. In yours, there is a\n> new \"textconv\" attribute that can be used. In mine, I subtly changed the\n> meaning of the \"diff=foo\" attribute to be \"use the diff driver named by\n> diff.foo.*\", and you would set diff.foo.textconv to your command. This\n> is a bit simpler to implement, and it provides a better path forward for\n> defining sets of diff tweaks.\n\nYes, that's an interesting approach too.\n\nOne point in favor of mine is that the \"textconv\" thing is no\nnecessarily limited to diff-ing. That would be really cool to have\n\"blame\" use it too for example (with quite bad performance, but that\nwould be the first time I see a tool able to do this).\n\nWell, OTOH, one could argue that \"blame\" is based on diff-ing, and\ntherefore it's natural to define a diff filter to tell how \"blame\"\nshould work.\n\n> For example, one of the limitations of the current syntax is that you\n> can't say \"Choose automatically whether this is binary or text, but if\n> it is text, use this hunk header.\" But with my scheme it is easy to do:\n>\n> in attributes:\n> file diff=foo\n>\n> in config:\n> [diff \"foo\"]\n> xfuncname = \"some regex\"\n> binary = auto\n\nNo sure that would actually be useful in real life, but it doesn't\nharm to have it. And the argument \"better path forward for defining\nsets of diff tweaks\" is a good one IMO.\n\n-- \nMatthieu\n"},{"id":"91785","messageId":"vpq8wtc1tf7.fsf@bauges.imag.fr","threadId":"15693","inReplyTo":"20080928041526.GB24214@coredump.intra.peff.net","subject":"Re: [PATCH] Implement a textconv filter for \"git diff\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-09-28T10:00:44Z","receivedAt":"2008-09-28T10:00:44Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Jeff King writes:\n\n> On Sun, Sep 28, 2008 at 04:06:56AM +0200, Matthieu Moy wrote:\n>\n>> +static int textconv_two_files(const char *textconv,\n>> +\t\t\t const char *name_a,\n>> +\t\t\t const char *name_b,\n>> +\t\t\t mmfile_t *mf1,\n>> +\t\t\t mmfile_t *mf2,\n>> +\t\t\t struct diff_filespec *one,\n>> +\t\t\t struct diff_filespec *two)\n>> +{\n>\n> Must we always be textconv'ing two files? What if I am comparing\n> \"v1.5:foo.odf\" to \"foo.txt\" in the working tree?\n\nHmm, why not, and the textconv could be different (like comparing\nold:foo.doc with ./foo.odt).\n\n> In my implementation, I textconv one at a time. I just did so from\n> fill_mmfile, so all of diff automagically just sees the converted text.\n\nOne has to be carefull not to call textconv for plumbing commands,\nsince the generated patches are not applicable, and only for the eyes\nof the reader.\n\n-- \nMatthieu\n"},{"id":"91786","messageId":"200809281307.21409.johannes.sixt@telecom.at","threadId":"15693","inReplyTo":"1222567618-22156-5-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH] Document the textconv filter.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-09-28T11:07:21Z","receivedAt":"2008-09-28T11:07:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sonntag, 28. September 2008, Matthieu Moy wrote:\n> +Converting files to text before a diff\n> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> +\n> +The attribute `textconv` affects 'git diff' in a way similar to the\n> +`diff` attribute, but with `textconv`, the user provides only a way to\n> +convert the file into text, and git takes care of doing the diff as\n> +usual (i.e. other options of 'git diff' such as '--color' remain\n> +available).\n> +\n> +The value of `textconv` must be a string, which is the textconv\n> +driver.\n\nWouldn't it be much more useful to have git parse stdout of the custom diff \ntool in order to colorize it? Of course, this would mean that external diff \ntools are more complicated and their stdout has to conform mostly to the git \ndiff syntax. But:\n\n> +To tell git to use the `exif` filter for jpeg images, use:\n> +\n> +----------------------------------------------------------------\n> +*.jpg textconv=exif\n> +----------------------------------------------------------------\n\nIn this very example it would be possible that the external diff driver shows \nthe differences in the image in a window and also produces EXIF information \non stdout.\n\n-- Hannes\n"},{"id":"91791","messageId":"vpq7i8wzc5b.fsf@bauges.imag.fr","threadId":"15693","inReplyTo":"200809281307.21409.johannes.sixt@telecom.at","subject":"Re: [PATCH] Document the textconv filter.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-09-28T12:29:52Z","receivedAt":"2008-09-28T12:29:52Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Johannes Sixt writes:\n\n> On Sonntag, 28. September 2008, Matthieu Moy wrote:\n>> +Converting files to text before a diff\n>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>> +\n>> +The attribute `textconv` affects 'git diff' in a way similar to the\n>> +`diff` attribute, but with `textconv`, the user provides only a way to\n>> +convert the file into text, and git takes care of doing the diff as\n>> +usual (i.e. other options of 'git diff' such as '--color' remain\n>> +available).\n>> +\n>> +The value of `textconv` must be a string, which is the textconv\n>> +driver.\n>\n> Wouldn't it be much more useful to have git parse stdout of the custom diff \n> tool in order to colorize it?\n\nThat would do it for --color, but not --color-words for example. The\nproblem with the GIT_EXTERNAL_DIFF is that the diff tool has to\nre-implement everything that \"git diff\" already do.\n\nTake my opendocument diff script :\n\nhttp://www-verimag.imag.fr/~moy/opendocument/git-oodiff\n\nThat's 77 lines of code as a wrapper to odt2txt and diff. With the\n\"textconv\" attribute, it's one line in .gitattributes, and two in\n~/.gitconfig.\n\n> Of course, this would mean that external diff tools are more\n> complicated and their stdout has to conform mostly to the git diff\n> syntax. But:\n>\n>> +To tell git to use the `exif` filter for jpeg images, use:\n>> +\n>> +----------------------------------------------------------------\n>> +*.jpg textconv=exif\n>> +----------------------------------------------------------------\n>\n> In this very example it would be possible that the external diff driver shows \n> the differences in the image in a window and also produces EXIF information \n> on stdout.\n\nYes, but that's really a specific example. Having git colorize the\ndiff would be a little extra, but that wouldn't reduce the effort to\nwrite the external diff tool itself, and doesn't give you _all_ of\ngit-diff, just --color.\n\n-- \nMatthieu\n"},{"id":"91806","messageId":"20080928161106.GA30199@coredump.intra.peff.net","threadId":"15693","inReplyTo":"vpqd4io1tla.fsf@bauges.imag.fr","subject":"Re: Implementation of a \"textconv\" filter for easy custom diff.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-09-28T16:11:07Z","receivedAt":"2008-09-28T16:11:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 28, 2008 at 11:57:05AM +0200, Matthieu Moy wrote:\n\n> > Neat. I started on something like this quite a while ago,\n> \n> Did you publish/send it anywhere?\n\nNo, I was waiting to clean it up and test it a bit more.\n\n> Well, OTOH, one could argue that \"blame\" is based on diff-ing, and\n> therefore it's natural to define a diff filter to tell how \"blame\"\n> should work.\n\nYes, I would have made that argument. ;)\n\n> > [diff \"foo\"]\n> > xfuncname = \"some regex\"\n> > binary = auto\n> \n> No sure that would actually be useful in real life, but it doesn't\n> harm to have it. And the argument \"better path forward for defining\n> sets of diff tweaks\" is a good one IMO.\n\nYes, I think currently most diff options supersede the decision about\nwhether or not it's binary (like textconv, in which you probably assume\nthe result is diff-able as text). xfuncname doesn't, but the example is\nperhaps a bit contrived. So I do think of it as more of a way for future\nexpansion.\n\nI seem to recall actually running into this as part of the textconv work\nI was doing, but now I can't remember the exact details. So that's not\nthat compelling an argumen.t :)\n\n-Peff\n"},{"id":"91807","messageId":"20080928161250.GB30199@coredump.intra.peff.net","threadId":"15693","inReplyTo":"vpq8wtc1tf7.fsf@bauges.imag.fr","subject":"Re: [PATCH] Implement a textconv filter for \"git diff\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-09-28T16:12:50Z","receivedAt":"2008-09-28T16:12:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 28, 2008 at 12:00:44PM +0200, Matthieu Moy wrote:\n\n> Hmm, why not, and the textconv could be different (like comparing\n> old:foo.doc with ./foo.odt).\n\nExactly.\n\n> One has to be carefull not to call textconv for plumbing commands,\n> since the generated patches are not applicable, and only for the eyes\n> of the reader.\n\nRight, but that is prevented by not loading the appropriate diff driver\nconfig, I believe (and I didn't look closely at your implementation in\nthat respect, but I believe it is the same as mine, because both\nporcelain and plumbing use the code in diff.c).\n\n-Peff\n"},{"id":"91954","messageId":"vpqk5ctfyp6.fsf@bauges.imag.fr","threadId":"15693","inReplyTo":"20080928161106.GA30199@coredump.intra.peff.net","subject":"Re: Implementation of a \"textconv\" filter for easy custom diff.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-09-30T15:19:49Z","receivedAt":"2008-09-30T15:19:49Z","isPatch":false,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Jeff King writes:\n\n> On Sun, Sep 28, 2008 at 11:57:05AM +0200, Matthieu Moy wrote:\n>\n>> > Neat. I started on something like this quite a while ago,\n>> \n>> Did you publish/send it anywhere?\n>\n> No, I was waiting to clean it up and test it a bit more.\n\nWell, if you have time and you think your code is better than mine, I\ncan let you continue on this (and you can pick the parts you want in\nmine). Otherwise, I'd be interested in seeing your draft to\nincorporate the good things in my version.\n\nCheers,\n\n-- \nMatthieu\n"},{"id":"91960","messageId":"20080930164545.GA20305@sigill.intra.peff.net","threadId":"15693","inReplyTo":"vpqk5ctfyp6.fsf@bauges.imag.fr","subject":"Re: Implementation of a \"textconv\" filter for easy custom diff.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-09-30T16:45:46Z","receivedAt":"2008-09-30T16:45:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 30, 2008 at 05:19:49PM +0200, Matthieu Moy wrote:\n\n> Well, if you have time and you think your code is better than mine, I\n> can let you continue on this (and you can pick the parts you want in\n> mine). Otherwise, I'd be interested in seeing your draft to\n> incorporate the good things in my version.\n\nI am about 90% done cleaning it up for preparation (there is a bit of\nrefactoring, and I want to make sure I get that just right). I'll post\nit in the next day or so.\n\n-Peff\n"},{"id":"92361","messageId":"20081005214114.GA21875@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20080930164545.GA20305@sigill.intra.peff.net","subject":"[PATCH 0/4] diff text conversion filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-05T21:41:14Z","receivedAt":"2008-10-05T21:41:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 30, 2008 at 12:45:45PM -0400, Jeff King wrote:\n\n> I am about 90% done cleaning it up for preparation (there is a bit of\n> refactoring, and I want to make sure I get that just right). I'll post\n> it in the next day or so.\n\nSorry, I didn't get a chance to look at this until today. Patch series\nwill follow. It is still missing documentation updates and tests, but I\nwanted to get you something to look at (and as I am proposing a new\nmeaning for \"diff driver\", I would be curious to hear the general\ncomments).\n\nThis is on top of 'next', because it would otherwise conflict with the\nfuncname pattern changes there.\n\n-Peff\n"},{"id":"92362","messageId":"20081005214235.GA21925@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081005214114.GA21875@coredump.intra.peff.net","subject":"[PATCH 1/4] t4012: use test_cmp instead of cmp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-05T21:42:35Z","receivedAt":"2008-10-05T21:42:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This makes erroneous output slightly easier to see. We also\nflip the argument order to match our usual style.\n\nSigned-off-by: Jeff King \n---\nJust a cleanup I noticed while testing.\n\n t/t4012-diff-binary.sh | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh\nindex b8ec6e9..0f954a3 100755\n--- a/t/t4012-diff-binary.sh\n+++ b/t/t4012-diff-binary.sh\n@@ -25,11 +25,11 @@ cat > expected <<\\EOF\n EOF\n test_expect_success 'diff without --binary' \\\n \t'git diff | git apply --stat --summary >current &&\n-\t cmp current expected'\n+\t test_cmp expected current'\n \n test_expect_success 'diff with --binary' \\\n \t'git diff --binary | git apply --stat --summary >current &&\n-\t cmp current expected'\n+\t test_cmp expected current'\n \n # apply needs to be able to skip the binary material correctly\n # in order to report the line number of a corrupt patch.\n-- \n1.6.0.2.639.g4d7f.dirty\n"},{"id":"92363","messageId":"20081005214321.GB21925@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081005214114.GA21875@coredump.intra.peff.net","subject":"[PATCH 2/4] diff: unify external diff and funcname parsing code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-05T21:43:21Z","receivedAt":"2008-10-05T21:43:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Both sets of code assume that one specifies a diff profile\nas a gitattribute via the \"diff=foo\" attribute. They then\npull information about that profile from the config as\ndiff.foo.*.\n\nThe code for each is currently completely separate from the\nother, which has several disadvantages:\n\n - there is duplication as we maintain code to create and\n search the separate lists of external drivers and\n funcname patterns\n\n - it is difficult to add new profile options, since it is\n unclear where they should go\n\n - the code is difficult to follow, as we rely on the\n \"check if this file is binary\" code to find the funcname\n pattern as a side effect. This is the first step in\n refactoring the binary-checking code.\n\nThis patch factors out these diff profiles into \"userdiff\"\ndrivers. A file with \"diff=foo\" uses the \"foo\" driver, which\nis specified by a single struct.\n\nNote that one major difference between the two pieces of\ncode is that the funcname patterns are always loaded,\nwhereas external drivers are loaded only for the \"git diff\"\nporcelain; the new code takes care to retain that situation.\n\nSigned-off-by: Jeff King \n---\nThis is obviously a large and difficult to read diff. But I hope the\nresult is a bit more readable.\n\n Makefile | 2 +\n diff.c | 241 +++++++-----------------------------------------------------\n userdiff.c | 151 +++++++++++++++++++++++++++++++++++++\n userdiff.h | 23 ++++++\n 4 files changed, 203 insertions(+), 214 deletions(-)\n create mode 100644 userdiff.c\n create mode 100644 userdiff.h\n\ndiff --git a/Makefile b/Makefile\nindex 308dc70..d6f3695 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -389,6 +389,7 @@ LIB_H += transport.h\n LIB_H += tree.h\n LIB_H += tree-walk.h\n LIB_H += unpack-trees.h\n+LIB_H += userdiff.h\n LIB_H += utf8.h\n LIB_H += wt-status.h\n \n@@ -485,6 +486,7 @@ LIB_OBJS += tree-diff.o\n LIB_OBJS += tree.o\n LIB_OBJS += tree-walk.o\n LIB_OBJS += unpack-trees.o\n+LIB_OBJS += userdiff.o\n LIB_OBJS += usage.o\n LIB_OBJS += utf8.o\n LIB_OBJS += walker.o\ndiff --git a/diff.c b/diff.c\nindex 4e4e439..08f335f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -11,6 +11,7 @@\n #include \"attr.h\"\n #include \"run-command.h\"\n #include \"utf8.h\"\n+#include \"userdiff.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -56,80 +57,6 @@ 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-\tconst char *name;\n-\tstruct ll_diff_driver *next;\n-\tconst char *cmd;\n-} *user_diff, **user_diff_tail;\n-\n-/*\n- * Currently there is only \"diff..command\" variable;\n- * because there are \"diff.color.\" variables, we are parsing\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-{\n-\tconst char *name;\n-\tint namelen;\n-\tstruct ll_diff_driver *drv;\n-\n-\tname = var + 5;\n-\tnamelen = ep - name;\n-\tfor (drv = user_diff; drv; drv = drv->next)\n-\t\tif (!strncmp(drv->name, name, namelen) && !drv->name[namelen])\n-\t\t\tbreak;\n-\tif (!drv) {\n-\t\tdrv = xcalloc(1, sizeof(struct ll_diff_driver));\n-\t\tdrv->name = xmemdupz(name, namelen);\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}\n-\n-\treturn git_config_string(&(drv->cmd), var, value);\n-}\n-\n-/*\n- * 'diff..funcname' attribute can be specified in the configuration\n- * to define a customized regexp to find the beginning of a function to\n- * be used for hunk header lines of \"diff -p\" style output.\n- */\n-struct funcname_pattern_entry {\n-\tchar *name;\n-\tchar *pattern;\n-\tint cflags;\n-};\n-static struct funcname_pattern_list {\n-\tstruct funcname_pattern_list *next;\n-\tstruct funcname_pattern_entry e;\n-} *funcname_pattern_list;\n-\n-static int parse_funcname_pattern(const char *var, const char *ep, const char *value, int cflags)\n-{\n-\tconst char *name;\n-\tint namelen;\n-\tstruct funcname_pattern_list *pp;\n-\n-\tname = var + 5; /* \"diff.\" */\n-\tnamelen = ep - name;\n-\n-\tfor (pp = funcname_pattern_list; pp; pp = pp->next)\n-\t\tif (!strncmp(pp->e.name, name, namelen) && !pp->e.name[namelen])\n-\t\t\tbreak;\n-\tif (!pp) {\n-\t\tpp = xcalloc(1, sizeof(*pp));\n-\t\tpp->e.name = xmemdupz(name, namelen);\n-\t\tpp->next = funcname_pattern_list;\n-\t\tfuncname_pattern_list = pp;\n-\t}\n-\tfree(pp->e.pattern);\n-\tpp->e.pattern = xstrdup(value);\n-\tpp->e.cflags = cflags;\n-\treturn 0;\n-}\n-\n /*\n * These are to give UI layer defaults.\n * The core-level commands such as git-diff-files should\n@@ -162,11 +89,11 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t}\n \tif (!strcmp(var, \"diff.external\"))\n \t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n-\tif (!prefixcmp(var, \"diff.\")) {\n-\t\tconst char *ep = strrchr(var, '.');\n \n-\t\tif (ep != var + 4 && !strcmp(ep, \".command\"))\n-\t\t\treturn parse_lldiff_command(var, ep, value);\n+\tswitch (userdiff_config_porcelain(var, value)) {\n+\t\tcase 0: break;\n+\t\tcase -1: return -1;\n+\t\tdefault: return 0;\n \t}\n \n \treturn git_diff_basic_config(var, value, cb);\n@@ -193,21 +120,10 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tif (!prefixcmp(var, \"diff.\")) {\n-\t\tconst char *ep = strrchr(var, '.');\n-\t\tif (ep != var + 4) {\n-\t\t\tif (!strcmp(ep, \".funcname\")) {\n-\t\t\t\tif (!value)\n-\t\t\t\t\treturn config_error_nonbool(var);\n-\t\t\t\treturn parse_funcname_pattern(var, ep, value,\n-\t\t\t\t\t0);\n-\t\t\t} else if (!strcmp(ep, \".xfuncname\")) {\n-\t\t\t\tif (!value)\n-\t\t\t\t\treturn config_error_nonbool(var);\n-\t\t\t\treturn parse_funcname_pattern(var, ep, value,\n-\t\t\t\t\tREG_EXTENDED);\n-\t\t\t}\n-\t\t}\n+\tswitch (userdiff_config_basic(var, value)) {\n+\t\tcase 0: break;\n+\t\tcase -1: return -1;\n+\t\tdefault: return 0;\n \t}\n \n \treturn git_color_default_config(var, value, cb);\n@@ -1355,46 +1271,24 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two)\n \temit_binary_diff_body(file, two, one);\n }\n \n-static void setup_diff_attr_check(struct git_attr_check *check)\n-{\n-\tstatic struct git_attr *attr_diff;\n-\n-\tif (!attr_diff) {\n-\t\tattr_diff = git_attr(\"diff\", 4);\n-\t}\n-\tcheck[0].attr = attr_diff;\n-}\n-\n static void diff_filespec_check_attr(struct diff_filespec *one)\n {\n-\tstruct git_attr_check attr_diff_check;\n+\tstruct userdiff_driver *drv;\n \tint check_from_data = 0;\n \n \tif (one->checked_attr)\n \t\treturn;\n \n-\tsetup_diff_attr_check(&attr_diff_check);\n+\tdrv = userdiff_find_by_path(one->path);\n \tone->is_binary = 0;\n-\tone->funcname_pattern_ident = NULL;\n \n-\tif (!git_checkattr(one->path, 1, &attr_diff_check)) {\n-\t\tconst char *value;\n-\n-\t\t/* binaryness */\n-\t\tvalue = attr_diff_check.value;\n-\t\tif (ATTR_TRUE(value))\n-\t\t\t;\n-\t\telse if (ATTR_FALSE(value))\n-\t\t\tone->is_binary = 1;\n-\t\telse\n-\t\t\tcheck_from_data = 1;\n-\n-\t\t/* funcname pattern ident */\n-\t\tif (ATTR_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value))\n-\t\t\t;\n-\t\telse\n-\t\t\tone->funcname_pattern_ident = value;\n-\t}\n+\t/* binaryness */\n+\tif (drv == USERDIFF_ATTR_TRUE)\n+\t\t;\n+\telse if (drv == USERDIFF_ATTR_FALSE)\n+\t\tone->is_binary = 1;\n+\telse\n+\t\tcheck_from_data = 1;\n \n \tif (check_from_data) {\n \t\tif (!one->data && DIFF_FILE_VALID(one))\n@@ -1411,70 +1305,12 @@ int diff_filespec_is_binary(struct diff_filespec *one)\n \treturn one->is_binary;\n }\n \n-static const struct funcname_pattern_entry *funcname_pattern(const char *ident)\n-{\n-\tstruct funcname_pattern_list *pp;\n-\n-\tfor (pp = funcname_pattern_list; pp; pp = pp->next)\n-\t\tif (!strcmp(ident, pp->e.name))\n-\t\t\treturn &pp->e;\n-\treturn NULL;\n-}\n-\n-static const struct funcname_pattern_entry builtin_funcname_pattern[] = {\n-\t{ \"bibtex\", \"(@[a-zA-Z]{1,}[ \\t]*\\\\{{0,1}[ \\t]*[^ \\t\\\"@',\\\\#}{~%]*).*$\",\n-\t REG_EXTENDED },\n-\t{ \"html\", \"^[ \\t]*(<[Hh][1-6][ \\t].*>.*)$\", REG_EXTENDED },\n-\t{ \"java\",\n-\t \"!^[ \\t]*(catch|do|for|if|instanceof|new|return|switch|throw|while)\\n\"\n-\t \"^[ \\t]*(([ \\t]*[A-Za-z_][A-Za-z_0-9]*){2,}[ \\t]*\\\\([^;]*)$\",\n-\t REG_EXTENDED },\n-\t{ \"pascal\",\n-\t \"^((procedure|function|constructor|destructor|interface|\"\n-\t\t\"implementation|initialization|finalization)[ \\t]*.*)$\"\n-\t \"\\n\"\n-\t \"^(.*=[ \\t]*(class|record).*)$\",\n-\t REG_EXTENDED },\n-\t{ \"php\", \"^[\\t ]*((function|class).*)\", REG_EXTENDED },\n-\t{ \"python\", \"^[ \\t]*((class|def)[ \\t].*)$\", REG_EXTENDED },\n-\t{ \"ruby\", \"^[ \\t]*((class|module|def)[ \\t].*)$\",\n-\t REG_EXTENDED },\n-\t{ \"tex\",\n-\t \"^(\\\\\\\\((sub)*section|chapter|part)\\\\*{0,1}\\\\{.*)$\",\n-\t REG_EXTENDED },\n-};\n-\n-static const struct funcname_pattern_entry *diff_funcname_pattern(struct diff_filespec *one)\n+static const struct userdiff_funcname *diff_funcname_pattern(struct diff_filespec *one)\n {\n-\tconst char *ident;\n-\tconst struct funcname_pattern_entry *pe;\n-\tint i;\n-\n-\tdiff_filespec_check_attr(one);\n-\tident = one->funcname_pattern_ident;\n-\n-\tif (!ident)\n-\t\t/*\n-\t\t * If the config file has \"funcname.default\" defined, that\n-\t\t * regexp is used; otherwise NULL is returned and xemit uses\n-\t\t * the built-in default.\n-\t\t */\n-\t\treturn funcname_pattern(\"default\");\n-\n-\t/* Look up custom \"funcname.$ident\" regexp from config. */\n-\tpe = funcname_pattern(ident);\n-\tif (pe)\n-\t\treturn pe;\n-\n-\t/*\n-\t * And define built-in fallback patterns here. Note that\n-\t * these can be overridden by the user's config settings.\n-\t */\n-\tfor (i = 0; i < ARRAY_SIZE(builtin_funcname_pattern); i++)\n-\t\tif (!strcmp(ident, builtin_funcname_pattern[i].name))\n-\t\t\treturn &builtin_funcname_pattern[i];\n-\n-\treturn NULL;\n+\tstruct userdiff_driver *drv = userdiff_find_by_path(one->path);\n+\tif (!drv)\n+\t\tdrv = userdiff_find_by_name(\"default\");\n+\treturn drv && drv->funcname.pattern ? &drv->funcname : NULL;\n }\n \n void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const char *b)\n@@ -1568,7 +1404,7 @@ static void builtin_diff(const char *name_a,\n \t\txdemitconf_t xecfg;\n \t\txdemitcb_t ecb;\n \t\tstruct emit_callback ecbdata;\n-\t\tconst struct funcname_pattern_entry *pe;\n+\t\tconst struct userdiff_funcname *pe;\n \n \t\tpe = diff_funcname_pattern(one);\n \t\tif (!pe)\n@@ -2108,29 +1944,6 @@ static void run_external_diff(const char *pgm,\n \t}\n }\n \n-static const char *external_diff_attr(const char *name)\n-{\n-\tstruct git_attr_check attr_diff_check;\n-\n-\tif (!name)\n-\t\treturn NULL;\n-\n-\tsetup_diff_attr_check(&attr_diff_check);\n-\tif (!git_checkattr(name, 1, &attr_diff_check)) {\n-\t\tconst char *value = attr_diff_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_diff_driver *drv;\n-\n-\t\t\tfor (drv = user_diff; drv; drv = drv->next)\n-\t\t\t\tif (!strcmp(drv->name, value))\n-\t\t\t\t\treturn drv->cmd;\n-\t\t}\n-\t}\n-\treturn NULL;\n-}\n-\n static void run_diff_cmd(const char *pgm,\n \t\t\t const char *name,\n \t\t\t const char *other,\n@@ -2144,9 +1957,9 @@ static void run_diff_cmd(const char *pgm,\n \tif (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))\n \t\tpgm = NULL;\n \telse {\n-\t\tconst char *cmd = external_diff_attr(attr_path);\n-\t\tif (cmd)\n-\t\t\tpgm = cmd;\n+\t\tstruct userdiff_driver *drv = userdiff_find_by_path(attr_path);\n+\t\tif (drv && drv->external)\n+\t\t\tpgm = drv->external;\n \t}\n \n \tif (pgm) {\ndiff --git a/userdiff.c b/userdiff.c\nnew file mode 100644\nindex 0000000..3406adc\n--- /dev/null\n+++ b/userdiff.c\n@@ -0,0 +1,151 @@\n+#include \"userdiff.h\"\n+#include \"cache.h\"\n+#include \"attr.h\"\n+\n+static struct userdiff_driver *drivers;\n+static int ndrivers;\n+static int drivers_alloc;\n+\n+#define FUNCNAME(name, pattern) \\\n+\t{ name, NULL, { pattern, REG_EXTENDED } }\n+static struct userdiff_driver builtin_drivers[] = {\n+FUNCNAME(\"html\", \"^[ \\t]*(<[Hh][1-6][ \\t].*>.*)$\"),\n+FUNCNAME(\"java\",\n+\t \"!^[ \\t]*(catch|do|for|if|instanceof|new|return|switch|throw|while)\\n\"\n+\t \"^[ \\t]*(([ \\t]*[A-Za-z_][A-Za-z_0-9]*){2,}[ \\t]*\\\\([^;]*)$\"),\n+FUNCNAME(\"pascal\",\n+\t \"^((procedure|function|constructor|destructor|interface|\"\n+\t\t\"implementation|initialization|finalization)[ \\t]*.*)$\"\n+\t \"\\n\"\n+\t \"^(.*=[ \\t]*(class|record).*)$\"),\n+FUNCNAME(\"php\", \"^[\\t ]*((function|class).*)\"),\n+FUNCNAME(\"python\", \"^[ \\t]*((class|def)[ \\t].*)$\"),\n+FUNCNAME(\"ruby\", \"^[ \\t]*((class|module|def)[ \\t].*)$\"),\n+FUNCNAME(\"bibtex\", \"(@[a-zA-Z]{1,}[ \\t]*\\\\{{0,1}[ \\t]*[^ \\t\\\"@',\\\\#}{~%]*).*$\"),\n+FUNCNAME(\"tex\", \"^(\\\\\\\\((sub)*section|chapter|part)\\\\*{0,1}\\\\{.*)$\"),\n+};\n+#undef FUNCNAME\n+\n+static struct userdiff_driver driver_true = {\n+\t\"diff=true\",\n+\tNULL,\n+\t{ NULL, 0 }\n+};\n+struct userdiff_driver *USERDIFF_ATTR_TRUE = &driver_true;\n+\n+static struct userdiff_driver driver_false = {\n+\t\"!diff\",\n+\tNULL,\n+\t{ NULL, 0 }\n+};\n+struct userdiff_driver *USERDIFF_ATTR_FALSE = &driver_false;\n+\n+static struct userdiff_driver *userdiff_find_by_namelen(const char *k, int len)\n+{\n+\tint i;\n+\tfor (i = 0; i < ndrivers; i++) {\n+\t\tstruct userdiff_driver *drv = drivers + i;\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+}\n+\n+static struct userdiff_driver *parse_driver(const char *var,\n+\t\tconst char *value, const char *type)\n+{\n+\tstruct userdiff_driver *drv;\n+\tconst char *dot;\n+\tconst char *name;\n+\tint namelen;\n+\n+\tif (prefixcmp(var, \"diff.\"))\n+\t\treturn NULL;\n+\tdot = strrchr(var, '.');\n+\tif (dot == var + 4)\n+\t\treturn NULL;\n+\tif (strcmp(type, dot+1))\n+\t\treturn NULL;\n+\n+\tname = var + 5;\n+\tnamelen = dot - name;\n+\tdrv = userdiff_find_by_namelen(name, namelen);\n+\tif (!drv) {\n+\t\tALLOC_GROW(drivers, ndrivers+1, drivers_alloc);\n+\t\tdrv = &drivers[ndrivers++];\n+\t\tmemset(drv, 0, sizeof(*drv));\n+\t\tdrv->name = xmemdupz(name, namelen);\n+\t}\n+\treturn drv;\n+}\n+\n+static int parse_funcname(struct userdiff_funcname *f, const char *k,\n+\t\tconst char *v, int cflags)\n+{\n+\tif (git_config_string(&f->pattern, k, v) < 0)\n+\t\treturn -1;\n+\tf->cflags = cflags;\n+\treturn 1;\n+}\n+\n+static int parse_string(const char **d, const char *k, const char *v)\n+{\n+\tif (git_config_string(d, k, v) < 0)\n+\t\treturn -1;\n+\treturn 1;\n+}\n+\n+int userdiff_config_basic(const char *k, const char *v)\n+{\n+\tstruct userdiff_driver *drv;\n+\n+\tif ((drv = parse_driver(k, v, \"funcname\")))\n+\t\treturn parse_funcname(&drv->funcname, k, v, 0);\n+\tif ((drv = parse_driver(k, v, \"xfuncname\")))\n+\t\treturn parse_funcname(&drv->funcname, k, v, REG_EXTENDED);\n+\n+\treturn 0;\n+}\n+\n+int userdiff_config_porcelain(const char *k, const char *v)\n+{\n+\tstruct userdiff_driver *drv;\n+\n+\tif ((drv = parse_driver(k, v, \"command\")))\n+\t\treturn parse_string(&drv->external, k, v);\n+\n+\treturn 0;\n+}\n+\n+struct userdiff_driver *userdiff_find_by_name(const char *name) {\n+\tint len = strlen(name);\n+\treturn userdiff_find_by_namelen(name, len);\n+}\n+\n+struct userdiff_driver *userdiff_find_by_path(const char *path)\n+{\n+\tstatic struct git_attr *attr;\n+\tstruct git_attr_check check;\n+\n+\tif (!attr)\n+\t\tattr = git_attr(\"diff\", 4);\n+\tcheck.attr = attr;\n+\n+\tif (!path)\n+\t\treturn NULL;\n+\tif (git_checkattr(path, 1, &check))\n+\t\treturn NULL;\n+\n+\tif (ATTR_TRUE(check.value))\n+\t\treturn &driver_true;\n+\tif (ATTR_FALSE(check.value))\n+\t\treturn &driver_false;\n+\tif (ATTR_UNSET(check.value))\n+\t\treturn NULL;\n+\treturn userdiff_find_by_name(check.value);\n+}\ndiff --git a/userdiff.h b/userdiff.h\nnew file mode 100644\nindex 0000000..c64c5f5\n--- /dev/null\n+++ b/userdiff.h\n@@ -0,0 +1,23 @@\n+#ifndef USERDIFF_H\n+#define USERDIFF_H\n+\n+struct userdiff_funcname {\n+\tconst char *pattern;\n+\tint cflags;\n+};\n+\n+struct userdiff_driver {\n+\tconst char *name;\n+\tconst char *external;\n+\tstruct userdiff_funcname funcname;\n+};\n+\n+extern struct userdiff_driver *USERDIFF_ATTR_TRUE;\n+extern struct userdiff_driver *USERDIFF_ATTR_FALSE;\n+\n+int userdiff_config_basic(const char *k, const char *v);\n+int userdiff_config_porcelain(const char *k, const char *v);\n+struct userdiff_driver *userdiff_find_by_name(const char *name);\n+struct userdiff_driver *userdiff_find_by_path(const char *path);\n+\n+#endif /* USERDIFF */\n-- \n1.6.0.2.639.g4d7f.dirty\n"},{"id":"92364","messageId":"20081005214336.GC21925@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081005214114.GA21875@coredump.intra.peff.net","subject":"[PATCH 3/4] diff: introduce diff..binary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-05T21:43:36Z","receivedAt":"2008-10-05T21:43:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"diff\" gitattribute is somewhat overloaded right now. It\ncan say one of three things:\n\n 1. this file is definitely binary, or definitely not\n (i.e., diff or !diff)\n 2. this file should use an external diff engine (i.e.,\n diff=foo, diff.foo.command = custom-script)\n 3. this file should use particular funcname patterns\n (i.e., diff=foo, diff.foo.(x?)funcname = some-regex)\n\nMost of the time, there is no conflict between these uses,\nsince using one implies that the other is irrelevant (e.g.,\nan external diff engine will decide for itself whether the\nfile is binary).\n\nHowever, there is at least one conflicting situation: there\nis no way to say \"use the regular rules to determine whether\nthis file is binary, but if we do diff it textually, use\nthis funcname pattern.\" That is, currently setting diff=foo\nindicates that the file is definitely text.\n\nThis patch introduces a \"binary\" config option for a diff\ndriver, so that one can explicitly set diff.foo.binary. We\ndefault this value to \"don't know\". That is, setting a diff\nattribute to \"foo\" and using \"diff.foo.funcname\" will have\nno effect on the binaryness of a file. To get the current\nbehavior, one can set diff.foo.binary to true.\n\nThis patch also has one additional advantage: it cleans up\nthe interface to the userdiff code a bit. Before, calling\ncode had to know more about whether attributes were false,\ntrue, or unset to determine binaryness. Now that binaryness\nis a property of a driver, we can represent these situations\njust by passing back a driver struct.\n\nSigned-off-by: Jeff King \n---\n diff.c | 52 ++++++++++++++++++++++------------------------------\n diffcore.h | 8 ++++++--\n userdiff.c | 19 ++++++++++++++++---\n userdiff.h | 4 +---\n 4 files changed, 45 insertions(+), 38 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 08f335f..ba4f6fa 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1271,46 +1271,37 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two)\n \temit_binary_diff_body(file, two, one);\n }\n \n-static void diff_filespec_check_attr(struct diff_filespec *one)\n+void diff_filespec_load_driver(struct diff_filespec *one)\n {\n-\tstruct userdiff_driver *drv;\n-\tint check_from_data = 0;\n-\n-\tif (one->checked_attr)\n-\t\treturn;\n-\n-\tdrv = userdiff_find_by_path(one->path);\n-\tone->is_binary = 0;\n-\n-\t/* binaryness */\n-\tif (drv == USERDIFF_ATTR_TRUE)\n-\t\t;\n-\telse if (drv == USERDIFF_ATTR_FALSE)\n-\t\tone->is_binary = 1;\n-\telse\n-\t\tcheck_from_data = 1;\n-\n-\tif (check_from_data) {\n-\t\tif (!one->data && DIFF_FILE_VALID(one))\n-\t\t\tdiff_populate_filespec(one, 0);\n-\n-\t\tif (one->data)\n-\t\t\tone->is_binary = buffer_is_binary(one->data, one->size);\n-\t}\n+\tif (!one->driver)\n+\t\tone->driver = userdiff_find_by_path(one->path);\n+\tif (!one->driver)\n+\t\tone->driver = userdiff_find_by_name(\"default\");\n }\n \n int diff_filespec_is_binary(struct diff_filespec *one)\n {\n-\tdiff_filespec_check_attr(one);\n+\tif (one->is_binary == -1) {\n+\t\tdiff_filespec_load_driver(one);\n+\t\tif (one->driver->binary != -1)\n+\t\t\tone->is_binary = one->driver->binary;\n+\t\telse {\n+\t\t\tif (!one->data && DIFF_FILE_VALID(one))\n+\t\t\t\tdiff_populate_filespec(one, 0);\n+\t\t\tif (one->data)\n+\t\t\t\tone->is_binary = buffer_is_binary(one->data,\n+\t\t\t\t\t\tone->size);\n+\t\t\tif (one->is_binary == -1)\n+\t\t\t\tone->is_binary = 0;\n+\t\t}\n+\t}\n \treturn one->is_binary;\n }\n \n static const struct userdiff_funcname *diff_funcname_pattern(struct diff_filespec *one)\n {\n-\tstruct userdiff_driver *drv = userdiff_find_by_path(one->path);\n-\tif (!drv)\n-\t\tdrv = userdiff_find_by_name(\"default\");\n-\treturn drv && drv->funcname.pattern ? &drv->funcname : NULL;\n+\tdiff_filespec_load_driver(one);\n+\treturn one->driver->funcname.pattern ? &one->driver->funcname : NULL;\n }\n \n void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const char *b)\n@@ -1558,6 +1549,7 @@ struct diff_filespec *alloc_filespec(const char *path)\n \tspec->path = (char *)(spec + 1);\n \tmemcpy(spec->path, path, namelen+1);\n \tspec->count = 1;\n+\tspec->is_binary = -1;\n \treturn spec;\n }\n \ndiff --git a/diffcore.h b/diffcore.h\nindex 8ae3578..713cca7 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -22,6 +22,8 @@\n \n #define MINIMUM_BREAK_SIZE 400 /* do not break a file smaller than this */\n \n+struct userdiff_driver;\n+\n struct diff_filespec {\n \tunsigned char sha1[20];\n \tchar *path;\n@@ -40,8 +42,10 @@ struct diff_filespec {\n #define DIFF_FILE_VALID(spec) (((spec)->mode) != 0)\n \tunsigned should_free : 1; /* data should be free()'ed */\n \tunsigned should_munmap : 1; /* data should be munmap()'ed */\n-\tunsigned checked_attr : 1;\n-\tunsigned is_binary : 1; /* data should be considered \"binary\" */\n+\n+\tstruct userdiff_driver *driver;\n+\t/* data should be considered \"binary\"; -1 means \"don't know yet\" */\n+\tint is_binary;\n };\n \n extern struct diff_filespec *alloc_filespec(const char *);\ndiff --git a/userdiff.c b/userdiff.c\nindex 3406adc..ac6d4a1 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -7,7 +7,7 @@ static int ndrivers;\n static int drivers_alloc;\n \n #define FUNCNAME(name, pattern) \\\n-\t{ name, NULL, { pattern, REG_EXTENDED } }\n+\t{ name, NULL, -1, { pattern, REG_EXTENDED } }\n static struct userdiff_driver builtin_drivers[] = {\n FUNCNAME(\"html\", \"^[ \\t]*(<[Hh][1-6][ \\t].*>.*)$\"),\n FUNCNAME(\"java\",\n@@ -23,22 +23,23 @@ FUNCNAME(\"python\", \"^[ \\t]*((class|def)[ \\t].*)$\"),\n FUNCNAME(\"ruby\", \"^[ \\t]*((class|module|def)[ \\t].*)$\"),\n FUNCNAME(\"bibtex\", \"(@[a-zA-Z]{1,}[ \\t]*\\\\{{0,1}[ \\t]*[^ \\t\\\"@',\\\\#}{~%]*).*$\"),\n FUNCNAME(\"tex\", \"^(\\\\\\\\((sub)*section|chapter|part)\\\\*{0,1}\\\\{.*)$\"),\n+{ \"default\", NULL, -1, { NULL, 0 } },\n };\n #undef FUNCNAME\n \n static struct userdiff_driver driver_true = {\n \t\"diff=true\",\n \tNULL,\n+\t0,\n \t{ NULL, 0 }\n };\n-struct userdiff_driver *USERDIFF_ATTR_TRUE = &driver_true;\n \n static struct userdiff_driver driver_false = {\n \t\"!diff\",\n \tNULL,\n+\t1,\n \t{ NULL, 0 }\n };\n-struct userdiff_driver *USERDIFF_ATTR_FALSE = &driver_false;\n \n static struct userdiff_driver *userdiff_find_by_namelen(const char *k, int len)\n {\n@@ -80,6 +81,7 @@ static struct userdiff_driver *parse_driver(const char *var,\n \t\tdrv = &drivers[ndrivers++];\n \t\tmemset(drv, 0, sizeof(*drv));\n \t\tdrv->name = xmemdupz(name, namelen);\n+\t\tdrv->binary = -1;\n \t}\n \treturn drv;\n }\n@@ -100,6 +102,15 @@ static int parse_string(const char **d, const char *k, const char *v)\n \treturn 1;\n }\n \n+static int parse_tristate(int *b, const char *k, const char *v)\n+{\n+\tif (v && !strcasecmp(v, \"auto\"))\n+\t\t*b = -1;\n+\telse\n+\t\t*b = git_config_bool(k, v);\n+\treturn 1;\n+}\n+\n int userdiff_config_basic(const char *k, const char *v)\n {\n \tstruct userdiff_driver *drv;\n@@ -108,6 +119,8 @@ int userdiff_config_basic(const char *k, const char *v)\n \t\treturn parse_funcname(&drv->funcname, k, v, 0);\n \tif ((drv = parse_driver(k, v, \"xfuncname\")))\n \t\treturn parse_funcname(&drv->funcname, k, v, REG_EXTENDED);\n+\tif ((drv = parse_driver(k, v, \"binary\")))\n+\t\treturn parse_tristate(&drv->binary, k, v);\n \n \treturn 0;\n }\ndiff --git a/userdiff.h b/userdiff.h\nindex c64c5f5..1c1eb04 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -9,12 +9,10 @@ struct userdiff_funcname {\n struct userdiff_driver {\n \tconst char *name;\n \tconst char *external;\n+\tint binary;\n \tstruct userdiff_funcname funcname;\n };\n \n-extern struct userdiff_driver *USERDIFF_ATTR_TRUE;\n-extern struct userdiff_driver *USERDIFF_ATTR_FALSE;\n-\n int userdiff_config_basic(const char *k, const char *v);\n int userdiff_config_porcelain(const char *k, const char *v);\n struct userdiff_driver *userdiff_find_by_name(const char *name);\n-- \n1.6.0.2.639.g4d7f.dirty\n"},{"id":"92365","messageId":"20081005214345.GD21925@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081005214114.GA21875@coredump.intra.peff.net","subject":"[PATCH 4/4] diff: add filter for converting binary to text","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-05T21:43:45Z","receivedAt":"2008-10-05T21:43:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When diffing binary files, it is sometimes nice to see the\ndifferences of a canonical text form rather than either a\nbinary patch or simply \"binary files differ.\"\n\nUntil now, the only option for doing this was to define an\nexternal diff command to perform the diff. This was a lot of\nwork, since the external command needed to take care of\ndoing the diff itself (including mode changes), and lost the\nbenefit of git's colorization and other options.\n\nThis patch adds a text conversion option, which converts a\nfile to its canonical format before performing the diff.\nThis is less flexible than an arbitrary external diff, but\nis much less work to set up. For example:\n\n $ echo '*.jpg diff=exif' >>.gitattributes\n $ git config diff.exif.textconv exiftool\n $ git config diff.exif.binary false\n\nallows one to see jpg diffs represented by the text output\nof exiftool.\n\nSigned-off-by: Jeff King \n---\n diff.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++--\n userdiff.c | 2 ++\n userdiff.h | 1 +\n 3 files changed, 50 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ba4f6fa..421da29 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -38,6 +38,9 @@ static char diff_colors[][COLOR_MAXLEN] = {\n \t\"\\033[41m\",\t/* WHITESPACE (red background) */\n };\n \n+static void diff_filespec_load_driver(struct diff_filespec *one);\n+static char *run_textconv(const char *, struct diff_filespec *, size_t *);\n+\n static int parse_diff_color_slot(const char *var, int ofs)\n {\n \tif (!strcasecmp(var+ofs, \"plain\"))\n@@ -291,8 +294,19 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)\n \t}\n \telse if (diff_populate_filespec(one, 0))\n \t\treturn -1;\n-\tmf->ptr = one->data;\n-\tmf->size = one->size;\n+\n+\tdiff_filespec_load_driver(one);\n+\tif (one->driver->textconv) {\n+\t\tsize_t size;\n+\t\tmf->ptr = run_textconv(one->driver->textconv, one, &size);\n+\t\tif (!mf->ptr)\n+\t\t\treturn -1;\n+\t\tmf->size = size;\n+\t}\n+\telse {\n+\t\tmf->ptr = one->data;\n+\t\tmf->size = one->size;\n+\t}\n \treturn 0;\n }\n \n@@ -3374,3 +3388,34 @@ void diff_unmerge(struct diff_options *options,\n \tfill_filespec(one, sha1, mode);\n \tdiff_queue(&diff_queued_diff, one, two)->is_unmerged = 1;\n }\n+\n+static char *run_textconv(const char *pgm, struct diff_filespec *spec,\n+\t\tsize_t *outsize)\n+{\n+\tstruct diff_tempfile temp;\n+\tconst char *argv[3];\n+\tconst char **arg = argv;\n+\tstruct child_process child;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tprepare_temp_file(spec->path, &temp, spec);\n+\t*arg++ = pgm;\n+\t*arg++ = temp.name;\n+\t*arg = NULL;\n+\n+\tmemset(&child, 0, sizeof(child));\n+\tchild.argv = argv;\n+\tchild.out = -1;\n+\tif (start_command(&child) != 0 ||\n+\t strbuf_read(&buf, child.out, 0) < 0 ||\n+\t finish_command(&child) != 0) {\n+\t\tif (temp.name == temp.tmp_path)\n+\t\t\tunlink(temp.name);\n+\t\terror(\"error running textconv command '%s'\", pgm);\n+\t\treturn NULL;\n+\t}\n+\tif (temp.name == temp.tmp_path)\n+\t\tunlink(temp.name);\n+\n+\treturn strbuf_detach(&buf, outsize);\n+}\ndiff --git a/userdiff.c b/userdiff.c\nindex ac6d4a1..02f225f 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -131,6 +131,8 @@ int userdiff_config_porcelain(const char *k, const char *v)\n \n \tif ((drv = parse_driver(k, v, \"command\")))\n \t\treturn parse_string(&drv->external, k, v);\n+\tif ((drv = parse_driver(k, v, \"textconv\")))\n+\t\treturn parse_string(&drv->textconv, k, v);\n \n \treturn 0;\n }\ndiff --git a/userdiff.h b/userdiff.h\nindex 1c1eb04..f29c18f 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -11,6 +11,7 @@ struct userdiff_driver {\n \tconst char *external;\n \tint binary;\n \tstruct userdiff_funcname funcname;\n+\tconst char *textconv;\n };\n \n int userdiff_config_basic(const char *k, const char *v);\n-- \n1.6.0.2.639.g4d7f.dirty\n"},{"id":"92367","messageId":"gcbdk1$kpe$1@ger.gmane.org","threadId":"15693","inReplyTo":"20081005214114.GA21875@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] diff text conversion filter","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-05T22:03:48Z","receivedAt":"2008-10-05T22:03:48Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jeff King wrote:\n\n> On Tue, Sep 30, 2008 at 12:45:45PM -0400, Jeff King wrote:\n> \n>> I am about 90% done cleaning it up for preparation (there is a bit of\n>> refactoring, and I want to make sure I get that just right). I'll post\n>> it in the next day or so.\n> \n> Sorry, I didn't get a chance to look at this until today. Patch series\n> will follow. It is still missing documentation updates and tests, but I\n> wanted to get you something to look at (and as I am proposing a new\n> meaning for \"diff driver\", I would be curious to hear the general\n> comments).\n> \n> This is on top of 'next', because it would otherwise conflict with the\n> funcname pattern changes there.\n\nDocumentation, pretty please?\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"92387","messageId":"48E9B036.6090805@viscovery.net","threadId":"15693","inReplyTo":"20081005214114.GA21875@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] diff text conversion filter","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-06T06:29:10Z","receivedAt":"2008-10-06T06:29:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> On Tue, Sep 30, 2008 at 12:45:45PM -0400, Jeff King wrote:\n> \n>> I am about 90% done cleaning it up for preparation (there is a bit of\n>> refactoring, and I want to make sure I get that just right). I'll post\n>> it in the next day or so.\n> \n> Sorry, I didn't get a chance to look at this until today. Patch series\n> will follow. It is still missing documentation updates and tests, but I\n> wanted to get you something to look at (and as I am proposing a new\n> meaning for \"diff driver\", I would be curious to hear the general\n> comments).\n> \n> This is on top of 'next', because it would otherwise conflict with the\n> funcname pattern changes there.\n\nDoes the series in any way change whether plumbing and porcelain invoke\nthe external diff drivers? I have this particular use-case, which I'd like\nthat still works:\n\n- In .git/info/attributes I have specified a diff driver:\n\n *.doc diff=docdiff\n\n The driver runs a script that literally loads the two version of the\n file into MS Word and uses Word's diffing capability.\n\n- git-gui should not use the diff driver. That is, plumbing should bypass\n the diff driver and say \"Binary files differ\". [*]\n\n- Running 'git diff foo.doc', i.e. porcelain, from a command line should\n use the diff driver.\n\n[*] I would not mind seeing a simplified textual diff in git-gui.\n\n-- Hannes\n"},{"id":"92388","messageId":"20081006065212.GA19175@sigill.intra.peff.net","threadId":"15693","inReplyTo":"48E9B036.6090805@viscovery.net","subject":"Re: [PATCH 0/4] diff text conversion filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-06T06:52:13Z","receivedAt":"2008-10-06T06:52:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 06, 2008 at 08:29:10AM +0200, Johannes Sixt wrote:\n\n> Does the series in any way change whether plumbing and porcelain invoke\n> the external diff drivers? I have this particular use-case, which I'd like\n> that still works:\n\nNo, it tries to keep the behavior the same as it is now (in 2/4, note\nhow the diff driver config reading is split into porcelain and plumbing\nsections). Let me know if you have a test case that doesn't work, or\nthat you would like me to try.\n\n-Peff\n"},{"id":"92398","messageId":"48E9D26F.1030007@viscovery.net","threadId":"15693","inReplyTo":"20081006065212.GA19175@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] diff text conversion filter","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-06T08:55:11Z","receivedAt":"2008-10-06T08:55:11Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> On Mon, Oct 06, 2008 at 08:29:10AM +0200, Johannes Sixt wrote:\n> \n>> Does the series in any way change whether plumbing and porcelain invoke\n>> the external diff drivers? I have this particular use-case, which I'd like\n>> that still works:\n> \n> No, it tries to keep the behavior the same as it is now (in 2/4, note\n> how the diff driver config reading is split into porcelain and plumbing\n> sections). Let me know if you have a test case that doesn't work, or\n> that you would like me to try.\n\nA quick check shows that my use-case still works as expected. Thanks.\n\n-- Hannes\n"},{"id":"92419","messageId":"vpqtzbpwy9h.fsf@bauges.imag.fr","threadId":"15693","inReplyTo":"48E9B036.6090805@viscovery.net","subject":"Re: [PATCH 0/4] diff text conversion filter","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-10-06T15:15:22Z","receivedAt":"2008-10-06T15:15:22Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Johannes Sixt writes:\n\n> Does the series in any way change whether plumbing and porcelain invoke\n> the external diff drivers? I have this particular use-case, which I'd like\n> that still works:\n>\n> - In .git/info/attributes I have specified a diff driver:\n>\n> *.doc diff=docdiff\n>\n> The driver runs a script that literally loads the two version of the\n> file into MS Word and uses Word's diffing capability.\n>\n> - git-gui should not use the diff driver. That is, plumbing should bypass\n> the diff driver and say \"Binary files differ\". [*]\n\nActually, I understand you don't want git gui and gitk to load MS-Word\nanytime you click something, but I'd love to see the textconv+diff in\ngitk.\n\n(yeah, that's pretty hard to specify right, the ideal requirement\nseems to be \"in a gui, use the good part of the diff driver, but not\nthe other\" :-\\).\n\n-- \nMatthieu\n"},{"id":"92471","messageId":"20081007012044.GA4217@coredump.intra.peff.net","threadId":"15693","inReplyTo":"vpqtzbpwy9h.fsf@bauges.imag.fr","subject":"Re: [PATCH 0/4] diff text conversion filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-07T01:20:44Z","receivedAt":"2008-10-07T01:20:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 06, 2008 at 05:15:22PM +0200, Matthieu Moy wrote:\n\n> Actually, I understand you don't want git gui and gitk to load MS-Word\n> anytime you click something, but I'd love to see the textconv+diff in\n> gitk.\n> \n> (yeah, that's pretty hard to specify right, the ideal requirement\n> seems to be \"in a gui, use the good part of the diff driver, but not\n> the other\" :-\\).\n\nI think it is even more complex than that. Sometimes when doing \"git\nshow\" I want to see the textconv'd version, and sometimes I don't. So I\nreally want a command-line flag or environment variable that I can use\nto control it (with a sane default).\n\nSo probably the way forward is to stop relying on \"oh, we just didn't\nparse some of the config, so it won't get used\" to just parsing the\nconfig all the time and having a diffopt for \"do textconv, do external\ndiff, etc\". And then plumbing can make sure that those flags never get\nset, and porcelain can tie them to command-line options.\n\n-Peff\n"},{"id":"92487","messageId":"48EAF902.3040402@viscovery.net","threadId":"15693","inReplyTo":"20081007012044.GA4217@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] diff text conversion filter","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-07T05:52:02Z","receivedAt":"2008-10-07T05:52:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> On Mon, Oct 06, 2008 at 05:15:22PM +0200, Matthieu Moy wrote:\n> \n>> Actually, I understand you don't want git gui and gitk to load MS-Word\n>> anytime you click something, but I'd love to see the textconv+diff in\n>> gitk.\n>>\n>> (yeah, that's pretty hard to specify right, the ideal requirement\n>> seems to be \"in a gui, use the good part of the diff driver, but not\n>> the other\" :-\\).\n> \n> I think it is even more complex than that. Sometimes when doing \"git\n> show\" I want to see the textconv'd version, and sometimes I don't. So I\n> really want a command-line flag or environment variable that I can use\n> to control it (with a sane default).\n\nHow about this: If I run 'git show -- foo.doc' (foo.doc resolves to a\nsingle path, obviously), I want MS Word, but for other uses of 'git show'\nI don't. I think that heuristics could be very effective: With a plain\n'git show' I get the overview of the change, and with 'git show --\nfoo.doc' I drill down into a single document.\n\nOr this: 'git show -p' uses the textconv'd version, 'git show' does not\n(\"Binary files differ\").\n\nBTW, also with 'git diff' I sometimes don't want MS Word to pop up...\n\n-- Hannes\n"},{"id":"92488","messageId":"20081007060014.GA7965@coredump.intra.peff.net","threadId":"15693","inReplyTo":"48EAF902.3040402@viscovery.net","subject":"Re: [PATCH 0/4] diff text conversion filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-07T06:00:14Z","receivedAt":"2008-10-07T06:00:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 07, 2008 at 07:52:02AM +0200, Johannes Sixt wrote:\n\n> How about this: If I run 'git show -- foo.doc' (foo.doc resolves to a\n> single path, obviously), I want MS Word, but for other uses of 'git show'\n> I don't. I think that heuristics could be very effective: With a plain\n> 'git show' I get the overview of the change, and with 'git show --\n> foo.doc' I drill down into a single document.\n\nHrm. I am not opposed to heuristics, but in this case, I don't like the\none you have proposed. ;P\n\nMy specific case that prompted this work is a repository full of\npictures and videos, where I rarely (if ever) change the media content,\nbut frequently change exif tags. So my \"usual\" case is to want to see\n\"git log -p\" with the textconv'd version. The commit diffs are otherwise\ntotally meaningless.\n\nBut I think for many others, they are primarily working with text data,\nbut have some (for example) binary word processor documents. Triggering\nsuch conversions for a single file might make more sense there.\n\nSo I'm not sure there is a heuristic that serves both desires. Which is\nwhy I would lean towards a command-line argument, backed by a config\noption for those repositories which really want it all the time (and let\nme reiterate that such a config option would still have _no_ impact on\nplumbing; applying text conversion there is just plain wrong).\n\n> Or this: 'git show -p' uses the textconv'd version, 'git show' does not\n> (\"Binary files differ\").\n\nAt that point, is there really an advantage over \"git show --textconv\"?\n\n> BTW, also with 'git diff' I sometimes don't want MS Word to pop up...\n\nYes. It is annoying that git can't simply read our minds. :)\n\n-Peff\n"},{"id":"92493","messageId":"vpqprmdlylq.fsf@bauges.imag.fr","threadId":"15693","inReplyTo":"20081007060014.GA7965@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] diff text conversion filter","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-10-07T06:15:45Z","receivedAt":"2008-10-07T06:15:45Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Jeff King writes:\n\n> On Tue, Oct 07, 2008 at 07:52:02AM +0200, Johannes Sixt wrote:\n>\n>> How about this: If I run 'git show -- foo.doc' (foo.doc resolves to a\n>> single path, obviously), I want MS Word, but for other uses of 'git show'\n>> I don't. I think that heuristics could be very effective: With a plain\n>> 'git show' I get the overview of the change, and with 'git show --\n>> foo.doc' I drill down into a single document.\n>\n> Hrm. I am not opposed to heuristics, but in this case, I don't like the\n> one you have proposed. ;P\n>\n> My specific case that prompted this work is a repository full of\n> pictures and videos, where I rarely (if ever) change the media content,\n> but frequently change exif tags. So my \"usual\" case is to want to see\n> \"git log -p\" with the textconv'd version. The commit diffs are otherwise\n> totally meaningless.\n\nSo, you disagree about \"git log\" not showing the textconv, but you\nstill agree with half of the proposal :-P. When the user explicitely\nrequests a single file, he does want textconv (requesting a diff for a\nsingle file and be happy with \"binary files differ\" would be\nstrange ...).\n\nIt seems quite clear to me that we won't get a heuristic right for\neverybody (some diff driver are fast, some are slow, some require an\nexternal GUI, some don't, ...). Better let the thing be nicely\nconfigurable IMHO.\n\nOne proposal: have a diff..activate with several values:\n\n* \"always\": activate the diff driver in any porcelain\n* \"diff\": activate it only for \"git diff\", as currently\n* \"singlefile\": Johannes's heuristic proposal\n\nThat way, one could say easily \"activate exiftags filter all the time,\nbut MS-Word only when I request a diff for a single file\", and this\nleaves room for other values if the need be. Well, there's no room for\n\"use MS-Word native diff tool in git-gui but antiword/catdoc +\ntextconv in 'git log -p'\" here, but do we want it?\n\nOr is all that just overkill?\n\n-- \nMatthieu\n"},{"id":"92536","messageId":"48EB7D74.40302@viscovery.net","threadId":"15693","inReplyTo":"20081005214336.GC21925@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] diff: introduce diff..binary","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-07T15:17:08Z","receivedAt":"2008-10-07T15:17:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> The \"diff\" gitattribute is somewhat overloaded right now. It\n> can say one of three things:\n> \n> 1. this file is definitely binary, or definitely not\n> (i.e., diff or !diff)\n> 2. this file should use an external diff engine (i.e.,\n> diff=foo, diff.foo.command = custom-script)\n> 3. this file should use particular funcname patterns\n> (i.e., diff=foo, diff.foo.(x?)funcname = some-regex)\n> \n> Most of the time, there is no conflict between these uses,\n> since using one implies that the other is irrelevant (e.g.,\n> an external diff engine will decide for itself whether the\n> file is binary).\n> \n> However, there is at least one conflicting situation: there\n> is no way to say \"use the regular rules to determine whether\n> this file is binary, but if we do diff it textually, use\n> this funcname pattern.\" That is, currently setting diff=foo\n> indicates that the file is definitely text.\n\nI don't get your point. Can you provide an example?\n\nThe reason why I'd like to understand the issue is because I like the\ndiff.foo.textconv that you introduce in patch 4/4, but I dislike that I\nhave to set diff.foo.binary to false in order to use the textconv. So, why\nis this .binary needed?\n\n> This patch introduces a \"binary\" config option for a diff\n> driver, so that one can explicitly set diff.foo.binary. We\n> default this value to \"don't know\". That is, setting a diff\n> attribute to \"foo\" and using \"diff.foo.funcname\" will have\n> no effect on the binaryness of a file. To get the current\n> behavior, one can set diff.foo.binary to true.\n\n-- Hannes\n"},{"id":"92537","messageId":"20081007153543.GA26531@coredump.intra.peff.net","threadId":"15693","inReplyTo":"48EB7D74.40302@viscovery.net","subject":"Re: [PATCH 3/4] diff: introduce diff..binary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-07T15:35:43Z","receivedAt":"2008-10-07T15:35:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 07, 2008 at 05:17:08PM +0200, Johannes Sixt wrote:\n\n> > However, there is at least one conflicting situation: there\n> > is no way to say \"use the regular rules to determine whether\n> > this file is binary, but if we do diff it textually, use\n> > this funcname pattern.\" That is, currently setting diff=foo\n> > indicates that the file is definitely text.\n> \n> I don't get your point. Can you provide an example?\n\nLet's say I have a subdirectory of files, some of which are binary. But\nfor those that _aren't_ binary, I want to use a particular funcname\npattern. So I do this:\n\n echo '* diff=foo' >subdir/.gitattributes\n git config diff.foo.funcname some-regex\n\nUnder the old behavior, I have just claimed all of the subdir as text.\nBut that's not necessarily what I wanted.\n\nIn practice, this doesn't happen much, because funcname tends to follow\nthe file extension, as does binary-ness. So you get \"*.java diff=java\",\nand you really would be insane to have binary files named *.java. But I\nthink it makes the concept clearer to explain, and the code simpler, if\nthe various facets of a diff driver are orthogonal. In particular, I\nthink this makes things cleaner for adding new driver properties in the\nfuture.\n\nAs to your complaint (which I think is valid)...\n\n> The reason why I'd like to understand the issue is because I like the\n> diff.foo.textconv that you introduce in patch 4/4, but I dislike that I\n> have to set diff.foo.binary to false in order to use the textconv. So, why\n> is this .binary needed?\n\nI think this .binary is something we can and should get rid of; as it\nstands now, you always end up having to set it along with .textconv. I\nhave considered two ways:\n\n - because textconv is for converting binary to text for diffing, the\n result should always be text. So whenever we do the conversion, we\n should note that it is no longer binary, and automatically treat the\n result as a text diff. So in this case, we are explicitly saying\n that binaryness is _not_ orthogonal to this property of the diff\n driver.\n\n - textconv should arguably just be \"canonicalize\" or similar. That is,\n there is no reason you couldn't take something like text XML and\n canonicalize it for a more readable diff. Or even canonicalize a\n binary file to get a smaller or more sensible binary diff, if you\n wanted to.\n\n In that case, I think the right thing is to set it back to \"unknown,\n check the file contents\" if .binary is not set. So you really are\n saying \"this is just a conversion, treat the canonicalized contents\n exactly as you would have treated the actual contents, including\n binary detection magic\".\n\nAnd both of those obviously involve changes to patch 4/4.\n\n-Peff\n"},{"id":"92539","messageId":"20081007154646.GB26531@coredump.intra.peff.net","threadId":"15693","inReplyTo":"vpqprmdlylq.fsf@bauges.imag.fr","subject":"Re: [PATCH 0/4] diff text conversion filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-07T15:46:46Z","receivedAt":"2008-10-07T15:46:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 07, 2008 at 08:15:45AM +0200, Matthieu Moy wrote:\n\n> One proposal: have a diff..activate with several values:\n> \n> * \"always\": activate the diff driver in any porcelain\n> * \"diff\": activate it only for \"git diff\", as currently\n> * \"singlefile\": Johannes's heuristic proposal\n> \n> That way, one could say easily \"activate exiftags filter all the time,\n> but MS-Word only when I request a diff for a single file\", and this\n> leaves room for other values if the need be. Well, there's no room for\n> \"use MS-Word native diff tool in git-gui but antiword/catdoc +\n> textconv in 'git log -p'\" here, but do we want it?\n> \n> Or is all that just overkill?\n\nActually, as soon as I read the first part of your mail I thought of the\n\"ms-word vs antiword\" situation. The example you gave seems plausible.\nAnd it seems to me that it is really a superset of the problem we are\ndiscussing. That is, gitattributes is really just saying \"this is a\n'foo' type file\". And if we have a mechanism flexible enough to say \"in\nthis situation, this is how you handle 'foo' type files\", then that\nwould work as the basis for implementing these heuristics.\n\nFor example, maybe you could set up some mapping like:\n\n git diff --diffdriver=foo,bar\n\nwhere a file with gitattribute diff=x would look for the driver config\nfor x.foo, then x.bar, and then finally fall back to just x. Then you\ncould easily have (ignoring the fact that I'm not sure about the config\nsyntax for having _3_ section parts):\n\n git config diff.doc.graphical.command ms-word-diff-script\n git config diff.doc.textconv antiword\n\nand then git-gui would be configured to diff with:\n\n git diff --diffdriver=graphical\n\nwhereas a regular \"git diff\" would always fall back to\n\"diff.doc.textconv\". Make sense?\n\n-Peff\n"},{"id":"92541","messageId":"48EB864C.6020000@viscovery.net","threadId":"15693","inReplyTo":"20081007153543.GA26531@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] diff: introduce diff..binary","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-07T15:54:52Z","receivedAt":"2008-10-07T15:54:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> On Tue, Oct 07, 2008 at 05:17:08PM +0200, Johannes Sixt wrote:\n> \n>>> However, there is at least one conflicting situation: there\n>>> is no way to say \"use the regular rules to determine whether\n>>> this file is binary, but if we do diff it textually, use\n>>> this funcname pattern.\" That is, currently setting diff=foo\n>>> indicates that the file is definitely text.\n>> I don't get your point. Can you provide an example?\n> \n> Let's say I have a subdirectory of files, some of which are binary. But\n> for those that _aren't_ binary, I want to use a particular funcname\n> pattern. So I do this:\n> \n> echo '* diff=foo' >subdir/.gitattributes\n> git config diff.foo.funcname some-regex\n> \n> Under the old behavior, I have just claimed all of the subdir as text.\n> But that's not necessarily what I wanted.\n\nNo, you claimed that all of the files are of type \"foo\". If this is not\nwhat you wanted, be more specific.\n\n> In practice, this doesn't happen much, because funcname tends to follow\n> the file extension, as does binary-ness. So you get \"*.java diff=java\",\n> and you really would be insane to have binary files named *.java. But I\n> think it makes the concept clearer to explain, and the code simpler, if\n> the various facets of a diff driver are orthogonal. In particular, I\n> think this makes things cleaner for adding new driver properties in the\n> future.\n\nI tend to disagree. \"Binaryness\" is subordinate to the \"type\" of the file,\nwhich is what the diff attribute basically specifies.\n\n> As to your complaint (which I think is valid)...\n> \n>> The reason why I'd like to understand the issue is because I like the\n>> diff.foo.textconv that you introduce in patch 4/4, but I dislike that I\n>> have to set diff.foo.binary to false in order to use the textconv. So, why\n>> is this .binary needed?\n> \n> I think this .binary is something we can and should get rid of; as it\n> stands now, you always end up having to set it along with .textconv. I\n> have considered two ways:\n> \n> - because textconv is for converting binary to text for diffing, the\n> result should always be text. So whenever we do the conversion, we\n> should note that it is no longer binary, and automatically treat the\n> result as a text diff. So in this case, we are explicitly saying\n> that binaryness is _not_ orthogonal to this property of the diff\n> driver.\n> \n> - textconv should arguably just be \"canonicalize\" or similar. That is,\n> there is no reason you couldn't take something like text XML and\n> canonicalize it for a more readable diff. Or even canonicalize a\n> binary file to get a smaller or more sensible binary diff, if you\n> wanted to.\n> \n> In that case, I think the right thing is to set it back to \"unknown,\n> check the file contents\" if .binary is not set. So you really are\n> saying \"this is just a conversion, treat the canonicalized contents\n> exactly as you would have treated the actual contents, including\n> binary detection magic\".\n\nI see your point. But this second item goes too far, in particular,\ncanonicalizing binary files to some other binary form is certainly not\nsomething that should happen under 'git diff' porcelain. (We already have\nclean/smudge filters for this purpose.)\n\nFor the purpose of generating diffs at the porcelain level (*not*\ngenerating patches to be applied, aka format-patch), we can safely stay\nwith the interpretation in the first item above.\n\n-- Hannes\n"},{"id":"92544","messageId":"48EB8B1C.9060201@viscovery.net","threadId":"15693","inReplyTo":"20081007154646.GB26531@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] diff text conversion filter","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-07T16:15:24Z","receivedAt":"2008-10-07T16:15:24Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> On Tue, Oct 07, 2008 at 08:15:45AM +0200, Matthieu Moy wrote:\n> \n>> One proposal: have a diff..activate with several values:\n>>\n>> * \"always\": activate the diff driver in any porcelain\n>> * \"diff\": activate it only for \"git diff\", as currently\n>> * \"singlefile\": Johannes's heuristic proposal\n>>\n>> That way, one could say easily \"activate exiftags filter all the time,\n>> but MS-Word only when I request a diff for a single file\", and this\n>> leaves room for other values if the need be. Well, there's no room for\n>> \"use MS-Word native diff tool in git-gui but antiword/catdoc +\n>> textconv in 'git log -p'\" here, but do we want it?\n>>\n>> Or is all that just overkill?\n> \n> Actually, as soon as I read the first part of your mail I thought of the\n> \"ms-word vs antiword\" situation. The example you gave seems plausible.\n> And it seems to me that it is really a superset of the problem we are\n> discussing. That is, gitattributes is really just saying \"this is a\n> 'foo' type file\". And if we have a mechanism flexible enough to say \"in\n> this situation, this is how you handle 'foo' type files\", then that\n> would work as the basis for implementing these heuristics.\n> \n> For example, maybe you could set up some mapping like:\n> \n> git diff --diffdriver=foo,bar\n> \n> where a file with gitattribute diff=x would look for the driver config\n> for x.foo, then x.bar, and then finally fall back to just x. Then you\n> could easily have (ignoring the fact that I'm not sure about the config\n> syntax for having _3_ section parts):\n> \n> git config diff.doc.graphical.command ms-word-diff-script\n> git config diff.doc.textconv antiword\n> \n> and then git-gui would be configured to diff with:\n> \n> git diff --diffdriver=graphical\n> \n> whereas a regular \"git diff\" would always fall back to\n> \"diff.doc.textconv\". Make sense?\n\nI think you are complicating things. We already have\n\n diff.doc.command ms-word-diff-script\n\nand with your 4-patch-series-under-discussion we would have\n\n diff.doc.texconv antiword\n\nand that should be sufficient. I'm proposing this heuristics:\n\n * If only textconv is given, all porcelains pick it.\n * If only command is given, all porcelains pick it.\n * If both are given, then\n - git log picks textconv.\n - git show and git diff:\n . if exactly one pathspec was given, pick command;\n . otherwise pick textconv\n\nPlumbing never picks any of them, just like today, nor should git\nformat-patch. The are other porcelains that could be sorted into this\nlist, like git blame and (the summary line of) git commit.\n\nBTW, please don't take git-gui as an example that would lauch MS-Word on\neach diff. (Neither would gitk.) Both rely on plumbing, and that's good.\ngitk has a menu entry \"External diff\", where the diff.doc.command could be\nhooked into.\n\n-- Hannes\n"},{"id":"92820","messageId":"7vabdaidwj.fsf@gitster.siamese.dyndns.org","threadId":"15693","inReplyTo":"20081007153543.GA26531@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] diff: introduce diff..binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-12T05:24:44Z","receivedAt":"2008-10-12T05:24:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Let's say I have a subdirectory of files, some of which are binary. But\n> for those that _aren't_ binary, I want to use a particular funcname\n> pattern. So I do this:\n>\n> echo '* diff=foo' >subdir/.gitattributes\n> git config diff.foo.funcname some-regex\n> ...\n> In practice, this doesn't happen much, because funcname tends to follow\n> the file extension, as does binary-ness.\n\nI find this a highly contrived example. Is this ever be useful in\npractice?\n\n>> The reason why I'd like to understand the issue is because I like the\n>> diff.foo.textconv that you introduce in patch 4/4, but I dislike that I\n>> have to set diff.foo.binary to false in order to use the textconv. So, why\n>> is this .binary needed?\n>\n> I think this .binary is something we can and should get rid of; as it\n> stands now, you always end up having to set it along with .textconv. I\n> have considered two ways:\n\nThe logic behind the original behaviour was that the file ought to be\n\"diff-able\" if you are setting up funcname pattern because the funcname\npattern only makes sense if you are doing the textual diff. In other\nwords, \"should we do textual diff?\" and \"what funcname pattern should we\nuse?\" are _not_ orthogonal, as wanting to configure the latter does imply\nthat you do want to see the change in the textual diff format.\n\nFor the same rationale, if you have .textconv, I think it is natural for\nus to say that you do want to see the change in the textual diff format.\nSo I'd agree that you can get rid of this .binary business by saying that\nhaving .textconv marks it diffable (IOW, I think your first alternative\nmakes more sense).\n"},{"id":"92889","messageId":"20081013012311.GE3768@coredump.intra.peff.net","threadId":"15693","inReplyTo":"7vabdaidwj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/4] diff: introduce diff..binary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-13T01:23:11Z","receivedAt":"2008-10-13T01:23:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 11, 2008 at 10:24:44PM -0700, Junio C Hamano wrote:\n\n> > echo '* diff=foo' >subdir/.gitattributes\n> > git config diff.foo.funcname some-regex\n> > ...\n> > In practice, this doesn't happen much, because funcname tends to follow\n> > the file extension, as does binary-ness.\n> \n> I find this a highly contrived example. Is this ever be useful in\n> practice?\n\nWell, I was doing something like it. But after reading JSixt's messages,\nI think I agree that I was probably abusing the attributes system.\n\n> The logic behind the original behaviour was that the file ought to be\n> \"diff-able\" if you are setting up funcname pattern because the funcname\n> pattern only makes sense if you are doing the textual diff. In other\n> words, \"should we do textual diff?\" and \"what funcname pattern should we\n> use?\" are _not_ orthogonal, as wanting to configure the latter does imply\n> that you do want to see the change in the textual diff format.\n\nYeah, I don't think I can really disagree with that. I had some vague\nnotion that it opens the path for adding orthogonal options later. But\nreally, I'm not sure that any exist, since they are, by definition\nrelated to the diff. Unless we want to have diff driver options for how\nto do a binary diff.\n\n> For the same rationale, if you have .textconv, I think it is natural for\n> us to say that you do want to see the change in the textual diff format.\n> So I'd agree that you can get rid of this .binary business by saying that\n> having .textconv marks it diffable (IOW, I think your first alternative\n> makes more sense).\n\nOK. My re-rolled series will keep the assumption that a diff=* attribute\nmakes a file non-binary. However, I will still include the 'binary'\nstruct member in the diff driver, as it greatly simplifies the code. It\nis trivial then to support \"diff.*.binary\" (which would default to\n'false') if we feel like it.\n\n-Peff\n"},{"id":"92890","messageId":"20081013012935.GF3768@coredump.intra.peff.net","threadId":"15693","inReplyTo":"48EB8B1C.9060201@viscovery.net","subject":"Re: [PATCH 0/4] diff text conversion filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-13T01:29:35Z","receivedAt":"2008-10-13T01:29:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 07, 2008 at 06:15:24PM +0200, Johannes Sixt wrote:\n\n> and that should be sufficient. I'm proposing this heuristics:\n> \n> * If only textconv is given, all porcelains pick it.\n> * If only command is given, all porcelains pick it.\n> * If both are given, then\n> - git log picks textconv.\n> - git show and git diff:\n> . if exactly one pathspec was given, pick command;\n> . otherwise pick textconv\n\nI am not 100% convinced that is sufficient, given Matthieu's other post.\nBut I will not personally be using external diff at all, so I am hard\npressed to come up with a counter example.\n\n> Plumbing never picks any of them, just like today, nor should git\n> format-patch. The are other porcelains that could be sorted into this\n> list, like git blame and (the summary line of) git commit.\n\nActually, I think blaming a text-conv'd version would be useful (at\nleast for my case). But cleary it should be optional, and probably\ndefaulting to off.\n\nOnce again, I think this is a good reason to move to an explicit diff\noption for \"respect textconv\" rather than relying on maybe or maybe not\nloading the config.\n\n> BTW, please don't take git-gui as an example that would lauch MS-Word on\n> each diff. (Neither would gitk.) Both rely on plumbing, and that's good.\n> gitk has a menu entry \"External diff\", where the diff.doc.command could be\n> hooked into.\n\nBut they are both porcelain. So in theory, they could behave differently\ndepending on what config is available (or if not them, some other\nthird-party porcelain). But I was just responding to Matthieu's\nuse-case, so I am not sure what exactly people prefer; I am lucky enough\nnot to version any MS-Word documents at all. :)\n\n-Peff\n"},{"id":"92896","messageId":"7vk5cddtzh.fsf@gitster.siamese.dyndns.org","threadId":"15693","inReplyTo":"20081013012311.GE3768@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] diff: introduce diff..binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-13T04:00:50Z","receivedAt":"2008-10-13T04:00:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> The logic behind the original behaviour was that the file ought to be\n>> \"diff-able\" if you are setting up funcname pattern because the funcname\n>> pattern only makes sense if you are doing the textual diff. In other\n>> words, \"should we do textual diff?\" and \"what funcname pattern should we\n>> use?\" are _not_ orthogonal, as wanting to configure the latter does imply\n>> that you do want to see the change in the textual diff format.\n>\n> Yeah, I don't think I can really disagree with that. I had some vague\n> notion that it opens the path for adding orthogonal options later. But\n> really, I'm not sure that any exist, since they are, by definition\n> related to the diff. Unless we want to have diff driver options for how\n> to do a binary diff.\n\nI actually have an independent worry about this whole thing.\n\nThe textconv filter is primarily for humans to be able to view the diff,\nas far as I understand it, but what would this facility do to the patch\nexchange workflow? There needs either one or both of the following:\n\n - A command line option to Porcelains to override textconv so that an\n applicable binary diff can be obtained upon request (or format-patch\n always disables textconv); and/or\n\n - You teach git-apply to use a reverse transformation of textconv, so\n that it does, upon reception of a textconv diff:\n\n (1) pass existing preimage through textconv;\n (2) apply the patch;\n (3) convert the result back to binary.\n\nEven if we do not implement the latter right now in the first round, if we\nwant to leave the door open for later for it, we would somehow need to\nmark such a patch between textconv output so that what kind of reverse\ntransformation needs to be applied.\n\nI'd say that format-patch should always disable textconv so that we won't\nhave to worry about it for now. If we enable textconv for format-patch,\nwe do need to include what textconv filter (or \"diff attribute\") was used\nfor the path in the output, so that the receiving end can pick a\ncorresponding reverse transformation.\n"},{"id":"92899","messageId":"20081013041525.GA32629@coredump.intra.peff.net","threadId":"15693","inReplyTo":"7vk5cddtzh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/4] diff: introduce diff..binary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-13T04:15:25Z","receivedAt":"2008-10-13T04:15:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 12, 2008 at 09:00:50PM -0700, Junio C Hamano wrote:\n\n> exchange workflow? There needs either one or both of the following:\n> \n> - A command line option to Porcelains to override textconv so that an\n> applicable binary diff can be obtained upon request (or format-patch\n> always disables textconv); and/or\n\nformat-patch should always disable textconv. I admit that I didn't test\nit, but assumed it was covered under the same code path as external\ndiff (i.e., just not reading in the config). But it doesn't seem to be.\n\nSo that is definitely broken in my patch series. But even once that is\nfixed, I agree there should be a porcelain switch for turning this off.\nI sometimes do \"git diff >patch\" and read the result into my MUA.\nObviously I would not want textconv'ing there.\n\n> - You teach git-apply to use a reverse transformation of textconv, so\n> that it does, upon reception of a textconv diff:\n> \n> (1) pass existing preimage through textconv;\n> (2) apply the patch;\n> (3) convert the result back to binary.\n\nThe problem with this approach is that it requires that the textconv be\na reversible mapping. And the two motivating examples (dumping exif tags\nand converting word processor documents to text) are not; they are lossy\nconversions.\n\nIt's possible that one could, given the binary preimage and the two\nlossy textconv'd versions, produce a custom binary merge that would just\nmunge the tags, or just munge the text, or whatever. But that is an\norder of magnitude more work than writing a textconv, which is usually\nas simple as \"/path/to/existing/conversion-script\".\n\nAnd the whole point of this exercise was to make it simple to set this\nconversion up.\n\n> I'd say that format-patch should always disable textconv so that we won't\n> have to worry about it for now.\n\nAgreed, if you remove \"for now\". I had never intended for these to be\nanything but a human-readable display (and while I am generally OK with\ngeneralizing functionality when we can, I think there is real value in\nthe simplicity here).\n\nIf somebody really wants to send patches with converted text for\nreference, I would suggest producing a patch with the textconv'd output\nas a comment, and including the full binary patch to actually be\napplied (and yes, obviously they a malicious attacker could make them\nnot match, but given the binary patch, we can trivially regenerate the\ntextconv'd version and confirm it).\n\n-Peff\n"},{"id":"92903","messageId":"48F2E65E.50506@viscovery.net","threadId":"15693","inReplyTo":"20081013041525.GA32629@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] diff: introduce diff..binary","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-13T06:10:38Z","receivedAt":"2008-10-13T06:10:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> On Sun, Oct 12, 2008 at 09:00:50PM -0700, Junio C Hamano wrote:\n>> - You teach git-apply to use a reverse transformation of textconv, so\n>> that it does, upon reception of a textconv diff:\n>>\n>> (1) pass existing preimage through textconv;\n>> (2) apply the patch;\n>> (3) convert the result back to binary.\n> \n> The problem with this approach is that it requires that the textconv be\n> a reversible mapping. And the two motivating examples (dumping exif tags\n> and converting word processor documents to text) are not; they are lossy\n> conversions.\n> \n> It's possible that one could, given the binary preimage and the two\n> lossy textconv'd versions, produce a custom binary merge that would just\n> munge the tags, or just munge the text, or whatever. But that is an\n> order of magnitude more work than writing a textconv, which is usually\n> as simple as \"/path/to/existing/conversion-script\".\n\nI fully agree with you. .texconv should only be used for human\nconsumption. We already have a reversible binary<->text conversion: the\nbinary diffs.\n\n-- Hannes\n"},{"id":"92913","messageId":"vpqtzbggbgy.fsf@bauges.imag.fr","threadId":"15693","inReplyTo":"7vk5cddtzh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/4] diff: introduce diff..binary","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-10-13T08:12:29Z","receivedAt":"2008-10-13T08:12:29Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Junio C Hamano writes:\n\n> The textconv filter is primarily for humans to be able to view the diff,\n> as far as I understand it, but what would this facility do to the patch\n> exchange workflow? There needs either one or both of the following:\n>\n> - A command line option to Porcelains to override textconv so that an\n> applicable binary diff can be obtained upon request (or format-patch\n> always disables textconv); and/or\n\nTo me (and others in this thread AIUI), the textconv should only\nreplace the (frustrating) \"binary files differ\" in porcelain.\n\n> - You teach git-apply to use a reverse transformation of textconv, so\n> that it does, upon reception of a textconv diff:\n>\n> (1) pass existing preimage through textconv;\n> (2) apply the patch;\n> (3) convert the result back to binary.\n\nI can imagine corner-case scenarios where this would be applicable and\nsomehow usefull (like: modifying _only_ the EXIF tags on one end, and\nexpect to be able to view and apply the tags modification on the other\nend), but I don't think that's something important to worry about. As\nother said, it would be really hard to set up for users, with little\nbenefits.\n\nOne possibility that would be simpler to set up would be to include\nboth the binary diff and the textconved diff in format-patch, and have\n\"git apply\" just verify that the textconv actually matches the binary\ndiff after applying it. But that reduces the security issue without\nreally solving it: one could very well send a binary patch that\nchanges both the image and the exiftags, and claim it only changes the\ntags.\n\nIn short: agreed with Peff ;-).\n\n-- \nMatthieu\n"},{"id":"92930","messageId":"7vwsgcbnx4.fsf@gitster.siamese.dyndns.org","threadId":"15693","inReplyTo":"20081013041525.GA32629@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] diff: introduce diff..binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-13T13:54:47Z","receivedAt":"2008-10-13T13:54:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It's possible that one could, given the binary preimage and the two\n> lossy textconv'd versions, produce a custom binary merge that would just\n> munge the tags, or just munge the text, or whatever. But that is an\n> order of magnitude more work than writing a textconv, which is usually\n> as simple as \"/path/to/existing/conversion-script\".\n>\n> And the whole point of this exercise was to make it simple to set this\n> conversion up.\n>\n>> I'd say that format-patch should always disable textconv so that we won't\n>> have to worry about it for now.\n>\n> Agreed, if you remove \"for now\". I had never intended for these to be\n> anything but a human-readable display (and while I am generally OK with\n> generalizing functionality when we can, I think there is real value in\n> the simplicity here).\n\nOk. I don't disagree with any of the above.\n\nWe however need to make it very clear that reversibility (aka ability to\nproduce applicable diff) is never the goal of the textconv mechanism (and\nsuggest your \"for applicable and reviewable diff, you produce the standard\nbinary diff and add the textconv diff as a comment\" in the documentation),\nas I am quite sure that 6 months down the road somebody who was not\ninvolved in this thread would complain.\n"},{"id":"93816","messageId":"20081024024631.GA20365@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081005214336.GC21925@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] diff: introduce diff..binary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-24T02:46:32Z","receivedAt":"2008-10-24T02:46:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 05, 2008 at 05:43:36PM -0400, Jeff King wrote:\n\n> However, there is at least one conflicting situation: there\n> is no way to say \"use the regular rules to determine whether\n> this file is binary, but if we do diff it textually, use\n> this funcname pattern.\" That is, currently setting diff=foo\n> indicates that the file is definitely text.\n\nHrm. I don't know what crack I was smoking when I wrote this (and then\nargued about it for weeks afterward). It is actually the _opposite_\nsituation. That is, once you have said \"diff=foo\", there is no way to\nsay \"btw, foo files are _definitely_ text.\"\n\nSee, this is the old code:\n\n> -static void diff_filespec_check_attr(struct diff_filespec *one)\n> +void diff_filespec_load_driver(struct diff_filespec *one)\n> {\n> -\tstruct userdiff_driver *drv;\n> -\tint check_from_data = 0;\n> -\n> -\tif (one->checked_attr)\n> -\t\treturn;\n> -\n> -\tdrv = userdiff_find_by_path(one->path);\n> -\tone->is_binary = 0;\n> -\n> -\t/* binaryness */\n> -\tif (drv == USERDIFF_ATTR_TRUE)\n> -\t\t;\n> -\telse if (drv == USERDIFF_ATTR_FALSE)\n> -\t\tone->is_binary = 1;\n> -\telse\n> -\t\tcheck_from_data = 1;\n> -\n> -\tif (check_from_data) {\n> -\t\tif (!one->data && DIFF_FILE_VALID(one))\n> -\t\t\tdiff_populate_filespec(one, 0);\n> -\n> -\t\tif (one->data)\n> -\t\t\tone->is_binary = buffer_is_binary(one->data, one->size);\n> -\t}\n> +\tif (!one->driver)\n> +\t\tone->driver = userdiff_find_by_path(one->path);\n> +\tif (!one->driver)\n> +\t\tone->driver = userdiff_find_by_name(\"default\");\n> }\n\nYou can clearly see that we use check_from_data as long as the value is\nnot true or false. Meaning if it is unspecified _or_ if it has a string\nvalue (actually, this text is hacked up by my previous patch, but you\ncan look at maint:diff.c and see that it is similar).\n\nAnd this makes sense. Otherwise, plumbing like \"git diff-tree\" would\nget mightily confused by external diff commands. For example, consider\nif you had \"foo diff=bar\" in your attributes file, and defined\n\"diff.bar.command\". That external diff would be used for git-diff, but\n_not_ for diff-tree. But it would be stupid for diff-tree to look at the\nattribute and say \"oh, we have a diff attr, it must be text.\"\n\nSo the patch is right to keep the binary value to \"-1\" except for the\nfew cases where it has been specified explicitly. I found this out when\nI tried to \"fix\" it to the old behavior tonight and discovered lots of\nbreakage.\n\nAnd this also means that diff.*.binary really _does_ do something\nuseful. If you have, for example, a \".foo\" file that looks like binary,\nbut really isn't, _and_ you want to set a custom hunk header for it,\npreviously you were out of luck. You could do one or the other. Now you\ncan do:\n\n git config diff.foo.xfuncname whatever\n git config diff.foo.binary false\n\nand get the desired effect.\n\nAs for the fallout from this with regards to our discussion last week,\nthere were two relevant points:\n\n - Junio suggested that anytime we use funcname, we always want text\n anyway. I think that is reasonable, but it has never been the case up\n until now (in fact, you were stuck with the _opposite_ until now, so\n my series at least makes it possible to say \"always text\", though\n you have to do it manually). So I will leave it for now, and\n if people feel strongly, my series provides a sane basis for a patch\n that does this automatically.\n\n - Johannes complained about having to set \"diff.foo.binary = false\"\n when we have set \"diff.foo.textconv\". I agree that having to set it\n is cumbersome, but what's worse is that it is wrong. You are saying\n \"this file is not binary\" which is only _sometimes_ true. That is, it\n is only true if we are in a command which actually performs the text\n conversion. Plumbing sees _just_ the binary half, which is outright\n wrong (and which became painfully obvious once I wrote some tests).\n\n The solution is that textconv'd data should always be treated as\n text, and that takes some refactoring of my patches. I will post a\n series dealing with this in a minute.\n\nHopefully that explanation made sense. This turned out to be a lot\ntrickier than I thought (in combination with my apparent crack habit),\nand I just spent several hours trying to figure out all of the niggling\ndetails. But I realize the rest of you haven't thought about it for at\nleast a week. :)\n\n-Peff\n"},{"id":"93817","messageId":"20081024024859.GA2831@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081024024631.GA20365@coredump.intra.peff.net","subject":"[PATCH 1/5] diff: add missing static declaration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-24T02:48:59Z","receivedAt":"2008-10-24T02:48:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function isn't used outside of diff.c; the 'static' was\nsimply overlooked in the original writing.\n\nSigned-off-by: Jeff King \n---\nJust a cleanup.\n\n diff.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e368fef..d1fd594 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1282,7 +1282,7 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two)\n \temit_binary_diff_body(file, two, one);\n }\n \n-void diff_filespec_load_driver(struct diff_filespec *one)\n+static void diff_filespec_load_driver(struct diff_filespec *one)\n {\n \tif (!one->driver)\n \t\tone->driver = userdiff_find_by_path(one->path);\n-- \n1.6.0.3.518.gdb328.dirty\n"},{"id":"93818","messageId":"20081024025017.GB2831@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081024024631.GA20365@coredump.intra.peff.net","subject":"[PATCH 2/5] add userdiff textconv tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-24T02:50:17Z","receivedAt":"2008-10-24T02:50:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These tests provide a basic sanity check that textconv'd\nfiles work. The tests try to describe how this configuration\n_should_ work; thus some of the tests are marked to expect\nfailure.\n\nIn particular, we fail to actually textconv anything because\nthe 'diff.foo.binary' config option is not set, which will\nbe fixed in the next patch.\n\nSigned-off-by: Jeff King \n---\nNote that this just tests actual patch generation. Diffstat, etc, are\nleft for later, as is discussed in the next patch.\n\n t/t4030-diff-textconv.sh | 80 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 80 insertions(+), 0 deletions(-)\n create mode 100755 t/t4030-diff-textconv.sh\n\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nnew file mode 100755\nindex 0000000..d0d7691\n--- /dev/null\n+++ b/t/t4030-diff-textconv.sh\n@@ -0,0 +1,80 @@\n+#!/bin/sh\n+\n+test_description='diff.*.textconv tests'\n+. ./test-lib.sh\n+\n+find_diff() {\n+\tsed '1,/^index /d' | sed '/^-- $/,$d'\n+}\n+\n+cat >expect.binary <<'EOF'\n+Binary files a/file and b/file differ\n+EOF\n+\n+cat >expect.text <<'EOF'\n+--- a/file\n++++ b/file\n+@@ -1 +1,2 @@\n+ 0\n++1\n+EOF\n+\n+cat >hexdump <<'EOF'\n+#!/bin/sh\n+perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' \"$1\"\n+EOF\n+chmod +x hexdump\n+\n+test_expect_success 'setup binary file with history' '\n+\tprintf \"\\\\0\\\\n\" >file &&\n+\tgit add file &&\n+\tgit commit -m one &&\n+\tprintf \"\\\\1\\\\n\" >>file &&\n+\tgit add file &&\n+\tgit commit -m two\n+'\n+\n+test_expect_success 'file is considered binary by porcelain' '\n+\tgit diff HEAD^ HEAD >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.binary actual\n+'\n+\n+test_expect_success 'file is considered binary by plumbing' '\n+\tgit diff-tree -p HEAD^ HEAD >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.binary actual\n+'\n+\n+test_expect_success 'setup textconv filter' '\n+\techo file diff=foo >.gitattributes &&\n+\tgit config diff.foo.textconv \"$PWD\"/hexdump\n+'\n+\n+test_expect_failure 'diff produces text' '\n+\tgit diff HEAD^ HEAD >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.text actual\n+'\n+\n+test_expect_success 'diff-tree produces binary' '\n+\tgit diff-tree -p HEAD^ HEAD >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.binary actual\n+'\n+\n+test_expect_failure 'log produces text' '\n+\tgit log -1 -p >log &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.text actual\n+'\n+\n+# actually passes, but only because textconv is broken\n+# and its failure mode happens to be the same as success\n+test_expect_failure 'format-patch produces binary' '\n+\tgit format-patch --no-binary --stdout HEAD^ >patch &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.binary actual\n+'\n+\n+test_done\n-- \n1.6.0.3.518.gdb328.dirty\n"},{"id":"93820","messageId":"20081024025330.GC2831@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081024024631.GA20365@coredump.intra.peff.net","subject":"[PATCH 3/5] refactor userdiff textconv code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-24T02:53:30Z","receivedAt":"2008-10-24T02:53:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The initial implementation inserted itself at the level of\nfill_mmfile. That is, the diff_filespec always contained the\nactual file data, but we set up the xdl mmfile with the fake\ndata. This had several disadvantages:\n\n - fill_mmfile gets called from several places, but we don't\n necessarily want all of them to see the textconv'd\n version (e.g., whitespace checking should probably\n consider it binary)\n\n - the filespec itself carried no information about\n binaryness. If we have textconv'd it, then it is no\n longer binary and we need to mark that somehow (which\n lets us avoid having to manually set the filetype to\n non-binary in the diff driver config)\n\nThis patch moves the text conversion to a\ndiff_filespec_textconv function; this function can be called\nto insert the text-converted contents of the file into the\nfilespec. These contents will survive through any\ndiff_populate_filespec calls, meaning that it is safe to\npass the resulting filespec to other diff functions which\nwill look at the content.\n\nWe now turn on the text conversion only for actual diffs,\nnot diffstats or whitespace checking. We may eventually want\nit for other formats, too, but we will be conservative for\nnow.\n\nSigned-off-by: Jeff King \n---\nI think this is much cleaner. I have a nagging worry that a\ntext-converted filespec might live longer than I expect. Maybe it would\nmake sense to free the filespec data after the diff has been generated\n(and then further populate_filespec calls would just restore the\noriginal data).\n\nAlso, I'm happy to hear comments on which functionality should get\ntext-converted. I think I like seeing the --stat report the binary\nchanges.\n\n diff.c | 51 +++++++++++++++++++++++++++++++++++----------\n diffcore.h | 1 +\n t/t4030-diff-textconv.sh | 6 +---\n 3 files changed, 42 insertions(+), 16 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex d1fd594..89bd2ff 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -284,6 +284,36 @@ static void emit_rewrite_diff(const char *name_a,\n \t\tcopy_file_with_prefix(o->file, '+', two->data, two->size, new, reset);\n }\n \n+static int diff_filespec_textconv(struct diff_filespec *one)\n+{\n+\tsize_t size;\n+\tchar *buf;\n+\n+\tif (one->data_is_textconv)\n+\t\treturn 0;\n+\n+\tif (!DIFF_FILE_VALID(one))\n+\t\treturn 0;\n+\n+\tdiff_filespec_load_driver(one);\n+\tif (!one->driver->textconv)\n+\t\treturn 0;\n+\n+\tif (diff_populate_filespec(one, 0) < 0)\n+\t\treturn -1;\n+\n+\tbuf = run_textconv(one->driver->textconv, one, &size);\n+\tif (!buf)\n+\t\treturn -1;\n+\n+\tdiff_free_filespec_blob(one);\n+\tone->data = buf;\n+\tone->size = size;\n+\tone->should_free = 1;\n+\tone->data_is_textconv = 1;\n+\treturn 0;\n+}\n+\n static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)\n {\n \tif (!DIFF_FILE_VALID(one)) {\n@@ -294,18 +324,8 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)\n \telse if (diff_populate_filespec(one, 0))\n \t\treturn -1;\n \n-\tdiff_filespec_load_driver(one);\n-\tif (one->driver->textconv) {\n-\t\tsize_t size;\n-\t\tmf->ptr = run_textconv(one->driver->textconv, one, &size);\n-\t\tif (!mf->ptr)\n-\t\t\treturn -1;\n-\t\tmf->size = size;\n-\t}\n-\telse {\n-\t\tmf->ptr = one->data;\n-\t\tmf->size = one->size;\n-\t}\n+\tmf->ptr = one->data;\n+\tmf->size = one->size;\n \treturn 0;\n }\n \n@@ -1292,6 +1312,8 @@ static void diff_filespec_load_driver(struct diff_filespec *one)\n \n int diff_filespec_is_binary(struct diff_filespec *one)\n {\n+\tif (one->data_is_textconv)\n+\t\treturn 0;\n \tif (one->is_binary == -1) {\n \t\tdiff_filespec_load_driver(one);\n \t\tif (one->driver->binary != -1)\n@@ -1387,6 +1409,10 @@ static void builtin_diff(const char *name_a,\n \t\t}\n \t}\n \n+\tif (diff_filespec_textconv(one) < 0 ||\n+\t diff_filespec_textconv(two) < 0)\n+\t\t\tdie(\"unable to read files to diff\");\n+\n \tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n \t\tdie(\"unable to read files to diff\");\n \n@@ -1778,6 +1804,7 @@ void diff_free_filespec_blob(struct diff_filespec *s)\n \t\ts->should_free = s->should_munmap = 0;\n \t\ts->data = NULL;\n \t}\n+\ts->data_is_textconv = 0;\n }\n \n void diff_free_filespec_data(struct diff_filespec *s)\ndiff --git a/diffcore.h b/diffcore.h\nindex 713cca7..33aebc2 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -42,6 +42,7 @@ struct diff_filespec {\n #define DIFF_FILE_VALID(spec) (((spec)->mode) != 0)\n \tunsigned should_free : 1; /* data should be free()'ed */\n \tunsigned should_munmap : 1; /* data should be munmap()'ed */\n+\tunsigned data_is_textconv : 1;\n \n \tstruct userdiff_driver *driver;\n \t/* data should be considered \"binary\"; -1 means \"don't know yet\" */\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex d0d7691..28d3640 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -51,7 +51,7 @@ test_expect_success 'setup textconv filter' '\n \tgit config diff.foo.textconv \"$PWD\"/hexdump\n '\n \n-test_expect_failure 'diff produces text' '\n+test_expect_success 'diff produces text' '\n \tgit diff HEAD^ HEAD >diff &&\n \tfind_diff actual &&\n \ttest_cmp expect.text actual\n@@ -63,14 +63,12 @@ test_expect_success 'diff-tree produces binary' '\n \ttest_cmp expect.binary actual\n '\n \n-test_expect_failure 'log produces text' '\n+test_expect_success 'log produces text' '\n \tgit log -1 -p >log &&\n \tfind_diff actual &&\n \ttest_cmp expect.text actual\n '\n \n-# actually passes, but only because textconv is broken\n-# and its failure mode happens to be the same as success\n test_expect_failure 'format-patch produces binary' '\n \tgit format-patch --no-binary --stdout HEAD^ >patch &&\n \tfind_diff actual &&\n-- \n1.6.0.3.518.gdb328.dirty\n"},{"id":"93821","messageId":"20081024025553.GD2831@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081024024631.GA20365@coredump.intra.peff.net","subject":"[PATCH 4/5] userdiff: require explicitly allowing textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-24T02:55:54Z","receivedAt":"2008-10-24T02:55:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Diffs that have been produced with textconv almost certainly\ncannot be applied, so we want to be careful not to generate\nthem in things like format-patch.\n\nThis introduces a new diff options, ALLOW_TEXTCONV, which\ncontrols this behavior. It is off by default, but is\nexplicitly turned on for the \"log\" family of commands, as\nwell as the \"diff\" porcelain.\n\nBecause both text conversion and external diffing are\ncontrolled by these diff options, we can get rid of the\n\"plumbing versus porcelain\" distinction when reading the\nconfig. This was an attempt to control the same thing, but\nsuffered from being too coarse-grained.\n\nSigned-off-by: Jeff King \n---\nThis gives us a nice run-time knob to tweak. There was a lot of\ndiscussion last time around about exactly when text conversion should\nhappen. The more I think about it, I'm wondering if perhaps this should\nbe disabled by default, and require \"git log --textconv\" to activate.\n\nJohannes, you had mentioned some heuristics. Maybe you would like to\ntake a stab at implementing them on top of this?\n\n builtin-diff.c | 1 +\n builtin-log.c | 1 +\n diff.c | 24 ++++++++++--------------\n diff.h | 1 +\n t/t4030-diff-textconv.sh | 2 +-\n userdiff.c | 10 +---------\n userdiff.h | 3 +--\n 7 files changed, 16 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 9c8c295..2de5834 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -300,6 +300,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t}\n \tDIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);\n \tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n+\tDIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);\n \n \t/*\n \t * If the user asked for our exit code then don't start a\ndiff --git a/builtin-log.c b/builtin-log.c\nindex a0944f7..75d698f 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -59,6 +59,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\t} else\n \t\t\tdie(\"unrecognized argument: %s\", arg);\n \t}\n+\tDIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);\n }\n \n /*\ndiff --git a/diff.c b/diff.c\nindex 89bd2ff..52feba7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -93,12 +93,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"diff.external\"))\n \t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n \n-\tswitch (userdiff_config_porcelain(var, value)) {\n-\t\tcase 0: break;\n-\t\tcase -1: return -1;\n-\t\tdefault: return 0;\n-\t}\n-\n \treturn git_diff_basic_config(var, value, cb);\n }\n \n@@ -109,6 +103,12 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tswitch (userdiff_config(var, value)) {\n+\t\tcase 0: break;\n+\t\tcase -1: return -1;\n+\t\tdefault: return 0;\n+\t}\n+\n \tif (!prefixcmp(var, \"diff.color.\") || !prefixcmp(var, \"color.diff.\")) {\n \t\tint slot = parse_diff_color_slot(var, 11);\n \t\tif (!value)\n@@ -123,12 +123,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tswitch (userdiff_config_basic(var, value)) {\n-\t\tcase 0: break;\n-\t\tcase -1: return -1;\n-\t\tdefault: return 0;\n-\t}\n-\n \treturn git_color_default_config(var, value, cb);\n }\n \n@@ -1409,9 +1403,11 @@ static void builtin_diff(const char *name_a,\n \t\t}\n \t}\n \n-\tif (diff_filespec_textconv(one) < 0 ||\n-\t diff_filespec_textconv(two) < 0)\n+\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n+\t\tif (diff_filespec_textconv(one) < 0 ||\n+\t\t diff_filespec_textconv(two) < 0)\n \t\t\tdie(\"unable to read files to diff\");\n+\t}\n \n \tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n \t\tdie(\"unable to read files to diff\");\ndiff --git a/diff.h b/diff.h\nindex a49d865..42582ed 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -65,6 +65,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n #define DIFF_OPT_IGNORE_SUBMODULES (1 << 18)\n #define DIFF_OPT_DIRSTAT_CUMULATIVE (1 << 19)\n #define DIFF_OPT_DIRSTAT_BY_FILE (1 << 20)\n+#define DIFF_OPT_ALLOW_TEXTCONV (1 << 21)\n #define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)\n #define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)\n #define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag)\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 28d3640..e456746 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -69,7 +69,7 @@ test_expect_success 'log produces text' '\n \ttest_cmp expect.text actual\n '\n \n-test_expect_failure 'format-patch produces binary' '\n+test_expect_success 'format-patch produces binary' '\n \tgit format-patch --no-binary --stdout HEAD^ >patch &&\n \tfind_diff actual &&\n \ttest_cmp expect.binary actual\ndiff --git a/userdiff.c b/userdiff.c\nindex d95257a..3681062 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -120,7 +120,7 @@ static int parse_tristate(int *b, const char *k, const char *v)\n \treturn 1;\n }\n \n-int userdiff_config_basic(const char *k, const char *v)\n+int userdiff_config(const char *k, const char *v)\n {\n \tstruct userdiff_driver *drv;\n \n@@ -130,14 +130,6 @@ int userdiff_config_basic(const char *k, const char *v)\n \t\treturn parse_funcname(&drv->funcname, k, v, REG_EXTENDED);\n \tif ((drv = parse_driver(k, v, \"binary\")))\n \t\treturn parse_tristate(&drv->binary, k, v);\n-\n-\treturn 0;\n-}\n-\n-int userdiff_config_porcelain(const char *k, const char *v)\n-{\n-\tstruct userdiff_driver *drv;\n-\n \tif ((drv = parse_driver(k, v, \"command\")))\n \t\treturn parse_string(&drv->external, k, v);\n \tif ((drv = parse_driver(k, v, \"textconv\")))\ndiff --git a/userdiff.h b/userdiff.h\nindex f29c18f..ba29457 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -14,8 +14,7 @@ struct userdiff_driver {\n \tconst char *textconv;\n };\n \n-int userdiff_config_basic(const char *k, const char *v);\n-int userdiff_config_porcelain(const char *k, const char *v);\n+int userdiff_config(const char *k, const char *v);\n struct userdiff_driver *userdiff_find_by_name(const char *name);\n struct userdiff_driver *userdiff_find_by_path(const char *path);\n \n-- \n1.6.0.3.518.gdb328.dirty\n"},{"id":"93822","messageId":"20081024025610.GE2831@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081024024631.GA20365@coredump.intra.peff.net","subject":"[PATCH 5/5] document the diff driver textconv feature","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-24T02:56:10Z","receivedAt":"2008-10-24T02:56:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This patch also changes the term \"custom diff driver\" to\n\"external diff driver\"; now that there are more facets of a\n\"custom driver\" than just external diffing, it makes sense\nto refer to the configuration of \"diff.foo.*\" as the \"foo\ndiff driver\", with \"diff.foo.command\" as the \"external\ndriver for foo\".\n\nSigned-off-by: Jeff King \n---\n Documentation/gitattributes.txt | 66 +++++++++++++++++++++++++++++++--------\n 1 files changed, 53 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 2694559..314e2d3 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -213,10 +213,12 @@ with `crlf`, and then `ident` and fed to `filter`.\n Generating diff text\n ~~~~~~~~~~~~~~~~~~~~\n \n-The attribute `diff` affects if 'git-diff' generates textual\n-patch for the path or just says `Binary files differ`. It also\n-can affect what line is shown on the hunk header `@@ -k,l +n,m @@`\n-line.\n+The attribute `diff` affects how 'git' generates diffs for particular\n+files. It can tell git whether to generate a textual patch for the path\n+or to treat the path as a binary file. It can also affect what line is\n+shown on the hunk header `@@ -k,l +n,m @@` line, tell git to use an\n+external command to generate the diff, or ask git to convert binary\n+files to a text format before generating the diff.\n \n Set::\n \n@@ -227,7 +229,8 @@ Set::\n Unset::\n \n \tA path to which the `diff` attribute is unset will\n-\tgenerate `Binary files differ`.\n+\tgenerate `Binary files differ` (or a binary patch, if\n+\tbinary patches are enabled).\n \n Unspecified::\n \n@@ -238,21 +241,21 @@ Unspecified::\n \n String::\n \n-\tDiff is shown using the specified custom diff driver.\n-\tThe driver program is given its input using the same\n-\tcalling convention as used for GIT_EXTERNAL_DIFF\n-\tprogram. This name is also used for custom hunk header\n-\tselection.\n+\tDiff is shown using the specified diff driver. Each driver may\n+\tspecify one or more options, as described in the following\n+\tsection. The options for the diff driver \"foo\" are defined\n+\tby the configuration variables in the \"diff.foo\" section of the\n+\tgit config file.\n \n \n-Defining a custom diff driver\n-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n+Defining an external diff driver\n+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n \n The definition of a diff driver is done in `gitconfig`, not\n `gitattributes` file, so strictly speaking this manual page is a\n wrong place to talk about it. However...\n \n-To define a custom diff driver `jcdiff`, add a section to your\n+To define an external diff driver `jcdiff`, add a section to your\n `$GIT_DIR/config` file (or `$HOME/.gitconfig` file) like this:\n \n ----------------------------------------------------------------\n@@ -328,6 +331,43 @@ patterns are available:\n - `tex` suitable for source code for LaTeX documents.\n \n \n+Performing text diffs of binary files\n+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n+\n+Sometimes it is desirable to see the diff of a text-converted\n+version of some binary files. For example, a word processor\n+document can be converted to an ASCII text representation, and\n+the diff of the text shown. Even though this conversion loses\n+some information, the resulting diff is useful for human\n+viewing (but cannot be applied directly).\n+\n+The `textconv` config option is used to define a program for\n+performing such a conversion. The program should take a single\n+argument, the name of a file to convert, and produce the\n+resulting text on stdout.\n+\n+For example, to show the diff of the exif information of a\n+file instead of the binary information (assuming you have the\n+exif tool installed):\n+\n+------------------------\n+[diff \"jpg\"]\n+\ttextconv = exif\n+------------------------\n+\n+NOTE: The text conversion is generally a one-way conversion;\n+in this example, we lose the actual image contents and focus\n+just on the text data. This means that diffs generated by\n+textconv are _not_ suitable for applying. For this reason,\n+only `git diff` and the `git log` family of commands (i.e.,\n+log, whatchanged, show) will perform text conversion. `git\n+format-patch` will never generate this output. If you want to\n+send somebody a text-converted diff of a binary file (e.g.,\n+because it quickly conveys the changes you have made), you\n+should generate it separately and send it as a comment _in\n+addition to_ the usual binary diff that you might send.\n+\n+\n Performing a three-way merge\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \n-- \n1.6.0.3.518.gdb328.dirty\n"},{"id":"93830","messageId":"4901730B.7020906@viscovery.net","threadId":"15693","inReplyTo":"20081024024631.GA20365@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] diff: introduce diff..binary","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-24T07:02:35Z","receivedAt":"2008-10-24T07:02:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Thank you for picking up the topic again and continuing on it. Your series\nworks nicely here, even without setting diff.*.binary.\n\n-- Hannes\n"},{"id":"93831","messageId":"4901738A.3010107@viscovery.net","threadId":"15693","inReplyTo":"20081024025553.GD2831@coredump.intra.peff.net","subject":"Re: [PATCH 4/5] userdiff: require explicitly allowing textconv","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-24T07:04:42Z","receivedAt":"2008-10-24T07:04:42Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> This introduces a new diff options, ALLOW_TEXTCONV, which\n> controls this behavior. It is off by default, but is\n> explicitly turned on for the \"log\" family of commands, as\n> well as the \"diff\" porcelain.\n...\n> Johannes, you had mentioned some heuristics. Maybe you would like to\n> take a stab at implementing them on top of this?\n\nI'll put it on my todo list for this weekend.\n\n-- Hannes\n"},{"id":"93833","messageId":"4901762A.3090003@viscovery.net","threadId":"15693","inReplyTo":"20081024025330.GC2831@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] refactor userdiff textconv code","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-24T07:15:54Z","receivedAt":"2008-10-24T07:15:54Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> This patch moves the text conversion to a\n> diff_filespec_textconv function; this function can be called\n> to insert the text-converted contents of the file into the\n> filespec. These contents will survive through any\n> diff_populate_filespec calls, meaning that it is safe to\n> pass the resulting filespec to other diff functions which\n> will look at the content.\n\nWhat do we do when symlinks are involved? Pilot error? Or should we\nexclude them from textconv (and diff.*.command, for that matter)\nat all times? See a test case below.\n\n> Also, I'm happy to hear comments on which functionality should get\n> text-converted. I think I like seeing the --stat report the binary\n> changes.\n\nAt this time I also think that it is enough to apply textconv only\nto generate diffs.\n\nTest case follows:\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex e456746..dab4338 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -19,6 +19,18 @@ cat >expect.text <<'EOF'\n +1\n EOF\n\n+cat >expect.typechange <<'EOF'\n+Binary files a/file and /dev/null differ\n+diff --git a/file b/file\n+new file mode 120000\n+index ad8b3d2..67be421\n+--- /dev/null\n++++ b/file\n+@@ -0,0 +1 @@\n++frotz\n+\\ No newline at end of file\n+EOF\n+\n cat >hexdump <<'EOF'\n #!/bin/sh\n perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' \"$1\"\n@@ -75,4 +87,16 @@ test_expect_success 'format-patch produces binary' '\n \ttest_cmp expect.binary actual\n '\n\n+# make a symlink the hard way that works on symlink-challenged file systems\n+test_expect_failure 'textconv does not kick in if there is a type change' '\n+\techo -n frotz > file &&\n+\tgit add file &&\n+\tgit ls-files -s | sed -e s/100644/120000/ |\n+\t\tgit update-index --index-info &&\n+\tgit commit -m typechange &&\n+\tgit show >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.typechange actual\n+'\n+\n test_done\n"},{"id":"93848","messageId":"20081024124043.GA4806@coredump.intra.peff.net","threadId":"15693","inReplyTo":"4901762A.3090003@viscovery.net","subject":"Re: [PATCH 3/5] refactor userdiff textconv code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-24T12:40:44Z","receivedAt":"2008-10-24T12:40:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 24, 2008 at 09:15:54AM +0200, Johannes Sixt wrote:\n\n> What do we do when symlinks are involved? Pilot error? Or should we\n> exclude them from textconv (and diff.*.command, for that matter)\n> at all times? See a test case below.\n\nHrm, I hadn't really considered that. I would guess they should not be\nexcluded from diff.*.command; the external driver gets the mode, so it\nis free to represent symlinks in whatever way it wants.\n\nBut clearly they are going to always be text, so there is no point in\ntextconv'ing them.\n\n> +cat >expect.typechange <<'EOF'\n> +Binary files a/file and /dev/null differ\n> +diff --git a/file b/file\n> +new file mode 120000\n> +index ad8b3d2..67be421\n> +--- /dev/null\n> ++++ b/file\n> +@@ -0,0 +1 @@\n> ++frotz\n> +\\ No newline at end of file\n> +EOF\n> +\n\nI find this output a bit confusing, since it actually breaks it into a\ndeletion and an addition. But I guess that is orthogonal to the textconv\nissue, and intended. I don't think I've ever actually needed to diff a\nsymlink before.\n\n-Peff\n"},{"id":"93853","messageId":"20081024135132.GA11568@coredump.intra.peff.net","threadId":"15693","inReplyTo":"4901762A.3090003@viscovery.net","subject":"Re: [PATCH 3/5] refactor userdiff textconv code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-24T13:51:32Z","receivedAt":"2008-10-24T13:51:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 24, 2008 at 09:15:54AM +0200, Johannes Sixt wrote:\n\n> +cat >expect.typechange <<'EOF'\n> +Binary files a/file and /dev/null differ\n> +diff --git a/file b/file\n> +new file mode 120000\n> +index ad8b3d2..67be421\n> +--- /dev/null\n> ++++ b/file\n> +@@ -0,0 +1 @@\n> ++frotz\n> +\\ No newline at end of file\n> +EOF\n\nActually, I don't think this is right. The typechange has been broken\ninto two parts: the removal of the file contents and the addition of the\nsymlink. So the first part _should_ use the textconv, since it is just\ncomparing the file contents to /dev/null. But the second part should\nnot, since it is by definition just the text of the symlink. Ditto for\ngitlinks, which have a special text representation. So how about this?\n\n-- >8 --\nonly textconv regular files\n\nWe treat symlinks as text containing the results of the\nsymlink, so it doesn't make much sense to text-convert them.\n\nSimilarly gitlink components just end up as the text\n\"Subproject commit $sha1\", which we should leave intact.\n\nNote that a typechange may be broken into two parts: the\nremoval of the old part and the addition of the new. In that\ncase, we _do_ show the textconv for any part which is the\naddition or removal of a file we would ordinarily textconv,\nsince it is purely acting on the file contents.\n\nSigned-off-by: Jeff King \n---\n diff.c | 3 +++\n t/t4030-diff-textconv.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 52feba7..ce1317d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -289,6 +289,9 @@ static int diff_filespec_textconv(struct diff_filespec *one)\n \tif (!DIFF_FILE_VALID(one))\n \t\treturn 0;\n \n+\tif (!S_ISREG(one->mode))\n+\t\treturn 0;\n+\n \tdiff_filespec_load_driver(one);\n \tif (!one->driver->textconv)\n \t\treturn 0;\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex e456746..a5cd99b 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -19,6 +19,22 @@ cat >expect.text <<'EOF'\n +1\n EOF\n \n+cat >expect.typechange <<'EOF'\n+--- a/file\n++++ /dev/null\n+@@ -1,2 +0,0 @@\n+-0\n+-1\n+diff --git a/file b/file\n+new file mode 120000\n+index ad8b3d2..67be421\n+--- /dev/null\n++++ b/file\n+@@ -0,0 +1 @@\n++frotz\n+\\ No newline at end of file\n+EOF\n+\n cat >hexdump <<'EOF'\n #!/bin/sh\n perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' \"$1\"\n@@ -75,4 +91,16 @@ test_expect_success 'format-patch produces binary' '\n \ttest_cmp expect.binary actual\n '\n \n+# make a symlink the hard way that works on symlink-challenged file systems\n+test_expect_failure 'textconv does not act on symlinks' '\n+\techo -n frotz > file &&\n+\tgit add file &&\n+\tgit ls-files -s | sed -e s/100644/120000/ |\n+\t\tgit update-index --index-info &&\n+\tgit commit -m typechange &&\n+\tgit show >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.typechange actual\n+'\n+\n test_done\n-- \n1.6.0.3.523.ge05eb.dirty\n"},{"id":"93856","messageId":"4901D557.90106@viscovery.net","threadId":"15693","inReplyTo":"20081024135132.GA11568@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] refactor userdiff textconv code","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-24T14:01:59Z","receivedAt":"2008-10-24T14:01:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> On Fri, Oct 24, 2008 at 09:15:54AM +0200, Johannes Sixt wrote:\n> Actually, I don't think this is right. The typechange has been broken\n> into two parts: the removal of the file contents and the addition of the\n> symlink. So the first part _should_ use the textconv, since it is just\n> comparing the file contents to /dev/null. But the second part should\n> not, since it is by definition just the text of the symlink. Ditto for\n> gitlinks, which have a special text representation. So how about this?\n\nYour reasoning makes sense, of course. (I thought that forgoing textconv\nin these cases would have a simpler implementation; but it can't be a lot\nsimpler than yours.)\n\n> diff --git a/diff.c b/diff.c\n> index 52feba7..ce1317d 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -289,6 +289,9 @@ static int diff_filespec_textconv(struct diff_filespec *one)\n> \tif (!DIFF_FILE_VALID(one))\n> \t\treturn 0;\n> \n> +\tif (!S_ISREG(one->mode))\n> +\t\treturn 0;\n> +\n> \tdiff_filespec_load_driver(one);\n> \tif (!one->driver->textconv)\n> \t\treturn 0;\n...\n> +test_expect_failure 'textconv does not act on symlinks' '\n\nCan we trust your solution if you still have 'test_expect_failure' here? ;-)\n\n-- Hannes\n"},{"id":"93857","messageId":"20081024140822.GA11612@coredump.intra.peff.net","threadId":"15693","inReplyTo":"4901D557.90106@viscovery.net","subject":"Re: [PATCH 3/5] refactor userdiff textconv code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-24T14:08:22Z","receivedAt":"2008-10-24T14:08:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 24, 2008 at 04:01:59PM +0200, Johannes Sixt wrote:\n\n> Your reasoning makes sense, of course. (I thought that forgoing textconv\n> in these cases would have a simpler implementation; but it can't be a lot\n> simpler than yours.)\n\nYeah, I think preventing it entirely for this case would be much more\ncomplex, since you would have to realize much sooner that a textconv was\ngoing to happen.\n\n> > +test_expect_failure 'textconv does not act on symlinks' '\n> Can we trust your solution if you still have 'test_expect_failure' here? ;-)\n\nHeh. It helps if I commit before format-patch. ;) Fortunately, this was\nthe only change I missed out on sending.\n\n-Peff\n"},{"id":"93883","messageId":"7v3ail3dfy.fsf@gitster.siamese.dyndns.org","threadId":"15693","inReplyTo":"20081024025330.GC2831@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] refactor userdiff textconv code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-24T21:12:17Z","receivedAt":"2008-10-24T21:12:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think this is much cleaner. I have a nagging worry that a\n> text-converted filespec might live longer than I expect. Maybe it would\n> make sense to free the filespec data after the diff has been generated\n> (and then further populate_filespec calls would just restore the\n> original data).\n\nEither that or drop data_is_textconv and have two sets of pair\nin filespec, one for real data and another purely for diff text\ngeneration.\n\nBut I have a very naïve question.\n\nWhy isn't this just primarily a patch to diff.c::builtin_diff() function?\n\nThat is, you let fill_mmfile() to fill the real data in mf1 and mf2 as\nbefore, ask one/two if they have textconv, and if so, convert mf1 and mf2\nin place, and let xdl work on it, like...\n\n\n\tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n\t\tdie(\"unable to read files to diff\");\n\n+\tif (has_textconv(one))\n+\t\tapply_textconv(one, &mf1);\n+\tif (has_textconv(two))\n+\t\tapply_textconv(two, &mf2);\n\n\tif (!DIFF_OPT_TST(o, TEXT) &&\n-\t (diff_filespec_is_binary(one) || diff_filespec_is_binary(two))) {\n+\t ( (diff_filespec_is_binary(one) && !has_textconv(one)) ||\n+\t (diff_filespec_is_binary(two) && !has_textconv(two)) )) {\n\t\t/* Quite common confusing case */\n\t\t...\n\t}\n\telse {\n\t\t/* Crazy xdl interfaces.. */\n\t\tconst char *diffopts = getenv(\"GIT_DIFF_OPTS\");\n\t\t...\n"},{"id":"93895","messageId":"20081024225001.GA13978@coredump.intra.peff.net","threadId":"15693","inReplyTo":"7v3ail3dfy.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/5] refactor userdiff textconv code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-24T22:50:01Z","receivedAt":"2008-10-24T22:50:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 24, 2008 at 02:12:17PM -0700, Junio C Hamano wrote:\n\n> Either that or drop data_is_textconv and have two sets of pair\n> in filespec, one for real data and another purely for diff text\n> generation.\n\nI thought about that. My reasoning against it was two-fold:\n\n 1. We don't want to keep two copies in memory unnecessarily. Of\n course, we could easily free the original, but just store the\n information in a different pointer to make sure they never get\n confused. So that is a non-issue.\n\n 2. I had some vague notion that it is more convenient in the long run\n to do this to the filespec, since we can then transparently pass the\n munged filespec around and pretend like the converted text was the\n original content.\n\n However, I'm not sure exactly _where_ we would want to do this.\n The obvious places are for patch, for diffstat, or for whitespace\n checking. But all of those places use mmfile, so we can munge them\n in the same way. I haven't looked at using this with blame, but I do\n think \"git blame --textconv foo.jpg\" would be useful.\n\n (Actually not true. I did just look for 30 seconds at using this\n with blame, but blame doesn't seem to to use builtin_diff at all).\n\n> That is, you let fill_mmfile() to fill the real data in mf1 and mf2 as\n> before, ask one/two if they have textconv, and if so, convert mf1 and mf2\n> in place, and let xdl work on it, like...\n\nI think your example would work fine, if there is no other advantage to\nhaving a transparently-munged diff_filespec (as above).\n\n-Peff\n"},{"id":"93897","messageId":"20081024225605.GA16774@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081024225001.GA13978@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] refactor userdiff textconv code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-24T22:56:05Z","receivedAt":"2008-10-24T22:56:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 24, 2008 at 06:50:01PM -0400, Jeff King wrote:\n\n> However, I'm not sure exactly _where_ we would want to do this.\n> The obvious places are for patch, for diffstat, or for whitespace\n> checking. But all of those places use mmfile, so we can munge them\n> in the same way. I haven't looked at using this with blame, but I do\n> think \"git blame --textconv foo.jpg\" would be useful.\n> \n> (Actually not true. I did just look for 30 seconds at using this\n> with blame, but blame doesn't seem to to use builtin_diff at all).\n\nAh, I see. It looks like we would have to munge fill_blob_sha1 in\nbuiltin-blame.c. In which case totally splitting this out from\ndiff_filespec is even better, since we don't have one there.\n\nSo let me try to re-roll my series based on your suggestion, and then\nI'll see if I can add \"blame --textconv\" on top.\n\n-Peff\n"},{"id":"93901","messageId":"20081025004815.GA23851@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081024225605.GA16774@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] refactor userdiff textconv code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-25T00:48:15Z","receivedAt":"2008-10-25T00:48:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 24, 2008 at 06:56:05PM -0400, Jeff King wrote:\n\n> Ah, I see. It looks like we would have to munge fill_blob_sha1 in\n> builtin-blame.c. In which case totally splitting this out from\n> diff_filespec is even better, since we don't have one there.\n> \n> So let me try to re-roll my series based on your suggestion, and then\n> I'll see if I can add \"blame --textconv\" on top.\n\nActually, the blame thing is a little trickier than that. The textconv\nstuff is still tied to a diff_filespec because it uses the diff\nprepare_temp_file interface. So I will have to work around that if I\nwant to add blame support.\n\nHowever, I re-did the existing patches according to your suggestions,\nand I think the result is pretty reasonable. Six patches to follow.\n\n-Peff\n"},{"id":"93902","messageId":"20081025005058.GA23903@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081025004815.GA23851@coredump.intra.peff.net","subject":"[PATCH 1/7] diff: add missing static declaration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-25T00:50:58Z","receivedAt":"2008-10-25T00:50:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function isn't used outside of diff.c; the 'static' was\nsimply overlooked in the original writing.\n\nSigned-off-by: Jeff King \n---\nOops, it is actually 7 patches. This one is the same as before.\n\n diff.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e368fef..d1fd594 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1282,7 +1282,7 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two)\n \temit_binary_diff_body(file, two, one);\n }\n \n-void diff_filespec_load_driver(struct diff_filespec *one)\n+static void diff_filespec_load_driver(struct diff_filespec *one)\n {\n \tif (!one->driver)\n \t\tone->driver = userdiff_find_by_path(one->path);\n-- \n1.6.0.3.523.g38597.dirty\n"},{"id":"93903","messageId":"20081025005141.GB23903@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081025004815.GA23851@coredump.intra.peff.net","subject":"[PATCH 2/7] add userdiff textconv tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-25T00:51:41Z","receivedAt":"2008-10-25T00:51:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These tests provide a basic sanity check that textconv'd\nfiles work. The tests try to describe how this configuration\n_should_ work; thus some of the tests are marked to expect\nfailure.\n\nIn particular, we fail to actually textconv anything because\nthe 'diff.foo.binary' config option is not set, which will\nbe fixed in the next patch.\n\nThis also means that some \"expect_failure\" tests actually\nseem to be fixed; in reality, this is just because textconv\nis broken and its failure mode happens to make these tests\nwork.\n\nSigned-off-by: Jeff King \n---\nThis brings in Johannes' typechange test, and adds a diffstat test\nwhich we will fix along the way.\n\n t/t4030-diff-textconv.sh | 118 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 118 insertions(+), 0 deletions(-)\n create mode 100755 t/t4030-diff-textconv.sh\n\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nnew file mode 100755\nindex 0000000..1b09648\n--- /dev/null\n+++ b/t/t4030-diff-textconv.sh\n@@ -0,0 +1,118 @@\n+#!/bin/sh\n+\n+test_description='diff.*.textconv tests'\n+. ./test-lib.sh\n+\n+find_diff() {\n+\tsed '1,/^index /d' | sed '/^-- $/,$d'\n+}\n+\n+cat >expect.binary <<'EOF'\n+Binary files a/file and b/file differ\n+EOF\n+\n+cat >expect.text <<'EOF'\n+--- a/file\n++++ b/file\n+@@ -1 +1,2 @@\n+ 0\n++1\n+EOF\n+\n+cat >hexdump <<'EOF'\n+#!/bin/sh\n+perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' \"$1\"\n+EOF\n+chmod +x hexdump\n+\n+test_expect_success 'setup binary file with history' '\n+\tprintf \"\\\\0\\\\n\" >file &&\n+\tgit add file &&\n+\tgit commit -m one &&\n+\tprintf \"\\\\1\\\\n\" >>file &&\n+\tgit add file &&\n+\tgit commit -m two\n+'\n+\n+test_expect_success 'file is considered binary by porcelain' '\n+\tgit diff HEAD^ HEAD >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.binary actual\n+'\n+\n+test_expect_success 'file is considered binary by plumbing' '\n+\tgit diff-tree -p HEAD^ HEAD >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.binary actual\n+'\n+\n+test_expect_success 'setup textconv filters' '\n+\techo file diff=foo >.gitattributes &&\n+\tgit config diff.foo.textconv \"$PWD\"/hexdump &&\n+\tgit config diff.fail.textconv false\n+'\n+\n+test_expect_failure 'diff produces text' '\n+\tgit diff HEAD^ HEAD >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.text actual\n+'\n+\n+test_expect_success 'diff-tree produces binary' '\n+\tgit diff-tree -p HEAD^ HEAD >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.binary actual\n+'\n+\n+test_expect_failure 'log produces text' '\n+\tgit log -1 -p >log &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.text actual\n+'\n+\n+test_expect_failure 'format-patch produces binary' '\n+\tgit format-patch --no-binary --stdout HEAD^ >patch &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.binary actual\n+'\n+\n+cat >expect.stat <<'EOF'\n+ file | Bin 2 -> 4 bytes\n+ 1 files changed, 0 insertions(+), 0 deletions(-)\n+EOF\n+test_expect_failure 'diffstat does not run textconv' '\n+\techo file diff=fail >.gitattributes &&\n+\tgit diff --stat HEAD^ HEAD >actual &&\n+\ttest_cmp expect.stat actual\n+'\n+# restore working setup\n+echo file diff=foo >.gitattributes\n+\n+cat >expect.typechange <<'EOF'\n+--- a/file\n++++ /dev/null\n+@@ -1,2 +0,0 @@\n+-0\n+-1\n+diff --git a/file b/file\n+new file mode 120000\n+index ad8b3d2..67be421\n+--- /dev/null\n++++ b/file\n+@@ -0,0 +1 @@\n++frotz\n+\\ No newline at end of file\n+EOF\n+# make a symlink the hard way that works on symlink-challenged file systems\n+test_expect_failure 'textconv does not act on symlinks' '\n+\techo -n frotz > file &&\n+\tgit add file &&\n+\tgit ls-files -s | sed -e s/100644/120000/ |\n+\t\tgit update-index --index-info &&\n+\tgit commit -m typechange &&\n+\tgit show >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.typechange actual\n+'\n+\n+test_done\n-- \n1.6.0.3.523.g38597.dirty\n"},{"id":"93904","messageId":"20081025005225.GC23903@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081025004815.GA23851@coredump.intra.peff.net","subject":"[PATCH 3/7] textconv: assume text-converted contents are not binary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-25T00:52:25Z","receivedAt":"2008-10-25T00:52:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Previously, we would use a 'textconv' filter when filling\nthe mmfile_t, but then still check the diff_filespec struct,\nwhich was not modified at all by the text conversion,\nwhether it was binary. And in general, it was, since the\npoint of text conversion is to munge binary contents. This\nmeant that the user had to manually set \"diff.foo.binary =\nfalse\" to convince the text conversion to actually happen.\n\nThis patch pulls the logic for \"can we textconv this\ndiff_filespec?\" into a separate function. We can then check\nthis to see if a filespec which is binary would have been\nconverted.\n\nSigned-off-by: Jeff King \n---\nThis used to be part 1 of the big refactoring patch.\n\n diff.c | 18 ++++++++++++++----\n t/t4030-diff-textconv.sh | 4 ++--\n 2 files changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex d1fd594..8fad215 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -284,8 +284,18 @@ static void emit_rewrite_diff(const char *name_a,\n \t\tcopy_file_with_prefix(o->file, '+', two->data, two->size, new, reset);\n }\n \n+static const char *get_textconv(struct diff_filespec *one)\n+{\n+\tif (!DIFF_FILE_VALID(one))\n+\t\treturn NULL;\n+\tdiff_filespec_load_driver(one);\n+\treturn one->driver->textconv;\n+}\n+\n static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)\n {\n+\tconst char *textconv;\n+\n \tif (!DIFF_FILE_VALID(one)) {\n \t\tmf->ptr = (char *)\"\"; /* does not matter */\n \t\tmf->size = 0;\n@@ -294,10 +304,9 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)\n \telse if (diff_populate_filespec(one, 0))\n \t\treturn -1;\n \n-\tdiff_filespec_load_driver(one);\n-\tif (one->driver->textconv) {\n+\tif ((textconv = get_textconv(one))) {\n \t\tsize_t size;\n-\t\tmf->ptr = run_textconv(one->driver->textconv, one, &size);\n+\t\tmf->ptr = run_textconv(textconv, one, &size);\n \t\tif (!mf->ptr)\n \t\t\treturn -1;\n \t\tmf->size = size;\n@@ -1391,7 +1400,8 @@ static void builtin_diff(const char *name_a,\n \t\tdie(\"unable to read files to diff\");\n \n \tif (!DIFF_OPT_TST(o, TEXT) &&\n-\t (diff_filespec_is_binary(one) || diff_filespec_is_binary(two))) {\n+\t ( (diff_filespec_is_binary(one) && !get_textconv(one)) ||\n+\t (diff_filespec_is_binary(two) && !get_textconv(two)) )) {\n \t\t/* Quite common confusing case */\n \t\tif (mf1.size == mf2.size &&\n \t\t !memcmp(mf1.ptr, mf2.ptr, mf1.size))\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 1b09648..af94e1a 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -52,7 +52,7 @@ test_expect_success 'setup textconv filters' '\n \tgit config diff.fail.textconv false\n '\n \n-test_expect_failure 'diff produces text' '\n+test_expect_success 'diff produces text' '\n \tgit diff HEAD^ HEAD >diff &&\n \tfind_diff actual &&\n \ttest_cmp expect.text actual\n@@ -64,7 +64,7 @@ test_expect_success 'diff-tree produces binary' '\n \ttest_cmp expect.binary actual\n '\n \n-test_expect_failure 'log produces text' '\n+test_expect_success 'log produces text' '\n \tgit log -1 -p >log &&\n \tfind_diff actual &&\n \ttest_cmp expect.text actual\n-- \n1.6.0.3.523.g38597.dirty\n"},{"id":"93905","messageId":"20081025005256.GD23903@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081025004815.GA23851@coredump.intra.peff.net","subject":"[PATCH 4/7] textconv: don't convert for every operation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-25T00:52:56Z","receivedAt":"2008-10-25T00:52:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since we do the text conversion in fill_mmfile, the\nconversion was happening any time we fed data to xdiff,\nincluding diffstat, whitespace checking, etc.\n\nThis patch makes it optional for each caller, and\nconservatively chooses to turn it on only for actual patch\ngeneration.\n\nSigned-off-by: Jeff King \n---\nAnd this was part 2 of the big refactoring patch.\n\n diff.c | 19 ++++++++++---------\n t/t4030-diff-textconv.sh | 2 +-\n 2 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 8fad215..fcdfd7b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -292,7 +292,8 @@ static const char *get_textconv(struct diff_filespec *one)\n \treturn one->driver->textconv;\n }\n \n-static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)\n+static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one,\n+\t\tint try_textconv)\n {\n \tconst char *textconv;\n \n@@ -304,7 +305,7 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)\n \telse if (diff_populate_filespec(one, 0))\n \t\treturn -1;\n \n-\tif ((textconv = get_textconv(one))) {\n+\tif (try_textconv && (textconv = get_textconv(one))) {\n \t\tsize_t size;\n \t\tmf->ptr = run_textconv(textconv, one, &size);\n \t\tif (!mf->ptr)\n@@ -1396,7 +1397,7 @@ static void builtin_diff(const char *name_a,\n \t\t}\n \t}\n \n-\tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n+\tif (fill_mmfile(&mf1, one, 1) < 0 || fill_mmfile(&mf2, two, 1) < 0)\n \t\tdie(\"unable to read files to diff\");\n \n \tif (!DIFF_OPT_TST(o, TEXT) &&\n@@ -1486,7 +1487,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tdata->added = count_lines(two->data, two->size);\n \t\tgoto free_and_return;\n \t}\n-\tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n+\tif (fill_mmfile(&mf1, one, 0) < 0 || fill_mmfile(&mf2, two, 0) < 0)\n \t\tdie(\"unable to read files to diff\");\n \n \tif (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {\n@@ -1528,7 +1529,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \tdata.o = o;\n \tdata.ws_rule = whitespace_rule(attr_path);\n \n-\tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n+\tif (fill_mmfile(&mf1, one, 0) < 0 || fill_mmfile(&mf2, two, 0) < 0)\n \t\tdie(\"unable to read files to diff\");\n \n \t/*\n@@ -2085,8 +2086,8 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)\n \n \t\tif (DIFF_OPT_TST(o, BINARY)) {\n \t\t\tmmfile_t mf;\n-\t\t\tif ((!fill_mmfile(&mf, one) && diff_filespec_is_binary(one)) ||\n-\t\t\t (!fill_mmfile(&mf, two) && diff_filespec_is_binary(two)))\n+\t\t\tif ((!fill_mmfile(&mf, one, 0) && diff_filespec_is_binary(one)) ||\n+\t\t\t (!fill_mmfile(&mf, two, 0) && diff_filespec_is_binary(two)))\n \t\t\t\tabbrev = 40;\n \t\t}\n \t\tstrbuf_addf(&msg, \"index %.*s..%.*s\",\n@@ -2983,8 +2984,8 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \n \t\tdiff_fill_sha1_info(p->one);\n \t\tdiff_fill_sha1_info(p->two);\n-\t\tif (fill_mmfile(&mf1, p->one) < 0 ||\n-\t\t\t\tfill_mmfile(&mf2, p->two) < 0)\n+\t\tif (fill_mmfile(&mf1, p->one, 0) < 0 ||\n+\t\t\t\tfill_mmfile(&mf2, p->two, 0) < 0)\n \t\t\treturn error(\"unable to read files to diff\");\n \n \t\tlen1 = remove_space(p->one->path, strlen(p->one->path));\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex af94e1a..090a21d 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -80,7 +80,7 @@ cat >expect.stat <<'EOF'\n file | Bin 2 -> 4 bytes\n 1 files changed, 0 insertions(+), 0 deletions(-)\n EOF\n-test_expect_failure 'diffstat does not run textconv' '\n+test_expect_success 'diffstat does not run textconv' '\n \techo file diff=fail >.gitattributes &&\n \tgit diff --stat HEAD^ HEAD >actual &&\n \ttest_cmp expect.stat actual\n-- \n1.6.0.3.523.g38597.dirty\n"},{"id":"93906","messageId":"20081025005438.GE23903@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081025004815.GA23851@coredump.intra.peff.net","subject":"[PATCH 5/7] userdiff: require explicitly allowing textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-25T00:54:39Z","receivedAt":"2008-10-25T00:54:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Diffs that have been produced with textconv almost certainly\ncannot be applied, so we want to be careful not to generate\nthem in things like format-patch.\n\nThis introduces a new diff options, ALLOW_TEXTCONV, which\ncontrols this behavior. It is off by default, but is\nexplicitly turned on for the \"log\" family of commands, as\nwell as the \"diff\" porcelain.\n\nBecause both text conversion and external diffing are\ncontrolled by these diff options, we can get rid of the\n\"plumbing versus porcelain\" distinction when reading the\nconfig. This was an attempt to control the same thing, but\nsuffered from being too coarse-grained.\n\nSigned-off-by: Jeff King \n---\nSame idea as before, but adapted to the new calling convention for\nfill_mmfile. It makes the \"did we textconv this?\" test a little bit\nmore hairy. We could perhaps just set a \"we did a textconv\" flag on the\nmmfile, but I am also fine with it as-is (especially since there is only\none call-site).\n\n builtin-diff.c | 1 +\n builtin-log.c | 1 +\n diff.c | 27 ++++++++++++---------------\n diff.h | 1 +\n t/t4030-diff-textconv.sh | 2 +-\n userdiff.c | 10 +---------\n userdiff.h | 3 +--\n 7 files changed, 18 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 9c8c295..2de5834 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -300,6 +300,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t}\n \tDIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);\n \tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n+\tDIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);\n \n \t/*\n \t * If the user asked for our exit code then don't start a\ndiff --git a/builtin-log.c b/builtin-log.c\nindex a0944f7..75d698f 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -59,6 +59,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\t} else\n \t\t\tdie(\"unrecognized argument: %s\", arg);\n \t}\n+\tDIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);\n }\n \n /*\ndiff --git a/diff.c b/diff.c\nindex fcdfd7b..e3bb1ed 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -93,12 +93,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"diff.external\"))\n \t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n \n-\tswitch (userdiff_config_porcelain(var, value)) {\n-\t\tcase 0: break;\n-\t\tcase -1: return -1;\n-\t\tdefault: return 0;\n-\t}\n-\n \treturn git_diff_basic_config(var, value, cb);\n }\n \n@@ -109,6 +103,12 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tswitch (userdiff_config(var, value)) {\n+\t\tcase 0: break;\n+\t\tcase -1: return -1;\n+\t\tdefault: return 0;\n+\t}\n+\n \tif (!prefixcmp(var, \"diff.color.\") || !prefixcmp(var, \"color.diff.\")) {\n \t\tint slot = parse_diff_color_slot(var, 11);\n \t\tif (!value)\n@@ -123,12 +123,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tswitch (userdiff_config_basic(var, value)) {\n-\t\tcase 0: break;\n-\t\tcase -1: return -1;\n-\t\tdefault: return 0;\n-\t}\n-\n \treturn git_color_default_config(var, value, cb);\n }\n \n@@ -1397,12 +1391,15 @@ static void builtin_diff(const char *name_a,\n \t\t}\n \t}\n \n-\tif (fill_mmfile(&mf1, one, 1) < 0 || fill_mmfile(&mf2, two, 1) < 0)\n+\tif (fill_mmfile(&mf1, one, DIFF_OPT_TST(o, ALLOW_TEXTCONV)) < 0 ||\n+\t fill_mmfile(&mf2, two, DIFF_OPT_TST(o, ALLOW_TEXTCONV)) < 0)\n \t\tdie(\"unable to read files to diff\");\n \n \tif (!DIFF_OPT_TST(o, TEXT) &&\n-\t ( (diff_filespec_is_binary(one) && !get_textconv(one)) ||\n-\t (diff_filespec_is_binary(two) && !get_textconv(two)) )) {\n+\t ( (diff_filespec_is_binary(one) &&\n+\t !(DIFF_OPT_TST(o, ALLOW_TEXTCONV) && get_textconv(one))) ||\n+\t (diff_filespec_is_binary(two) &&\n+\t !(DIFF_OPT_TST(o, ALLOW_TEXTCONV) && get_textconv(two))) )) {\n \t\t/* Quite common confusing case */\n \t\tif (mf1.size == mf2.size &&\n \t\t !memcmp(mf1.ptr, mf2.ptr, mf1.size))\ndiff --git a/diff.h b/diff.h\nindex a49d865..42582ed 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -65,6 +65,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n #define DIFF_OPT_IGNORE_SUBMODULES (1 << 18)\n #define DIFF_OPT_DIRSTAT_CUMULATIVE (1 << 19)\n #define DIFF_OPT_DIRSTAT_BY_FILE (1 << 20)\n+#define DIFF_OPT_ALLOW_TEXTCONV (1 << 21)\n #define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)\n #define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)\n #define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag)\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 090a21d..1df48ae 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -70,7 +70,7 @@ test_expect_success 'log produces text' '\n \ttest_cmp expect.text actual\n '\n \n-test_expect_failure 'format-patch produces binary' '\n+test_expect_success 'format-patch produces binary' '\n \tgit format-patch --no-binary --stdout HEAD^ >patch &&\n \tfind_diff actual &&\n \ttest_cmp expect.binary actual\ndiff --git a/userdiff.c b/userdiff.c\nindex d95257a..3681062 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -120,7 +120,7 @@ static int parse_tristate(int *b, const char *k, const char *v)\n \treturn 1;\n }\n \n-int userdiff_config_basic(const char *k, const char *v)\n+int userdiff_config(const char *k, const char *v)\n {\n \tstruct userdiff_driver *drv;\n \n@@ -130,14 +130,6 @@ int userdiff_config_basic(const char *k, const char *v)\n \t\treturn parse_funcname(&drv->funcname, k, v, REG_EXTENDED);\n \tif ((drv = parse_driver(k, v, \"binary\")))\n \t\treturn parse_tristate(&drv->binary, k, v);\n-\n-\treturn 0;\n-}\n-\n-int userdiff_config_porcelain(const char *k, const char *v)\n-{\n-\tstruct userdiff_driver *drv;\n-\n \tif ((drv = parse_driver(k, v, \"command\")))\n \t\treturn parse_string(&drv->external, k, v);\n \tif ((drv = parse_driver(k, v, \"textconv\")))\ndiff --git a/userdiff.h b/userdiff.h\nindex f29c18f..ba29457 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -14,8 +14,7 @@ struct userdiff_driver {\n \tconst char *textconv;\n };\n \n-int userdiff_config_basic(const char *k, const char *v);\n-int userdiff_config_porcelain(const char *k, const char *v);\n+int userdiff_config(const char *k, const char *v);\n struct userdiff_driver *userdiff_find_by_name(const char *name);\n struct userdiff_driver *userdiff_find_by_path(const char *path);\n \n-- \n1.6.0.3.523.g38597.dirty\n"},{"id":"93907","messageId":"20081025005452.GF23903@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081025004815.GA23851@coredump.intra.peff.net","subject":"[PATCH 6/7] document the diff driver textconv feature","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-25T00:54:53Z","receivedAt":"2008-10-25T00:54:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This patch also changes the term \"custom diff driver\" to\n\"external diff driver\"; now that there are more facets of a\n\"custom driver\" than just external diffing, it makes sense\nto refer to the configuration of \"diff.foo.*\" as the \"foo\ndiff driver\", with \"diff.foo.command\" as the \"external\ndriver for foo\".\n\nSigned-off-by: Jeff King \n---\nSame as before.\n\n Documentation/gitattributes.txt | 66 +++++++++++++++++++++++++++++++--------\n 1 files changed, 53 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 2694559..314e2d3 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -213,10 +213,12 @@ with `crlf`, and then `ident` and fed to `filter`.\n Generating diff text\n ~~~~~~~~~~~~~~~~~~~~\n \n-The attribute `diff` affects if 'git-diff' generates textual\n-patch for the path or just says `Binary files differ`. It also\n-can affect what line is shown on the hunk header `@@ -k,l +n,m @@`\n-line.\n+The attribute `diff` affects how 'git' generates diffs for particular\n+files. It can tell git whether to generate a textual patch for the path\n+or to treat the path as a binary file. It can also affect what line is\n+shown on the hunk header `@@ -k,l +n,m @@` line, tell git to use an\n+external command to generate the diff, or ask git to convert binary\n+files to a text format before generating the diff.\n \n Set::\n \n@@ -227,7 +229,8 @@ Set::\n Unset::\n \n \tA path to which the `diff` attribute is unset will\n-\tgenerate `Binary files differ`.\n+\tgenerate `Binary files differ` (or a binary patch, if\n+\tbinary patches are enabled).\n \n Unspecified::\n \n@@ -238,21 +241,21 @@ Unspecified::\n \n String::\n \n-\tDiff is shown using the specified custom diff driver.\n-\tThe driver program is given its input using the same\n-\tcalling convention as used for GIT_EXTERNAL_DIFF\n-\tprogram. This name is also used for custom hunk header\n-\tselection.\n+\tDiff is shown using the specified diff driver. Each driver may\n+\tspecify one or more options, as described in the following\n+\tsection. The options for the diff driver \"foo\" are defined\n+\tby the configuration variables in the \"diff.foo\" section of the\n+\tgit config file.\n \n \n-Defining a custom diff driver\n-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n+Defining an external diff driver\n+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n \n The definition of a diff driver is done in `gitconfig`, not\n `gitattributes` file, so strictly speaking this manual page is a\n wrong place to talk about it. However...\n \n-To define a custom diff driver `jcdiff`, add a section to your\n+To define an external diff driver `jcdiff`, add a section to your\n `$GIT_DIR/config` file (or `$HOME/.gitconfig` file) like this:\n \n ----------------------------------------------------------------\n@@ -328,6 +331,43 @@ patterns are available:\n - `tex` suitable for source code for LaTeX documents.\n \n \n+Performing text diffs of binary files\n+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n+\n+Sometimes it is desirable to see the diff of a text-converted\n+version of some binary files. For example, a word processor\n+document can be converted to an ASCII text representation, and\n+the diff of the text shown. Even though this conversion loses\n+some information, the resulting diff is useful for human\n+viewing (but cannot be applied directly).\n+\n+The `textconv` config option is used to define a program for\n+performing such a conversion. The program should take a single\n+argument, the name of a file to convert, and produce the\n+resulting text on stdout.\n+\n+For example, to show the diff of the exif information of a\n+file instead of the binary information (assuming you have the\n+exif tool installed):\n+\n+------------------------\n+[diff \"jpg\"]\n+\ttextconv = exif\n+------------------------\n+\n+NOTE: The text conversion is generally a one-way conversion;\n+in this example, we lose the actual image contents and focus\n+just on the text data. This means that diffs generated by\n+textconv are _not_ suitable for applying. For this reason,\n+only `git diff` and the `git log` family of commands (i.e.,\n+log, whatchanged, show) will perform text conversion. `git\n+format-patch` will never generate this output. If you want to\n+send somebody a text-converted diff of a binary file (e.g.,\n+because it quickly conveys the changes you have made), you\n+should generate it separately and send it as a comment _in\n+addition to_ the usual binary diff that you might send.\n+\n+\n Performing a three-way merge\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \n-- \n1.6.0.3.523.g38597.dirty\n"},{"id":"93908","messageId":"20081025005530.GG23903@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081025004815.GA23851@coredump.intra.peff.net","subject":"[PATCH 7/7] only textconv regular files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-25T00:55:30Z","receivedAt":"2008-10-25T00:55:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We treat symlinks as text containing the results of the\nsymlink, so it doesn't make much sense to text-convert them.\n\nSimilarly gitlink components just end up as the text\n\"Subproject commit $sha1\", which we should leave intact.\n\nNote that a typechange may be broken into two parts: the\nremoval of the old part and the addition of the new. In that\ncase, we _do_ show the textconv for any part which is the\naddition or removal of a file we would ordinarily textconv,\nsince it is purely acting on the file contents.\n\nSigned-off-by: Jeff King \n---\nSimilar to before, except we already have the expect_failure test now.\n\n diff.c | 2 ++\n t/t4030-diff-textconv.sh | 2 +-\n 2 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e3bb1ed..a87ce97 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -282,6 +282,8 @@ static const char *get_textconv(struct diff_filespec *one)\n {\n \tif (!DIFF_FILE_VALID(one))\n \t\treturn NULL;\n+\tif (!S_ISREG(one->mode))\n+\t\treturn NULL;\n \tdiff_filespec_load_driver(one);\n \treturn one->driver->textconv;\n }\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 1df48ae..3945731 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -104,7 +104,7 @@ index ad8b3d2..67be421\n \\ No newline at end of file\n EOF\n # make a symlink the hard way that works on symlink-challenged file systems\n-test_expect_failure 'textconv does not act on symlinks' '\n+test_expect_success 'textconv does not act on symlinks' '\n \techo -n frotz > file &&\n \tgit add file &&\n \tgit ls-files -s | sed -e s/100644/120000/ |\n-- \n1.6.0.3.523.g38597.dirty\n"},{"id":"93911","messageId":"7vhc71b5ai.fsf@gitster.siamese.dyndns.org","threadId":"15693","inReplyTo":"20081025005256.GD23903@coredump.intra.peff.net","subject":"Re: [PATCH 4/7] textconv: don't convert for every operation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-25T05:41:09Z","receivedAt":"2008-10-25T05:41:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> -static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)\n> +static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one,\n> +\t\tint try_textconv)\n> {\n> \tconst char *textconv;\n> \n> @@ -304,7 +305,7 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)\n> \telse if (diff_populate_filespec(one, 0))\n> \t\treturn -1;\n> \n> -\tif ((textconv = get_textconv(one))) {\n> +\tif (try_textconv && (textconv = get_textconv(one))) {\n> \t\tsize_t size;\n> \t\tmf->ptr = run_textconv(textconv, one, &size);\n> \t\tif (!mf->ptr)\n> @@ -1396,7 +1397,7 @@ static void builtin_diff(const char *name_a,\n> \t\t}\n> \t}\n\nIsn't this function \"fill_mmfile()\" and its callers leaky as a sieve?\n\nThe original fill_mmfile() was to only borrow one->data and users of\nmmfile_t never had to worry about freeing what's in mf->ptr (and freeing\nthem would have been actively wrong).\n\nI am also somewhat worried about the performance impact of running\nget_textconv() to the same filespec many times when no textconv is\ndefined, which is the normal case we should optimize for. It appears that\ndiff_filespec_load_driver() is optimized for a wrong case (i.e. \"we\nalready know this needs a custom driver and we know which one\").\n\nI am inclined to suggest reverting the whole series (including the ones\nthat are already in 'master') and start over from scratch, limiting the\nrun_textconv() to only inside diff.c::builtin_diff() (in the else {} block\nwhere \"Crazy xcl interfaces..\" comment appears).\n"},{"id":"93913","messageId":"20081025071912.GA24287@coredump.intra.peff.net","threadId":"15693","inReplyTo":"7vhc71b5ai.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 4/7] textconv: don't convert for every operation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-25T07:19:12Z","receivedAt":"2008-10-25T07:19:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 24, 2008 at 10:41:09PM -0700, Junio C Hamano wrote:\n\n> Isn't this function \"fill_mmfile()\" and its callers leaky as a sieve?\n\nYes, it is (for the textconv case; the other is unaffected). That was\nanother reason I had attached the data to the diff_filespec, but\nobviously I forgot about that when re-rolling the patch.\n\n> I am also somewhat worried about the performance impact of running\n> get_textconv() to the same filespec many times when no textconv is\n> defined, which is the normal case we should optimize for. It appears that\n> diff_filespec_load_driver() is optimized for a wrong case (i.e. \"we\n> already know this needs a custom driver and we know which one\").\n\nNo, it is the same as before. We always end up with a driver at the end\nof the function, so further calls will be no-ops. So we do exactly one\nattribute lookup per filespec, caching the result.\n\n> I am inclined to suggest reverting the whole series (including the ones\n> that are already in 'master') and start over from scratch, limiting the\n> run_textconv() to only inside diff.c::builtin_diff() (in the else {} block\n> where \"Crazy xcl interfaces..\" comment appears).\n\nI am not terribly opposed to that (I was quite surprised to see the\noriginal make it to 'next', let alone 'master'). OTOH, the real reason\nto do so would be to keep a clean history, and it is already too late\nfor that.\n\nI think it makes sense to figure out _what_ needs fixed first, because\nit might be somewhat minor. So far I see:\n\n - leak from fill_mmfile; this definitely needs fixed. The quick fix is\n minor (free if we did a textconv). A more involved fix is to pull it\n out of fill_mmfile entirely and put the code directly into\n builtin_diff, which would be part of a re-roll of this latest\n series. But see below.\n\n - Keep fill_mmfile allocation semantics clear. I was trying to keep\n it simple for other fill_mmfile callers to opt-in to textconv, even\n if they chose to do it by some user-controlled mechanism instead of\n by default (e.g., diff.TextconvDiffstat or something). But maybe\n that is not of value to us. Again, that is a re-roll of this series.\n\n - performance considerations with driver loading. I believe this is a\n non-issue. So either you are reading the code wrong, or I am not\n understanding your concern correctly (or _I_ am reading the code\n wrong, of course).\n\nOthers?\n\n-Peff\n"},{"id":"93933","messageId":"7v3aika5l7.fsf@gitster.siamese.dyndns.org","threadId":"15693","inReplyTo":"20081025071912.GA24287@coredump.intra.peff.net","subject":"Re: [PATCH 4/7] textconv: don't convert for every operation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-25T18:32:20Z","receivedAt":"2008-10-25T18:32:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> I am also somewhat worried about the performance impact of running\n>> get_textconv() to the same filespec many times when no textconv is\n>> defined, which is the normal case we should optimize for. It appears that\n>> diff_filespec_load_driver() is optimized for a wrong case (i.e. \"we\n>> already know this needs a custom driver and we know which one\").\n>\n> No, it is the same as before. We always end up with a driver at the end\n> of the function, so further calls will be no-ops. So we do exactly one\n> attribute lookup per filespec, caching the result.\n\nAh, I missed that driver named \"default\". Sorry for the noise.\n\n> I think it makes sense to figure out _what_ needs fixed first, because\n> it might be somewhat minor. So far I see:\n>\n> - leak from fill_mmfile; this definitely needs fixed. The quick fix is\n> minor (free if we did a textconv). A more involved fix is to pull it\n> out of fill_mmfile entirely and put the code directly into\n> builtin_diff, which would be part of a re-roll of this latest\n> series. But see below.\n>\n> - Keep fill_mmfile allocation semantics clear. I was trying to keep\n> it simple for other fill_mmfile callers to opt-in to textconv, even\n> if they chose to do it by some user-controlled mechanism instead of\n> by default (e.g., diff.TextconvDiffstat or something). But maybe\n> that is not of value to us. Again, that is a re-roll of this series.\n\nIt would be either:\n\n (1) non-textconv users call fill_mmfile(&mf, ..., 0), use mf and return\n without clean-up as before, while textconv users do:\n\n \tfill_mmfile(&mf, ..., 1);\n use mf;\n if (mf->ptr != one->data)\n\t\tclean up mf->ptr;\n return;\n\nor\n\n (2) non-textconv users are unchanged from v1.6.0, while textconv users\n do:\n\n\tconst char *textconv = get_textconv(...);\n\tfill_mmfile(&mf, ...); /* no change to fill_mmfile() */\n\tif (textconv)\n \tmunge_mmfile(&mf);\n\tuse mf;\n if (textconv)\n \tcleanup_mmfile(&mf);\n\nThe end result may not be that much different, but I find the latter\neasier to follow for three reasons:\n\n * we expect that majority of the users of fill_mmfile() are non textconv\n users. I'd feel safer to keep their codepath the same as v1.6.0;\n\n * fill_mmfile() semantics is the same as long before -- it just gives it\n a borrowed pointer;\n\n * the code that makes a change to mmfile that requires clean-up does so\n explicitly, and that is followed by an explicit clean-up, both\n contained in the same function;\n\n> - performance considerations with driver loading. I believe this is a\n> non-issue. So either you are reading the code wrong,...\n\nYes I was. Thanks and sorry.\n"},{"id":"93938","messageId":"20081025193554.GA27457@coredump.intra.peff.net","threadId":"15693","inReplyTo":"7v3aika5l7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 4/7] textconv: don't convert for every operation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-25T19:35:54Z","receivedAt":"2008-10-25T19:35:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 25, 2008 at 11:32:20AM -0700, Junio C Hamano wrote:\n\n> The end result may not be that much different, but I find the latter\n> easier to follow for three reasons:\n\nYour reasoning here is sound. My initial thought was to make it a\none-liner change to \"turn on\" text conversion in other spots. But\n\n a) it didn't turn out that way, since you still have to keep track of\n the \"did I textconv\" when determining binary-ness, and whether to\n free()\n\n b) we don't actually want to do it anywhere else (except perhaps, as I\n mentioned, in blame, but that is a totally different interface\n anyway)\n\nI will re-roll my latest series according to your recommendation\n(unless you are set on reverting the first part; if so, please tell me\nasap so I can work under that assumption).\n\n-Peff\n"},{"id":"93947","messageId":"7vzlks6yer.fsf@gitster.siamese.dyndns.org","threadId":"15693","inReplyTo":"20081025193554.GA27457@coredump.intra.peff.net","subject":"Re: [PATCH 4/7] textconv: don't convert for every operation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-25T23:35:40Z","receivedAt":"2008-10-25T23:35:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I will re-roll my latest series according to your recommendation\n> (unless you are set on reverting the first part; if so, please tell me\n> asap so I can work under that assumption).\n\nNo, reverting and reapplying is more work than incremental updates.\nPlease refine on top of what's already in.\n"},{"id":"93949","messageId":"7vvdvg6xto.fsf@gitster.siamese.dyndns.org","threadId":"15693","inReplyTo":"20081025193554.GA27457@coredump.intra.peff.net","subject":"Re: [PATCH 4/7] textconv: don't convert for every operation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-25T23:48:19Z","receivedAt":"2008-10-25T23:48:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Another place that would benefit from ALLOW_TEXTCONV is the function\nwt_status_print_verbose() in wt-status.c, where we generate preview diff\nfor \"git commit -v\". The output from that function is purely for human\nconsumption and does not have to be applicable.\n"},{"id":"93956","messageId":"20081026043802.GA14530@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081025193554.GA27457@coredump.intra.peff.net","subject":"Re: [PATCH 4/7] textconv: don't convert for every operation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-26T04:38:03Z","receivedAt":"2008-10-26T04:38:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 25, 2008 at 03:35:54PM -0400, Jeff King wrote:\n\n> I will re-roll my latest series according to your recommendation\n> (unless you are set on reverting the first part; if so, please tell me\n> asap so I can work under that assumption).\n\nAnd here it is. 8 patches this time, and I tried to reorder with\n\"obviously correct\" at the beginning and \"I'm not so sure\" at the end.\n\n 1/8: diff: add missing static declaration\n 2/8: document the diff driver textconv feature\n 3/8: add userdiff textconv tests\n 4/8: refactor userdiff textconv code\n 5/8: userdiff: require explicitly allowing textconv\n 6/8: only textconv regular files\n 7/8: wt-status: load diff ui config\n 8/8: enable textconv for diff in verbose status/commit\n\n-Peff\n"},{"id":"93957","messageId":"20081026044127.GA21047@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081026043802.GA14530@coredump.intra.peff.net","subject":"[PATCH v3 1/8] diff: add missing static declaration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-26T04:41:28Z","receivedAt":"2008-10-26T04:41:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function isn't used outside of diff.c; the 'static' was\nsimply overlooked in the original writing.\n\nSigned-off-by: Jeff King \n---\nSame as before.\n\n diff.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e368fef..d1fd594 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1282,7 +1282,7 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two)\n \temit_binary_diff_body(file, two, one);\n }\n \n-void diff_filespec_load_driver(struct diff_filespec *one)\n+static void diff_filespec_load_driver(struct diff_filespec *one)\n {\n \tif (!one->driver)\n \t\tone->driver = userdiff_find_by_path(one->path);\n-- \n1.6.0.3.524.ge8b2e.dirty\n"},{"id":"93958","messageId":"20081026044152.GB21047@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081026043802.GA14530@coredump.intra.peff.net","subject":"[PATCH v3 2/8] document the diff driver textconv feature","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-26T04:41:52Z","receivedAt":"2008-10-26T04:41:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This patch also changes the term \"custom diff driver\" to\n\"external diff driver\"; now that there are more facets of a\n\"custom driver\" than just external diffing, it makes sense\nto refer to the configuration of \"diff.foo.*\" as the \"foo\ndiff driver\", with \"diff.foo.command\" as the \"external\ndriver for foo\".\n\nSigned-off-by: Jeff King \n---\nSame as before, but re-ordered to beginning since it is\nnon-controversial.\n\n Documentation/gitattributes.txt | 66 +++++++++++++++++++++++++++++++--------\n 1 files changed, 53 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 2694559..314e2d3 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -213,10 +213,12 @@ with `crlf`, and then `ident` and fed to `filter`.\n Generating diff text\n ~~~~~~~~~~~~~~~~~~~~\n \n-The attribute `diff` affects if 'git-diff' generates textual\n-patch for the path or just says `Binary files differ`. It also\n-can affect what line is shown on the hunk header `@@ -k,l +n,m @@`\n-line.\n+The attribute `diff` affects how 'git' generates diffs for particular\n+files. It can tell git whether to generate a textual patch for the path\n+or to treat the path as a binary file. It can also affect what line is\n+shown on the hunk header `@@ -k,l +n,m @@` line, tell git to use an\n+external command to generate the diff, or ask git to convert binary\n+files to a text format before generating the diff.\n \n Set::\n \n@@ -227,7 +229,8 @@ Set::\n Unset::\n \n \tA path to which the `diff` attribute is unset will\n-\tgenerate `Binary files differ`.\n+\tgenerate `Binary files differ` (or a binary patch, if\n+\tbinary patches are enabled).\n \n Unspecified::\n \n@@ -238,21 +241,21 @@ Unspecified::\n \n String::\n \n-\tDiff is shown using the specified custom diff driver.\n-\tThe driver program is given its input using the same\n-\tcalling convention as used for GIT_EXTERNAL_DIFF\n-\tprogram. This name is also used for custom hunk header\n-\tselection.\n+\tDiff is shown using the specified diff driver. Each driver may\n+\tspecify one or more options, as described in the following\n+\tsection. The options for the diff driver \"foo\" are defined\n+\tby the configuration variables in the \"diff.foo\" section of the\n+\tgit config file.\n \n \n-Defining a custom diff driver\n-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n+Defining an external diff driver\n+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n \n The definition of a diff driver is done in `gitconfig`, not\n `gitattributes` file, so strictly speaking this manual page is a\n wrong place to talk about it. However...\n \n-To define a custom diff driver `jcdiff`, add a section to your\n+To define an external diff driver `jcdiff`, add a section to your\n `$GIT_DIR/config` file (or `$HOME/.gitconfig` file) like this:\n \n ----------------------------------------------------------------\n@@ -328,6 +331,43 @@ patterns are available:\n - `tex` suitable for source code for LaTeX documents.\n \n \n+Performing text diffs of binary files\n+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n+\n+Sometimes it is desirable to see the diff of a text-converted\n+version of some binary files. For example, a word processor\n+document can be converted to an ASCII text representation, and\n+the diff of the text shown. Even though this conversion loses\n+some information, the resulting diff is useful for human\n+viewing (but cannot be applied directly).\n+\n+The `textconv` config option is used to define a program for\n+performing such a conversion. The program should take a single\n+argument, the name of a file to convert, and produce the\n+resulting text on stdout.\n+\n+For example, to show the diff of the exif information of a\n+file instead of the binary information (assuming you have the\n+exif tool installed):\n+\n+------------------------\n+[diff \"jpg\"]\n+\ttextconv = exif\n+------------------------\n+\n+NOTE: The text conversion is generally a one-way conversion;\n+in this example, we lose the actual image contents and focus\n+just on the text data. This means that diffs generated by\n+textconv are _not_ suitable for applying. For this reason,\n+only `git diff` and the `git log` family of commands (i.e.,\n+log, whatchanged, show) will perform text conversion. `git\n+format-patch` will never generate this output. If you want to\n+send somebody a text-converted diff of a binary file (e.g.,\n+because it quickly conveys the changes you have made), you\n+should generate it separately and send it as a comment _in\n+addition to_ the usual binary diff that you might send.\n+\n+\n Performing a three-way merge\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \n-- \n1.6.0.3.524.ge8b2e.dirty\n"},{"id":"93959","messageId":"20081026044225.GC21047@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081026043802.GA14530@coredump.intra.peff.net","subject":"[PATCH v3 3/8] add userdiff textconv tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-26T04:42:25Z","receivedAt":"2008-10-26T04:42:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These tests provide a basic sanity check that textconv'd\nfiles work. The tests try to describe how this configuration\n_should_ work; thus some of the tests are marked to expect\nfailure.\n\nIn particular, we fail to actually textconv anything because\nthe 'diff.foo.binary' config option is not set, which will\nbe fixed in the next patch.\n\nThis also means that some \"expect_failure\" tests actually\nseem to be fixed; in reality, this is just because textconv\nis broken and its failure mode happens to make these tests\nwork.\n\nSigned-off-by: Jeff King \n---\nSame as before.\n\n t/t4030-diff-textconv.sh | 118 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 118 insertions(+), 0 deletions(-)\n create mode 100755 t/t4030-diff-textconv.sh\n\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nnew file mode 100755\nindex 0000000..1b09648\n--- /dev/null\n+++ b/t/t4030-diff-textconv.sh\n@@ -0,0 +1,118 @@\n+#!/bin/sh\n+\n+test_description='diff.*.textconv tests'\n+. ./test-lib.sh\n+\n+find_diff() {\n+\tsed '1,/^index /d' | sed '/^-- $/,$d'\n+}\n+\n+cat >expect.binary <<'EOF'\n+Binary files a/file and b/file differ\n+EOF\n+\n+cat >expect.text <<'EOF'\n+--- a/file\n++++ b/file\n+@@ -1 +1,2 @@\n+ 0\n++1\n+EOF\n+\n+cat >hexdump <<'EOF'\n+#!/bin/sh\n+perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' \"$1\"\n+EOF\n+chmod +x hexdump\n+\n+test_expect_success 'setup binary file with history' '\n+\tprintf \"\\\\0\\\\n\" >file &&\n+\tgit add file &&\n+\tgit commit -m one &&\n+\tprintf \"\\\\1\\\\n\" >>file &&\n+\tgit add file &&\n+\tgit commit -m two\n+'\n+\n+test_expect_success 'file is considered binary by porcelain' '\n+\tgit diff HEAD^ HEAD >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.binary actual\n+'\n+\n+test_expect_success 'file is considered binary by plumbing' '\n+\tgit diff-tree -p HEAD^ HEAD >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.binary actual\n+'\n+\n+test_expect_success 'setup textconv filters' '\n+\techo file diff=foo >.gitattributes &&\n+\tgit config diff.foo.textconv \"$PWD\"/hexdump &&\n+\tgit config diff.fail.textconv false\n+'\n+\n+test_expect_failure 'diff produces text' '\n+\tgit diff HEAD^ HEAD >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.text actual\n+'\n+\n+test_expect_success 'diff-tree produces binary' '\n+\tgit diff-tree -p HEAD^ HEAD >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.binary actual\n+'\n+\n+test_expect_failure 'log produces text' '\n+\tgit log -1 -p >log &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.text actual\n+'\n+\n+test_expect_failure 'format-patch produces binary' '\n+\tgit format-patch --no-binary --stdout HEAD^ >patch &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.binary actual\n+'\n+\n+cat >expect.stat <<'EOF'\n+ file | Bin 2 -> 4 bytes\n+ 1 files changed, 0 insertions(+), 0 deletions(-)\n+EOF\n+test_expect_failure 'diffstat does not run textconv' '\n+\techo file diff=fail >.gitattributes &&\n+\tgit diff --stat HEAD^ HEAD >actual &&\n+\ttest_cmp expect.stat actual\n+'\n+# restore working setup\n+echo file diff=foo >.gitattributes\n+\n+cat >expect.typechange <<'EOF'\n+--- a/file\n++++ /dev/null\n+@@ -1,2 +0,0 @@\n+-0\n+-1\n+diff --git a/file b/file\n+new file mode 120000\n+index ad8b3d2..67be421\n+--- /dev/null\n++++ b/file\n+@@ -0,0 +1 @@\n++frotz\n+\\ No newline at end of file\n+EOF\n+# make a symlink the hard way that works on symlink-challenged file systems\n+test_expect_failure 'textconv does not act on symlinks' '\n+\techo -n frotz > file &&\n+\tgit add file &&\n+\tgit ls-files -s | sed -e s/100644/120000/ |\n+\t\tgit update-index --index-info &&\n+\tgit commit -m typechange &&\n+\tgit show >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.typechange actual\n+'\n+\n+test_done\n-- \n1.6.0.3.524.ge8b2e.dirty\n"},{"id":"93960","messageId":"20081026044453.GD21047@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081026043802.GA14530@coredump.intra.peff.net","subject":"[PATCH v3 4/8] refactor userdiff textconv code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-26T04:44:53Z","receivedAt":"2008-10-26T04:44:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The original implementation of textconv put the conversion\ninto fill_mmfile. This was a bad idea for a number of\nreasons:\n\n - it made the semantics of fill_mmfile unclear. In some\n cases, it was allocating data (if a text conversion\n occurred), and in some cases not (if we could use the\n data directly from the filespec). But the caller had\n no idea which had happened, and so didn't know whether\n the memory should be freed\n\n - similarly, the caller had no idea if a text conversion\n had occurred, and so didn't know whether the contents\n should be treated as binary or not. This meant that we\n incorrectly guessed that text-converted content was\n binary and didn't actually show it (unless the user\n overrode us with \"diff.foo.binary = false\", which then\n created problems in plumbing where the text conversion\n did _not_ occur)\n\n - not all callers of fill_mmfile want the text contents. In\n particular, we don't really want diffstat, whitespace\n checks, patch id generation, etc, to look at the\n converted contents.\n\nThis patch pulls the conversion code directly into\nbuiltin_diff, so that we only see the conversion when\ngenerating an actual patch. We also then know whether we are\ndoing a conversion, so we can check the binary-ness and free\nthe data from the mmfile appropriately (the previous version\nleaked quite badly when text conversion was used)\n\nSigned-off-by: Jeff King \n---\nThis patch is the interesting one. I think your suggestion turned out\nfairly clean (and this reverts fill_mmfile back to its state before the\noriginal textconv series).\n\n diff.c | 48 +++++++++++++++++++++++++++++++++------------\n t/t4030-diff-textconv.sh | 6 ++--\n 2 files changed, 38 insertions(+), 16 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex d1fd594..6f01595 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -294,18 +294,8 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)\n \telse if (diff_populate_filespec(one, 0))\n \t\treturn -1;\n \n-\tdiff_filespec_load_driver(one);\n-\tif (one->driver->textconv) {\n-\t\tsize_t size;\n-\t\tmf->ptr = run_textconv(one->driver->textconv, one, &size);\n-\t\tif (!mf->ptr)\n-\t\t\treturn -1;\n-\t\tmf->size = size;\n-\t}\n-\telse {\n-\t\tmf->ptr = one->data;\n-\t\tmf->size = one->size;\n-\t}\n+\tmf->ptr = one->data;\n+\tmf->size = one->size;\n \treturn 0;\n }\n \n@@ -1323,6 +1313,14 @@ void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const\n \t\toptions->b_prefix = b;\n }\n \n+static const char *get_textconv(struct diff_filespec *one)\n+{\n+\tif (!DIFF_FILE_VALID(one))\n+\t\treturn NULL;\n+\tdiff_filespec_load_driver(one);\n+\treturn one->driver->textconv;\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@@ -1337,6 +1335,7 @@ static void builtin_diff(const char *name_a,\n \tconst char *set = diff_get_color_opt(o, DIFF_METAINFO);\n \tconst char *reset = diff_get_color_opt(o, DIFF_RESET);\n \tconst char *a_prefix, *b_prefix;\n+\tconst char *textconv_one, *textconv_two;\n \n \tdiff_set_mnemonic_prefix(o, \"a/\", \"b/\");\n \tif (DIFF_OPT_TST(o, REVERSE_DIFF)) {\n@@ -1390,8 +1389,12 @@ 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+\ttextconv_one = get_textconv(one);\n+\ttextconv_two = get_textconv(two);\n+\n \tif (!DIFF_OPT_TST(o, TEXT) &&\n-\t (diff_filespec_is_binary(one) || diff_filespec_is_binary(two))) {\n+\t ( (diff_filespec_is_binary(one) && !textconv_one) ||\n+\t (diff_filespec_is_binary(two) && !textconv_two) )) {\n \t\t/* Quite common confusing case */\n \t\tif (mf1.size == mf2.size &&\n \t\t !memcmp(mf1.ptr, mf2.ptr, mf1.size))\n@@ -1412,6 +1415,21 @@ static void builtin_diff(const char *name_a,\n \t\tstruct emit_callback ecbdata;\n \t\tconst struct userdiff_funcname *pe;\n \n+\t\tif (textconv_one) {\n+\t\t\tsize_t size;\n+\t\t\tmf1.ptr = run_textconv(textconv_one, one, &size);\n+\t\t\tif (!mf1.ptr)\n+\t\t\t\tdie(\"unable to read files to diff\");\n+\t\t\tmf1.size = size;\n+\t\t}\n+\t\tif (textconv_two) {\n+\t\t\tsize_t size;\n+\t\t\tmf2.ptr = run_textconv(textconv_two, two, &size);\n+\t\t\tif (!mf2.ptr)\n+\t\t\t\tdie(\"unable to read files to diff\");\n+\t\t\tmf2.size = size;\n+\t\t}\n+\n \t\tpe = diff_funcname_pattern(one);\n \t\tif (!pe)\n \t\t\tpe = diff_funcname_pattern(two);\n@@ -1443,6 +1461,10 @@ static void builtin_diff(const char *name_a,\n \t\t\t &xpp, &xecfg, &ecb);\n \t\tif (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))\n \t\t\tfree_diff_words_data(&ecbdata);\n+\t\tif (textconv_one)\n+\t\t\tfree(mf1.ptr);\n+\t\tif (textconv_two)\n+\t\t\tfree(mf2.ptr);\n \t}\n \n free_ab_and_return:\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 1b09648..090a21d 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -52,7 +52,7 @@ test_expect_success 'setup textconv filters' '\n \tgit config diff.fail.textconv false\n '\n \n-test_expect_failure 'diff produces text' '\n+test_expect_success 'diff produces text' '\n \tgit diff HEAD^ HEAD >diff &&\n \tfind_diff actual &&\n \ttest_cmp expect.text actual\n@@ -64,7 +64,7 @@ test_expect_success 'diff-tree produces binary' '\n \ttest_cmp expect.binary actual\n '\n \n-test_expect_failure 'log produces text' '\n+test_expect_success 'log produces text' '\n \tgit log -1 -p >log &&\n \tfind_diff actual &&\n \ttest_cmp expect.text actual\n@@ -80,7 +80,7 @@ cat >expect.stat <<'EOF'\n file | Bin 2 -> 4 bytes\n 1 files changed, 0 insertions(+), 0 deletions(-)\n EOF\n-test_expect_failure 'diffstat does not run textconv' '\n+test_expect_success 'diffstat does not run textconv' '\n \techo file diff=fail >.gitattributes &&\n \tgit diff --stat HEAD^ HEAD >actual &&\n \ttest_cmp expect.stat actual\n-- \n1.6.0.3.524.ge8b2e.dirty\n"},{"id":"93961","messageId":"20081026044555.GE21047@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081026043802.GA14530@coredump.intra.peff.net","subject":"[PATCH v3 5/8] userdiff: require explicitly allowing textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-26T04:45:55Z","receivedAt":"2008-10-26T04:45:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Diffs that have been produced with textconv almost certainly\ncannot be applied, so we want to be careful not to generate\nthem in things like format-patch.\n\nThis introduces a new diff options, ALLOW_TEXTCONV, which\ncontrols this behavior. It is off by default, but is\nexplicitly turned on for the \"log\" family of commands, as\nwell as the \"diff\" porcelain (but not diff-* plumbing).\n\nBecause both text conversion and external diffing are\ncontrolled by these diff options, we can get rid of the\n\"plumbing versus porcelain\" distinction when reading the\nconfig. This was an attempt to control the same thing, but\nsuffered from being too coarse-grained.\n\nSigned-off-by: Jeff King \n---\nThe new changes in 4/8 make this even cleaner than before.\n\n builtin-diff.c | 1 +\n builtin-log.c | 1 +\n diff.c | 26 +++++++++++---------------\n diff.h | 1 +\n t/t4030-diff-textconv.sh | 2 +-\n userdiff.c | 10 +---------\n userdiff.h | 3 +--\n 7 files changed, 17 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 9c8c295..2de5834 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -300,6 +300,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t}\n \tDIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);\n \tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n+\tDIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);\n \n \t/*\n \t * If the user asked for our exit code then don't start a\ndiff --git a/builtin-log.c b/builtin-log.c\nindex a0944f7..75d698f 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -59,6 +59,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\t} else\n \t\t\tdie(\"unrecognized argument: %s\", arg);\n \t}\n+\tDIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);\n }\n \n /*\ndiff --git a/diff.c b/diff.c\nindex 6f01595..608223a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -93,12 +93,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"diff.external\"))\n \t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n \n-\tswitch (userdiff_config_porcelain(var, value)) {\n-\t\tcase 0: break;\n-\t\tcase -1: return -1;\n-\t\tdefault: return 0;\n-\t}\n-\n \treturn git_diff_basic_config(var, value, cb);\n }\n \n@@ -109,6 +103,12 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tswitch (userdiff_config(var, value)) {\n+\t\tcase 0: break;\n+\t\tcase -1: return -1;\n+\t\tdefault: return 0;\n+\t}\n+\n \tif (!prefixcmp(var, \"diff.color.\") || !prefixcmp(var, \"color.diff.\")) {\n \t\tint slot = parse_diff_color_slot(var, 11);\n \t\tif (!value)\n@@ -123,12 +123,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tswitch (userdiff_config_basic(var, value)) {\n-\t\tcase 0: break;\n-\t\tcase -1: return -1;\n-\t\tdefault: return 0;\n-\t}\n-\n \treturn git_color_default_config(var, value, cb);\n }\n \n@@ -1335,7 +1329,7 @@ static void builtin_diff(const char *name_a,\n \tconst char *set = diff_get_color_opt(o, DIFF_METAINFO);\n \tconst char *reset = diff_get_color_opt(o, DIFF_RESET);\n \tconst char *a_prefix, *b_prefix;\n-\tconst char *textconv_one, *textconv_two;\n+\tconst char *textconv_one = NULL, *textconv_two = NULL;\n \n \tdiff_set_mnemonic_prefix(o, \"a/\", \"b/\");\n \tif (DIFF_OPT_TST(o, REVERSE_DIFF)) {\n@@ -1389,8 +1383,10 @@ 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-\ttextconv_one = get_textconv(one);\n-\ttextconv_two = get_textconv(two);\n+\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n+\t\ttextconv_one = get_textconv(one);\n+\t\ttextconv_two = get_textconv(two);\n+\t}\n \n \tif (!DIFF_OPT_TST(o, TEXT) &&\n \t ( (diff_filespec_is_binary(one) && !textconv_one) ||\ndiff --git a/diff.h b/diff.h\nindex a49d865..42582ed 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -65,6 +65,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n #define DIFF_OPT_IGNORE_SUBMODULES (1 << 18)\n #define DIFF_OPT_DIRSTAT_CUMULATIVE (1 << 19)\n #define DIFF_OPT_DIRSTAT_BY_FILE (1 << 20)\n+#define DIFF_OPT_ALLOW_TEXTCONV (1 << 21)\n #define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)\n #define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)\n #define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag)\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 090a21d..1df48ae 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -70,7 +70,7 @@ test_expect_success 'log produces text' '\n \ttest_cmp expect.text actual\n '\n \n-test_expect_failure 'format-patch produces binary' '\n+test_expect_success 'format-patch produces binary' '\n \tgit format-patch --no-binary --stdout HEAD^ >patch &&\n \tfind_diff actual &&\n \ttest_cmp expect.binary actual\ndiff --git a/userdiff.c b/userdiff.c\nindex d95257a..3681062 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -120,7 +120,7 @@ static int parse_tristate(int *b, const char *k, const char *v)\n \treturn 1;\n }\n \n-int userdiff_config_basic(const char *k, const char *v)\n+int userdiff_config(const char *k, const char *v)\n {\n \tstruct userdiff_driver *drv;\n \n@@ -130,14 +130,6 @@ int userdiff_config_basic(const char *k, const char *v)\n \t\treturn parse_funcname(&drv->funcname, k, v, REG_EXTENDED);\n \tif ((drv = parse_driver(k, v, \"binary\")))\n \t\treturn parse_tristate(&drv->binary, k, v);\n-\n-\treturn 0;\n-}\n-\n-int userdiff_config_porcelain(const char *k, const char *v)\n-{\n-\tstruct userdiff_driver *drv;\n-\n \tif ((drv = parse_driver(k, v, \"command\")))\n \t\treturn parse_string(&drv->external, k, v);\n \tif ((drv = parse_driver(k, v, \"textconv\")))\ndiff --git a/userdiff.h b/userdiff.h\nindex f29c18f..ba29457 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -14,8 +14,7 @@ struct userdiff_driver {\n \tconst char *textconv;\n };\n \n-int userdiff_config_basic(const char *k, const char *v);\n-int userdiff_config_porcelain(const char *k, const char *v);\n+int userdiff_config(const char *k, const char *v);\n struct userdiff_driver *userdiff_find_by_name(const char *name);\n struct userdiff_driver *userdiff_find_by_path(const char *path);\n \n-- \n1.6.0.3.524.ge8b2e.dirty\n"},{"id":"93962","messageId":"20081026044621.GF21047@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081026043802.GA14530@coredump.intra.peff.net","subject":"[PATCH v3 6/8] only textconv regular files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-26T04:46:21Z","receivedAt":"2008-10-26T04:46:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We treat symlinks as text containing the results of the\nsymlink, so it doesn't make much sense to text-convert them.\n\nSimilarly gitlink components just end up as the text\n\"Subproject commit $sha1\", which we should leave intact.\n\nNote that a typechange may be broken into two parts: the\nremoval of the old part and the addition of the new. In that\ncase, we _do_ show the textconv for any part which is the\naddition or removal of a file we would ordinarily textconv,\nsince it is purely acting on the file contents.\n\nSigned-off-by: Jeff King \n---\nSame as before.\n\n diff.c | 2 ++\n t/t4030-diff-textconv.sh | 2 +-\n 2 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 608223a..23d454e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1311,6 +1311,8 @@ static const char *get_textconv(struct diff_filespec *one)\n {\n \tif (!DIFF_FILE_VALID(one))\n \t\treturn NULL;\n+\tif (!S_ISREG(one->mode))\n+\t\treturn NULL;\n \tdiff_filespec_load_driver(one);\n \treturn one->driver->textconv;\n }\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 1df48ae..3945731 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -104,7 +104,7 @@ index ad8b3d2..67be421\n \\ No newline at end of file\n EOF\n # make a symlink the hard way that works on symlink-challenged file systems\n-test_expect_failure 'textconv does not act on symlinks' '\n+test_expect_success 'textconv does not act on symlinks' '\n \techo -n frotz > file &&\n \tgit add file &&\n \tgit ls-files -s | sed -e s/100644/120000/ |\n-- \n1.6.0.3.524.ge8b2e.dirty\n"},{"id":"93963","messageId":"20081026044935.GG21047@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081026043802.GA14530@coredump.intra.peff.net","subject":"[PATCH v3 7/8] wt-status: load diff ui config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-26T04:49:35Z","receivedAt":"2008-10-26T04:49:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When \"git status -v\" shows a diff, we did not respect the\nuser's usual diff preferences at all. Loading just\ngit_diff_basic_config would give us things like rename\nlimits and diff drivers. But it makes even more sense to\nload git_diff_ui_config, which gives us colorization if the\nuser has requested it.\n\nNote that we need to take special care to cancel\ncolorization when writing to the commit template file, as\ndescribed in the code comments.\n\nSigned-off-by: Jeff King \n---\nThis is necessary to do textconvs in the \"status -v\" diff (which will\ncome in the next patch).\n\nBut it makes me a little nervous. On one hand, I think it is definitely\nthe right thing for \"status -v\" to respect user options. But we do\nseveral _other_ diffs in addition, and those are more \"plumbing\" diffs.\nI think they should probably at least have diff_basic_config (e.g., for\nrename limits). But we are applying the diff_ui_config options to all\ndiffs. Looking over the available options, I _think_ there are no nasty\nsurprises. But you never know.\n\n t/t7502-commit.sh | 8 ++++++++\n wt-status.c | 10 +++++++++-\n 2 files changed, 17 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex 3eb9fae..ad42c78 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -89,6 +89,14 @@ test_expect_success 'verbose' '\n \n '\n \n+test_expect_success 'verbose respects diff config' '\n+\n+\tgit config color.diff always &&\n+\tgit status -v >actual &&\n+\tgrep \"\\[1mdiff --git\" actual &&\n+\tgit config --unset color.diff\n+'\n+\n test_expect_success 'cleanup commit messages (verbatim,-t)' '\n \n \techo >>negative &&\ndiff --git a/wt-status.c b/wt-status.c\nindex c3a9cab..54d2f58 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -303,6 +303,14 @@ static void wt_status_print_verbose(struct wt_status *s)\n \trev.diffopt.detect_rename = 1;\n \trev.diffopt.file = s->fp;\n \trev.diffopt.close_file = 0;\n+\t/*\n+\t * If we're not going to stdout, then we definitely don't\n+\t * want color, since we are going to the commit message\n+\t * file (and even the \"auto\" setting won't work, since it\n+\t * will have checked isatty on stdout).\n+\t */\n+\tif (s->fp != stdout)\n+\t\tDIFF_OPT_CLR(&rev.diffopt, COLOR_DIFF);\n \trun_diff_index(&rev, 1);\n }\n \n@@ -422,5 +430,5 @@ int git_status_config(const char *k, const char *v, void *cb)\n \t\t\treturn error(\"Invalid untracked files mode '%s'\", v);\n \t\treturn 0;\n \t}\n-\treturn git_color_default_config(k, v, cb);\n+\treturn git_diff_ui_config(k, v, cb);\n }\n-- \n1.6.0.3.524.ge8b2e.dirty\n"},{"id":"93964","messageId":"20081026045002.GH21047@coredump.intra.peff.net","threadId":"15693","inReplyTo":"20081026043802.GA14530@coredump.intra.peff.net","subject":"[PATCH v3 8/8] enable textconv for diff in verbose status/commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-26T04:50:02Z","receivedAt":"2008-10-26T04:50:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This diff is meant for human consumption, so it makes sense\nto apply text conversion here, as we would for the regular\ndiff porcelain.\n\nSigned-off-by: Jeff King \n---\nPretty straightforward, but depends on whether we find 7/8 acceptable.\n\n t/t4030-diff-textconv.sh | 8 ++++++++\n wt-status.c | 1 +\n 2 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 3945731..09967f5 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -76,6 +76,14 @@ test_expect_success 'format-patch produces binary' '\n \ttest_cmp expect.binary actual\n '\n \n+test_expect_success 'status -v produces text' '\n+\tgit reset --soft HEAD^ &&\n+\tgit status -v >diff &&\n+\tfind_diff actual &&\n+\ttest_cmp expect.text actual &&\n+\tgit reset --soft HEAD@{1}\n+'\n+\n cat >expect.stat <<'EOF'\n file | Bin 2 -> 4 bytes\n 1 files changed, 0 insertions(+), 0 deletions(-)\ndiff --git a/wt-status.c b/wt-status.c\nindex 54d2f58..6a7645e 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -301,6 +301,7 @@ static void wt_status_print_verbose(struct wt_status *s)\n \tsetup_revisions(0, NULL, &rev, s->reference);\n \trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\n \trev.diffopt.detect_rename = 1;\n+\tDIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);\n \trev.diffopt.file = s->fp;\n \trev.diffopt.close_file = 0;\n \t/*\n-- \n1.6.0.3.524.ge8b2e.dirty\n"},{"id":"93965","messageId":"20081026045200.GA21178@coredump.intra.peff.net","threadId":"15693","inReplyTo":"7vvdvg6xto.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 4/7] textconv: don't convert for every operation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-26T04:52:00Z","receivedAt":"2008-10-26T04:52:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 25, 2008 at 04:48:19PM -0700, Junio C Hamano wrote:\n\n> Another place that would benefit from ALLOW_TEXTCONV is the function\n> wt_status_print_verbose() in wt-status.c, where we generate preview diff\n> for \"git commit -v\". The output from that function is purely for human\n> consumption and does not have to be applicable.\n\nI agree, but it turned out not to be as straightforward as I had hoped,\nsince status doesn't even parse diff.* config. See patches 7 and 8 in\nthe series I just posted.\n\n-Peff\n"},{"id":"94021","messageId":"7vmygqiozb.fsf@gitster.siamese.dyndns.org","threadId":"15693","inReplyTo":"20081026044935.GG21047@coredump.intra.peff.net","subject":"Re: [PATCH v3 7/8] wt-status: load diff ui config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-27T05:30:48Z","receivedAt":"2008-10-27T05:30:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But it makes me a little nervous. On one hand, I think it is definitely\n> the right thing for \"status -v\" to respect user options. But we do\n> several _other_ diffs in addition, and those are more \"plumbing\" diffs.\n> I think they should probably at least have diff_basic_config (e.g., for\n> rename limits). But we are applying the diff_ui_config options to all\n> diffs. Looking over the available options, I _think_ there are no nasty\n> surprises. But you never know.\n\nUp to 6/8 are indisputably good changes. The next one means well, and\nthis one is a requisite step for it, but I agree that this feels somewhat\nrisky.\n"},{"id":"94031","messageId":"20081027082355.GB4085@sigill.intra.peff.net","threadId":"15693","inReplyTo":"7vmygqiozb.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v3 7/8] wt-status: load diff ui config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-27T08:23:55Z","receivedAt":"2008-10-27T08:23:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 26, 2008 at 10:30:48PM -0700, Junio C Hamano wrote:\n\n> > But it makes me a little nervous. On one hand, I think it is definitely\n> > the right thing for \"status -v\" to respect user options. But we do\n> > several _other_ diffs in addition, and those are more \"plumbing\" diffs.\n> > I think they should probably at least have diff_basic_config (e.g., for\n> > rename limits). But we are applying the diff_ui_config options to all\n> > diffs. Looking over the available options, I _think_ there are no nasty\n> > surprises. But you never know.\n> \n> Up to 6/8 are indisputably good changes. The next one means well, and\n> this one is a requisite step for it, but I agree that this feels somewhat\n> risky.\n\nI have to wonder that nobody ever complained about the lack of diff\nconfig here before. If I ever used \"git status -v\", I would certainly\nhave been turned off by the lack of color. But maybe everybody is using\n\"git commit -v\" and their editor is colorizing it. Or maybe they just\naren't using color (the only other noticeable thing would be rename\noptions).\n\n-Peff\n"}]}