{"thread":{"id":"33545","subject":"[PATCH 0/6] grep with textconv","startedAt":"2013-04-19T16:44:43Z","lastAt":"2013-05-16T03:31:51Z","messageCount":77,"participants":["Michael J Gruber","Junio C Hamano","Jeff King","Jeremy Rosen","Matthieu Moy","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"214838","messageId":"cover.1366389739.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":null,"subject":"[PATCH 0/6] grep with textconv","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-19T16:44:43Z","receivedAt":"2013-04-19T16:44:43Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"This series teaches show and grep to obey textconv:\nshow by default (like diff), grep only on request (--textconv).\nWe might switch the default for the latter also, of course.\nI'd actually like that.\n\nCompared to an earlier (historic) series this one comes with tests.\nBesides, it has been in use since.\n\nJeff King (1):\n grep: allow to use textconv filters\n\nMichael J Gruber (5):\n t4030: demonstrate behavior of show with textconv\n show: obey --textconv for blobs\n cat-file: do not die on --textconv without textconv filters\n t7008: demonstrate behavior of grep with textconv\n grep: obey --textconv for the case rev:path\n\n builtin/cat-file.c | 9 ++--\n builtin/grep.c | 13 +++---\n builtin/log.c | 24 +++++++++--\n grep.c | 100 +++++++++++++++++++++++++++++++++++++------\n grep.h | 1 +\n object.c | 26 ++++++++---\n object.h | 2 +\n t/t4030-diff-textconv.sh | 18 ++++++++\n t/t7008-grep-binary.sh | 39 +++++++++++++++++\n t/t8007-cat-file-textconv.sh | 20 +++------\n 10 files changed, 205 insertions(+), 47 deletions(-)\n\n-- \n1.8.2.1.728.ge98e8b0\n"},{"id":"214839","messageId":"a3162a9df3055532a818db264f43abc994325049.1366389739.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"cover.1366389739.git.git@drmicha.warpmail.net","subject":"[PATCH 1/6] t4030: demonstrate behavior of show with textconv","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-19T16:44:44Z","receivedAt":"2013-04-19T16:44:44Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"\"git show \" obeys the textconc setting while \"git show \"\ndoes not. Demonstrate this in the test.\n\nSigned-off-by: Michael J Gruber \n---\n t/t4030-diff-textconv.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 53ec330..f314ced 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -58,6 +58,12 @@ test_expect_success 'diff produces text' '\n \ttest_cmp expect.text actual\n '\n \n+test_expect_success 'show commit produces text' '\n+\tgit show 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@@ -84,6 +90,12 @@ test_expect_success 'status -v produces text' '\n \tgit reset --soft HEAD@{1}\n '\n \n+test_expect_success 'show blob produces binary' '\n+\tgit show HEAD:file >actual &&\n+\tprintf \"\\\\0\\\\n\\\\1\\\\n\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'grep-diff (-G) operates on textconv data (add)' '\n \techo one >expect &&\n \tgit log --root --format=%s -G0 >actual &&\n-- \n1.8.2.1.728.ge98e8b0\n"},{"id":"214840","messageId":"5a8c85faddf7f93ca16d284bde415a32dd76779a.1366389739.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"cover.1366389739.git.git@drmicha.warpmail.net","subject":"[PATCH 2/6] show: obey --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-19T16:44:45Z","receivedAt":"2013-04-19T16:44:45Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, \"diff\" and \"cat-file\" for blobs obey \"--textconv\" options\n(with the former defaulting to \"--textconv\" and the latter to\n\"--no-textconv\") whereas \"show\" does not obey this option, even though\nit takes diff options.\n\nMake \"show\" on blobs behave like \"diff\", i.e. obey \"--textconv\" by\ndefault and \"--no-textconv\" when given.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/log.c | 24 +++++++++++++++++++++---\n t/t4030-diff-textconv.sh | 8 +++++++-\n 2 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 5f3ed77..fe0275e 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -436,10 +436,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)\n \tstrbuf_release(&out);\n }\n \n-static int show_blob_object(const unsigned char *sha1, struct rev_info *rev)\n+static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)\n {\n+\tunsigned char sha1c[20];\n+\tstruct object_context obj_context;\n+\tchar *buf;\n+\tunsigned long size;\n+\n \tfflush(stdout);\n-\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n+\tif (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))\n+\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n+\n+\tif (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))\n+\t\tdie(\"Not a valid object name %s\", obj_name);\n+\tif (!obj_context.path[0] ||\n+\t !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, &buf, &size))\n+\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n+\n+\tif (!buf)\n+\t\tdie(\"git show %s: bad file\", obj_name);\n+\n+\twrite_or_die(1, buf, size);\n+\treturn 0;\n }\n \n static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)\n@@ -525,7 +543,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\tconst char *name = objects[i].name;\n \t\tswitch (o->type) {\n \t\tcase OBJ_BLOB:\n-\t\t\tret = show_blob_object(o->sha1, NULL);\n+\t\t\tret = show_blob_object(o->sha1, &rev, name);\n \t\t\tbreak;\n \t\tcase OBJ_TAG: {\n \t\t\tstruct tag *t = (struct tag *)o;\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex f314ced..f9d55e1 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -90,8 +90,14 @@ test_expect_success 'status -v produces text' '\n \tgit reset --soft HEAD@{1}\n '\n \n-test_expect_success 'show blob produces binary' '\n+test_expect_success 'show blob produces text' '\n \tgit show HEAD:file >actual &&\n+\tprintf \"0\\\\n1\\\\n\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'show --no-textconv blob produces binary' '\n+\tgit show --no-textconv HEAD:file >actual &&\n \tprintf \"\\\\0\\\\n\\\\1\\\\n\" >expect &&\n \ttest_cmp expect actual\n '\n-- \n1.8.2.1.728.ge98e8b0\n"},{"id":"214841","messageId":"06f2d51bf0479f3231b707d88d8d04fcd306c973.1366389739.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"cover.1366389739.git.git@drmicha.warpmail.net","subject":"[PATCH 3/6] cat-file: do not die on --textconv without textconv filters","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-19T16:44:46Z","receivedAt":"2013-04-19T16:44:46Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"When a command is supposed to use textconv filters (by default or with\n\"--textconv\") and none are configured then the blob is output without\nconversion; the only exception to this rule is \"cat-file --textconv\".\n\nMake it behave like the rest of textconv aware commands.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/cat-file.c | 9 +++++----\n t/t8007-cat-file-textconv.sh | 20 +++++---------------\n 2 files changed, 10 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 40f87b4..dd4e063 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -146,10 +146,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \t\t\tdie(\"git cat-file --textconv %s: must be \",\n \t\t\t obj_name);\n \n-\t\tif (!textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n-\t\t\tdie(\"git cat-file --textconv: unable to run textconv on %s\",\n-\t\t\t obj_name);\n-\t\tbreak;\n+\t\tif (textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n+\t\t\tbreak;\n+\n+\t\t/* otherwise expect a blob */\n+\t\texp_type = \"blob\";\n \n \tcase 0:\n \t\tif (type_from_string(exp_type) == OBJ_BLOB) {\ndiff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh\nindex 78a0085..83c6636 100755\n--- a/t/t8007-cat-file-textconv.sh\n+++ b/t/t8007-cat-file-textconv.sh\n@@ -22,11 +22,11 @@ test_expect_success 'setup ' '\n '\n \n cat >expected <result\n+\tgit cat-file --textconv :one.bin >result &&\n \ttest_cmp expected result\n '\n \n@@ -36,10 +36,6 @@ test_expect_success 'setup textconv filters' '\n \tgit config diff.test.cachetextconv false\n '\n \n-cat >expected <result &&\n \ttest_cmp expected result\n@@ -73,25 +69,19 @@ test_expect_success 'cat-file --textconv on previous commit' '\n '\n \n test_expect_success SYMLINKS 'cat-file without --textconv (symlink)' '\n+\tprintf \"%s\" \"one.bin\" >expected &&\n \tgit cat-file blob :symlink.bin >result &&\n-\tprintf \"%s\" \"one.bin\" >expected\n \ttest_cmp expected result\n '\n \n \n test_expect_success SYMLINKS 'cat-file --textconv on index (symlink)' '\n-\t! git cat-file --textconv :symlink.bin 2>result &&\n-\tcat >expected <<\\EOF &&\n-fatal: git cat-file --textconv: unable to run textconv on :symlink.bin\n-EOF\n+\tgit cat-file --textconv :symlink.bin >result &&\n \ttest_cmp expected result\n '\n \n test_expect_success SYMLINKS 'cat-file --textconv on HEAD (symlink)' '\n-\t! git cat-file --textconv HEAD:symlink.bin 2>result &&\n-\tcat >expected <result &&\n \ttest_cmp expected result\n '\n \n-- \n1.8.2.1.728.ge98e8b0\n"},{"id":"214842","messageId":"b5e2a5d967855df362319edfb511686236b176ad.1366389739.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"cover.1366389739.git.git@drmicha.warpmail.net","subject":"[PATCH 4/6] t7008: demonstrate behavior of grep with textconv","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-19T16:44:47Z","receivedAt":"2013-04-19T16:44:47Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, \"git grep\" does not invoke any textconv filters. Demonstrate\nthis in the tests.\n\nSigned-off-by: Michael J Gruber \n---\n t/t7008-grep-binary.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex 26f8319..a1fd0b2 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -145,4 +145,23 @@ test_expect_success 'grep respects not-binary diff attribute' '\n \ttest_cmp expect actual\n '\n \n+cat >nul_to_q_textconv <<'EOF'\n+#!/bin/sh\n+\"$PERL_PATH\" -pe 'y/\\000/Q/' < \"$1\"\n+EOF\n+chmod +x nul_to_q_textconv\n+\n+test_expect_success 'setup textconv filters' '\n+\techo a diff=foo >.gitattributes &&\n+\tgit config diff.foo.textconv \"\\\"$(pwd)\\\"\"/nul_to_q_textconv\n+'\n+\n+test_expect_success 'grep does not obey textconv' '\n+\ttest_must_fail git grep Qfile\n+'\n+\n+test_expect_success 'grep blob does not obey textconv' '\n+\ttest_must_fail git grep Qfile HEAD:a\n+'\n+\n test_done\n-- \n1.8.2.1.728.ge98e8b0\n"},{"id":"214843","messageId":"2e4b789c1578660b8b62eabd9e0418a3edbc8f6a.1366389739.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"cover.1366389739.git.git@drmicha.warpmail.net","subject":"[PATCH 5/6] grep: allow to use textconv filters","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-19T16:44:48Z","receivedAt":"2013-04-19T16:44:48Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"From: Jeff King \n\nRecently and not so recently, we made sure that log/grep type operations\nuse textconv filters when a userfacing diff would do the same:\n\nef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28)\nb1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28)\n0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)\n\n\"git grep\" currently does not use textconv filters at all, that is\nneither for displaying the match and context nor for the actual grepping.\n\nIntroduce an option \"--textconv\" which makes git grep use any configured\ntextconv filters for grepping and output purposes. It is off by default.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/grep.c | 2 +\n grep.c | 100 ++++++++++++++++++++++++++++++++++++++++++-------\n grep.h | 1 +\n t/t7008-grep-binary.sh | 18 +++++++++\n 4 files changed, 107 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 159e65d..00ee57d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -659,6 +659,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_SET_INT('I', NULL, &opt.binary,\n \t\t\tN_(\"don't match patterns in binary files\"),\n \t\t\tGREP_BINARY_NOMATCH),\n+\t\tOPT_BOOL(0, \"textconv\", &opt.allow_textconv,\n+\t\t\t N_(\"process binary files with textconv filters\")),\n \t\t{ OPTION_INTEGER, 0, \"max-depth\", &opt.max_depth, N_(\"depth\"),\n \t\t\tN_(\"descend at most levels\"), PARSE_OPT_NONEG,\n \t\t\tNULL, 1 },\ndiff --git a/grep.c b/grep.c\nindex bb548ca..c668034 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -2,6 +2,8 @@\n #include \"grep.h\"\n #include \"userdiff.h\"\n #include \"xdiff-interface.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n \n static int grep_source_load(struct grep_source *gs);\n static int grep_source_is_binary(struct grep_source *gs);\n@@ -1322,6 +1324,58 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n \tfwrite(buf, size, 1, stdout);\n }\n \n+static int fill_textconv_grep(struct userdiff_driver *driver,\n+\t\t\t struct grep_source *gs)\n+{\n+\tstruct diff_filespec *df;\n+\tchar *buf;\n+\tsize_t size;\n+\n+\tif (!driver || !driver->textconv)\n+\t\treturn grep_source_load(gs);\n+\n+\t/*\n+\t * The textconv interface is intimately tied to diff_filespecs, so we\n+\t * have to pretend to be one. If we could unify the grep_source\n+\t * and diff_filespec structs, this mess could just go away.\n+\t */\n+\tdf = alloc_filespec(gs->path);\n+\tswitch (gs->type) {\n+\tcase GREP_SOURCE_SHA1:\n+\t\tfill_filespec(df, gs->identifier, 1, 0100644);\n+\t\tbreak;\n+\tcase GREP_SOURCE_FILE:\n+\t\tfill_filespec(df, null_sha1, 0, 0100644);\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: attempt to textconv something without a path?\");\n+\t}\n+\n+\t/*\n+\t * fill_textconv is not remotely thread-safe; it may load objects\n+\t * behind the scenes, and it modifies the global diff tempfile\n+\t * structure.\n+\t */\n+\tgrep_read_lock();\n+\tsize = fill_textconv(driver, df, &buf);\n+\tgrep_read_unlock();\n+\tfree_filespec(df);\n+\n+\t/*\n+\t * The normal fill_textconv usage by the diff machinery would just keep\n+\t * the textconv'd buf separate from the diff_filespec. But much of the\n+\t * grep code passes around a grep_source and assumes that its \"buf\"\n+\t * pointer is the beginning of the thing we are searching. So let's\n+\t * install our textconv'd version into the grep_source, taking care not\n+\t * to leak any existing buffer.\n+\t */\n+\tgrep_source_clear_data(gs);\n+\tgs->buf = buf;\n+\tgs->size = size;\n+\n+\treturn 0;\n+}\n+\n static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)\n {\n \tchar *bol;\n@@ -1332,6 +1386,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \tunsigned count = 0;\n \tint try_lookahead = 0;\n \tint show_function = 0;\n+\tstruct userdiff_driver *textconv = NULL;\n \tenum grep_context ctx = GREP_CONTEXT_HEAD;\n \txdemitconf_t xecfg;\n \n@@ -1353,19 +1408,36 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t}\n \topt->last_shown = 0;\n \n-\tswitch (opt->binary) {\n-\tcase GREP_BINARY_DEFAULT:\n-\t\tif (grep_source_is_binary(gs))\n-\t\t\tbinary_match_only = 1;\n-\t\tbreak;\n-\tcase GREP_BINARY_NOMATCH:\n-\t\tif (grep_source_is_binary(gs))\n-\t\t\treturn 0; /* Assume unmatch */\n-\t\tbreak;\n-\tcase GREP_BINARY_TEXT:\n-\t\tbreak;\n-\tdefault:\n-\t\tdie(\"bug: unknown binary handling mode\");\n+\tif (opt->allow_textconv) {\n+\t\tgrep_source_load_driver(gs);\n+\t\t/*\n+\t\t * We might set up the shared textconv cache data here, which\n+\t\t * is not thread-safe.\n+\t\t */\n+\t\tgrep_attr_lock();\n+\t\ttextconv = userdiff_get_textconv(gs->driver);\n+\t\tgrep_attr_unlock();\n+\t}\n+\n+\t/*\n+\t * We know the result of a textconv is text, so we only have to care\n+\t * about binary handling if we are not using it.\n+\t */\n+\tif (!textconv) {\n+\t\tswitch (opt->binary) {\n+\t\tcase GREP_BINARY_DEFAULT:\n+\t\t\tif (grep_source_is_binary(gs))\n+\t\t\t\tbinary_match_only = 1;\n+\t\t\tbreak;\n+\t\tcase GREP_BINARY_NOMATCH:\n+\t\t\tif (grep_source_is_binary(gs))\n+\t\t\t\treturn 0; /* Assume unmatch */\n+\t\t\tbreak;\n+\t\tcase GREP_BINARY_TEXT:\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tdie(\"bug: unknown binary handling mode\");\n+\t\t}\n \t}\n \n \tmemset(&xecfg, 0, sizeof(xecfg));\n@@ -1373,7 +1445,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \n \ttry_lookahead = should_lookahead(opt);\n \n-\tif (grep_source_load(gs) < 0)\n+\tif (fill_textconv_grep(textconv, gs) < 0)\n \t\treturn 0;\n \n \tbol = gs->buf;\ndiff --git a/grep.h b/grep.h\nindex e4a1df5..eaaced1 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -107,6 +107,7 @@ struct grep_opt {\n #define GREP_BINARY_NOMATCH\t1\n #define GREP_BINARY_TEXT\t2\n \tint binary;\n+\tint allow_textconv;\n \tint extended;\n \tint use_reflog_filter;\n \tint pcre;\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex a1fd0b2..a7fe94a 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -160,8 +160,26 @@ test_expect_success 'grep does not obey textconv' '\n \ttest_must_fail git grep Qfile\n '\n \n+test_expect_success 'grep --textconv does obey textconv' '\n+\techo \"a:binaryQfile\" >expect &&\n+\tgit grep --textconv Qfile >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --no-textconv does not obey textconv' '\n+\ttest_must_fail git grep Qfile\n+'\n+\n test_expect_success 'grep blob does not obey textconv' '\n \ttest_must_fail git grep Qfile HEAD:a\n '\n \n+test_expect_success 'grep --textconv blob does not obey textconv' '\n+\ttest_must_fail git grep --textconv Qfile HEAD:a\n+'\n+\n+test_expect_success 'grep --no-textconv blob does not obey textconv' '\n+\ttest_must_fail git grep --no-textconv Qfile HEAD:a\n+'\n+\n test_done\n-- \n1.8.2.1.728.ge98e8b0\n"},{"id":"214844","messageId":"717ec305e9bd056a44b1da5cc478d314db2920e5.1366389739.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"cover.1366389739.git.git@drmicha.warpmail.net","subject":"[PATCH 6/6] grep: obey --textconv for the case rev:path","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-19T16:44:49Z","receivedAt":"2013-04-19T16:44:49Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Make \"grep\" obey the \"--textconv\" option also for the object case, i.e.\nwhen used with an argument \"rev:path\".\n\nSigned-off-by: Michael J Gruber \n---\n builtin/grep.c | 11 ++++++-----\n object.c | 26 ++++++++++++++++++++------\n object.h | 2 ++\n t/t7008-grep-binary.sh | 6 ++++--\n 4 files changed, 32 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 00ee57d..bb7f970 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -458,10 +458,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n }\n \n static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n-\t\t struct object *obj, const char *name)\n+\t\t struct object *obj, const char *name, struct object_context *oc)\n {\n \tif (obj->type == OBJ_BLOB)\n-\t\treturn grep_sha1(opt, obj->sha1, name, 0, NULL);\n+\t\treturn grep_sha1(opt, obj->sha1, name, 0, oc ? oc->path : NULL);\n \tif (obj->type == OBJ_COMMIT || obj->type == OBJ_TREE) {\n \t\tstruct tree_desc tree;\n \t\tvoid *data;\n@@ -503,7 +503,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object *real_obj;\n \t\treal_obj = deref_tag(list->objects[i].item, NULL, 0);\n-\t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name)) {\n+\t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name, list->objects[i].context)) {\n \t\t\thit = 1;\n \t\t\tif (opt->status_only)\n \t\t\t\tbreak;\n@@ -820,12 +820,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tunsigned char sha1[20];\n+\t\tstruct object_context oc;\n \t\t/* Is it a rev? */\n-\t\tif (!get_sha1(arg, sha1)) {\n+\t\tif (!get_sha1_with_context(arg, 0, sha1, &oc)) {\n \t\t\tstruct object *object = parse_object_or_die(sha1, arg);\n \t\t\tif (!seen_dashdash)\n \t\t\t\tverify_non_filename(prefix, arg);\n-\t\t\tadd_object_array(object, arg, &list);\n+\t\t\tadd_object_array_with_context(object, arg, &list, xmemdupz(&oc, sizeof(struct object_context)));\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--\")) {\ndiff --git a/object.c b/object.c\nindex 20703f5..c8ffc9e 100644\n--- a/object.c\n+++ b/object.c\n@@ -255,12 +255,7 @@ int object_list_contains(struct object_list *list, struct object *obj)\n \treturn 0;\n }\n \n-void add_object_array(struct object *obj, const char *name, struct object_array *array)\n-{\n-\tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n-}\n-\n-void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n+static void add_object_array_with_mode_context(struct object *obj, const char *name, struct object_array *array, unsigned mode, struct object_context *context)\n {\n \tunsigned nr = array->nr;\n \tunsigned alloc = array->alloc;\n@@ -275,9 +270,28 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj\n \tobjects[nr].item = obj;\n \tobjects[nr].name = name;\n \tobjects[nr].mode = mode;\n+\tobjects[nr].context = context;\n \tarray->nr = ++nr;\n }\n \n+void add_object_array(struct object *obj, const char *name, struct object_array *array)\n+{\n+\tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n+}\n+\n+void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n+{\n+\tadd_object_array_with_mode_context(obj, name, array, mode, NULL);\n+}\n+\n+void add_object_array_with_context(struct object *obj, const char *name, struct object_array *array, struct object_context *context)\n+{\n+\tif (context)\n+\t\tadd_object_array_with_mode_context(obj, name, array, context->mode, context);\n+\telse\n+\t\tadd_object_array_with_mode_context(obj, name, array, S_IFINVALID, context);\n+}\n+\n void object_array_remove_duplicates(struct object_array *array)\n {\n \tunsigned int ref, src, dst;\ndiff --git a/object.h b/object.h\nindex 97d384b..695847d 100644\n--- a/object.h\n+++ b/object.h\n@@ -13,6 +13,7 @@ struct object_array {\n \t\tstruct object *item;\n \t\tconst char *name;\n \t\tunsigned mode;\n+\t\tstruct object_context *context;\n \t} *objects;\n };\n \n@@ -85,6 +86,7 @@ int object_list_contains(struct object_list *list, struct object *obj);\n /* Object array handling .. */\n void add_object_array(struct object *obj, const char *name, struct object_array *array);\n void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode);\n+void add_object_array_with_context(struct object *obj, const char *name, struct object_array *array, struct object_context *context);\n void object_array_remove_duplicates(struct object_array *);\n \n void clear_object_flags(unsigned flags);\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex a7fe94a..ef78abe 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -174,8 +174,10 @@ test_expect_success 'grep blob does not obey textconv' '\n \ttest_must_fail git grep Qfile HEAD:a\n '\n \n-test_expect_success 'grep --textconv blob does not obey textconv' '\n-\ttest_must_fail git grep --textconv Qfile HEAD:a\n+test_expect_success 'grep --textconv blob does obey textconv' '\n+\techo \"HEAD:a:binaryQfile\" >expect &&\n+\tgit grep --textconv Qfile HEAD:a >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'grep --no-textconv blob does not obey textconv' '\n-- \n1.8.2.1.728.ge98e8b0\n"},{"id":"214858","messageId":"7vli8e1j4y.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"06f2d51bf0479f3231b707d88d8d04fcd306c973.1366389739.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 3/6] cat-file: do not die on --textconv without textconv filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-19T18:15:57Z","receivedAt":"2013-04-19T18:15:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> When a command is supposed to use textconv filters (by default or with\n> \"--textconv\") and none are configured then the blob is output without\n> conversion; the only exception to this rule is \"cat-file --textconv\".\n\nI am of two minds. Because cat-file is mostly a low-level plumbing,\nI do not necessarily think it is a bad behaviour for it to error out\nwhen it was asked to apply textconv where there is no filter or when\nthe filter fails to produce an output. On the other hand, it\ncertainly makes it more convenient for callers that do not care too\ndeeply, taking textconv as a mere hint just like Porcelains do.\n\nBut assuming that this is the direction we would want to go...\n\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 40f87b4..dd4e063 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -146,10 +146,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n> \t\t\tdie(\"git cat-file --textconv %s: must be \",\n> \t\t\t obj_name);\n> \n> -\t\tif (!textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n> -\t\t\tdie(\"git cat-file --textconv: unable to run textconv on %s\",\n> -\t\t\t obj_name);\n> -\t\tbreak;\n> +\t\tif (textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n> +\t\t\tbreak;\n> +\n> +\t\t/* otherwise expect a blob */\n> +\t\texp_type = \"blob\";\n\nPlease use the constant string blob_type that is available for all\ncallers including this one.\n"},{"id":"214860","messageId":"7vhaj21ir3.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"cover.1366389739.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 0/6] grep with textconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-19T18:24:16Z","receivedAt":"2013-04-19T18:24:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> This series teaches show and grep to obey textconv: show by\n> default (like diff), grep only on request (--textconv). We might\n> switch the default for the latter also, of course. I'd actually\n> like that.\n>\n> Compared to an earlier (historic) series this one comes with tests.\n\nIt would have been nicer if you referred to the previous thread\n\ncf.\n\n http://thread.gmane.org/gmane.comp.version-control.git/215385\n\n> grep: allow to use textconv filters\n\nThis looked mostly sensible except for one minor \"eh, do we really\nneed to assume textconv output is text, or wouldn't using the same\ncodepath for raw blob and textconv result to make them consistently\nhonor opt->binary easier to explain?\".\n\n> t4030: demonstrate behavior of show with textconv\n> t7008: demonstrate behavior of grep with textconv\n\nIt somehow felt they are better together in the patches that\nimplement the features they exercise.\n\n> show: obey --textconv for blobs\n> cat-file: do not die on --textconv without textconv filters\n> grep: obey --textconv for the case rev:path\n\nI just let my eyes coast over these but didn't see anything\nobviously wrong.\n\nBy the way, \"git log --no-merges | grep obey | wc -l\" shows that we\nsay \"honor an option\" a lot more than \"obey an option\". We may want\nto be consistent here.\n"},{"id":"214899","messageId":"20130420040400.GA24970@sigill.intra.peff.net","threadId":"33545","inReplyTo":"a3162a9df3055532a818db264f43abc994325049.1366389739.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 1/6] t4030: demonstrate behavior of show with textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-20T04:04:00Z","receivedAt":"2013-04-20T04:04:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 19, 2013 at 06:44:44PM +0200, Michael J Gruber wrote:\n\n> \"git show \" obeys the textconc setting while \"git show \"\n> does not. Demonstrate this in the test.\n\ns/textconc/textconv\n\n> diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\n> index 53ec330..f314ced 100755\n> --- a/t/t4030-diff-textconv.sh\n> +++ b/t/t4030-diff-textconv.sh\n> @@ -58,6 +58,12 @@ test_expect_success 'diff produces text' '\n> \ttest_cmp expect.text actual\n> '\n> \n> +test_expect_success 'show commit produces text' '\n> +\tgit show HEAD >diff &&\n> +\tfind_diff actual &&\n> +\ttest_cmp expect.text actual\n> +'\n\nMakes sense.\n\n> +test_expect_success 'show blob produces binary' '\n> +\tgit show HEAD:file >actual &&\n> +\tprintf \"\\\\0\\\\n\\\\1\\\\n\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n\nI think this is probably the right thing. I can see instances where one\nwould want the converted contents, but we have \"cat-file --textconv\" for\nthat.\n\n-Peff\n"},{"id":"214900","messageId":"20130420040643.GB24970@sigill.intra.peff.net","threadId":"33545","inReplyTo":"5a8c85faddf7f93ca16d284bde415a32dd76779a.1366389739.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 2/6] show: obey --textconv for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-20T04:06:43Z","receivedAt":"2013-04-20T04:06:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 19, 2013 at 06:44:45PM +0200, Michael J Gruber wrote:\n\n> Currently, \"diff\" and \"cat-file\" for blobs obey \"--textconv\" options\n> (with the former defaulting to \"--textconv\" and the latter to\n> \"--no-textconv\") whereas \"show\" does not obey this option, even though\n> it takes diff options.\n> \n> Make \"show\" on blobs behave like \"diff\", i.e. obey \"--textconv\" by\n> default and \"--no-textconv\" when given.\n\nWait, this does the opposite of the last patch. If we do want to do\nthis, shouldn't the last one have been an \"expect_failure\"?\n\nI'm not convinced this is the right thing to do, though. It would break:\n\n git show HEAD:file.c >file.c\n\nAdmittedly, such people should be using \"checkout\" or \"cat-file\", so I\ndo not mind too much breaking them if there is a good reason. But I am\nnot sure what that reason is.\n\n-Peff\n"},{"id":"214901","messageId":"20130420041737.GC24970@sigill.intra.peff.net","threadId":"33545","inReplyTo":"06f2d51bf0479f3231b707d88d8d04fcd306c973.1366389739.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 3/6] cat-file: do not die on --textconv without textconv filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-20T04:17:37Z","receivedAt":"2013-04-20T04:17:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 19, 2013 at 06:44:46PM +0200, Michael J Gruber wrote:\n\n> -\t\t\tdie(\"git cat-file --textconv: unable to run textconv on %s\",\n> -\t\t\t obj_name);\n> -\t\tbreak;\n> +\t\tif (textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n> +\t\t\tbreak;\n> +\n> +\t\t/* otherwise expect a blob */\n> +\t\texp_type = \"blob\";\n> \n> \tcase 0:\n> \t\tif (type_from_string(exp_type) == OBJ_BLOB) {\n\nI'm not sure this is right. What happens with:\n\n git cat-file --textconv HEAD:Documentation\n\nWe have failed to textconv, but should we be expecting a blob?\n\n-Peff\n"},{"id":"214902","messageId":"20130420042445.GD24970@sigill.intra.peff.net","threadId":"33545","inReplyTo":"717ec305e9bd056a44b1da5cc478d314db2920e5.1366389739.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 6/6] grep: obey --textconv for the case rev:path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-20T04:24:46Z","receivedAt":"2013-04-20T04:24:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 19, 2013 at 06:44:49PM +0200, Michael J Gruber wrote:\n\n> @@ -820,12 +820,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \tfor (i = 0; i < argc; i++) {\n> \t\tconst char *arg = argv[i];\n> \t\tunsigned char sha1[20];\n> +\t\tstruct object_context oc;\n> \t\t/* Is it a rev? */\n> -\t\tif (!get_sha1(arg, sha1)) {\n> +\t\tif (!get_sha1_with_context(arg, 0, sha1, &oc)) {\n> \t\t\tstruct object *object = parse_object_or_die(sha1, arg);\n> \t\t\tif (!seen_dashdash)\n> \t\t\t\tverify_non_filename(prefix, arg);\n> -\t\t\tadd_object_array(object, arg, &list);\n> +\t\t\tadd_object_array_with_context(object, arg, &list, xmemdupz(&oc, sizeof(struct object_context)));\n\nHrm. I'm not excited about the extra allocation here. Who frees it?\n\n> +void add_object_array(struct object *obj, const char *name, struct object_array *array)\n> +{\n> +\tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n> +}\n> +\n> +void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n> +{\n> +\tadd_object_array_with_mode_context(obj, name, array, mode, NULL);\n> +}\n> +\n> +void add_object_array_with_context(struct object *obj, const char *name, struct object_array *array, struct object_context *context)\n> +{\n> +\tif (context)\n> +\t\tadd_object_array_with_mode_context(obj, name, array, context->mode, context);\n> +\telse\n> +\t\tadd_object_array_with_mode_context(obj, name, array, S_IFINVALID, context);\n> +}\n\nAnd this mass of almost-the-same functions is gross, too, especially\ngiven that the object_context contains a mode itself.\n\nUnfortunately, I'm not sure if I have a more pleasant suggestion. I seem\nto recall wrestling with this issue during the last round, too.\n\n-Peff\n"},{"id":"214903","messageId":"20130420042629.GE24970@sigill.intra.peff.net","threadId":"33545","inReplyTo":"7vhaj21ir3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/6] grep with textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-20T04:26:29Z","receivedAt":"2013-04-20T04:26:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 19, 2013 at 11:24:16AM -0700, Junio C Hamano wrote:\n\n> > grep: allow to use textconv filters\n> \n> This looked mostly sensible except for one minor \"eh, do we really\n> need to assume textconv output is text, or wouldn't using the same\n> codepath for raw blob and textconv result to make them consistently\n> honor opt->binary easier to explain?\".\n\nI don't mind re-checking the textconv output for binary-ness. But I did\nit that way for consistency with the diff code-path, which also assumes\nthat textconv output is not binary.\n\n> By the way, \"git log --no-merges | grep obey | wc -l\" shows that we\n> say \"honor an option\" a lot more than \"obey an option\". We may want\n> to be consistent here.\n\nYeah, it is a pretty minor thing, but I agree that \"honor\" sounds\nbetter.\n\n-Peff\n"},{"id":"214904","messageId":"20130420043122.GF24970@sigill.intra.peff.net","threadId":"33545","inReplyTo":"2e4b789c1578660b8b62eabd9e0418a3edbc8f6a.1366389739.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 5/6] grep: allow to use textconv filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-20T04:31:22Z","receivedAt":"2013-04-20T04:31:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 19, 2013 at 06:44:48PM +0200, Michael J Gruber wrote:\n\n> From: Jeff King \n> \n> Recently and not so recently, we made sure that log/grep type operations\n> use textconv filters when a userfacing diff would do the same:\n> \n> ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28)\n> b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28)\n> 0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)\n> \n> \"git grep\" currently does not use textconv filters at all, that is\n> neither for displaying the match and context nor for the actual grepping.\n> \n> Introduce an option \"--textconv\" which makes git grep use any configured\n> textconv filters for grepping and output purposes. It is off by default.\n> \n> Signed-off-by: Michael J Gruber \n> ---\n> builtin/grep.c | 2 +\n> grep.c | 100 ++++++++++++++++++++++++++++++++++++++++++-------\n> grep.h | 1 +\n> t/t7008-grep-binary.sh | 18 +++++++++\n> 4 files changed, 107 insertions(+), 14 deletions(-)\n\nThis patch, of course, is flawless. :)\n\nFeel free to add:\n\n Signed-off-by: Jeff King \n\n> +\t/*\n> +\t * We know the result of a textconv is text, so we only have to care\n> +\t * about binary handling if we are not using it.\n> +\t */\n> +\tif (!textconv) {\n> +\t\tswitch (opt->binary) {\n> +\t\tcase GREP_BINARY_DEFAULT:\n> +\t\t\tif (grep_source_is_binary(gs))\n> +\t\t\t\tbinary_match_only = 1;\n> +\t\t\tbreak;\n> +\t\tcase GREP_BINARY_NOMATCH:\n> +\t\t\tif (grep_source_is_binary(gs))\n> +\t\t\t\treturn 0; /* Assume unmatch */\n> +\t\t\tbreak;\n> +\t\tcase GREP_BINARY_TEXT:\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tdie(\"bug: unknown binary handling mode\");\n> +\t\t}\n> \t}\n\nJunio mentioned checking the textconv output for binary-ness. Doing that\nwould involve removing the outer conditional here. But it's not quite so\nsimple, as we don't load the textconv results until later, and\ngrep_source_is_binary will lazily load the contents. I think it would be\nsufficient to fill_textconv_grep() right before, and then\ngrep_source_is_binary would rely on the cached buffer.\n\n-Peff\n"},{"id":"214926","messageId":"517298D4.3030802@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7vhaj21ir3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/6] grep with textconv","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-20T13:32:04Z","receivedAt":"2013-04-20T13:32:04Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 19.04.2013 20:24:\n> Michael J Gruber writes:\n> \n>> This series teaches show and grep to obey textconv: show by\n>> default (like diff), grep only on request (--textconv). We might\n>> switch the default for the latter also, of course. I'd actually\n>> like that.\n>>\n>> Compared to an earlier (historic) series this one comes with tests.\n> \n> It would have been nicer if you referred to the previous thread\n> \n> cf.\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/215385\n\nYes, sorry, I was on a slow mobile connection due to DSL breakage...\n\n>> grep: allow to use textconv filters\n> \n> This looked mostly sensible except for one minor \"eh, do we really\n> need to assume textconv output is text, or wouldn't using the same\n> codepath for raw blob and textconv result to make them consistently\n> honor opt->binary easier to explain?\".\n>\n\nI think we assume in general that textconv produces text, which is maybe\nnot completely surprising given its name ;)\n\n>> t4030: demonstrate behavior of show with textconv\n>> t7008: demonstrate behavior of grep with textconv\n> \n> It somehow felt they are better together in the patches that\n> implement the features they exercise.\n\nI added them after the fact. They can be squashed in, of course. On the\nother hand you don't see the change in behavior that the latter patches\nintroduce any more if you that; which is why I left them separate at\nleast for review purposes and for camparing to the previous series which\nI had failed to reference.\n\n>> show: obey --textconv for blobs\n>> cat-file: do not die on --textconv without textconv filters\n>> grep: obey --textconv for the case rev:path\n> \n> I just let my eyes coast over these but didn't see anything\n> obviously wrong.\n> \n> By the way, \"git log --no-merges | grep obey | wc -l\" shows that we\n> say \"honor an option\" a lot more than \"obey an option\". We may want\n> to be consistent here.\n\nOkay, let's be honorable rather than obedient.\n\nMichael\n"},{"id":"214922","messageId":"5172999C.1050407@drmicha.warpmail.net","threadId":"33545","inReplyTo":"20130420040400.GA24970@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] t4030: demonstrate behavior of show with textconv","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-20T13:35:24Z","receivedAt":"2013-04-20T13:35:24Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 20.04.2013 06:04:\n> On Fri, Apr 19, 2013 at 06:44:44PM +0200, Michael J Gruber wrote:\n> \n>> \"git show \" obeys the textconc setting while \"git show \"\n>> does not. Demonstrate this in the test.\n> \n> s/textconc/textconv\n\nThanks, plus s/obey/honor/\n\n>> diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\n>> index 53ec330..f314ced 100755\n>> --- a/t/t4030-diff-textconv.sh\n>> +++ b/t/t4030-diff-textconv.sh\n>> @@ -58,6 +58,12 @@ test_expect_success 'diff produces text' '\n>> \ttest_cmp expect.text actual\n>> '\n>> \n>> +test_expect_success 'show commit produces text' '\n>> +\tgit show HEAD >diff &&\n>> +\tfind_diff actual &&\n>> +\ttest_cmp expect.text actual\n>> +'\n> \n> Makes sense.\n> \n>> +test_expect_success 'show blob produces binary' '\n>> +\tgit show HEAD:file >actual &&\n>> +\tprintf \"\\\\0\\\\n\\\\1\\\\n\" >expect &&\n>> +\ttest_cmp expect actual\n>> +'\n> \n> I think this is probably the right thing. I can see instances where one\n> would want the converted contents, but we have \"cat-file --textconv\" for\n> that.\n> \n\nBy that you mean that this behavior is to stay as is?\n\nMy reasoning is twofold:\n\n- consistency between \"git show commit\" and \"git show blob\"\n\n- \"git show\" is a user facing command, and as such should produce output\nconsumable by humans; whereas \"git cat-file\" is plumbing and should\nproduce raw data unless told otherwise (-p, --textconv).\n\nMichael\n"},{"id":"214925","messageId":"51729A6D.3030501@drmicha.warpmail.net","threadId":"33545","inReplyTo":"20130420040643.GB24970@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] show: obey --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-20T13:38:53Z","receivedAt":"2013-04-20T13:38:53Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 20.04.2013 06:06:\n> On Fri, Apr 19, 2013 at 06:44:45PM +0200, Michael J Gruber wrote:\n> \n>> Currently, \"diff\" and \"cat-file\" for blobs obey \"--textconv\" options\n>> (with the former defaulting to \"--textconv\" and the latter to\n>> \"--no-textconv\") whereas \"show\" does not obey this option, even though\n>> it takes diff options.\n>>\n>> Make \"show\" on blobs behave like \"diff\", i.e. obey \"--textconv\" by\n>> default and \"--no-textconv\" when given.\n> \n> Wait, this does the opposite of the last patch. If we do want to do\n> this, shouldn't the last one have been an \"expect_failure\"?\n\nThe last patch just documents the status quo, which is not a bug per se.\nTherefore, no failure, but change in the definition of \"success\".\n\n> I'm not convinced this is the right thing to do, though. It would break:\n> \n> git show HEAD:file.c >file.c\n> \n> Admittedly, such people should be using \"checkout\" or \"cat-file\", so I\n> do not mind too much breaking them if there is a good reason. But I am\n> not sure what that reason is.\n\nMy reasoning is twofold:\n\n- consistency between \"git show commit\" and \"git show blob\"\n\n- \"git show\" is a user facing command, and as such should produce output\nconsumable by humans; whereas \"git cat-file\" is plumbing and should\nproduce raw data unless told otherwise (-p, --textconv).\n\n(Sorry for the repeat.)\n\nMichael\n"},{"id":"214923","messageId":"5172A5E4.7020902@drmicha.warpmail.net","threadId":"33545","inReplyTo":"20130420041737.GC24970@sigill.intra.peff.net","subject":"Re: [PATCH 3/6] cat-file: do not die on --textconv without textconv filters","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-20T14:27:48Z","receivedAt":"2013-04-20T14:27:48Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 20.04.2013 06:17:\n> On Fri, Apr 19, 2013 at 06:44:46PM +0200, Michael J Gruber wrote:\n> \n>> -\t\t\tdie(\"git cat-file --textconv: unable to run textconv on %s\",\n>> -\t\t\t obj_name);\n>> -\t\tbreak;\n>> +\t\tif (textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n>> +\t\t\tbreak;\n>> +\n>> +\t\t/* otherwise expect a blob */\n>> +\t\texp_type = \"blob\";\n>> \n>> \tcase 0:\n>> \t\tif (type_from_string(exp_type) == OBJ_BLOB) {\n> \n> I'm not sure this is right. What happens with:\n> \n> git cat-file --textconv HEAD:Documentation\n> \n> We have failed to textconv, but should we be expecting a blob?\n\nVery true, thanks. I'll reorder so that the --textconv case continues\n(without break) into the -p case. I think it makes sense to consider\n\"--textconv\" to be \"at least as pretty as -p\".\n\nMichael\n"},{"id":"214924","messageId":"5172A969.9000106@drmicha.warpmail.net","threadId":"33545","inReplyTo":"20130420042445.GD24970@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] grep: obey --textconv for the case rev:path","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-20T14:42:49Z","receivedAt":"2013-04-20T14:42:49Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 20.04.2013 06:24:\n> On Fri, Apr 19, 2013 at 06:44:49PM +0200, Michael J Gruber wrote:\n> \n>> @@ -820,12 +820,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>> \tfor (i = 0; i < argc; i++) {\n>> \t\tconst char *arg = argv[i];\n>> \t\tunsigned char sha1[20];\n>> +\t\tstruct object_context oc;\n>> \t\t/* Is it a rev? */\n>> -\t\tif (!get_sha1(arg, sha1)) {\n>> +\t\tif (!get_sha1_with_context(arg, 0, sha1, &oc)) {\n>> \t\t\tstruct object *object = parse_object_or_die(sha1, arg);\n>> \t\t\tif (!seen_dashdash)\n>> \t\t\t\tverify_non_filename(prefix, arg);\n>> -\t\t\tadd_object_array(object, arg, &list);\n>> +\t\t\tadd_object_array_with_context(object, arg, &list, xmemdupz(&oc, sizeof(struct object_context)));\n> \n> Hrm. I'm not excited about the extra allocation here. Who frees it?\n> \n>> +void add_object_array(struct object *obj, const char *name, struct object_array *array)\n>> +{\n>> +\tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n>> +}\n>> +\n>> +void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n>> +{\n>> +\tadd_object_array_with_mode_context(obj, name, array, mode, NULL);\n>> +}\n>> +\n>> +void add_object_array_with_context(struct object *obj, const char *name, struct object_array *array, struct object_context *context)\n>> +{\n>> +\tif (context)\n>> +\t\tadd_object_array_with_mode_context(obj, name, array, context->mode, context);\n>> +\telse\n>> +\t\tadd_object_array_with_mode_context(obj, name, array, S_IFINVALID, context);\n>> +}\n> \n> And this mass of almost-the-same functions is gross, too, especially\n> given that the object_context contains a mode itself.\n\nWell, it's just providing different ways to call into the one and only\nfunction, in order to satisfy different callers' needs. It's not unheard\nof (or rather: unseen) in our code, is it?\n\n> Unfortunately, I'm not sure if I have a more pleasant suggestion. I seem\n> to recall wrestling with this issue during the last round, too.\n\nYes, I think that's what we ended up with. At least it's just one\ncontext struct per argument to grep, so it's not that bad after all.\n\nI vaguely seem to recall we had some more general framework cooking but\nI may be wrong (I was offline due to sickness for a while). It was about\nattaching some additional info to something. Yes, I said \"vaguely\" ...\n\nMichael\n"},{"id":"214953","messageId":"20130421033710.GA18890@sigill.intra.peff.net","threadId":"33545","inReplyTo":"51729A6D.3030501@drmicha.warpmail.net","subject":"Re: [PATCH 2/6] show: obey --textconv for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-21T03:37:10Z","receivedAt":"2013-04-21T03:37:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 20, 2013 at 03:38:53PM +0200, Michael J Gruber wrote:\n\n> > Wait, this does the opposite of the last patch. If we do want to do\n> > this, shouldn't the last one have been an \"expect_failure\"?\n> \n> The last patch just documents the status quo, which is not a bug per se.\n> Therefore, no failure, but change in the definition of \"success\".\n\nIMHO, the series is easier to review if you it does not go back and\nforth. If you have one patch that says \"X is the right behavior\", and\nthen another patch that flips it to say \"Y is the right behavior\", the\nreviewer would read each in sequence and want to be convinced by your\narguments for X and Y. But you probably cannot make a good argument for\nX if you are trying to end up at Y. :)\n\nSo I'd much rather see the test introduced with the desired end\nbehavior, marked as expect_failure, and the commit message contain an\nargument about why Y is a good thing (and squashing the tests in with\nthe actual fix is often even better, because the fix itself would want\nto contain the same argument).\n\nJust my two cents as a reviewer.\n\n> My reasoning is twofold:\n> \n> - consistency between \"git show commit\" and \"git show blob\"\n\nI'm not sure I agree with this line of reasoning. \"git show commit\" is\nshowing a diff, not the file contents; textconv has always been about\nmunging the contents to produce a textual diff. It may be reasonable to\nextend its definition to \"this is the preferred human view of this\ncontent, and that happens to be what you would want to produce a diff\".\nBut I do not think it is necessarily inconsistent not to apply it for\nthe blob case.\n\n> - \"git show\" is a user facing command, and as such should produce output\n> consumable by humans; whereas \"git cat-file\" is plumbing and should\n> produce raw data unless told otherwise (-p, --textconv).\n\nThat holds if the textconv is the only (or best) human-readable version\nof the file. And maybe that is reasonable. But is it possible that\nsomebody uses \"textconv\" to produce a better diff of some already\nhuman-readable format? For example, imagine I define a textconv for XML\nfiles that normalizes the formatting to make diffs less noisy. When I am\nnot looking at a diff, what is the best human-readable version? The\noriginal, or the normalized one? I'm not sure.\n\nNote that I'm somewhat playing devil's advocate here. For the cases\nwhere I have used textconv in the real world, I think I would probably\nprefer seeing the converted contents, and I am happy to call it user\nerror if I use \"git show HEAD:foo.jpg >bar.jpg\" accidentally. But I also\nwant to make sure we are not regressing somebody else unnecessarily.\n\n-Peff\n"},{"id":"214954","messageId":"20130421034152.GB18890@sigill.intra.peff.net","threadId":"33545","inReplyTo":"5172A969.9000106@drmicha.warpmail.net","subject":"Re: [PATCH 6/6] grep: obey --textconv for the case rev:path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-21T03:41:52Z","receivedAt":"2013-04-21T03:41:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 20, 2013 at 04:42:49PM +0200, Michael J Gruber wrote:\n\n> > And this mass of almost-the-same functions is gross, too, especially\n> > given that the object_context contains a mode itself.\n> \n> Well, it's just providing different ways to call into the one and only\n> function, in order to satisfy different callers' needs. It's not unheard\n> of (or rather: unseen) in our code, is it?\n\nNo, we have instances of it already. And they're ugly, too. :) I think\nwhen we hit more than 2 or 3 wrappers it is time to start thinking\nwhether they can be consolidated. I think it is mostly the overlap in\ncontext and mode that makes me find this one particularly ugly. But it's\nprobably not solvable without some pretty heavy refactoring.\n\n> I vaguely seem to recall we had some more general framework cooking but\n> I may be wrong (I was offline due to sickness for a while). It was about\n> attaching some additional info to something. Yes, I said \"vaguely\" ...\n\nYeah, I really wanted to keep the context inside the object_array, but\nit means either wasting a lot of space (due to over-large buffers) or\nhaving the array elements be variable-sized (with a flex-array for the\npathname). And object_array entries already have a memory-leak problem\nfrom the \"name\" field, which I think we just punt on elsewhere. So I\nthink this is probably the lesser of the possible evils.\n\n-Peff\n"},{"id":"215069","messageId":"517510F6.7040301@drmicha.warpmail.net","threadId":"33545","inReplyTo":"20130421033710.GA18890@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] show: obey --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-22T10:29:10Z","receivedAt":"2013-04-22T10:29:10Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 21.04.2013 05:37:\n> On Sat, Apr 20, 2013 at 03:38:53PM +0200, Michael J Gruber wrote:\n> \n>>> Wait, this does the opposite of the last patch. If we do want to do\n>>> this, shouldn't the last one have been an \"expect_failure\"?\n>>\n>> The last patch just documents the status quo, which is not a bug per se.\n>> Therefore, no failure, but change in the definition of \"success\".\n> \n> IMHO, the series is easier to review if you it does not go back and\n> forth. If you have one patch that says \"X is the right behavior\", and\n> then another patch that flips it to say \"Y is the right behavior\", the\n> reviewer would read each in sequence and want to be convinced by your\n> arguments for X and Y. But you probably cannot make a good argument for\n> X if you are trying to end up at Y. :)\n> \n> So I'd much rather see the test introduced with the desired end\n> behavior, marked as expect_failure, and the commit message contain an\n> argument about why Y is a good thing (and squashing the tests in with\n> the actual fix is often even better, because the fix itself would want\n> to contain the same argument).\n> \n> Just my two cents as a reviewer.\n> \n>> My reasoning is twofold:\n>>\n>> - consistency between \"git show commit\" and \"git show blob\"\n> \n> I'm not sure I agree with this line of reasoning. \"git show commit\" is\n> showing a diff, not the file contents; textconv has always been about\n> munging the contents to produce a textual diff. It may be reasonable to\n> extend its definition to \"this is the preferred human view of this\n> content, and that happens to be what you would want to produce a diff\".\n> But I do not think it is necessarily inconsistent not to apply it for\n> the blob case.\n> \n>> - \"git show\" is a user facing command, and as such should produce output\n>> consumable by humans; whereas \"git cat-file\" is plumbing and should\n>> produce raw data unless told otherwise (-p, --textconv).\n> \n> That holds if the textconv is the only (or best) human-readable version\n> of the file. And maybe that is reasonable. But is it possible that\n> somebody uses \"textconv\" to produce a better diff of some already\n> human-readable format? For example, imagine I define a textconv for XML\n> files that normalizes the formatting to make diffs less noisy. When I am\n> not looking at a diff, what is the best human-readable version? The\n> original, or the normalized one? I'm not sure.\n> \n> Note that I'm somewhat playing devil's advocate here. For the cases\n> where I have used textconv in the real world, I think I would probably\n> prefer seeing the converted contents, and I am happy to call it user\n> error if I use \"git show HEAD:foo.jpg >bar.jpg\" accidentally. But I also\n> want to make sure we are not regressing somebody else unnecessarily.\n\nYes, the thing is that textconv helps diff by converting content (to\ntext) before the (textual) diff. So it's somehow a double-faced beast.\n\nIt's clearly activated by a \"diff\" attribute; so that would be a strong\nargument against my patch, at least against defaulting to --textconv for\nblobs.\n\nOTOH, textconv does have this aspect of converting text to a form\ndigestable by humans (pre-diff, granted), which is the argument for\ndefaulting to --textconv in porcellain.\n\nWe could use a separate attribute \"show\" in addition to \"diff\", but I\ndon't think it's worth going there, unless there is a strong use case\nfor \"diff-specific textconv\" which one would not want to apply when\nshowing just the content.\n\nMichael\n"},{"id":"215089","messageId":"7vwqrupoy2.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"20130421033710.GA18890@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] show: obey --textconv for blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-22T15:25:41Z","receivedAt":"2013-04-22T15:25:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Just my two cents as a reviewer.\n>\n>> My reasoning is twofold:\n>> \n>> - consistency between \"git show commit\" and \"git show blob\"\n>\n> I'm not sure I agree with this line of reasoning. \"git show commit\" is\n> showing a diff, not the file contents; textconv has always been about\n> munging the contents to produce a textual diff. It may be reasonable to\n> extend its definition to \"this is the preferred human view of this\n> content, and that happens to be what you would want to produce a diff\".\n> But I do not think it is necessarily inconsistent not to apply it for\n> the blob case.\n\nTrue. Applying textconv to otherwise unreadable blobs is often\nuseful, but I agree that it is unexpected if it is done by default,\nespecially given that many people have learned to do:\n\n\tgit show HEAD~4:binary-gob >old-binary-gob\n\nto recover old version of binary contents to a temporary file when\nchecking the sanity of or restoring the breakage in the new one.\n\nIt of course does _not_ forbid\n\n\tgit show --textconv HEAD~4:binary-gob | less\n\nbut I doubt it is a good idea to turn it on by default this late in\nthe game.\n"},{"id":"215090","messageId":"20130422152905.GA11886@sigill.intra.peff.net","threadId":"33545","inReplyTo":"7vwqrupoy2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/6] show: obey --textconv for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-22T15:29:05Z","receivedAt":"2013-04-22T15:29:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 22, 2013 at 08:25:41AM -0700, Junio C Hamano wrote:\n\n> True. Applying textconv to otherwise unreadable blobs is often\n> useful, but I agree that it is unexpected if it is done by default,\n> especially given that many people have learned to do:\n> \n> \tgit show HEAD~4:binary-gob >old-binary-gob\n> \n> to recover old version of binary contents to a temporary file when\n> checking the sanity of or restoring the breakage in the new one.\n> \n> It of course does _not_ forbid\n> \n> \tgit show --textconv HEAD~4:binary-gob | less\n> \n> but I doubt it is a good idea to turn it on by default this late in\n> the game.\n\nExactly. I certainly do not mind it as an option, and I am on the fence\nregarding it as a default (I think it might have been a sane thing to do\nfrom the start, but at this point the change-of-behavior makes me\nhesitate). So I am perfectly willing to go either way, depending on what\nothers think.\n\nI'm going to be out of email contact the rest of the week, so I'll let\nyou two talk it out. :)\n\n-Peff\n"},{"id":"215092","messageId":"1547528401.1826118.1366645060312.JavaMail.root@openwide.fr","threadId":"33545","inReplyTo":"20130422152905.GA11886@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] show: obey --textconv for blobs","fromName":"Jeremy Rosen","fromEmail":"jeremy.rosen@openwide.fr","sentAt":"2013-04-22T15:37:40Z","receivedAt":"2013-04-22T15:37:40Z","isPatch":true,"sender":{"key":"jeremy.rosen@openwide.fr","avatar":null},"body":"> > \tgit show --textconv HEAD~4:binary-gob | less\n> > \n> > but I doubt it is a good idea to turn it on by default this late in\n> > the game.\n> \n> Exactly. I certainly do not mind it as an option, and I am on the\n> fence\n> regarding it as a default (I think it might have been a sane thing to\n> do\n> from the start, but at this point the change-of-behavior makes me\n> hesitate). So I am perfectly willing to go either way, depending on\n> what\n> others think.\n> \n\n\nsome features detect if they are piping to a terminal... couldn't we do\nsomething like that ?\n"},{"id":"215096","messageId":"vpqfvyi7e86.fsf@grenoble-inp.fr","threadId":"33545","inReplyTo":"1547528401.1826118.1366645060312.JavaMail.root@openwide.fr","subject":"Re: [PATCH 2/6] show: obey --textconv for blobs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-04-22T15:54:33Z","receivedAt":"2013-04-22T15:54:33Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeremy Rosen writes:\n\n> some features detect if they are piping to a terminal... couldn't we do\n> something like that ?\n\nThat's OK for convenience features like colors or so, but that would be\nreally, really unexpected to have\n\n$ git show HEAD:file\nfoo\n$ git show HEAD:file > tmp\n$ cat tmp\nbar\n\nIMHO.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"215200","messageId":"51764D3E.9020403@drmicha.warpmail.net","threadId":"33545","inReplyTo":"vpqfvyi7e86.fsf@grenoble-inp.fr","subject":"Re: [PATCH 2/6] show: obey --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-23T08:58:38Z","receivedAt":"2013-04-23T08:58:38Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Matthieu Moy venit, vidit, dixit 22.04.2013 17:54:\n> Jeremy Rosen writes:\n> \n>> some features detect if they are piping to a terminal... couldn't we do\n>> something like that ?\n> \n> That's OK for convenience features like colors or so, but that would be\n> really, really unexpected to have\n> \n> $ git show HEAD:file\n> foo\n> $ git show HEAD:file > tmp\n> $ cat tmp\n> bar\n> \n> IMHO.\n\nYes, I'd either do it by default in general (my preference) or on\nrequest, but not based on tty.\n\nAnother point of input: You can do\n\ngit show commit \n\nand also with other object types, of course. On the other hand, there is\na single rev.diffopt. Besides the nuisance of having to track whether\ntextconv has been specified explicitely and flipping the bit in\nrev.diffopt per argument (or adding a parameter), which is an\nimplementation detail, it would mean that the default for different\narguments in the argument list is different, depending on type. And that\nis a usablility issue, I would argue:\n\nIs textconv on by default for git show? Yes and no, for some arguments\nyes, for others no.\n\nThat's what I want to cure ;)\n\nMichael\n"},{"id":"215208","messageId":"cover.1366718624.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"517298D4.3030802@drmicha.warpmail.net","subject":"[PATCHv2 0/7] grep with textconv","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-23T12:11:52Z","receivedAt":"2013-04-23T12:11:52Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Here's a reroll, with the following changes:\n\n* Use \"honor\" for obey\".\n\n* Fixed the issue with --textconv and non-blobs.\n\n* Restructured tests as per Jeff's preference.\n\n* Added 7/ which flips the default for git grep to textconv.\n\nJeff King (1):\n grep: allow to use textconv filters\n\nMichael J Gruber (6):\n t4030: demonstrate behavior of show with textconv\n show: obey --textconv for blobs\n cat-file: do not die on --textconv without textconv filters\n t7008: demonstrate behavior of grep with textconv\n grep: honor --textconv for the case rev:path\n git grep: honor textconv by default\n\n Documentation/git-grep.txt | 9 +++-\n builtin/cat-file.c | 18 ++++----\n builtin/grep.c | 13 +++---\n builtin/log.c | 24 ++++++++--\n grep.c | 102 +++++++++++++++++++++++++++++++++++++------\n grep.h | 1 +\n object.c | 26 ++++++++---\n object.h | 2 +\n t/t4030-diff-textconv.sh | 18 ++++++++\n t/t7008-grep-binary.sh | 43 ++++++++++++++++++\n t/t8007-cat-file-textconv.sh | 20 +++------\n 11 files changed, 222 insertions(+), 54 deletions(-)\n\n-- \n1.8.2.1.799.g1ac2534\n"},{"id":"215209","messageId":"8a6cbd3ca4e2cb1e5376262c3efa8e3a222767de.1366718624.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"517298D4.3030802@drmicha.warpmail.net","subject":"[PATCHv2 1/7] t4030: demonstrate behavior of show with textconv","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-23T12:11:53Z","receivedAt":"2013-04-23T12:11:53Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"\"git show \" honors the textconv setting while \"git show \"\ndoes not. Demonstrate this in the test.\n\nSigned-off-by: Michael J Gruber \n---\n t/t4030-diff-textconv.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 53ec330..260ea92 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -58,6 +58,12 @@ test_expect_success 'diff produces text' '\n \ttest_cmp expect.text actual\n '\n \n+test_expect_success 'show commit produces text' '\n+\tgit show 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@@ -84,6 +90,12 @@ test_expect_success 'status -v produces text' '\n \tgit reset --soft HEAD@{1}\n '\n \n+test_expect_failure 'show blob produces text' '\n+\tgit show HEAD:file >actual &&\n+\tprintf \"0\\\\n1\\\\n\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'grep-diff (-G) operates on textconv data (add)' '\n \techo one >expect &&\n \tgit log --root --format=%s -G0 >actual &&\n-- \n1.8.2.1.799.g1ac2534\n"},{"id":"215212","messageId":"c631e41a9f9b02f1ad5e40dd4bcaf18670b27c59.1366718624.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"517298D4.3030802@drmicha.warpmail.net","subject":"[PATCHv2 2/7] show: obey --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-23T12:11:54Z","receivedAt":"2013-04-23T12:11:54Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, \"diff\" and \"cat-file\" for blobs honor \"--textconv\" options\n(with the former defaulting to \"--textconv\" and the latter to\n\"--no-textconv\") whereas \"show\" does not honor this option, even though\nit takes diff options.\n\nMake \"show\" on blobs behave like \"diff\", i.e. honor \"--textconv\" by\ndefault and \"--no-textconv\" when given.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/log.c | 24 +++++++++++++++++++++---\n t/t4030-diff-textconv.sh | 8 +++++++-\n 2 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 5f3ed77..fe0275e 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -436,10 +436,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)\n \tstrbuf_release(&out);\n }\n \n-static int show_blob_object(const unsigned char *sha1, struct rev_info *rev)\n+static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)\n {\n+\tunsigned char sha1c[20];\n+\tstruct object_context obj_context;\n+\tchar *buf;\n+\tunsigned long size;\n+\n \tfflush(stdout);\n-\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n+\tif (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))\n+\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n+\n+\tif (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))\n+\t\tdie(\"Not a valid object name %s\", obj_name);\n+\tif (!obj_context.path[0] ||\n+\t !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, &buf, &size))\n+\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n+\n+\tif (!buf)\n+\t\tdie(\"git show %s: bad file\", obj_name);\n+\n+\twrite_or_die(1, buf, size);\n+\treturn 0;\n }\n \n static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)\n@@ -525,7 +543,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\tconst char *name = objects[i].name;\n \t\tswitch (o->type) {\n \t\tcase OBJ_BLOB:\n-\t\t\tret = show_blob_object(o->sha1, NULL);\n+\t\t\tret = show_blob_object(o->sha1, &rev, name);\n \t\t\tbreak;\n \t\tcase OBJ_TAG: {\n \t\t\tstruct tag *t = (struct tag *)o;\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 260ea92..f9d55e1 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -90,12 +90,18 @@ test_expect_success 'status -v produces text' '\n \tgit reset --soft HEAD@{1}\n '\n \n-test_expect_failure 'show blob produces text' '\n+test_expect_success 'show blob produces text' '\n \tgit show HEAD:file >actual &&\n \tprintf \"0\\\\n1\\\\n\" >expect &&\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'show --no-textconv blob produces binary' '\n+\tgit show --no-textconv HEAD:file >actual &&\n+\tprintf \"\\\\0\\\\n\\\\1\\\\n\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'grep-diff (-G) operates on textconv data (add)' '\n \techo one >expect &&\n \tgit log --root --format=%s -G0 >actual &&\n-- \n1.8.2.1.799.g1ac2534\n"},{"id":"215213","messageId":"10c691f7003f1f211f265abb177dd2a1b511b7e2.1366718624.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"517298D4.3030802@drmicha.warpmail.net","subject":"[PATCHv2 3/7] cat-file: do not die on --textconv without textconv filters","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-23T12:11:55Z","receivedAt":"2013-04-23T12:11:55Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"When a command is supposed to use textconv filters (by default or with\n\"--textconv\") and none are configured then the blob is output without\nconversion; the only exception to this rule is \"cat-file --textconv\".\n\nMake it behave like the rest of textconv aware commands.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/cat-file.c | 18 ++++++++----------\n t/t8007-cat-file-textconv.sh | 20 +++++---------------\n 2 files changed, 13 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 045cee7..bd62373 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -48,6 +48,14 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \tcase 'e':\n \t\treturn !has_sha1_file(sha1);\n \n+\tcase 'c':\n+\t\tif (!obj_context.path[0])\n+\t\t\tdie(\"git cat-file --textconv %s: must be \",\n+\t\t\t obj_name);\n+\n+\t\tif (textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n+\t\t\tbreak;\n+\n \tcase 'p':\n \t\ttype = sha1_object_info(sha1, NULL);\n \t\tif (type < 0)\n@@ -70,16 +78,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \t\t/* otherwise just spit out the data */\n \t\tbreak;\n \n-\tcase 'c':\n-\t\tif (!obj_context.path[0])\n-\t\t\tdie(\"git cat-file --textconv %s: must be \",\n-\t\t\t obj_name);\n-\n-\t\tif (!textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n-\t\t\tdie(\"git cat-file --textconv: unable to run textconv on %s\",\n-\t\t\t obj_name);\n-\t\tbreak;\n-\n \tcase 0:\n \t\tif (type_from_string(exp_type) == OBJ_BLOB) {\n \t\t\tunsigned char blob_sha1[20];\ndiff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh\nindex 78a0085..83c6636 100755\n--- a/t/t8007-cat-file-textconv.sh\n+++ b/t/t8007-cat-file-textconv.sh\n@@ -22,11 +22,11 @@ test_expect_success 'setup ' '\n '\n \n cat >expected <result\n+\tgit cat-file --textconv :one.bin >result &&\n \ttest_cmp expected result\n '\n \n@@ -36,10 +36,6 @@ test_expect_success 'setup textconv filters' '\n \tgit config diff.test.cachetextconv false\n '\n \n-cat >expected <result &&\n \ttest_cmp expected result\n@@ -73,25 +69,19 @@ test_expect_success 'cat-file --textconv on previous commit' '\n '\n \n test_expect_success SYMLINKS 'cat-file without --textconv (symlink)' '\n+\tprintf \"%s\" \"one.bin\" >expected &&\n \tgit cat-file blob :symlink.bin >result &&\n-\tprintf \"%s\" \"one.bin\" >expected\n \ttest_cmp expected result\n '\n \n \n test_expect_success SYMLINKS 'cat-file --textconv on index (symlink)' '\n-\t! git cat-file --textconv :symlink.bin 2>result &&\n-\tcat >expected <<\\EOF &&\n-fatal: git cat-file --textconv: unable to run textconv on :symlink.bin\n-EOF\n+\tgit cat-file --textconv :symlink.bin >result &&\n \ttest_cmp expected result\n '\n \n test_expect_success SYMLINKS 'cat-file --textconv on HEAD (symlink)' '\n-\t! git cat-file --textconv HEAD:symlink.bin 2>result &&\n-\tcat >expected <result &&\n \ttest_cmp expected result\n '\n \n-- \n1.8.2.1.799.g1ac2534\n"},{"id":"215211","messageId":"5137a5a48ae6c70ad716d985a22d53ec311ee05a.1366718624.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"517298D4.3030802@drmicha.warpmail.net","subject":"[PATCHv2 4/7] t7008: demonstrate behavior of grep with textconv","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-23T12:11:56Z","receivedAt":"2013-04-23T12:11:56Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, \"git grep\" does not honor any textconv filters. Demonstrate\nthis in the tests.\n\nSigned-off-by: Michael J Gruber \n---\n t/t7008-grep-binary.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex 26f8319..126fe4c 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -145,4 +145,27 @@ test_expect_success 'grep respects not-binary diff attribute' '\n \ttest_cmp expect actual\n '\n \n+cat >nul_to_q_textconv <<'EOF'\n+#!/bin/sh\n+\"$PERL_PATH\" -pe 'y/\\000/Q/' < \"$1\"\n+EOF\n+chmod +x nul_to_q_textconv\n+\n+test_expect_success 'setup textconv filters' '\n+\techo a diff=foo >.gitattributes &&\n+\tgit config diff.foo.textconv \"\\\"$(pwd)\\\"\"/nul_to_q_textconv\n+'\n+\n+test_expect_failure 'grep does not honor textconv' '\n+\techo \"a:binaryQfile\" >expect &&\n+\tgit grep Qfile >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep blob does not honor textconv' '\n+\techo \"HEAD:a:binaryQfile\" >expect &&\n+\tgit grep Qfile HEAD:a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.2.1.799.g1ac2534\n"},{"id":"215214","messageId":"4f28b481a8124f058a5a5bfb0fbd33c24d2f7dbb.1366718624.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"517298D4.3030802@drmicha.warpmail.net","subject":"[PATCHv2 5/7] grep: allow to use textconv filters","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-23T12:11:57Z","receivedAt":"2013-04-23T12:11:57Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"From: Jeff King \n\nRecently and not so recently, we made sure that log/grep type operations\nuse textconv filters when a userfacing diff would do the same:\n\nef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28)\nb1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28)\n0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)\n\n\"git grep\" currently does not use textconv filters at all, that is\nneither for displaying the match and context nor for the actual grepping.\n\nIntroduce an option \"--textconv\" which makes git grep use any configured\ntextconv filters for grepping and output purposes. It is off by default.\n\nSigned-off-by: Jeff King \nSigned-off-by: Michael J Gruber \n---\n Documentation/git-grep.txt | 9 +++-\n builtin/grep.c | 2 +\n grep.c | 100 ++++++++++++++++++++++++++++++++++++++-------\n grep.h | 1 +\n t/t7008-grep-binary.sh | 20 +++++++++\n 5 files changed, 117 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 50d46e1..a5c5a27 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -9,7 +9,7 @@ git-grep - Print lines matching a pattern\n SYNOPSIS\n --------\n [verse]\n-'git grep' [-a | --text] [-I] [-i | --ignore-case] [-w | --word-regexp]\n+'git grep' [-a | --text] [-I] [--textconv] [-i | --ignore-case] [-w | --word-regexp]\n \t [-v | --invert-match] [-h|-H] [--full-name]\n \t [-E | --extended-regexp] [-G | --basic-regexp]\n \t [-P | --perl-regexp]\n@@ -80,6 +80,13 @@ OPTIONS\n --text::\n \tProcess binary files as if they were text.\n \n+--textconv::\n+\tHonor textconv filter settings.\n+\n+--no-textconv::\n+\tDo not honor textconv filter settings.\n+\tThis is the default.\n+\n -i::\n --ignore-case::\n \tIgnore case differences between the patterns and the\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 159e65d..00ee57d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -659,6 +659,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_SET_INT('I', NULL, &opt.binary,\n \t\t\tN_(\"don't match patterns in binary files\"),\n \t\t\tGREP_BINARY_NOMATCH),\n+\t\tOPT_BOOL(0, \"textconv\", &opt.allow_textconv,\n+\t\t\t N_(\"process binary files with textconv filters\")),\n \t\t{ OPTION_INTEGER, 0, \"max-depth\", &opt.max_depth, N_(\"depth\"),\n \t\t\tN_(\"descend at most levels\"), PARSE_OPT_NONEG,\n \t\t\tNULL, 1 },\ndiff --git a/grep.c b/grep.c\nindex bb548ca..c668034 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -2,6 +2,8 @@\n #include \"grep.h\"\n #include \"userdiff.h\"\n #include \"xdiff-interface.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n \n static int grep_source_load(struct grep_source *gs);\n static int grep_source_is_binary(struct grep_source *gs);\n@@ -1322,6 +1324,58 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n \tfwrite(buf, size, 1, stdout);\n }\n \n+static int fill_textconv_grep(struct userdiff_driver *driver,\n+\t\t\t struct grep_source *gs)\n+{\n+\tstruct diff_filespec *df;\n+\tchar *buf;\n+\tsize_t size;\n+\n+\tif (!driver || !driver->textconv)\n+\t\treturn grep_source_load(gs);\n+\n+\t/*\n+\t * The textconv interface is intimately tied to diff_filespecs, so we\n+\t * have to pretend to be one. If we could unify the grep_source\n+\t * and diff_filespec structs, this mess could just go away.\n+\t */\n+\tdf = alloc_filespec(gs->path);\n+\tswitch (gs->type) {\n+\tcase GREP_SOURCE_SHA1:\n+\t\tfill_filespec(df, gs->identifier, 1, 0100644);\n+\t\tbreak;\n+\tcase GREP_SOURCE_FILE:\n+\t\tfill_filespec(df, null_sha1, 0, 0100644);\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: attempt to textconv something without a path?\");\n+\t}\n+\n+\t/*\n+\t * fill_textconv is not remotely thread-safe; it may load objects\n+\t * behind the scenes, and it modifies the global diff tempfile\n+\t * structure.\n+\t */\n+\tgrep_read_lock();\n+\tsize = fill_textconv(driver, df, &buf);\n+\tgrep_read_unlock();\n+\tfree_filespec(df);\n+\n+\t/*\n+\t * The normal fill_textconv usage by the diff machinery would just keep\n+\t * the textconv'd buf separate from the diff_filespec. But much of the\n+\t * grep code passes around a grep_source and assumes that its \"buf\"\n+\t * pointer is the beginning of the thing we are searching. So let's\n+\t * install our textconv'd version into the grep_source, taking care not\n+\t * to leak any existing buffer.\n+\t */\n+\tgrep_source_clear_data(gs);\n+\tgs->buf = buf;\n+\tgs->size = size;\n+\n+\treturn 0;\n+}\n+\n static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)\n {\n \tchar *bol;\n@@ -1332,6 +1386,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \tunsigned count = 0;\n \tint try_lookahead = 0;\n \tint show_function = 0;\n+\tstruct userdiff_driver *textconv = NULL;\n \tenum grep_context ctx = GREP_CONTEXT_HEAD;\n \txdemitconf_t xecfg;\n \n@@ -1353,19 +1408,36 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t}\n \topt->last_shown = 0;\n \n-\tswitch (opt->binary) {\n-\tcase GREP_BINARY_DEFAULT:\n-\t\tif (grep_source_is_binary(gs))\n-\t\t\tbinary_match_only = 1;\n-\t\tbreak;\n-\tcase GREP_BINARY_NOMATCH:\n-\t\tif (grep_source_is_binary(gs))\n-\t\t\treturn 0; /* Assume unmatch */\n-\t\tbreak;\n-\tcase GREP_BINARY_TEXT:\n-\t\tbreak;\n-\tdefault:\n-\t\tdie(\"bug: unknown binary handling mode\");\n+\tif (opt->allow_textconv) {\n+\t\tgrep_source_load_driver(gs);\n+\t\t/*\n+\t\t * We might set up the shared textconv cache data here, which\n+\t\t * is not thread-safe.\n+\t\t */\n+\t\tgrep_attr_lock();\n+\t\ttextconv = userdiff_get_textconv(gs->driver);\n+\t\tgrep_attr_unlock();\n+\t}\n+\n+\t/*\n+\t * We know the result of a textconv is text, so we only have to care\n+\t * about binary handling if we are not using it.\n+\t */\n+\tif (!textconv) {\n+\t\tswitch (opt->binary) {\n+\t\tcase GREP_BINARY_DEFAULT:\n+\t\t\tif (grep_source_is_binary(gs))\n+\t\t\t\tbinary_match_only = 1;\n+\t\t\tbreak;\n+\t\tcase GREP_BINARY_NOMATCH:\n+\t\t\tif (grep_source_is_binary(gs))\n+\t\t\t\treturn 0; /* Assume unmatch */\n+\t\t\tbreak;\n+\t\tcase GREP_BINARY_TEXT:\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tdie(\"bug: unknown binary handling mode\");\n+\t\t}\n \t}\n \n \tmemset(&xecfg, 0, sizeof(xecfg));\n@@ -1373,7 +1445,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \n \ttry_lookahead = should_lookahead(opt);\n \n-\tif (grep_source_load(gs) < 0)\n+\tif (fill_textconv_grep(textconv, gs) < 0)\n \t\treturn 0;\n \n \tbol = gs->buf;\ndiff --git a/grep.h b/grep.h\nindex e4a1df5..eaaced1 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -107,6 +107,7 @@ struct grep_opt {\n #define GREP_BINARY_NOMATCH\t1\n #define GREP_BINARY_TEXT\t2\n \tint binary;\n+\tint allow_textconv;\n \tint extended;\n \tint use_reflog_filter;\n \tint pcre;\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex 126fe4c..1eae6a4 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -162,10 +162,30 @@ test_expect_failure 'grep does not honor textconv' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'grep --textconv does honor textconv' '\n+\techo \"a:binaryQfile\" >expect &&\n+\tgit grep --textconv Qfile >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --no-textconv does not honor textconv' '\n+\ttest_must_fail git grep --no-textconv Qfile\n+'\n+\n test_expect_failure 'grep blob does not honor textconv' '\n \techo \"HEAD:a:binaryQfile\" >expect &&\n \tgit grep Qfile HEAD:a >actual &&\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'grep --textconv blob does not honor textconv' '\n+\techo \"HEAD:a:binaryQfile\" >expect &&\n+\tgit grep --textconv Qfile HEAD:a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --no-textconv blob does not honor textconv' '\n+\ttest_must_fail git grep --no-textconv Qfile HEAD:a\n+'\n+\n test_done\n-- \n1.8.2.1.799.g1ac2534\n"},{"id":"215210","messageId":"805accf9664ea7f9cd8de2cfa6d2e17601720767.1366718624.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"517298D4.3030802@drmicha.warpmail.net","subject":"[PATCHv2 6/7] grep: honor --textconv for the case rev:path","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-23T12:11:58Z","receivedAt":"2013-04-23T12:11:58Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Make \"grep\" honor the \"--textconv\" option also for the object case, i.e.\nwhen used with an argument \"rev:path\".\n\nSigned-off-by: Michael J Gruber \n---\n builtin/grep.c | 11 ++++++-----\n object.c | 26 ++++++++++++++++++++------\n object.h | 2 ++\n t/t7008-grep-binary.sh | 2 +-\n 4 files changed, 29 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 00ee57d..bb7f970 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -458,10 +458,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n }\n \n static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n-\t\t struct object *obj, const char *name)\n+\t\t struct object *obj, const char *name, struct object_context *oc)\n {\n \tif (obj->type == OBJ_BLOB)\n-\t\treturn grep_sha1(opt, obj->sha1, name, 0, NULL);\n+\t\treturn grep_sha1(opt, obj->sha1, name, 0, oc ? oc->path : NULL);\n \tif (obj->type == OBJ_COMMIT || obj->type == OBJ_TREE) {\n \t\tstruct tree_desc tree;\n \t\tvoid *data;\n@@ -503,7 +503,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object *real_obj;\n \t\treal_obj = deref_tag(list->objects[i].item, NULL, 0);\n-\t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name)) {\n+\t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name, list->objects[i].context)) {\n \t\t\thit = 1;\n \t\t\tif (opt->status_only)\n \t\t\t\tbreak;\n@@ -820,12 +820,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tunsigned char sha1[20];\n+\t\tstruct object_context oc;\n \t\t/* Is it a rev? */\n-\t\tif (!get_sha1(arg, sha1)) {\n+\t\tif (!get_sha1_with_context(arg, 0, sha1, &oc)) {\n \t\t\tstruct object *object = parse_object_or_die(sha1, arg);\n \t\t\tif (!seen_dashdash)\n \t\t\t\tverify_non_filename(prefix, arg);\n-\t\t\tadd_object_array(object, arg, &list);\n+\t\t\tadd_object_array_with_context(object, arg, &list, xmemdupz(&oc, sizeof(struct object_context)));\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--\")) {\ndiff --git a/object.c b/object.c\nindex 20703f5..c8ffc9e 100644\n--- a/object.c\n+++ b/object.c\n@@ -255,12 +255,7 @@ int object_list_contains(struct object_list *list, struct object *obj)\n \treturn 0;\n }\n \n-void add_object_array(struct object *obj, const char *name, struct object_array *array)\n-{\n-\tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n-}\n-\n-void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n+static void add_object_array_with_mode_context(struct object *obj, const char *name, struct object_array *array, unsigned mode, struct object_context *context)\n {\n \tunsigned nr = array->nr;\n \tunsigned alloc = array->alloc;\n@@ -275,9 +270,28 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj\n \tobjects[nr].item = obj;\n \tobjects[nr].name = name;\n \tobjects[nr].mode = mode;\n+\tobjects[nr].context = context;\n \tarray->nr = ++nr;\n }\n \n+void add_object_array(struct object *obj, const char *name, struct object_array *array)\n+{\n+\tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n+}\n+\n+void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n+{\n+\tadd_object_array_with_mode_context(obj, name, array, mode, NULL);\n+}\n+\n+void add_object_array_with_context(struct object *obj, const char *name, struct object_array *array, struct object_context *context)\n+{\n+\tif (context)\n+\t\tadd_object_array_with_mode_context(obj, name, array, context->mode, context);\n+\telse\n+\t\tadd_object_array_with_mode_context(obj, name, array, S_IFINVALID, context);\n+}\n+\n void object_array_remove_duplicates(struct object_array *array)\n {\n \tunsigned int ref, src, dst;\ndiff --git a/object.h b/object.h\nindex 97d384b..695847d 100644\n--- a/object.h\n+++ b/object.h\n@@ -13,6 +13,7 @@ struct object_array {\n \t\tstruct object *item;\n \t\tconst char *name;\n \t\tunsigned mode;\n+\t\tstruct object_context *context;\n \t} *objects;\n };\n \n@@ -85,6 +86,7 @@ int object_list_contains(struct object_list *list, struct object *obj);\n /* Object array handling .. */\n void add_object_array(struct object *obj, const char *name, struct object_array *array);\n void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode);\n+void add_object_array_with_context(struct object *obj, const char *name, struct object_array *array, struct object_context *context);\n void object_array_remove_duplicates(struct object_array *);\n \n void clear_object_flags(unsigned flags);\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex 1eae6a4..10b2c8b 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -178,7 +178,7 @@ test_expect_failure 'grep blob does not honor textconv' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'grep --textconv blob does not honor textconv' '\n+test_expect_success 'grep --textconv blob does honor textconv' '\n \techo \"HEAD:a:binaryQfile\" >expect &&\n \tgit grep --textconv Qfile HEAD:a >actual &&\n \ttest_cmp expect actual\n-- \n1.8.2.1.799.g1ac2534\n"},{"id":"215215","messageId":"043047afd2915dd8f3a68cf164dc516d4c0bb5c2.1366718624.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"517298D4.3030802@drmicha.warpmail.net","subject":"[PATCHv2 7/7] git grep: honor textconv by default","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-23T12:11:59Z","receivedAt":"2013-04-23T12:11:59Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, \"git grep\" does not honor textconv settings by default.\nMake it honor them by default just like \"git log --grep\" does.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-grep.txt | 2 +-\n grep.c | 2 ++\n t/t7008-grep-binary.sh | 4 ++--\n 3 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex a5c5a27..f54ac0c 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -82,10 +82,10 @@ OPTIONS\n \n --textconv::\n \tHonor textconv filter settings.\n+\tThis is the default.\n \n --no-textconv::\n \tDo not honor textconv filter settings.\n-\tThis is the default.\n \n -i::\n --ignore-case::\ndiff --git a/grep.c b/grep.c\nindex c668034..161d3f0 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -31,6 +31,7 @@ void init_grep_defaults(void)\n \topt->max_depth = -1;\n \topt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n \topt->extended_regexp_option = 0;\n+\topt->allow_textconv = 1;\n \tstrcpy(opt->color_context, \"\");\n \tstrcpy(opt->color_filename, \"\");\n \tstrcpy(opt->color_function, \"\");\n@@ -134,6 +135,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n \topt->pathname = def->pathname;\n \topt->regflags = def->regflags;\n \topt->relative = def->relative;\n+\topt->allow_textconv = def->allow_textconv;\n \n \tstrcpy(opt->color_context, def->color_context);\n \tstrcpy(opt->color_filename, def->color_filename);\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex 10b2c8b..2fc9d9c 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -156,7 +156,7 @@ test_expect_success 'setup textconv filters' '\n \tgit config diff.foo.textconv \"\\\"$(pwd)\\\"\"/nul_to_q_textconv\n '\n \n-test_expect_failure 'grep does not honor textconv' '\n+test_expect_success 'grep does honor textconv' '\n \techo \"a:binaryQfile\" >expect &&\n \tgit grep Qfile >actual &&\n \ttest_cmp expect actual\n@@ -172,7 +172,7 @@ test_expect_success 'grep --no-textconv does not honor textconv' '\n \ttest_must_fail git grep --no-textconv Qfile\n '\n \n-test_expect_failure 'grep blob does not honor textconv' '\n+test_expect_success 'grep blob does honor textconv' '\n \techo \"HEAD:a:binaryQfile\" >expect &&\n \tgit grep Qfile HEAD:a >actual &&\n \ttest_cmp expect actual\n-- \n1.8.2.1.799.g1ac2534\n"},{"id":"215235","messageId":"7vehe1l1sh.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"8a6cbd3ca4e2cb1e5376262c3efa8e3a222767de.1366718624.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 1/7] t4030: demonstrate behavior of show with textconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T15:11:42Z","receivedAt":"2013-04-23T15:11:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> \"git show \" honors the textconv setting while \"git show \"\n> does not. Demonstrate this in the test.\n\nShould \"git show \" run textconv by default? I somehow had an\nimpression that people were against it during the discussion on the\nprevious round.\n\n>\n> Signed-off-by: Michael J Gruber \n> ---\n> t/t4030-diff-textconv.sh | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n>\n> diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\n> index 53ec330..260ea92 100755\n> --- a/t/t4030-diff-textconv.sh\n> +++ b/t/t4030-diff-textconv.sh\n> @@ -58,6 +58,12 @@ test_expect_success 'diff produces text' '\n> \ttest_cmp expect.text actual\n> '\n> \n> +test_expect_success 'show commit produces text' '\n> +\tgit show 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> @@ -84,6 +90,12 @@ test_expect_success 'status -v produces text' '\n> \tgit reset --soft HEAD@{1}\n> '\n> \n> +test_expect_failure 'show blob produces text' '\n> +\tgit show HEAD:file >actual &&\n> +\tprintf \"0\\\\n1\\\\n\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'grep-diff (-G) operates on textconv data (add)' '\n> \techo one >expect &&\n> \tgit log --root --format=%s -G0 >actual &&\n"},{"id":"215236","messageId":"7va9opl1om.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"c631e41a9f9b02f1ad5e40dd4bcaf18670b27c59.1366718624.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 2/7] show: obey --textconv for blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T15:14:01Z","receivedAt":"2013-04-23T15:14:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n>> Subject: Re: [PATCHv2 2/7] show: obey --textconv for blobs\n\ns/obey/honor/;\n\n> Currently, \"diff\" and \"cat-file\" for blobs honor \"--textconv\" options\n> (with the former defaulting to \"--textconv\" and the latter to\n> \"--no-textconv\") whereas \"show\" does not honor this option, even though\n> it takes diff options.\n>\n> Make \"show\" on blobs behave like \"diff\", i.e. honor \"--textconv\" by\n> default and \"--no-textconv\" when given.\n\nIt is the right thing to do to teach it to react to --[no-]textconv;\nI am not sure if the default is right, though.\n\n> Signed-off-by: Michael J Gruber \n> ---\n> builtin/log.c | 24 +++++++++++++++++++++---\n> t/t4030-diff-textconv.sh | 8 +++++++-\n> 2 files changed, 28 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 5f3ed77..fe0275e 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -436,10 +436,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)\n> \tstrbuf_release(&out);\n> }\n> \n> -static int show_blob_object(const unsigned char *sha1, struct rev_info *rev)\n> +static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)\n> {\n> +\tunsigned char sha1c[20];\n> +\tstruct object_context obj_context;\n> +\tchar *buf;\n> +\tunsigned long size;\n> +\n> \tfflush(stdout);\n> -\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n> +\tif (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))\n> +\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n> +\n> +\tif (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))\n> +\t\tdie(\"Not a valid object name %s\", obj_name);\n> +\tif (!obj_context.path[0] ||\n> +\t !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, &buf, &size))\n> +\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n> +\n> +\tif (!buf)\n> +\t\tdie(\"git show %s: bad file\", obj_name);\n> +\n> +\twrite_or_die(1, buf, size);\n> +\treturn 0;\n> }\n> \n> static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)\n> @@ -525,7 +543,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n> \t\tconst char *name = objects[i].name;\n> \t\tswitch (o->type) {\n> \t\tcase OBJ_BLOB:\n> -\t\t\tret = show_blob_object(o->sha1, NULL);\n> +\t\t\tret = show_blob_object(o->sha1, &rev, name);\n> \t\t\tbreak;\n> \t\tcase OBJ_TAG: {\n> \t\t\tstruct tag *t = (struct tag *)o;\n> diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\n> index 260ea92..f9d55e1 100755\n> --- a/t/t4030-diff-textconv.sh\n> +++ b/t/t4030-diff-textconv.sh\n> @@ -90,12 +90,18 @@ test_expect_success 'status -v produces text' '\n> \tgit reset --soft HEAD@{1}\n> '\n> \n> -test_expect_failure 'show blob produces text' '\n> +test_expect_success 'show blob produces text' '\n> \tgit show HEAD:file >actual &&\n> \tprintf \"0\\\\n1\\\\n\" >expect &&\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'show --no-textconv blob produces binary' '\n> +\tgit show --no-textconv HEAD:file >actual &&\n> +\tprintf \"\\\\0\\\\n\\\\1\\\\n\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'grep-diff (-G) operates on textconv data (add)' '\n> \techo one >expect &&\n> \tgit log --root --format=%s -G0 >actual &&\n"},{"id":"215237","messageId":"7v61zdl1m6.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"10c691f7003f1f211f265abb177dd2a1b511b7e2.1366718624.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 3/7] cat-file: do not die on --textconv without textconv filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T15:15:29Z","receivedAt":"2013-04-23T15:15:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> When a command is supposed to use textconv filters (by default or with\n> \"--textconv\") and none are configured then the blob is output without\n> conversion; the only exception to this rule is \"cat-file --textconv\".\n>\n> Make it behave like the rest of textconv aware commands.\n>\n> Signed-off-by: Michael J Gruber \n> ---\n> builtin/cat-file.c | 18 ++++++++----------\n> t/t8007-cat-file-textconv.sh | 20 +++++---------------\n> 2 files changed, 13 insertions(+), 25 deletions(-)\n>\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 045cee7..bd62373 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -48,6 +48,14 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n> \tcase 'e':\n> \t\treturn !has_sha1_file(sha1);\n> \n> +\tcase 'c':\n> +\t\tif (!obj_context.path[0])\n> +\t\t\tdie(\"git cat-file --textconv %s: must be \",\n> +\t\t\t obj_name);\n> +\n> +\t\tif (textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n> +\t\t\tbreak;\n> +\n> \tcase 'p':\n\nYeah, falling back to the 'p' case is a lot more sensible.\n\n> \t\ttype = sha1_object_info(sha1, NULL);\n> \t\tif (type < 0)\n> @@ -70,16 +78,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n> \t\t/* otherwise just spit out the data */\n> \t\tbreak;\n> \n> -\tcase 'c':\n> -\t\tif (!obj_context.path[0])\n> -\t\t\tdie(\"git cat-file --textconv %s: must be \",\n> -\t\t\t obj_name);\n> -\n> -\t\tif (!textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n> -\t\t\tdie(\"git cat-file --textconv: unable to run textconv on %s\",\n> -\t\t\t obj_name);\n> -\t\tbreak;\n> -\n> \tcase 0:\n> \t\tif (type_from_string(exp_type) == OBJ_BLOB) {\n> \t\t\tunsigned char blob_sha1[20];\n> diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh\n> index 78a0085..83c6636 100755\n> --- a/t/t8007-cat-file-textconv.sh\n> +++ b/t/t8007-cat-file-textconv.sh\n> @@ -22,11 +22,11 @@ test_expect_success 'setup ' '\n> '\n> \n> cat >expected < -fatal: git cat-file --textconv: unable to run textconv on :one.bin\n> +bin: test version 2\n> EOF\n> \n> test_expect_success 'no filter specified' '\n> -\tgit cat-file --textconv :one.bin 2>result\n> +\tgit cat-file --textconv :one.bin >result &&\n> \ttest_cmp expected result\n> '\n> \n> @@ -36,10 +36,6 @@ test_expect_success 'setup textconv filters' '\n> \tgit config diff.test.cachetextconv false\n> '\n> \n> -cat >expected < -bin: test version 2\n> -EOF\n> -\n> test_expect_success 'cat-file without --textconv' '\n> \tgit cat-file blob :one.bin >result &&\n> \ttest_cmp expected result\n> @@ -73,25 +69,19 @@ test_expect_success 'cat-file --textconv on previous commit' '\n> '\n> \n> test_expect_success SYMLINKS 'cat-file without --textconv (symlink)' '\n> +\tprintf \"%s\" \"one.bin\" >expected &&\n> \tgit cat-file blob :symlink.bin >result &&\n> -\tprintf \"%s\" \"one.bin\" >expected\n> \ttest_cmp expected result\n> '\n> \n> \n> test_expect_success SYMLINKS 'cat-file --textconv on index (symlink)' '\n> -\t! git cat-file --textconv :symlink.bin 2>result &&\n> -\tcat >expected <<\\EOF &&\n> -fatal: git cat-file --textconv: unable to run textconv on :symlink.bin\n> -EOF\n> +\tgit cat-file --textconv :symlink.bin >result &&\n> \ttest_cmp expected result\n> '\n> \n> test_expect_success SYMLINKS 'cat-file --textconv on HEAD (symlink)' '\n> -\t! git cat-file --textconv HEAD:symlink.bin 2>result &&\n> -\tcat >expected < -fatal: git cat-file --textconv: unable to run textconv on HEAD:symlink.bin\n> -EOF\n> +\tgit cat-file --textconv HEAD:symlink.bin >result &&\n> \ttest_cmp expected result\n> '\n"},{"id":"215238","messageId":"7v1ua1l1ki.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"5137a5a48ae6c70ad716d985a22d53ec311ee05a.1366718624.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 4/7] t7008: demonstrate behavior of grep with textconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T15:16:29Z","receivedAt":"2013-04-23T15:16:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Currently, \"git grep\" does not honor any textconv filters. Demonstrate\n> this in the tests.\n>\n> Signed-off-by: Michael J Gruber \n> ---\n> t/t7008-grep-binary.sh | 23 +++++++++++++++++++++++\n> 1 file changed, 23 insertions(+)\n>\n> diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\n> index 26f8319..126fe4c 100755\n> --- a/t/t7008-grep-binary.sh\n> +++ b/t/t7008-grep-binary.sh\n> @@ -145,4 +145,27 @@ test_expect_success 'grep respects not-binary diff attribute' '\n> \ttest_cmp expect actual\n> '\n> \n> +cat >nul_to_q_textconv <<'EOF'\n> +#!/bin/sh\n> +\"$PERL_PATH\" -pe 'y/\\000/Q/' < \"$1\"\n> +EOF\n> +chmod +x nul_to_q_textconv\n> +\n> +test_expect_success 'setup textconv filters' '\n> +\techo a diff=foo >.gitattributes &&\n> +\tgit config diff.foo.textconv \"\\\"$(pwd)\\\"\"/nul_to_q_textconv\n> +'\n> +\n> +test_expect_failure 'grep does not honor textconv' '\n> +\techo \"a:binaryQfile\" >expect &&\n> +\tgit grep Qfile >actual &&\n\nThis should pass --textconv to \"git grep\".\n\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_failure 'grep blob does not honor textconv' '\n> +\techo \"HEAD:a:binaryQfile\" >expect &&\n> +\tgit grep Qfile HEAD:a >actual &&\n\nLikewise.\n\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"215240","messageId":"7vwqrtjmtx.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"043047afd2915dd8f3a68cf164dc516d4c0bb5c2.1366718624.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 7/7] git grep: honor textconv by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T15:20:10Z","receivedAt":"2013-04-23T15:20:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Currently, \"git grep\" does not honor textconv settings by default.\n> Make it honor them by default just like \"git log --grep\" does.\n\n\"git log --grep\" looks for strings in the log message which never\ngoes through textconv filters.\n\nPuzzled....\n\nIf you meant -S/-G, it justifies use of textconv because we are\ngenerating diff and the user defines textconv to get a reasonable\noutput for otherwise undiffable contents.\n\nI do not know if it is sensible to apply textconv by default for\n\"grep\" (or for that matter \"git show\" that gives blob contents).\n\n>\n> Signed-off-by: Michael J Gruber \n> ---\n> Documentation/git-grep.txt | 2 +-\n> grep.c | 2 ++\n> t/t7008-grep-binary.sh | 4 ++--\n> 3 files changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n> index a5c5a27..f54ac0c 100644\n> --- a/Documentation/git-grep.txt\n> +++ b/Documentation/git-grep.txt\n> @@ -82,10 +82,10 @@ OPTIONS\n> \n> --textconv::\n> \tHonor textconv filter settings.\n> +\tThis is the default.\n> \n> --no-textconv::\n> \tDo not honor textconv filter settings.\n> -\tThis is the default.\n> \n> -i::\n> --ignore-case::\n> diff --git a/grep.c b/grep.c\n> index c668034..161d3f0 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -31,6 +31,7 @@ void init_grep_defaults(void)\n> \topt->max_depth = -1;\n> \topt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n> \topt->extended_regexp_option = 0;\n> +\topt->allow_textconv = 1;\n> \tstrcpy(opt->color_context, \"\");\n> \tstrcpy(opt->color_filename, \"\");\n> \tstrcpy(opt->color_function, \"\");\n> @@ -134,6 +135,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n> \topt->pathname = def->pathname;\n> \topt->regflags = def->regflags;\n> \topt->relative = def->relative;\n> +\topt->allow_textconv = def->allow_textconv;\n> \n> \tstrcpy(opt->color_context, def->color_context);\n> \tstrcpy(opt->color_filename, def->color_filename);\n> diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\n> index 10b2c8b..2fc9d9c 100755\n> --- a/t/t7008-grep-binary.sh\n> +++ b/t/t7008-grep-binary.sh\n> @@ -156,7 +156,7 @@ test_expect_success 'setup textconv filters' '\n> \tgit config diff.foo.textconv \"\\\"$(pwd)\\\"\"/nul_to_q_textconv\n> '\n> \n> -test_expect_failure 'grep does not honor textconv' '\n> +test_expect_success 'grep does honor textconv' '\n> \techo \"a:binaryQfile\" >expect &&\n> \tgit grep Qfile >actual &&\n> \ttest_cmp expect actual\n> @@ -172,7 +172,7 @@ test_expect_success 'grep --no-textconv does not honor textconv' '\n> \ttest_must_fail git grep --no-textconv Qfile\n> '\n> \n> -test_expect_failure 'grep blob does not honor textconv' '\n> +test_expect_success 'grep blob does honor textconv' '\n> \techo \"HEAD:a:binaryQfile\" >expect &&\n> \tgit grep Qfile HEAD:a >actual &&\n> \ttest_cmp expect actual\n"},{"id":"215306","messageId":"5177AE7F.1040400@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7vwqrtjmtx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 7/7] git grep: honor textconv by default","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-24T10:05:51Z","receivedAt":"2013-04-24T10:05:51Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 23.04.2013 17:20:\n> Michael J Gruber writes:\n> \n>> Currently, \"git grep\" does not honor textconv settings by default.\n>> Make it honor them by default just like \"git log --grep\" does.\n> \n> \"git log --grep\" looks for strings in the log message which never\n> goes through textconv filters.\n> \n> Puzzled....\n> \n> If you meant -S/-G, it justifies use of textconv because we are\n> generating diff and the user defines textconv to get a reasonable\n> output for otherwise undiffable contents.\n\nSorry, yes, I meant \"log grep diff\", aka \"log -S/-G\".\n\n> I do not know if it is sensible to apply textconv by default for\n> \"grep\" (or for that matter \"git show\" that gives blob contents).\n\nWell, that is the discussion that we were having, with no real end\nresult, which is why I haven't implemented this differently yet.\n\nMy point is that we apply textconv on \"log diff greps\" already, so why\nshould't we on content greps?\n\nThe question is really whether we should treat \"content\" similar to\n\"diff\", that's question both when comparing \"git log -S\" to \"git grep\"\nand \"git show \" to \"git show \".\n\nMy choice is clear, but others seem torn.\n\nFor \"git grep\", implementing a \"no-textconv\" default is simple, but for\n\"git show \" this appears to be cumbersome to me.\n\n>> Signed-off-by: Michael J Gruber \n>> ---\n>> Documentation/git-grep.txt | 2 +-\n>> grep.c | 2 ++\n>> t/t7008-grep-binary.sh | 4 ++--\n>> 3 files changed, 5 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n>> index a5c5a27..f54ac0c 100644\n>> --- a/Documentation/git-grep.txt\n>> +++ b/Documentation/git-grep.txt\n>> @@ -82,10 +82,10 @@ OPTIONS\n>> \n>> --textconv::\n>> \tHonor textconv filter settings.\n>> +\tThis is the default.\n>> \n>> --no-textconv::\n>> \tDo not honor textconv filter settings.\n>> -\tThis is the default.\n>> \n>> -i::\n>> --ignore-case::\n>> diff --git a/grep.c b/grep.c\n>> index c668034..161d3f0 100644\n>> --- a/grep.c\n>> +++ b/grep.c\n>> @@ -31,6 +31,7 @@ void init_grep_defaults(void)\n>> \topt->max_depth = -1;\n>> \topt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n>> \topt->extended_regexp_option = 0;\n>> +\topt->allow_textconv = 1;\n>> \tstrcpy(opt->color_context, \"\");\n>> \tstrcpy(opt->color_filename, \"\");\n>> \tstrcpy(opt->color_function, \"\");\n>> @@ -134,6 +135,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n>> \topt->pathname = def->pathname;\n>> \topt->regflags = def->regflags;\n>> \topt->relative = def->relative;\n>> +\topt->allow_textconv = def->allow_textconv;\n>> \n>> \tstrcpy(opt->color_context, def->color_context);\n>> \tstrcpy(opt->color_filename, def->color_filename);\n>> diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\n>> index 10b2c8b..2fc9d9c 100755\n>> --- a/t/t7008-grep-binary.sh\n>> +++ b/t/t7008-grep-binary.sh\n>> @@ -156,7 +156,7 @@ test_expect_success 'setup textconv filters' '\n>> \tgit config diff.foo.textconv \"\\\"$(pwd)\\\"\"/nul_to_q_textconv\n>> '\n>> \n>> -test_expect_failure 'grep does not honor textconv' '\n>> +test_expect_success 'grep does honor textconv' '\n>> \techo \"a:binaryQfile\" >expect &&\n>> \tgit grep Qfile >actual &&\n>> \ttest_cmp expect actual\n>> @@ -172,7 +172,7 @@ test_expect_success 'grep --no-textconv does not honor textconv' '\n>> \ttest_must_fail git grep --no-textconv Qfile\n>> '\n>> \n>> -test_expect_failure 'grep blob does not honor textconv' '\n>> +test_expect_success 'grep blob does honor textconv' '\n>> \techo \"HEAD:a:binaryQfile\" >expect &&\n>> \tgit grep Qfile HEAD:a >actual &&\n>> \ttest_cmp expect actual\n"},{"id":"215307","messageId":"5177AF62.30104@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7v1ua1l1ki.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 4/7] t7008: demonstrate behavior of grep with textconv","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-24T10:09:38Z","receivedAt":"2013-04-24T10:09:38Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 23.04.2013 17:16:\n> Michael J Gruber writes:\n> \n>> Currently, \"git grep\" does not honor any textconv filters. Demonstrate\n>> this in the tests.\n>>\n>> Signed-off-by: Michael J Gruber \n>> ---\n>> t/t7008-grep-binary.sh | 23 +++++++++++++++++++++++\n>> 1 file changed, 23 insertions(+)\n>>\n>> diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\n>> index 26f8319..126fe4c 100755\n>> --- a/t/t7008-grep-binary.sh\n>> +++ b/t/t7008-grep-binary.sh\n>> @@ -145,4 +145,27 @@ test_expect_success 'grep respects not-binary diff attribute' '\n>> \ttest_cmp expect actual\n>> '\n>> \n>> +cat >nul_to_q_textconv <<'EOF'\n>> +#!/bin/sh\n>> +\"$PERL_PATH\" -pe 'y/\\000/Q/' < \"$1\"\n>> +EOF\n>> +chmod +x nul_to_q_textconv\n>> +\n>> +test_expect_success 'setup textconv filters' '\n>> +\techo a diff=foo >.gitattributes &&\n>> +\tgit config diff.foo.textconv \"\\\"$(pwd)\\\"\"/nul_to_q_textconv\n>> +'\n>> +\n>> +test_expect_failure 'grep does not honor textconv' '\n>> +\techo \"a:binaryQfile\" >expect &&\n>> +\tgit grep Qfile >actual &&\n> \n> This should pass --textconv to \"git grep\".\n\nBut \"git grep\" does not know that option yet, so the test would fail for\nthe wrong reason.\n\nThe point ist that I expect \"git grep\" to apply textconv filters by\ndefault, which it does not. (I know I might be the only one with this\nexpectation.)\n\nOr do we want to document the absence of that option?\n\n>> +\ttest_cmp expect actual\n>> +'\n>> +\n>> +test_expect_failure 'grep blob does not honor textconv' '\n>> +\techo \"HEAD:a:binaryQfile\" >expect &&\n>> +\tgit grep Qfile HEAD:a >actual &&\n> \n> Likewise.\n> \n>> +\ttest_cmp expect actual\n>> +'\n>> +\n>> test_done\n"},{"id":"215308","messageId":"5177AF6B.5040102@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7va9opl1om.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 2/7] show: obey --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-24T10:09:47Z","receivedAt":"2013-04-24T10:09:47Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 23.04.2013 17:14:\n> Michael J Gruber writes:\n> \n>>> Subject: Re: [PATCHv2 2/7] show: obey --textconv for blobs\n> \n> s/obey/honor/;\n\nI missed that one, thanks.\n\n>> Currently, \"diff\" and \"cat-file\" for blobs honor \"--textconv\" options\n>> (with the former defaulting to \"--textconv\" and the latter to\n>> \"--no-textconv\") whereas \"show\" does not honor this option, even though\n>> it takes diff options.\n>>\n>> Make \"show\" on blobs behave like \"diff\", i.e. honor \"--textconv\" by\n>> default and \"--no-textconv\" when given.\n> \n> It is the right thing to do to teach it to react to --[no-]textconv;\n> I am not sure if the default is right, though.\n\nThat is the question ;)\n\n>> Signed-off-by: Michael J Gruber \n>> ---\n>> builtin/log.c | 24 +++++++++++++++++++++---\n>> t/t4030-diff-textconv.sh | 8 +++++++-\n>> 2 files changed, 28 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/builtin/log.c b/builtin/log.c\n>> index 5f3ed77..fe0275e 100644\n>> --- a/builtin/log.c\n>> +++ b/builtin/log.c\n>> @@ -436,10 +436,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)\n>> \tstrbuf_release(&out);\n>> }\n>> \n>> -static int show_blob_object(const unsigned char *sha1, struct rev_info *rev)\n>> +static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)\n>> {\n>> +\tunsigned char sha1c[20];\n>> +\tstruct object_context obj_context;\n>> +\tchar *buf;\n>> +\tunsigned long size;\n>> +\n>> \tfflush(stdout);\n>> -\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n>> +\tif (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))\n>> +\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n>> +\n>> +\tif (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))\n>> +\t\tdie(\"Not a valid object name %s\", obj_name);\n>> +\tif (!obj_context.path[0] ||\n>> +\t !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, &buf, &size))\n>> +\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n>> +\n>> +\tif (!buf)\n>> +\t\tdie(\"git show %s: bad file\", obj_name);\n>> +\n>> +\twrite_or_die(1, buf, size);\n>> +\treturn 0;\n>> }\n>> \n>> static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)\n>> @@ -525,7 +543,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>> \t\tconst char *name = objects[i].name;\n>> \t\tswitch (o->type) {\n>> \t\tcase OBJ_BLOB:\n>> -\t\t\tret = show_blob_object(o->sha1, NULL);\n>> +\t\t\tret = show_blob_object(o->sha1, &rev, name);\n>> \t\t\tbreak;\n>> \t\tcase OBJ_TAG: {\n>> \t\t\tstruct tag *t = (struct tag *)o;\n>> diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\n>> index 260ea92..f9d55e1 100755\n>> --- a/t/t4030-diff-textconv.sh\n>> +++ b/t/t4030-diff-textconv.sh\n>> @@ -90,12 +90,18 @@ test_expect_success 'status -v produces text' '\n>> \tgit reset --soft HEAD@{1}\n>> '\n>> \n>> -test_expect_failure 'show blob produces text' '\n>> +test_expect_success 'show blob produces text' '\n>> \tgit show HEAD:file >actual &&\n>> \tprintf \"0\\\\n1\\\\n\" >expect &&\n>> \ttest_cmp expect actual\n>> '\n>> \n>> +test_expect_success 'show --no-textconv blob produces binary' '\n>> +\tgit show --no-textconv HEAD:file >actual &&\n>> +\tprintf \"\\\\0\\\\n\\\\1\\\\n\" >expect &&\n>> +\ttest_cmp expect actual\n>> +'\n>> +\n>> test_expect_success 'grep-diff (-G) operates on textconv data (add)' '\n>> \techo one >expect &&\n>> \tgit log --root --format=%s -G0 >actual &&\n"},{"id":"215335","messageId":"7vmwsnet0s.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"5177AF62.30104@drmicha.warpmail.net","subject":"Re: [PATCHv2 4/7] t7008: demonstrate behavior of grep with textconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-24T17:29:55Z","receivedAt":"2013-04-24T17:29:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n>>> +test_expect_failure 'grep does not honor textconv' '\n>>> +\techo \"a:binaryQfile\" >expect &&\n>>> +\tgit grep Qfile >actual &&\n>> \n>> This should pass --textconv to \"git grep\".\n>\n> But \"git grep\" does not know that option yet, so the test would fail for\n> the wrong reason.\n>\n> The point ist that I expect \"git grep\" to apply textconv filters by\n> default, which it does not. (I know I might be the only one with this\n> expectation.)\n>\n> Or do we want to document the absence of that option?\n\nFirst, whether you write expect_failure or expec_success, please\nlabel the test to say what is expected to happen in the ideal world.\nThe test in question says \"grep does not honor textconv\", but if you\nwant it to honor textconv in the ideal world, it should be \"grep\nhonors textconv (when it should)\".\n\nNow, from the point of view of testing \"git grep honors textconv\"\nmissing support at the command line parser level and a buggy\nimplementation of the command line parser that accepts but does not\ntrigger the feature are the same thing. The command would not honor\ntextconv either way.\n\nMarking the above as \"failure\" without explicitly asking for the\nfeature with \"--textconv\" means we want it to use textconv by\ndefault, but that is *not* what the test title says is testing.\n\nIn your patch, what the body of the text is really expecting is\n\"grep uses textconv by default\". If that is what it tests, then\npassing --textconv from the command line as I suggested would be\nwrong, but I was going by the title of the patch.\n"},{"id":"215337","messageId":"7vip3beszp.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"5177AF6B.5040102@drmicha.warpmail.net","subject":"Re: [PATCHv2 2/7] show: obey --textconv for blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-24T17:30:34Z","receivedAt":"2013-04-24T17:30:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Junio C Hamano venit, vidit, dixit 23.04.2013 17:14:\n>> Michael J Gruber writes:\n>> \n>>>> Subject: Re: [PATCHv2 2/7] show: obey --textconv for blobs\n>> \n>> s/obey/honor/;\n>\n> I missed that one, thanks.\n>\n>>> Currently, \"diff\" and \"cat-file\" for blobs honor \"--textconv\" options\n>>> (with the former defaulting to \"--textconv\" and the latter to\n>>> \"--no-textconv\") whereas \"show\" does not honor this option, even though\n>>> it takes diff options.\n>>>\n>>> Make \"show\" on blobs behave like \"diff\", i.e. honor \"--textconv\" by\n>>> default and \"--no-textconv\" when given.\n>> \n>> It is the right thing to do to teach it to react to --[no-]textconv;\n>> I am not sure if the default is right, though.\n>\n> That is the question ;)\n\nThen let me make it easier. It is not just \"I am not sure if\", but \"I\ndo not think that\".\n"},{"id":"215338","messageId":"7vehdzesr9.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"5177AE7F.1040400@drmicha.warpmail.net","subject":"Re: [PATCHv2 7/7] git grep: honor textconv by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-24T17:35:38Z","receivedAt":"2013-04-24T17:35:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> My point is that we apply textconv on \"log diff greps\" already, so why\n> should't we on content greps?\n\nI think you are going in circles. If you start from \"textconv is\nabout mangling blob contents\", then it would look natural to you\nthat \"show \", \"diff A B\", and \"grep \" would\nall first mangle the blob contents using textconv and then work on\nthem.\n\nBut diff..textconv is to mangle blob contents in preparation\nfor comparing with another.\n\nThat is why you explicitly ask \"cat-file --textconv\" to use the same\nmangling even when you are not comparing it with anything else.\n"},{"id":"215342","messageId":"vpqwqrrolpl.fsf@grenoble-inp.fr","threadId":"33545","inReplyTo":"7vehdzesr9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 7/7] git grep: honor textconv by default","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-04-24T17:57:42Z","receivedAt":"2013-04-24T17:57:42Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> But diff..textconv is to mangle blob contents in preparation\n> for comparing with another.\n\nand also in preparation for \"blame\".\n\nIn both cases (diff and blame), we're preparing to show the file content\nto the user, and showing the binary makes no sense.\n\nGrepping through the binary, on the other hand, can very well make\nsense, like:\n\n$ git grep foo\nfile.txt: some instance of foo\nbinary file bar.bin matches\n\nOne reason not to run the filter is performance: \"git grep\" is fast, and\nit's cool. My textconv filters are usually slow, and it's not a big\nproblem because the diff machinery will only invoke the textconv filter\nwhen the files are modified (i.e. hopefully not often for tracked binary\nfiles). OTOH, \"git grep\" would need to run the textconv filters for each\nbinary files being searched for.\n\nI tend to agree with Junio that it makes sense to keep it disabled by\ndefault. Perhaps a grep.textconv config option?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"215355","messageId":"7v38ufdaih.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"vpqwqrrolpl.fsf@grenoble-inp.fr","subject":"Re: [PATCHv2 7/7] git grep: honor textconv by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-24T18:55:02Z","receivedAt":"2013-04-24T18:55:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Grepping through the binary, on the other hand, can very well make\n> sense, like:\n>\n> $ git grep foo\n> file.txt: some instance of foo\n> binary file bar.bin matches\n\nYes, \n\nI am moderately negative on making it the default, mostly because it\ngoes against established expectations, but I did not mean to say\nthat an ability to pass blob contents through textconv before\nrunning grep should not exist. It would be a good option to have.\n"},{"id":"215588","messageId":"517A6C0C.1020506@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7v38ufdaih.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 7/7] git grep: honor textconv by default","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-26T11:59:08Z","receivedAt":"2013-04-26T11:59:08Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 24.04.2013 20:55:\n> Matthieu Moy writes:\n> \n>> Grepping through the binary, on the other hand, can very well make\n>> sense, like:\n>>\n>> $ git grep foo\n>> file.txt: some instance of foo\n>> binary file bar.bin matches\n\nBTW, textconv does not have to be slow - just use textconv-cache.\n\n> Yes, \n> \n> I am moderately negative on making it the default, mostly because it\n> goes against established expectations, but I did not mean to say\n> that an ability to pass blob contents through textconv before\n> running grep should not exist. It would be a good option to have.\n\nI'm still looking for a way to at least treat \"git grep\" and \"git show\nblob\" the same way. I understand that I cannot convince you to change\nthe default here. The two options that I see are:\n\n- Implement the --textconv option but leave the default as is. I did\nthat for \"git grep\" already (just drop 7/7) but it seems to be\ncumbersome for \"git show blob\". I have to recheck.\n\n- Implement a new attribute \"show\" analogous to \"diff\" which applies to\nthe blob case (\"git grep\" is a blob case, and so is \"git show blob\")\nwhich can specify a \"show\" driver, which is like a \"diff\" driver but\nunderstands textconv and cachetextconv options only.\nHere, the default would be \"--textconv\" in any case, but unless you\nspecify a \"show\" attribute and driver there is no change in current\nbehavior.\n\nThe second case is a bit like clean/smudge, so, alternatively, one could\nadd a textconv and cachetextconv option to \"filter\" rather than\nintroducing \"show\". I'm not sure how much the textconv machinery needs\nto change, though.\n\nMichael\n"},{"id":"215592","messageId":"vpqk3npctn8.fsf@grenoble-inp.fr","threadId":"33545","inReplyTo":"517A6C0C.1020506@drmicha.warpmail.net","subject":"Re: [PATCHv2 7/7] git grep: honor textconv by default","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-04-26T13:23:55Z","receivedAt":"2013-04-26T13:23:55Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Michael J Gruber writes:\n\n> BTW, textconv does not have to be slow - just use textconv-cache.\n\nRight, thanks for reminding me about this, I had forgotten its existance ;-).\n\n> I'm still looking for a way to at least treat \"git grep\" and \"git show\n> blob\" the same way.\n\nI agree they should be treated similarly.\n\n> - Implement the --textconv option but leave the default as is. I did\n> that for \"git grep\" already (just drop 7/7)\n\nThat seems sensible.\n\n> but it seems to be cumbersome for \"git show blob\". I have to recheck.\n\nIt should be possible to have a tri-state for the --[no-]textconv\noption: unset, set to true or set to false. But the code sharing between\nlog, show and diff might make that non-trivial.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"215838","messageId":"517E37A9.8040609@drmicha.warpmail.net","threadId":"33545","inReplyTo":"vpqk3npctn8.fsf@grenoble-inp.fr","subject":"Re: [PATCHv2 7/7] git grep: honor textconv by default","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-29T09:04:41Z","receivedAt":"2013-04-29T09:04:41Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Matthieu Moy venit, vidit, dixit 26.04.2013 15:23:\n> Michael J Gruber writes:\n> \n>> BTW, textconv does not have to be slow - just use textconv-cache.\n> \n> Right, thanks for reminding me about this, I had forgotten its existance ;-).\n> \n>> I'm still looking for a way to at least treat \"git grep\" and \"git show\n>> blob\" the same way.\n> \n> I agree they should be treated similarly.\n> \n>> - Implement the --textconv option but leave the default as is. I did\n>> that for \"git grep\" already (just drop 7/7)\n> \n> That seems sensible.\n> \n>> but it seems to be cumbersome for \"git show blob\". I have to recheck.\n> \n> It should be possible to have a tri-state for the --[no-]textconv\n> option: unset, set to true or set to false. But the code sharing between\n> log, show and diff might make that non-trivial.\n\nRight now it's a diffopt bit...\n\nMichael\n"},{"id":"215854","messageId":"7vy5c1l6nb.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"517E37A9.8040609@drmicha.warpmail.net","subject":"Re: [PATCHv2 7/7] git grep: honor textconv by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-29T15:04:56Z","receivedAt":"2013-04-29T15:04:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n>> It should be possible to have a tri-state for the --[no-]textconv\n>> option: unset, set to true or set to false. But the code sharing between\n>> log, show and diff might make that non-trivial.\n>\n> Right now it's a diffopt bit...\n\nI wonder if you can do something along the lines of the attached\npatch. The following discussion assumes that your default wants\ntextconv for generating patches, and no textconv for showing blobs,\nwhich is the case your \"it is a bit\" becomes an issue.\n\nThe basic structure is that:\n\n * There is an extra \"opt->touched_flags\" that keeps track of all\n the fields that have been touched by DIFF_OPT_SET and\n DIFF_OPT_CLR;\n\n * You may continue setting the default values to the flags, like\n commands in the \"log\" family do in cmd_log_init_defaults(), but\n after you finished setting the defaults, you clear the\n touched_flags field;\n\n * And then you let the usual callchain to call diff_opt_parse(),\n allowing the opt->flags be set or unset, while keeping track of\n which bits the user touched;\n\n * There is an optional callback \"opt->set_default\" that is called\n at the very beginning to lets you inspect touched_flags and\n update opt->flags appropriately, before the remainder of the\n diffcore machinery is set up, taking the opt->flags value into\n account.\n\nYour \"git show\" could start out with ALLOW_TEXTCONV set, but notice\nexplicit requests to --[no-]textconv from the command line in your\nset_default() callback. And then when it deals with a blob, check\nif the user touched ALLOW_TEXTCONV and appropriately act on that\nknowledge.\n\nThere would be three cases in your set_default callback:\n\n * flags has ALLOW_TEXTCONV set, and the bit was touched: the user\n explicitly said --textconv because she wants blobs to be mangled;\n\n * flags has ALLOW_TEXTCONV set, and the bit was not touched: the\n user did not say --textconv; do not mangle blobs;\n\n * flags has ALLOW_TEXTCONV unset; the user did not say --textconv,\n or explicitly said --no-textconv; do not mangle blobs.\n\nThe set_default callback can also be used to adjust defaults for\nfields that are not handled by the DIFF_OPT_SET/CLR/TST, by the way.\nYou can remember the address of the default value you fed to a\nstring field before entering the callchain to diff_opt_parse(), and\nin your set_default callback see if the value is still pointing at\nthe same piece of memory (in which case the user did not touch it).\n\n builtin/log.c | 1 +\n diff.c | 3 +++\n diff.h | 7 +++++--\n 3 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 6e56a50..c62ecd1 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -91,6 +91,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)\n \n \tif (default_date_mode)\n \t\trev->date_mode = parse_date_format(default_date_mode);\n+\trev->diffopt.touched_flags = 0;\n }\n \n static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\ndiff --git a/diff.c b/diff.c\nindex f0b3e7c..7c24872 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3213,6 +3213,9 @@ void diff_setup_done(struct diff_options *options)\n {\n \tint count = 0;\n \n+\tif (options->set_default)\n+\t\toptions->set_default(options);\n+\n \tif (options->output_format & DIFF_FORMAT_NAME)\n \t\tcount++;\n \tif (options->output_format & DIFF_FORMAT_NAME_STATUS)\ndiff --git a/diff.h b/diff.h\nindex 78b4091..5c2f878 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -87,8 +87,8 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)\n \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)\n+#define DIFF_OPT_SET(opts, flag) (((opts)->flags |= DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))\n+#define DIFF_OPT_CLR(opts, flag) (((opts)->flags &= ~DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))\n #define DIFF_XDL_TST(opts, flag) ((opts)->xdl_opts & XDF_##flag)\n #define DIFF_XDL_SET(opts, flag) ((opts)->xdl_opts |= XDF_##flag)\n #define DIFF_XDL_CLR(opts, flag) ((opts)->xdl_opts &= ~XDF_##flag)\n@@ -109,6 +109,7 @@ struct diff_options {\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n \tunsigned flags;\n+\tunsigned touched_flags;\n \tint use_color;\n \tint context;\n \tint interhunkcontext;\n@@ -145,6 +146,8 @@ struct diff_options {\n \t/* to support internal diff recursion by --follow hack*/\n \tint found_follow;\n \n+\tvoid (*set_default)(struct diff_options *);\n+\n \tFILE *file;\n \tint close_file;\n \n"},{"id":"216896","messageId":"280dde30d949c9c449ecb2b99f020de583c2079b.1368197380.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7vy5c1l6nb.fsf@alter.siamese.dyndns.org","subject":"[PATCHv3 1/7] t4030: demonstrate behavior of show with textconv","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-05-10T15:10:10Z","receivedAt":"2013-05-10T15:10:10Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"\"git show \" honors the --textconv option while \"git show \"\ndoes not. Demonstrate this in the test.\n\nSince the current behavior is supposed to stay as is, we expect the\ndefault for \"git show \" to remain --no-textconv.\n\nSigned-off-by: Michael J Gruber \n---\n t/t4030-diff-textconv.sh | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 53ec330..3950fc9 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -58,6 +58,12 @@ test_expect_success 'diff produces text' '\n \ttest_cmp expect.text actual\n '\n \n+test_expect_success 'show commit produces text' '\n+\tgit show 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@@ -84,6 +90,24 @@ test_expect_success 'status -v produces text' '\n \tgit reset --soft HEAD@{1}\n '\n \n+test_expect_success 'show blob produces binary' '\n+\tgit show HEAD:file >actual &&\n+\tprintf \"\\\\0\\\\n\\\\01\\\\n\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'show --textconv blob produces text' '\n+\tgit show --textconv HEAD:file >actual &&\n+\tprintf \"0\\\\n1\\\\n\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_success 'show --no-textconv blob produces binary' '\n+\tgit show --textconv HEAD:file >actual &&\n+\tprintf \"\\\\0\\\\n\\\\01\\\\n\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'grep-diff (-G) operates on textconv data (add)' '\n \techo one >expect &&\n \tgit log --root --format=%s -G0 >actual &&\n-- \n1.8.3.rc1.406.gf4dce7e\n"},{"id":"216900","messageId":"88fb8906050411d0fe8b56cea160a4bfa1abb699.1368197380.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7vy5c1l6nb.fsf@alter.siamese.dyndns.org","subject":"[PATCHv3 2/7] diff_opt: track whether flags have been set explicitly","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-05-10T15:10:11Z","receivedAt":"2013-05-10T15:10:11Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"From: Junio C Hamano \n\nThe diff_opt infrastructure sets flags based on defaults and command\nline options. Currently, it is impossible to detect whether a flag has\nbeen set as a default or on explicit request.\n\nAmend the structure so that this detection is possible:\n\n * There is an extra \"opt->touched_flags\" that keeps track of all\n the fields that have been touched by DIFF_OPT_SET and\n DIFF_OPT_CLR;\n\n * You may continue setting the default values to the flags, like\n commands in the \"log\" family do in cmd_log_init_defaults(), but\n after you finished setting the defaults, you clear the\n touched_flags field;\n\n * And then you let the usual callchain call diff_opt_parse(),\n allowing the opt->flags be set or unset, while keeping track of\n which bits the user touched;\n\n * There is an optional callback \"opt->set_default\" that is called\n at the very beginning to lets you inspect touched_flags and\n update opt->flags appropriately, before the remainder of the\n diffcore machinery is set up, taking the opt->flags value into\n account.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/technical/api-diff.txt | 10 +++++++++-\n builtin/log.c | 1 +\n diff.c | 3 +++\n diff.h | 8 ++++++--\n 4 files changed, 19 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/api-diff.txt b/Documentation/technical/api-diff.txt\nindex 2d2ebc0..8b001de 100644\n--- a/Documentation/technical/api-diff.txt\n+++ b/Documentation/technical/api-diff.txt\n@@ -28,7 +28,8 @@ Calling sequence\n \n * Call `diff_setup_done()`; this inspects the options set up so far for\n internal consistency and make necessary tweaking to it (e.g. if\n- textual patch output was asked, recursive behaviour is turned on).\n+ textual patch output was asked, recursive behaviour is turned on);\n+ the callback set_default in diff_options can be used to tweak this more.\n \n * As you find different pairs of files, call `diff_change()` to feed\n modified files, `diff_addremove()` to feed created or deleted files,\n@@ -115,6 +116,13 @@ Notable members are:\n \toperation, but some do not have anything to do with the diffcore\n \tlibrary.\n \n+`touched_flags`::\n+\tRecords whether a flag has been changed due to user request\n+\t(rather than just set/unset by default).\n+\n+`set_default`::\n+\tCallback which allows tweaking the options in diff_setup_done().\n+\n BINARY, TEXT;;\n \tAffects the way how a file that is seemingly binary is treated.\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex 9e21232..f19d779 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -111,6 +111,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)\n \n \tif (default_date_mode)\n \t\trev->date_mode = parse_date_format(default_date_mode);\n+\trev->diffopt.touched_flags = 0;\n }\n \n static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\ndiff --git a/diff.c b/diff.c\nindex f0b3e7c..7c24872 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3213,6 +3213,9 @@ void diff_setup_done(struct diff_options *options)\n {\n \tint count = 0;\n \n+\tif (options->set_default)\n+\t\toptions->set_default(options);\n+\n \tif (options->output_format & DIFF_FORMAT_NAME)\n \t\tcount++;\n \tif (options->output_format & DIFF_FORMAT_NAME_STATUS)\ndiff --git a/diff.h b/diff.h\nindex 78b4091..e995ae1 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -87,8 +87,9 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)\n \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)\n+#define DIFF_OPT_TOUCHED(opts, flag) ((opts)->touched_flags & DIFF_OPT_##flag)\n+#define DIFF_OPT_SET(opts, flag) (((opts)->flags |= DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))\n+#define DIFF_OPT_CLR(opts, flag) (((opts)->flags &= ~DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))\n #define DIFF_XDL_TST(opts, flag) ((opts)->xdl_opts & XDF_##flag)\n #define DIFF_XDL_SET(opts, flag) ((opts)->xdl_opts |= XDF_##flag)\n #define DIFF_XDL_CLR(opts, flag) ((opts)->xdl_opts &= ~XDF_##flag)\n@@ -109,6 +110,7 @@ struct diff_options {\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n \tunsigned flags;\n+\tunsigned touched_flags;\n \tint use_color;\n \tint context;\n \tint interhunkcontext;\n@@ -145,6 +147,8 @@ struct diff_options {\n \t/* to support internal diff recursion by --follow hack*/\n \tint found_follow;\n \n+\tvoid (*set_default)(struct diff_options *);\n+\n \tFILE *file;\n \tint close_file;\n \n-- \n1.8.3.rc1.406.gf4dce7e\n"},{"id":"216898","messageId":"c4ed1e0b67877e6453b8c269290e09e1672ce37d.1368197380.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7vy5c1l6nb.fsf@alter.siamese.dyndns.org","subject":"[PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-05-10T15:10:12Z","receivedAt":"2013-05-10T15:10:12Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, \"diff\" and \"cat-file\" for blobs honor \"--textconv\" options\n(with the former defaulting to \"--textconv\" and the latter to\n\"--no-textconv\") whereas \"show\" does not honor this option, even though\nit takes diff options.\n\nMake \"show\" on blobs behave like \"diff\", i.e. honor \"--textconv\" by\ndefault and \"--no-textconv\" when given.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/log.c | 25 ++++++++++++++++++++++---\n t/t4030-diff-textconv.sh | 6 +++---\n 2 files changed, 25 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex f19d779..dd3f108 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -437,10 +437,29 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)\n \tstrbuf_release(&out);\n }\n \n-static int show_blob_object(const unsigned char *sha1, struct rev_info *rev)\n+static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)\n {\n+\tunsigned char sha1c[20];\n+\tstruct object_context obj_context;\n+\tchar *buf;\n+\tunsigned long size;\n+\n \tfflush(stdout);\n-\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n+\tif (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||\n+\t !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))\n+\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n+\n+\tif (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))\n+\t\tdie(\"Not a valid object name %s\", obj_name);\n+\tif (!obj_context.path[0] ||\n+\t !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, &buf, &size))\n+\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n+\n+\tif (!buf)\n+\t\tdie(\"git show %s: bad file\", obj_name);\n+\n+\twrite_or_die(1, buf, size);\n+\treturn 0;\n }\n \n static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)\n@@ -526,7 +545,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\tconst char *name = objects[i].name;\n \t\tswitch (o->type) {\n \t\tcase OBJ_BLOB:\n-\t\t\tret = show_blob_object(o->sha1, NULL);\n+\t\t\tret = show_blob_object(o->sha1, &rev, name);\n \t\t\tbreak;\n \t\tcase OBJ_TAG: {\n \t\t\tstruct tag *t = (struct tag *)o;\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 3950fc9..0ebb028 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -96,14 +96,14 @@ test_expect_success 'show blob produces binary' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'show --textconv blob produces text' '\n+test_expect_success 'show --textconv blob produces text' '\n \tgit show --textconv HEAD:file >actual &&\n \tprintf \"0\\\\n1\\\\n\" >expect &&\n \ttest_cmp expect actual\n '\n \n-test_success 'show --no-textconv blob produces binary' '\n-\tgit show --textconv HEAD:file >actual &&\n+test_expect_success 'show --no-textconv blob produces binary' '\n+\tgit show --no-textconv HEAD:file >actual &&\n \tprintf \"\\\\0\\\\n\\\\01\\\\n\" >expect &&\n \ttest_cmp expect actual\n '\n-- \n1.8.3.rc1.406.gf4dce7e\n"},{"id":"216899","messageId":"b54866d8875298429f4756c9e5c268cbcbeee710.1368197380.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7vy5c1l6nb.fsf@alter.siamese.dyndns.org","subject":"[PATCHv3 4/7] cat-file: do not die on --textconv without textconv filters","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-05-10T15:10:13Z","receivedAt":"2013-05-10T15:10:13Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"When a command is supposed to use textconv filters (by default or with\n\"--textconv\") and none are configured then the blob is output without\nconversion; the only exception to this rule is \"cat-file --textconv\".\n\nMake it behave like the rest of textconv aware commands.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/cat-file.c | 18 ++++++++----------\n t/t8007-cat-file-textconv.sh | 20 +++++---------------\n 2 files changed, 13 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 045cee7..bd62373 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -48,6 +48,14 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \tcase 'e':\n \t\treturn !has_sha1_file(sha1);\n \n+\tcase 'c':\n+\t\tif (!obj_context.path[0])\n+\t\t\tdie(\"git cat-file --textconv %s: must be \",\n+\t\t\t obj_name);\n+\n+\t\tif (textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n+\t\t\tbreak;\n+\n \tcase 'p':\n \t\ttype = sha1_object_info(sha1, NULL);\n \t\tif (type < 0)\n@@ -70,16 +78,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \t\t/* otherwise just spit out the data */\n \t\tbreak;\n \n-\tcase 'c':\n-\t\tif (!obj_context.path[0])\n-\t\t\tdie(\"git cat-file --textconv %s: must be \",\n-\t\t\t obj_name);\n-\n-\t\tif (!textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n-\t\t\tdie(\"git cat-file --textconv: unable to run textconv on %s\",\n-\t\t\t obj_name);\n-\t\tbreak;\n-\n \tcase 0:\n \t\tif (type_from_string(exp_type) == OBJ_BLOB) {\n \t\t\tunsigned char blob_sha1[20];\ndiff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh\nindex 78a0085..83c6636 100755\n--- a/t/t8007-cat-file-textconv.sh\n+++ b/t/t8007-cat-file-textconv.sh\n@@ -22,11 +22,11 @@ test_expect_success 'setup ' '\n '\n \n cat >expected <result\n+\tgit cat-file --textconv :one.bin >result &&\n \ttest_cmp expected result\n '\n \n@@ -36,10 +36,6 @@ test_expect_success 'setup textconv filters' '\n \tgit config diff.test.cachetextconv false\n '\n \n-cat >expected <result &&\n \ttest_cmp expected result\n@@ -73,25 +69,19 @@ test_expect_success 'cat-file --textconv on previous commit' '\n '\n \n test_expect_success SYMLINKS 'cat-file without --textconv (symlink)' '\n+\tprintf \"%s\" \"one.bin\" >expected &&\n \tgit cat-file blob :symlink.bin >result &&\n-\tprintf \"%s\" \"one.bin\" >expected\n \ttest_cmp expected result\n '\n \n \n test_expect_success SYMLINKS 'cat-file --textconv on index (symlink)' '\n-\t! git cat-file --textconv :symlink.bin 2>result &&\n-\tcat >expected <<\\EOF &&\n-fatal: git cat-file --textconv: unable to run textconv on :symlink.bin\n-EOF\n+\tgit cat-file --textconv :symlink.bin >result &&\n \ttest_cmp expected result\n '\n \n test_expect_success SYMLINKS 'cat-file --textconv on HEAD (symlink)' '\n-\t! git cat-file --textconv HEAD:symlink.bin 2>result &&\n-\tcat >expected <result &&\n \ttest_cmp expected result\n '\n \n-- \n1.8.3.rc1.406.gf4dce7e\n"},{"id":"216897","messageId":"16e83cb70df76071a993612d4b69c5c528f4b1a5.1368197380.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7vy5c1l6nb.fsf@alter.siamese.dyndns.org","subject":"[PATCHv3 5/7] t7008: demonstrate behavior of grep with textconv","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-05-10T15:10:14Z","receivedAt":"2013-05-10T15:10:14Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, \"git grep\" does not honor any textconv filters, with nor\nwithout --textconv. Demonstrate this in the tests.\n\nThe default is expected to remain unchanged.\n\nSigned-off-by: Michael J Gruber \n---\n t/t7008-grep-binary.sh | 31 +++++++++++++++++++++++++++++++\n 1 file changed, 31 insertions(+)\n\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex 26f8319..1c0946f 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -145,4 +145,35 @@ test_expect_success 'grep respects not-binary diff attribute' '\n \ttest_cmp expect actual\n '\n \n+cat >nul_to_q_textconv <<'EOF'\n+#!/bin/sh\n+\"$PERL_PATH\" -pe 'y/\\000/Q/' < \"$1\"\n+EOF\n+chmod +x nul_to_q_textconv\n+\n+test_expect_success 'setup textconv filters' '\n+\techo a diff=foo >.gitattributes &&\n+\tgit config diff.foo.textconv \"\\\"$(pwd)\\\"\"/nul_to_q_textconv\n+'\n+\n+test_expect_success 'grep does not honor textconv' '\n+\ttest_must_fail git grep Qfile\n+'\n+\n+test_expect_failure 'grep --textconv honors textconv' '\n+\techo \"a:binaryQfile\" >expect &&\n+\tgit grep --textconv Qfile >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --no-textconv does not honor textconv' '\n+\ttest_must_fail git grep --no-textconv Qfile\n+'\n+\n+test_expect_failure 'grep --textconv blob honors textconv' '\n+\techo \"HEAD:a:binaryQfile\" >expect &&\n+\tgit grep --textconv Qfile HEAD:a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.3.rc1.406.gf4dce7e\n"},{"id":"216902","messageId":"576deff3929c68083ed87251dc5bb5e4dbe1b7e0.1368197380.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7vy5c1l6nb.fsf@alter.siamese.dyndns.org","subject":"[PATCHv3 6/7] grep: allow to use textconv filters","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-05-10T15:10:15Z","receivedAt":"2013-05-10T15:10:15Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"From: Jeff King \n\nRecently and not so recently, we made sure that log/grep type operations\nuse textconv filters when a userfacing diff would do the same:\n\nef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28)\nb1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28)\n0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)\n\n\"git grep\" currently does not use textconv filters at all, that is\nneither for displaying the match and context nor for the actual grepping,\neven when requested by --textconv.\n\nIntroduce an option \"--textconv\" which makes git grep use any configured\ntextconv filters for grepping and output purposes. It is off by default.\n\nSigned-off-by: Jeff King \nSigned-off-by: Michael J Gruber \n---\n Documentation/git-grep.txt | 9 +++-\n builtin/grep.c | 2 +\n grep.c | 100 ++++++++++++++++++++++++++++++++++++++-------\n grep.h | 1 +\n t/t7008-grep-binary.sh | 6 ++-\n 5 files changed, 102 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 50d46e1..a5c5a27 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -9,7 +9,7 @@ git-grep - Print lines matching a pattern\n SYNOPSIS\n --------\n [verse]\n-'git grep' [-a | --text] [-I] [-i | --ignore-case] [-w | --word-regexp]\n+'git grep' [-a | --text] [-I] [--textconv] [-i | --ignore-case] [-w | --word-regexp]\n \t [-v | --invert-match] [-h|-H] [--full-name]\n \t [-E | --extended-regexp] [-G | --basic-regexp]\n \t [-P | --perl-regexp]\n@@ -80,6 +80,13 @@ OPTIONS\n --text::\n \tProcess binary files as if they were text.\n \n+--textconv::\n+\tHonor textconv filter settings.\n+\n+--no-textconv::\n+\tDo not honor textconv filter settings.\n+\tThis is the default.\n+\n -i::\n --ignore-case::\n \tIgnore case differences between the patterns and the\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 159e65d..00ee57d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -659,6 +659,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_SET_INT('I', NULL, &opt.binary,\n \t\t\tN_(\"don't match patterns in binary files\"),\n \t\t\tGREP_BINARY_NOMATCH),\n+\t\tOPT_BOOL(0, \"textconv\", &opt.allow_textconv,\n+\t\t\t N_(\"process binary files with textconv filters\")),\n \t\t{ OPTION_INTEGER, 0, \"max-depth\", &opt.max_depth, N_(\"depth\"),\n \t\t\tN_(\"descend at most levels\"), PARSE_OPT_NONEG,\n \t\t\tNULL, 1 },\ndiff --git a/grep.c b/grep.c\nindex bb548ca..c668034 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -2,6 +2,8 @@\n #include \"grep.h\"\n #include \"userdiff.h\"\n #include \"xdiff-interface.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n \n static int grep_source_load(struct grep_source *gs);\n static int grep_source_is_binary(struct grep_source *gs);\n@@ -1322,6 +1324,58 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n \tfwrite(buf, size, 1, stdout);\n }\n \n+static int fill_textconv_grep(struct userdiff_driver *driver,\n+\t\t\t struct grep_source *gs)\n+{\n+\tstruct diff_filespec *df;\n+\tchar *buf;\n+\tsize_t size;\n+\n+\tif (!driver || !driver->textconv)\n+\t\treturn grep_source_load(gs);\n+\n+\t/*\n+\t * The textconv interface is intimately tied to diff_filespecs, so we\n+\t * have to pretend to be one. If we could unify the grep_source\n+\t * and diff_filespec structs, this mess could just go away.\n+\t */\n+\tdf = alloc_filespec(gs->path);\n+\tswitch (gs->type) {\n+\tcase GREP_SOURCE_SHA1:\n+\t\tfill_filespec(df, gs->identifier, 1, 0100644);\n+\t\tbreak;\n+\tcase GREP_SOURCE_FILE:\n+\t\tfill_filespec(df, null_sha1, 0, 0100644);\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: attempt to textconv something without a path?\");\n+\t}\n+\n+\t/*\n+\t * fill_textconv is not remotely thread-safe; it may load objects\n+\t * behind the scenes, and it modifies the global diff tempfile\n+\t * structure.\n+\t */\n+\tgrep_read_lock();\n+\tsize = fill_textconv(driver, df, &buf);\n+\tgrep_read_unlock();\n+\tfree_filespec(df);\n+\n+\t/*\n+\t * The normal fill_textconv usage by the diff machinery would just keep\n+\t * the textconv'd buf separate from the diff_filespec. But much of the\n+\t * grep code passes around a grep_source and assumes that its \"buf\"\n+\t * pointer is the beginning of the thing we are searching. So let's\n+\t * install our textconv'd version into the grep_source, taking care not\n+\t * to leak any existing buffer.\n+\t */\n+\tgrep_source_clear_data(gs);\n+\tgs->buf = buf;\n+\tgs->size = size;\n+\n+\treturn 0;\n+}\n+\n static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)\n {\n \tchar *bol;\n@@ -1332,6 +1386,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \tunsigned count = 0;\n \tint try_lookahead = 0;\n \tint show_function = 0;\n+\tstruct userdiff_driver *textconv = NULL;\n \tenum grep_context ctx = GREP_CONTEXT_HEAD;\n \txdemitconf_t xecfg;\n \n@@ -1353,19 +1408,36 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t}\n \topt->last_shown = 0;\n \n-\tswitch (opt->binary) {\n-\tcase GREP_BINARY_DEFAULT:\n-\t\tif (grep_source_is_binary(gs))\n-\t\t\tbinary_match_only = 1;\n-\t\tbreak;\n-\tcase GREP_BINARY_NOMATCH:\n-\t\tif (grep_source_is_binary(gs))\n-\t\t\treturn 0; /* Assume unmatch */\n-\t\tbreak;\n-\tcase GREP_BINARY_TEXT:\n-\t\tbreak;\n-\tdefault:\n-\t\tdie(\"bug: unknown binary handling mode\");\n+\tif (opt->allow_textconv) {\n+\t\tgrep_source_load_driver(gs);\n+\t\t/*\n+\t\t * We might set up the shared textconv cache data here, which\n+\t\t * is not thread-safe.\n+\t\t */\n+\t\tgrep_attr_lock();\n+\t\ttextconv = userdiff_get_textconv(gs->driver);\n+\t\tgrep_attr_unlock();\n+\t}\n+\n+\t/*\n+\t * We know the result of a textconv is text, so we only have to care\n+\t * about binary handling if we are not using it.\n+\t */\n+\tif (!textconv) {\n+\t\tswitch (opt->binary) {\n+\t\tcase GREP_BINARY_DEFAULT:\n+\t\t\tif (grep_source_is_binary(gs))\n+\t\t\t\tbinary_match_only = 1;\n+\t\t\tbreak;\n+\t\tcase GREP_BINARY_NOMATCH:\n+\t\t\tif (grep_source_is_binary(gs))\n+\t\t\t\treturn 0; /* Assume unmatch */\n+\t\t\tbreak;\n+\t\tcase GREP_BINARY_TEXT:\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tdie(\"bug: unknown binary handling mode\");\n+\t\t}\n \t}\n \n \tmemset(&xecfg, 0, sizeof(xecfg));\n@@ -1373,7 +1445,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \n \ttry_lookahead = should_lookahead(opt);\n \n-\tif (grep_source_load(gs) < 0)\n+\tif (fill_textconv_grep(textconv, gs) < 0)\n \t\treturn 0;\n \n \tbol = gs->buf;\ndiff --git a/grep.h b/grep.h\nindex e4a1df5..eaaced1 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -107,6 +107,7 @@ struct grep_opt {\n #define GREP_BINARY_NOMATCH\t1\n #define GREP_BINARY_TEXT\t2\n \tint binary;\n+\tint allow_textconv;\n \tint extended;\n \tint use_reflog_filter;\n \tint pcre;\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex 1c0946f..a91260a 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -160,7 +160,7 @@ test_expect_success 'grep does not honor textconv' '\n \ttest_must_fail git grep Qfile\n '\n \n-test_expect_failure 'grep --textconv honors textconv' '\n+test_expect_success 'grep --textconv honors textconv' '\n \techo \"a:binaryQfile\" >expect &&\n \tgit grep --textconv Qfile >actual &&\n \ttest_cmp expect actual\n@@ -176,4 +176,8 @@ test_expect_failure 'grep --textconv blob honors textconv' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'grep --no-textconv blob does not honor textconv' '\n+\ttest_must_fail git grep --no-textconv Qfile HEAD:a\n+'\n+\n test_done\n-- \n1.8.3.rc1.406.gf4dce7e\n"},{"id":"216901","messageId":"dd973eae534bed5f7106d54e06c7c2172595f402.1368197380.git.git@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7vy5c1l6nb.fsf@alter.siamese.dyndns.org","subject":"[PATCHv3 7/7] grep: honor --textconv for the case rev:path","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-05-10T15:10:16Z","receivedAt":"2013-05-10T15:10:16Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Make \"grep\" honor the \"--textconv\" option also for the object case, i.e.\nwhen used with an argument \"rev:path\".\n\nSigned-off-by: Michael J Gruber \n---\n builtin/grep.c | 11 ++++++-----\n object.c | 26 ++++++++++++++++++++------\n object.h | 2 ++\n t/t7008-grep-binary.sh | 6 +-----\n 4 files changed, 29 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 00ee57d..bb7f970 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -458,10 +458,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n }\n \n static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n-\t\t struct object *obj, const char *name)\n+\t\t struct object *obj, const char *name, struct object_context *oc)\n {\n \tif (obj->type == OBJ_BLOB)\n-\t\treturn grep_sha1(opt, obj->sha1, name, 0, NULL);\n+\t\treturn grep_sha1(opt, obj->sha1, name, 0, oc ? oc->path : NULL);\n \tif (obj->type == OBJ_COMMIT || obj->type == OBJ_TREE) {\n \t\tstruct tree_desc tree;\n \t\tvoid *data;\n@@ -503,7 +503,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object *real_obj;\n \t\treal_obj = deref_tag(list->objects[i].item, NULL, 0);\n-\t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name)) {\n+\t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name, list->objects[i].context)) {\n \t\t\thit = 1;\n \t\t\tif (opt->status_only)\n \t\t\t\tbreak;\n@@ -820,12 +820,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tunsigned char sha1[20];\n+\t\tstruct object_context oc;\n \t\t/* Is it a rev? */\n-\t\tif (!get_sha1(arg, sha1)) {\n+\t\tif (!get_sha1_with_context(arg, 0, sha1, &oc)) {\n \t\t\tstruct object *object = parse_object_or_die(sha1, arg);\n \t\t\tif (!seen_dashdash)\n \t\t\t\tverify_non_filename(prefix, arg);\n-\t\t\tadd_object_array(object, arg, &list);\n+\t\t\tadd_object_array_with_context(object, arg, &list, xmemdupz(&oc, sizeof(struct object_context)));\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--\")) {\ndiff --git a/object.c b/object.c\nindex 88d0bec..264b6df 100644\n--- a/object.c\n+++ b/object.c\n@@ -265,12 +265,7 @@ int object_list_contains(struct object_list *list, struct object *obj)\n \treturn 0;\n }\n \n-void add_object_array(struct object *obj, const char *name, struct object_array *array)\n-{\n-\tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n-}\n-\n-void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n+static void add_object_array_with_mode_context(struct object *obj, const char *name, struct object_array *array, unsigned mode, struct object_context *context)\n {\n \tunsigned nr = array->nr;\n \tunsigned alloc = array->alloc;\n@@ -285,9 +280,28 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj\n \tobjects[nr].item = obj;\n \tobjects[nr].name = name;\n \tobjects[nr].mode = mode;\n+\tobjects[nr].context = context;\n \tarray->nr = ++nr;\n }\n \n+void add_object_array(struct object *obj, const char *name, struct object_array *array)\n+{\n+\tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n+}\n+\n+void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n+{\n+\tadd_object_array_with_mode_context(obj, name, array, mode, NULL);\n+}\n+\n+void add_object_array_with_context(struct object *obj, const char *name, struct object_array *array, struct object_context *context)\n+{\n+\tif (context)\n+\t\tadd_object_array_with_mode_context(obj, name, array, context->mode, context);\n+\telse\n+\t\tadd_object_array_with_mode_context(obj, name, array, S_IFINVALID, context);\n+}\n+\n void object_array_remove_duplicates(struct object_array *array)\n {\n \tunsigned int ref, src, dst;\ndiff --git a/object.h b/object.h\nindex 97d384b..695847d 100644\n--- a/object.h\n+++ b/object.h\n@@ -13,6 +13,7 @@ struct object_array {\n \t\tstruct object *item;\n \t\tconst char *name;\n \t\tunsigned mode;\n+\t\tstruct object_context *context;\n \t} *objects;\n };\n \n@@ -85,6 +86,7 @@ int object_list_contains(struct object_list *list, struct object *obj);\n /* Object array handling .. */\n void add_object_array(struct object *obj, const char *name, struct object_array *array);\n void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode);\n+void add_object_array_with_context(struct object *obj, const char *name, struct object_array *array, struct object_context *context);\n void object_array_remove_duplicates(struct object_array *);\n \n void clear_object_flags(unsigned flags);\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex a91260a..b146406 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -170,14 +170,10 @@ test_expect_success 'grep --no-textconv does not honor textconv' '\n \ttest_must_fail git grep --no-textconv Qfile\n '\n \n-test_expect_failure 'grep --textconv blob honors textconv' '\n+test_expect_success 'grep --textconv blob honors textconv' '\n \techo \"HEAD:a:binaryQfile\" >expect &&\n \tgit grep --textconv Qfile HEAD:a >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'grep --no-textconv blob does not honor textconv' '\n-\ttest_must_fail git grep --no-textconv Qfile HEAD:a\n-'\n-\n test_done\n-- \n1.8.3.rc1.406.gf4dce7e\n"},{"id":"216905","messageId":"CAPig+cSAtXfeBYkw8Ob4i_ozETVwndONcQxH42ryU0ihKPTDjw@mail.gmail.com","threadId":"33545","inReplyTo":"88fb8906050411d0fe8b56cea160a4bfa1abb699.1368197380.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv3 2/7] diff_opt: track whether flags have been set explicitly","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-10T15:31:29Z","receivedAt":"2013-05-10T15:31:29Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 10, 2013 at 11:10 AM, Michael J Gruber\n wrote:\n> From: Junio C Hamano \n>\n> The diff_opt infrastructure sets flags based on defaults and command\n> line options. Currently, it is impossible to detect whether a flag has\n> been set as a default or on explicit request.\n>\n> Amend the structure so that this detection is possible:\n>\n> * There is an extra \"opt->touched_flags\" that keeps track of all\n> the fields that have been touched by DIFF_OPT_SET and\n> DIFF_OPT_CLR;\n>\n> * You may continue setting the default values to the flags, like\n> commands in the \"log\" family do in cmd_log_init_defaults(), but\n> after you finished setting the defaults, you clear the\n> touched_flags field;\n>\n> * And then you let the usual callchain call diff_opt_parse(),\n> allowing the opt->flags be set or unset, while keeping track of\n> which bits the user touched;\n>\n> * There is an optional callback \"opt->set_default\" that is called\n> at the very beginning to lets you inspect touched_flags and\n\ns/lets/let/\n\n> update opt->flags appropriately, before the remainder of the\n> diffcore machinery is set up, taking the opt->flags value into\n> account.\n>\n> Signed-off-by: Michael J Gruber \n"},{"id":"216915","messageId":"7vy5bm22f8.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"c4ed1e0b67877e6453b8c269290e09e1672ce37d.1368197380.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-10T17:02:51Z","receivedAt":"2013-05-10T17:02:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Currently, \"diff\" and \"cat-file\" for blobs honor \"--textconv\" options\n> (with the former defaulting to \"--textconv\" and the latter to\n> \"--no-textconv\") whereas \"show\" does not honor this option, even though\n> it takes diff options.\n>\n> Make \"show\" on blobs behave like \"diff\", i.e. honor \"--textconv\" by\n> default and \"--no-textconv\" when given.\n\nHmm...\n\n> +static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)\n> {\n> +\tunsigned char sha1c[20];\n> +\tstruct object_context obj_context;\n> +\tchar *buf;\n> +\tunsigned long size;\n> +\n> \tfflush(stdout);\n> -\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n> +\tif (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||\n> +\t !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))\n> +\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n\nIt is surprising that the necessary change is only this, but I think\nit is correct ;-). We ignore textconv when the command line did not\nmention --[no-]textconv, or the command line said --no-textconv\nexplicitly.\n\nThis (especially the first condition) may deserve an in-code comment\nfor anybody who wonders where this default behaviour is implemented.\n\nSo \"show\" on blobs does show the raw contents by default, but the\nuser can explicitly ask to enable textconv with --[no-]textconv. Is\nthe second paragraph in the log message still valid?\n\n> +\tif (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))\n> +\t\tdie(\"Not a valid object name %s\", obj_name);\n\nThis looks somewhat unfortunate.\n\nWe already have sha1[]; actually we not just know sha1[] but have\nthe struct object for it. How did we obtain it before we got here?\n\nWill we always have a valid name in rev.pending.objects->name? Will\nthat name convert back to the same sha1 we got in sha1[]?\n\nI think the answers are \"Yes (it is a command line argument), Yes\n(that is what setup_revisions() got by feeding the name to give us\nsha1[])\".\n\nI wonder if enriching rev_info->pending with the context information\nmight be a clean solution to avoid this redundant but unavoidable\nconversion, but that is a separate and future topic, I think.\n\n> +\tif (!obj_context.path[0] ||\n> +\t !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, &buf, &size))\n> +\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n> +\n> +\tif (!buf)\n> +\t\tdie(\"git show %s: bad file\", obj_name);\n> +\n> +\twrite_or_die(1, buf, size);\n> +\treturn 0;\n> }\n> \n> static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)\n> @@ -526,7 +545,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n> \t\tconst char *name = objects[i].name;\n> \t\tswitch (o->type) {\n> \t\tcase OBJ_BLOB:\n> -\t\t\tret = show_blob_object(o->sha1, NULL);\n> +\t\t\tret = show_blob_object(o->sha1, &rev, name);\n> \t\t\tbreak;\n> \t\tcase OBJ_TAG: {\n> \t\t\tstruct tag *t = (struct tag *)o;\n> diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\n> index 3950fc9..0ebb028 100755\n> --- a/t/t4030-diff-textconv.sh\n> +++ b/t/t4030-diff-textconv.sh\n> @@ -96,14 +96,14 @@ test_expect_success 'show blob produces binary' '\n> \ttest_cmp expect actual\n> '\n> \n> -test_expect_failure 'show --textconv blob produces text' '\n> +test_expect_success 'show --textconv blob produces text' '\n> \tgit show --textconv HEAD:file >actual &&\n> \tprintf \"0\\\\n1\\\\n\" >expect &&\n> \ttest_cmp expect actual\n> '\n> \n> -test_success 'show --no-textconv blob produces binary' '\n> -\tgit show --textconv HEAD:file >actual &&\n> +test_expect_success 'show --no-textconv blob produces binary' '\n> +\tgit show --no-textconv HEAD:file >actual &&\n> \tprintf \"\\\\0\\\\n\\\\01\\\\n\" >expect &&\n> \ttest_cmp expect actual\n> '\n"},{"id":"216917","messageId":"20130510173434.GA3154@sigill.intra.peff.net","threadId":"33545","inReplyTo":"7vy5bm22f8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-10T17:34:34Z","receivedAt":"2013-05-10T17:34:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 10, 2013 at 10:02:51AM -0700, Junio C Hamano wrote:\n\n> > Make \"show\" on blobs behave like \"diff\", i.e. honor \"--textconv\" by\n> > default and \"--no-textconv\" when given.\n> [...]\n> So \"show\" on blobs does show the raw contents by default, but the\n> user can explicitly ask to enable textconv with --[no-]textconv. Is\n> the second paragraph in the log message still valid?\n\nYes, I had the same thought...\n\n> > +\tif (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))\n> > +\t\tdie(\"Not a valid object name %s\", obj_name);\n> \n> This looks somewhat unfortunate.\n> [...]\n> I wonder if enriching rev_info->pending with the context information\n> might be a clean solution to avoid this redundant but unavoidable\n> conversion, but that is a separate and future topic, I think.\n\nIt would be, and indeed, that is similar to what the final patch does.\nThe problem is that it requires an extra allocation (we do not want to\nunconditionally put the object_context into the object_array because it\nis too big, so we add only a pointer). So having rev_info->pending store\nthat information would mean that callers would have to know to free it\nwhen freeing the pending array. We would have to either teach each\nexisting caller to do so, or perhaps enable the behavior only when a\ncertain flag is set (e.g., rev->keep_object_context or something).\n\n-Peff\n"},{"id":"216919","messageId":"7vfvxu1zla.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"20130510173434.GA3154@sigill.intra.peff.net","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-10T18:04:01Z","receivedAt":"2013-05-10T18:04:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, May 10, 2013 at 10:02:51AM -0700, Junio C Hamano wrote:\n>\n>> > Make \"show\" on blobs behave like \"diff\", i.e. honor \"--textconv\" by\n>> > default and \"--no-textconv\" when given.\n>> [...]\n>> So \"show\" on blobs does show the raw contents by default, but the\n>> user can explicitly ask to enable textconv with --[no-]textconv. Is\n>> the second paragraph in the log message still valid?\n>\n> Yes, I had the same thought...\n\nI'd rewrite the paragraph to something like:\n\n Make \"show\" on blobs honor \"--textconv\" when it is asked. The default\n is not to apply textconv, which is in line with what \"cat-file\" does.\n\n>> > +\tif (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))\n>> > +\t\tdie(\"Not a valid object name %s\", obj_name);\n>> \n>> This looks somewhat unfortunate.\n>> [...]\n>> I wonder if enriching rev_info->pending with the context information\n>> might be a clean solution to avoid this redundant but unavoidable\n>> conversion, but that is a separate and future topic, I think.\n>\n> It would be, and indeed, that is similar to what the final patch does.\n\nOK, I wasn't paying attention ;-)\n\n> The problem is that it requires an extra allocation (we do not want to\n> unconditionally put the object_context into the object_array because it\n> is too big, so we add only a pointer). So having rev_info->pending store\n> that information would mean that callers would have to know to free it\n> when freeing the pending array. We would have to either teach each\n> existing caller to do so, or perhaps enable the behavior only when a\n> certain flag is set (e.g., rev->keep_object_context or something).\n\nOne thing to notice is that those accessing rev->pending before\ncalling prepare_revision_walk(), as opposed to those receiving\nobjects in rev->commits via get_revision(), are the only ones that\ncare about the context and wants to act differently depending on\nwhere these came from and how they were specified.\n\nThat suggests at least two possibilities to me:\n\n - Perhaps we can place the context in rev->pending and clear them\n when prepare_revision_walk() moves them to rev->commits, without\n introducing rev->keep_object_context?\n\n - Perhaps instead of extending object-array, we can move this kind\n of information to rev_cmdline and enrich that structure?\n"},{"id":"216922","messageId":"7v7gj61z9h.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"dd973eae534bed5f7106d54e06c7c2172595f402.1368197380.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv3 7/7] grep: honor --textconv for the case rev:path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-10T18:11:06Z","receivedAt":"2013-05-10T18:11:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> diff --git a/object.h b/object.h\n> index 97d384b..695847d 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -13,6 +13,7 @@ struct object_array {\n> \t\tstruct object *item;\n> \t\tconst char *name;\n> \t\tunsigned mode;\n> +\t\tstruct object_context *context;\n> \t} *objects;\n> };\n\nfsck has to hold this for each and every objects in the repository\nit has found but hasn't inspected (i.e. pending), doesn't it? Do we\nreally want to add 8 bytes for each of them?\n"},{"id":"216924","messageId":"7v38tu1yb7.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"7v7gj61z9h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 7/7] grep: honor --textconv for the case rev:path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-10T18:31:40Z","receivedAt":"2013-05-10T18:31:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Michael J Gruber writes:\n>\n>> diff --git a/object.h b/object.h\n>> index 97d384b..695847d 100644\n>> --- a/object.h\n>> +++ b/object.h\n>> @@ -13,6 +13,7 @@ struct object_array {\n>> \t\tstruct object *item;\n>> \t\tconst char *name;\n>> \t\tunsigned mode;\n>> +\t\tstruct object_context *context;\n>> \t} *objects;\n>> };\n>\n> fsck has to hold this for each and every objects in the repository\n> it has found but hasn't inspected (i.e. pending), doesn't it? Do we\n> really want to add 8 bytes for each of them?\n\nPerhaps fsck does not even want \"name\" and \"mode\" for that matter.\n\nI wonder what improvement, if any, we would see with a change like\nthis patch in a large-ish repository.\n\n builtin/fsck.c | 35 ++++++++++++++++++++++++++++++-----\n 1 file changed, 30 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex bb9a2cd..c1de2a9 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -73,7 +73,32 @@ static int fsck_error_func(struct object *obj, int type, const char *err, ...)\n \treturn (type == FSCK_WARN) ? 0 : 1;\n }\n \n-static struct object_array pending;\n+static struct pending_object {\n+\tunsigned int nr;\n+\tunsigned int alloc;\n+\tstruct object **objects;\n+} pending;\n+\n+static int max_pending;\n+\n+static void add_pending(struct object *object)\n+{\n+\tunsigned nr = pending.nr;\n+\tunsigned alloc = pending.alloc;\n+\tstruct object **objects = pending.objects;\n+\n+\tif (nr >= alloc) {\n+\t\talloc = (alloc + 32) * 2;\n+\t\tobjects = xrealloc(objects, alloc * sizeof(*objects));\n+\t\tpending.alloc = alloc;\n+\t\tpending.objects = objects;\n+\t}\n+\tobjects[nr] = object;\n+\tpending.nr = ++nr;\n+\n+\tif (max_pending < nr)\n+\t\tmax_pending = nr;\n+}\n \n static int mark_object(struct object *obj, int type, void *data)\n {\n@@ -112,7 +137,7 @@ static int mark_object(struct object *obj, int type, void *data)\n \t\treturn 1;\n \t}\n \n-\tadd_object_array(obj, (void *) parent, &pending);\n+\tadd_pending(obj);\n \treturn 0;\n }\n \n@@ -148,15 +173,15 @@ static int traverse_reachable(void)\n \tif (show_progress)\n \t\tprogress = start_progress_delay(\"Checking connectivity\", 0, 0, 2);\n \twhile (pending.nr) {\n-\t\tstruct object_array_entry *entry;\n-\t\tstruct object *obj;\n+\t\tstruct object **entry, *obj;\n \n \t\tentry = pending.objects + --pending.nr;\n-\t\tobj = entry->item;\n+\t\tobj = *entry;\n \t\tresult |= traverse_one_object(obj);\n \t\tdisplay_progress(progress, ++nr);\n \t}\n \tstop_progress(&progress);\n+\tfprintf(stderr, \"max# pending objects = %d\\n\", max_pending);\n \treturn !!result;\n }\n \n"},{"id":"216956","messageId":"20130511002504.GA4849@sigill.intra.peff.net","threadId":"33545","inReplyTo":"7vfvxu1zla.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-11T00:25:05Z","receivedAt":"2013-05-11T00:25:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 10, 2013 at 11:04:01AM -0700, Junio C Hamano wrote:\n\n> One thing to notice is that those accessing rev->pending before\n> calling prepare_revision_walk(), as opposed to those receiving\n> objects in rev->commits via get_revision(), are the only ones that\n> care about the context and wants to act differently depending on\n> where these came from and how they were specified.\n> \n> That suggests at least two possibilities to me:\n> \n> - Perhaps we can place the context in rev->pending and clear them\n> when prepare_revision_walk() moves them to rev->commits, without\n> introducing rev->keep_object_context?\n> \n> - Perhaps instead of extending object-array, we can move this kind\n> of information to rev_cmdline and enrich that structure?\n\nWithout looking too closely to see whether it is feasible, I would think\nthe latter would end up being much more elegant, since I think it\nalready deals with some allocation issues already.\n\n-Peff\n"},{"id":"216980","messageId":"518E0741.1060008@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7vy5bm22f8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-05-11T08:54:25Z","receivedAt":"2013-05-11T08:54:25Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 10.05.2013 19:02:\n> Michael J Gruber writes:\n> \n>> Currently, \"diff\" and \"cat-file\" for blobs honor \"--textconv\" options\n>> (with the former defaulting to \"--textconv\" and the latter to\n>> \"--no-textconv\") whereas \"show\" does not honor this option, even though\n>> it takes diff options.\n>>\n>> Make \"show\" on blobs behave like \"diff\", i.e. honor \"--textconv\" by\n>> default and \"--no-textconv\" when given.\n> \n> Hmm...\n\nSorry, I overlooked that ;)\n\n>> +static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)\n>> {\n>> +\tunsigned char sha1c[20];\n>> +\tstruct object_context obj_context;\n>> +\tchar *buf;\n>> +\tunsigned long size;\n>> +\n>> \tfflush(stdout);\n>> -\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n>> +\tif (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||\n>> +\t !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))\n>> +\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n> \n> It is surprising that the necessary change is only this, but I think\n> it is correct ;-). We ignore textconv when the command line did not\n> mention --[no-]textconv, or the command line said --no-textconv\n> explicitly.\n> \n> This (especially the first condition) may deserve an in-code comment\n> for anybody who wonders where this default behaviour is implemented.\n\nIt's not as if we would document behavior by in-code comments in\ngeneral, do we? The usual answer is \"git log -S\" or \"git blame\".\n\n> So \"show\" on blobs does show the raw contents by default, but the\n> user can explicitly ask to enable textconv with --[no-]textconv. Is\n> the second paragraph in the log message still valid?\n> \n>> +\tif (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))\n>> +\t\tdie(\"Not a valid object name %s\", obj_name);\n> \n> This looks somewhat unfortunate.\n> \n> We already have sha1[]; actually we not just know sha1[] but have\n> the struct object for it. How did we obtain it before we got here?\n> \n> Will we always have a valid name in rev.pending.objects->name? Will\n> that name convert back to the same sha1 we got in sha1[]?\n> \n> I think the answers are \"Yes (it is a command line argument), Yes\n> (that is what setup_revisions() got by feeding the name to give us\n> sha1[])\".\n> \n> I wonder if enriching rev_info->pending with the context information\n> might be a clean solution to avoid this redundant but unavoidable\n> conversion, but that is a separate and future topic, I think.\n\nYes, I think both Jeff and I have thought it and came to the same\nconclusion - \"later\" ;)\n\n>> +\tif (!obj_context.path[0] ||\n>> +\t !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, &buf, &size))\n>> +\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n>> +\n>> +\tif (!buf)\n>> +\t\tdie(\"git show %s: bad file\", obj_name);\n>> +\n>> +\twrite_or_die(1, buf, size);\n>> +\treturn 0;\n>> }\n>> \n>> static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)\n>> @@ -526,7 +545,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>> \t\tconst char *name = objects[i].name;\n>> \t\tswitch (o->type) {\n>> \t\tcase OBJ_BLOB:\n>> -\t\t\tret = show_blob_object(o->sha1, NULL);\n>> +\t\t\tret = show_blob_object(o->sha1, &rev, name);\n>> \t\t\tbreak;\n>> \t\tcase OBJ_TAG: {\n>> \t\t\tstruct tag *t = (struct tag *)o;\n>> diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\n>> index 3950fc9..0ebb028 100755\n>> --- a/t/t4030-diff-textconv.sh\n>> +++ b/t/t4030-diff-textconv.sh\n>> @@ -96,14 +96,14 @@ test_expect_success 'show blob produces binary' '\n>> \ttest_cmp expect actual\n>> '\n>> \n>> -test_expect_failure 'show --textconv blob produces text' '\n>> +test_expect_success 'show --textconv blob produces text' '\n>> \tgit show --textconv HEAD:file >actual &&\n>> \tprintf \"0\\\\n1\\\\n\" >expect &&\n>> \ttest_cmp expect actual\n>> '\n>> \n>> -test_success 'show --no-textconv blob produces binary' '\n>> -\tgit show --textconv HEAD:file >actual &&\n>> +test_expect_success 'show --no-textconv blob produces binary' '\n>> +\tgit show --no-textconv HEAD:file >actual &&\n>> \tprintf \"\\\\0\\\\n\\\\01\\\\n\" >expect &&\n>> \ttest_cmp expect actual\n>> '\n"},{"id":"217076","messageId":"518E16B1.7000505@drmicha.warpmail.net","threadId":"33545","inReplyTo":"518E0741.1060008@drmicha.warpmail.net","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-05-11T10:00:17Z","receivedAt":"2013-05-11T10:00:17Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Adding to that:\n\nSomehow I still feel I should introduce a new attribute \"show\" (or a\nbetter name) similar to \"diff\" so that you can specifiy a diff driver to\nuse for showing a blob (or grepping it), which may or may not be the\nsame you use for \"diff\". This would be a much more fine-grained and\nsystematic way of setting a default for \"--textconv\" for blobs.\n\nOf course, some driver attributes would just not matter for coverting\nblobs, but that doesn't hurt.\n\nI'm just wondering whether it's worth the effort and whether I should\ndistinguish between \"show\" and grep\".\n\nSo, the sructure would be:\n\n\"--textconv\" is on by default for diff, show, grep.\n\ndiff looks for a textconv driver using the \"diff\" attribute.\nshow/grep look for a textconv driver using the \"show\" attribute.\n\nThat way, turning on \"--textconv\" by default does not affect anyone\nunless a user specifies the new attribute!\n\nAlso, all commands would behave \"the same way\" if you have both a diff\nand a show attribute set on the same files..\n\nMichael\n"},{"id":"217028","messageId":"7vr4hdxvtl.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"518E0741.1060008@drmicha.warpmail.net","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-11T17:36:38Z","receivedAt":"2013-05-11T17:36:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n>>> +\tif (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||\n>>> +\t !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))\n>>> +\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n>> \n>> It is surprising that the necessary change is only this, but I think\n>> it is correct ;-). We ignore textconv when the command line did not\n>> mention --[no-]textconv, or the command line said --no-textconv\n>> explicitly.\n>> \n>> This (especially the first condition) may deserve an in-code comment\n>> for anybody who wonders where this default behaviour is implemented.\n>\n> It's not as if we would document behavior by in-code comments in\n> general, do we? The usual answer is \"git log -S\" or \"git blame\".\n\nThe comment and the future reader I had in mind was more like\n\n\tDefault to --no-textconv, even though cmd_log_init_defaults()\n sets the bit, when the user did not explicitly ask for it.\n\nsought by somebody who wonders _where_ in the code we ignore\nALLOW_TEXTCONV that is set in cmd_log_init_defaults().\n\nThat is not something you can find with \"log -S\" or \"blame\", is it?\n"},{"id":"217037","messageId":"7vhai9wavg.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"20130511002504.GA4849@sigill.intra.peff.net","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-11T19:54:27Z","receivedAt":"2013-05-11T19:54:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, May 10, 2013 at 11:04:01AM -0700, Junio C Hamano wrote:\n>\n>> One thing to notice is that those accessing rev->pending before\n>> calling prepare_revision_walk(), as opposed to those receiving\n>> objects in rev->commits via get_revision(), are the only ones that\n>> care about the context and wants to act differently depending on\n>> where these came from and how they were specified.\n>> \n>> That suggests at least two possibilities to me:\n>> \n>> - Perhaps we can place the context in rev->pending and clear them\n>> when prepare_revision_walk() moves them to rev->commits, without\n>> introducing rev->keep_object_context?\n>> \n>> - Perhaps instead of extending object-array, we can move this kind\n>> of information to rev_cmdline and enrich that structure?\n>\n> Without looking too closely to see whether it is feasible, I would think\n> the latter would end up being much more elegant, since I think it\n> already deals with some allocation issues already.\n\nYeah. I am fairly reluctant to apply a change that makes entries in\nobject-array larger.\n"},{"id":"217077","messageId":"518F8785.4010401@drmicha.warpmail.net","threadId":"33545","inReplyTo":"7vr4hdxvtl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-05-12T12:13:57Z","receivedAt":"2013-05-12T12:13:57Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 11.05.2013 19:36:\n> Michael J Gruber writes:\n> \n>>>> +\tif (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||\n>>>> +\t !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))\n>>>> +\t\treturn stream_blob_to_fd(1, sha1, NULL, 0);\n>>>\n>>> It is surprising that the necessary change is only this, but I think\n>>> it is correct ;-). We ignore textconv when the command line did not\n>>> mention --[no-]textconv, or the command line said --no-textconv\n>>> explicitly.\n>>>\n>>> This (especially the first condition) may deserve an in-code comment\n>>> for anybody who wonders where this default behaviour is implemented.\n>>\n>> It's not as if we would document behavior by in-code comments in\n>> general, do we? The usual answer is \"git log -S\" or \"git blame\".\n> \n> The comment and the future reader I had in mind was more like\n> \n> \tDefault to --no-textconv, even though cmd_log_init_defaults()\n> sets the bit, when the user did not explicitly ask for it.\n> \n> sought by somebody who wonders _where_ in the code we ignore\n> ALLOW_TEXTCONV that is set in cmd_log_init_defaults().\n> \n> That is not something you can find with \"log -S\" or \"blame\", is it?\n> \n\nI'll refactor and restructure anyways. That will also get this whole\ndefault discussion out of the way:\n\nI'll try out the \"show attribute\" route as indicated. I'm not sure what\nto do about the object_array/context discussion, though.\n\nMichael\n"},{"id":"217117","messageId":"7vtxm7qxql.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"518E16B1.7000505@drmicha.warpmail.net","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-13T05:01:38Z","receivedAt":"2013-05-13T05:01:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Adding to that:\n>\n> Somehow I still feel I should introduce a new attribute \"show\" (or a\n> better name) similar to \"diff\" so that you can specifiy a diff driver to\n> use for showing a blob (or grepping it), which may or may not be the\n> same you use for \"diff\". This would be a much more fine-grained and\n> systematic way of setting a default for \"--textconv\" for blobs.\n>\n> Of course, some driver attributes would just not matter for coverting\n> blobs, but that doesn't hurt.\n>\n> I'm just wondering whether it's worth the effort and whether I should\n> distinguish between \"show\" and grep\".\n\nHaven't thought things through, but my gut feeling is that it is on\nthe other side of the line. We could of course add more features and\nover-engineered mechanisms, and the implementation may end up to be\neven modular and clean, but I cannot answer \"Yes\" with a confidence\nto the question \"Does such a fine grained control help the users?\"\nand cannot answer \"If so in what way?\" myself.\n"},{"id":"217152","messageId":"20130513115451.GA3903@sigill.intra.peff.net","threadId":"33545","inReplyTo":"7vtxm7qxql.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-13T11:55:08Z","receivedAt":"2013-05-13T11:55:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 12, 2013 at 10:01:38PM -0700, Junio C Hamano wrote:\n\n> Michael J Gruber writes:\n> \n> > Adding to that:\n> >\n> > Somehow I still feel I should introduce a new attribute \"show\" (or a\n> > better name) similar to \"diff\" so that you can specifiy a diff driver to\n> > use for showing a blob (or grepping it), which may or may not be the\n> > same you use for \"diff\". This would be a much more fine-grained and\n> > systematic way of setting a default for \"--textconv\" for blobs.\n> >\n> > Of course, some driver attributes would just not matter for coverting\n> > blobs, but that doesn't hurt.\n> >\n> > I'm just wondering whether it's worth the effort and whether I should\n> > distinguish between \"show\" and grep\".\n> \n> Haven't thought things through, but my gut feeling is that it is on\n> the other side of the line. We could of course add more features and\n> over-engineered mechanisms, and the implementation may end up to be\n> even modular and clean, but I cannot answer \"Yes\" with a confidence\n> to the question \"Does such a fine grained control help the users?\"\n> and cannot answer \"If so in what way?\" myself.\n\nYeah, I think the _most_ flexible thing is going to look something like:\n\n $ cat .gitattributes\n *.pdf diff=pdf show=pdf\n\n $ cat ~/.gitconfig\n [diff \"pdf\"]\n textconv = ...\n [show \"pdf\"]\n textconv = ...\n\nBut that obviously sucks, because in the common case that you want to\nuse the same command, you are repeating yourself in the config. You\ncould assume that the \"show\" attribute points us at a \"diff\" block. And\nthat makes sense for textconv, but what does it mean if you have\n\"show=foo\" and \"diff.foo.command\" set?\n\nIf the _only_ thing you would want to do with such a \"show\" mechanism is\nto display converted contents on show/grep, then we could lose the\nflexibility and say that \"show\" is a single-bit: do we respect diff\ntextconv for show/grep in this case, or not? And that leaves only the\nquestion of where to put it: is it a gitattribute, or does it go in the\nconfig?\n\nI don't think that it is a property of the file itself. That is, you do\nnot say \"foo files are inherently uninteresting to git-show, and\ntherefore we always convert them, whereas bar files do not have that\nproperty'. You say \"in my workflows, I expect to see converted results\nfrom grep/show\". And the latter points to using config, like either\n\"diff.*.showConverted\" (to allow per-type setting), or even\n\"grep.useTextconv\" and \"show.textConv\" (to allow setting it per-user for\nall types).\n\nAnd of course for any workflow-oriented config, you will sometimes want\nto override it for a particular operation. But that is why we have a\ncommand-line escape hatch, and that part is already implemented.\n\n-Peff\n"},{"id":"217191","messageId":"5190FF73.1080606@drmicha.warpmail.net","threadId":"33545","inReplyTo":"20130513115451.GA3903@sigill.intra.peff.net","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-05-13T14:57:55Z","receivedAt":"2013-05-13T14:57:55Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 13.05.2013 13:55:\n> On Sun, May 12, 2013 at 10:01:38PM -0700, Junio C Hamano wrote:\n> \n>> Michael J Gruber writes:\n>>\n>>> Adding to that:\n>>>\n>>> Somehow I still feel I should introduce a new attribute \"show\" (or a\n>>> better name) similar to \"diff\" so that you can specifiy a diff driver to\n>>> use for showing a blob (or grepping it), which may or may not be the\n>>> same you use for \"diff\". This would be a much more fine-grained and\n>>> systematic way of setting a default for \"--textconv\" for blobs.\n>>>\n>>> Of course, some driver attributes would just not matter for coverting\n>>> blobs, but that doesn't hurt.\n>>>\n>>> I'm just wondering whether it's worth the effort and whether I should\n>>> distinguish between \"show\" and grep\".\n>>\n>> Haven't thought things through, but my gut feeling is that it is on\n>> the other side of the line. We could of course add more features and\n>> over-engineered mechanisms, and the implementation may end up to be\n>> even modular and clean, but I cannot answer \"Yes\" with a confidence\n>> to the question \"Does such a fine grained control help the users?\"\n>> and cannot answer \"If so in what way?\" myself.\n> \n> Yeah, I think the _most_ flexible thing is going to look something like:\n> \n> $ cat .gitattributes\n> *.pdf diff=pdf show=pdf\n> \n> $ cat ~/.gitconfig\n> [diff \"pdf\"]\n> textconv = ...\n> [show \"pdf\"]\n> textconv = ...\n> \n> But that obviously sucks, because in the common case that you want to\n> use the same command, you are repeating yourself in the config. You\n> could assume that the \"show\" attribute points us at a \"diff\" block. And\n> that makes sense for textconv, but what does it mean if you have\n> \"show=foo\" and \"diff.foo.command\" set?\n\nI don't propose \"show drivers\". In your example above, you would point\nto the same diff driver.\n\nIf you use a diff driver just with the \"show\" attribute then only its\ntextconv config will be relevant.\n\nBut you do have the possibility to use different drivers for diff and\nshow. For example, for showing a file some sort of automatic pagination\nor line numbering can be helpful whereas it would hurt the diff case.\n\n> If the _only_ thing you would want to do with such a \"show\" mechanism is\n> to display converted contents on show/grep, then we could lose the\n> flexibility and say that \"show\" is a single-bit: do we respect diff\n> textconv for show/grep in this case, or not? And that leaves only the\n> question of where to put it: is it a gitattribute, or does it go in the\n> config?\n> \n> I don't think that it is a property of the file itself. That is, you do\n> not say \"foo files are inherently uninteresting to git-show, and\n> therefore we always convert them, whereas bar files do not have that\n> property'. You say \"in my workflows, I expect to see converted results\n> from grep/show\". And the latter points to using config, like either\n> \"diff.*.showConverted\" (to allow per-type setting), or even\n> \"grep.useTextconv\" and \"show.textConv\" (to allow setting it per-user for\n> all types).\n\nI strongly disagree here. I have textconv filters for pdf, gpg, odf,\nxls, doc, xoj... I know, ugly. At least some of them would benefit from\ndifferent filteres or different settings.\n\nThe way I propose it, a user would just have to add \"show=foo\" to the\n\"diff=foo\" lines without having to ad an extra filter, but with the\nflexibility to do so.\n\n> And of course for any workflow-oriented config, you will sometimes want\n> to override it for a particular operation. But that is why we have a\n> command-line escape hatch, and that part is already implemented.\n\nOne may ask what a purely ui output oriented setting like \"show\" has to\ndo in .gitattributes, of course, but that applies to \"diff\" as well.\nSeparating the two (one in attributes, one in config) looks artificial\nto me.\n\nMichael\n"},{"id":"217200","messageId":"7vhai6opju.fsf@alter.siamese.dyndns.org","threadId":"33545","inReplyTo":"5190FF73.1080606@drmicha.warpmail.net","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-13T15:41:25Z","receivedAt":"2013-05-13T15:41:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> But you do have the possibility to use different drivers for diff and\n> show. For example, for showing a file some sort of automatic pagination\n> or line numbering can be helpful whereas it would hurt the diff case.\n\nI do not find the example convincing (yet); it looks more like you\nare grasping for straws.\n\nYou would certainly do not want \"line numbering\" in grep. My gut\nfeeling is that normal users would expect to have a single \"text\nversion\" and pass that to \"pr\" (if they want pagination) or \"cat -n\"\n(if they want line numbering), regardless of where it comes from, be\nit \"git show --textconv\" or some other program output, but you seem\nto want to have different \"text version\"s for different purposes out\nof a single binary file....\n\n> I strongly disagree here. I have textconv filters for pdf, gpg, odf,\n> xls, doc, xoj... I know, ugly. At least some of them would benefit from\n> different filteres or different settings.\n\n.... and an example to show why it is useful would help here. I do\nnot feel that I have seen anything to substantiate \"at least some of\nthem would benefit\" yet.\n\nWould it follow that \"grep\" and \"cat-file\" should be controlled by\nyet two other knobs so that optionally the user can use different\n\"text version\"s meant for them?\n\n> The way I propose it, a user would just have to add \"show=foo\" to the\n> \"diff=foo\" lines without having to ad an extra filter, but with the\n> flexibility to do so.\n>\n>> And of course for any workflow-oriented config, you will sometimes want\n>> to override it for a particular operation. But that is why we have a\n>> command-line escape hatch, and that part is already implemented.\n>\n> One may ask what a purely ui output oriented setting like \"show\" has to\n> do in .gitattributes, of course, but that applies to \"diff\" as well.\n> Separating the two (one in attributes, one in config) looks artificial\n> to me.\n\nI am not sure what you mean by \"artificial\", but the separation of\nthe roles between attribute and config is not artificial at all. It\nis very much deliberate and done for a good reason.\n\nThe attribute specifies what the type of the file is project wide\nand is meant to go in in-tree .gitattrbute file, shared among people\non different platforms. It says things like \"These files are PDF\".\n\nThe config specifies what should happen to the type of a file on a\nparticular platform each user uses to work in the copy of the\nproject, i.e. repository. It says things like \"Pass PDF files\nthrough /opt/bin/pdf2txt\", which obviously cannot be shared across\nplatforms.\n"},{"id":"217497","messageId":"20130516033151.GB13296@sigill.intra.peff.net","threadId":"33545","inReplyTo":"5190FF73.1080606@drmicha.warpmail.net","subject":"Re: [PATCHv3 3/7] show: honor --textconv for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-16T03:31:51Z","receivedAt":"2013-05-16T03:31:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 13, 2013 at 04:57:55PM +0200, Michael J Gruber wrote:\n\n> > I don't think that it is a property of the file itself. That is, you do\n> > not say \"foo files are inherently uninteresting to git-show, and\n> > therefore we always convert them, whereas bar files do not have that\n> > property'. You say \"in my workflows, I expect to see converted results\n> > from grep/show\". And the latter points to using config, like either\n> > \"diff.*.showConverted\" (to allow per-type setting), or even\n> > \"grep.useTextconv\" and \"show.textConv\" (to allow setting it per-user for\n> > all types).\n> \n> I strongly disagree here. I have textconv filters for pdf, gpg, odf,\n> xls, doc, xoj... I know, ugly. At least some of them would benefit from\n> different filteres or different settings.\n\nOK. I was speaking mostly from intuition, and I suspect you have more\nreal-world experience here. So I am willing to admit that my \"you do not\nsay...\" above was a strawman. :)\n\n> The way I propose it, a user would just have to add \"show=foo\" to the\n> \"diff=foo\" lines without having to ad an extra filter, but with the\n> flexibility to do so.\n\nYes, I think that would work OK. The only problem is that it is a bit\nweird to pointing \"show=foo\" to \"diff.foo.*\", especially when most of\nthe driver options are ignored. But if we can accept that wrinkle in the\nUI, I think it would otherwise do what users want.\n\n> One may ask what a purely ui output oriented setting like \"show\" has to\n> do in .gitattributes, of course, but that applies to \"diff\" as well.\n> Separating the two (one in attributes, one in config) looks artificial\n> to me.\n\nI think the point is that the attribute says \"a property of this path is\nthat it has type X\". And then the config says \"when you see type X, do\nthis thing with it\".\n\nSo arguably \"diff=X\" is wrong in the first place. It should be \"type=X\",\nand we should have \"diff.X\", \"merge.X\", etc in the config. And\ndiff.*.textconv is potentially misplaced; it is not really about diffing\nat all, but rather about creating a human-readable presentation for the\nfile. I don't think it is so bad that it is worth the pain of fixing it\nnow, though. It is a historical weirdness that \"diff=X\" means \"present\nthe path according to the rules in X\", but we can live with that.\n\nBut if we think of it that way, then automatically respecting textconv\nfor \"git show\" is a sensible thing to do. Hmph. Now I may have convinced\nmyself that flipping the default is the right thing. :)\n\nSo if it is not clear, I am pretty on the fence about how the defaults\nshould be handled, or what would surprise users the least. Either way,\nthough, it would probably make sense to have a configurable option. And\nwith the reasoning above for the split between attributes/config, it\nwould make sense to me for that option to be a boolean\n\"diff.X.showtextconv\". Which seems totally odd and broken (we are not\ndoing a diff at all!), but that is where the textconv config lives, for\nhistorical reasons.\n\n-Peff\n"}]}