{"thread":{"id":"32824","subject":"[WIP/RFH/RFD/PATCH] grep: allow to use textconv filters","startedAt":"2013-02-04T15:27:31Z","lastAt":"2013-02-08T11:27:44Z","messageCount":37,"participants":["Michael J Gruber","Junio C Hamano","Jeff King","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"208612","messageId":"2c0641ea4df6a872a4466efe0c0124f304f44c3e.1359991521.git.git@drmicha.warpmail.net","threadId":"32824","inReplyTo":null,"subject":"[WIP/RFH/RFD/PATCH] grep: allow to use textconv filters","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-04T15:27:31Z","receivedAt":"2013-02-04T15:27:31Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Recently 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 a binary mode \"--textconv\" (in addition to \"--text\" and \"-I\")\nwhich makes git grep use any configured textconv filters for grepping\nand output purposes.\n\nSigned-off-by: Michael J Gruber \n---\n\nNotes:\n I'm somehow stuck in textconv/filespec/... hell, so I'm sending this out\n in request for help. I'm sure there are people for whom it's a breeze to\n get this right.\n \n The difficulty is in getting the different cases (blob/sha1 vs.\n worktree) right, and in making the changes minimally invasive. It seems\n that some more refactoring could help: \"git show --textconv\" does not\n use textconv filters when used on blobs either. (It does for diffs, of\n course.) Most existing helper functions are tailored for diffs.\n\n Nota bene: --textconv does not affect \"diff --stat\" either...\n\n builtin/grep.c | 5 ++++-\n grep.c | 47 +++++++++++++++++++++++++++++------------------\n grep.h | 3 ++-\n 3 files changed, 35 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 8025964..2181c22 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -96,7 +96,7 @@ static void add_work(struct grep_opt *opt, enum grep_source_type type,\n \n \tgrep_source_init(&todo[todo_end].source, type, name, path, id);\n \tif (opt->binary != GREP_BINARY_TEXT)\n-\t\tgrep_source_load_driver(&todo[todo_end].source);\n+\t\tgrep_source_load_driver(&todo[todo_end].source, opt);\n \ttodo[todo_end].done = 0;\n \tstrbuf_reset(&todo[todo_end].out);\n \ttodo_end = (todo_end + 1) % ARRAY_SIZE(todo);\n@@ -659,6 +659,9 @@ 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_SET_INT(0, \"textconv\", &opt.binary,\n+\t\t\tN_(\"process binary files with textconv filters\"),\n+\t\t\tGREP_BINARY_TEXTCONV),\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 4bd1b8b..410b7b8 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1,10 +1,12 @@\n #include \"cache.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n #include \"grep.h\"\n #include \"userdiff.h\"\n #include \"xdiff-interface.h\"\n \n-static int grep_source_load(struct grep_source *gs);\n-static int grep_source_is_binary(struct grep_source *gs);\n+static int grep_source_load(struct grep_source *gs, struct grep_opt *opt);\n+static int grep_source_is_binary(struct grep_source *gs, struct grep_opt *opt);\n \n static struct grep_opt grep_defaults;\n \n@@ -1174,7 +1176,7 @@ static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bo\n {\n \txdemitconf_t *xecfg = opt->priv;\n \tif (xecfg && !xecfg->find_func) {\n-\t\tgrep_source_load_driver(gs);\n+\t\tgrep_source_load_driver(gs, opt);\n \t\tif (gs->driver->funcname.pattern) {\n \t\t\tconst struct userdiff_funcname *pe = &gs->driver->funcname;\n \t\t\txdiff_set_find_func(xecfg, pe->pattern, pe->cflags);\n@@ -1354,14 +1356,15 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \n \tswitch (opt->binary) {\n \tcase GREP_BINARY_DEFAULT:\n-\t\tif (grep_source_is_binary(gs))\n+\t\tif (grep_source_is_binary(gs, opt))\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\tif (grep_source_is_binary(gs, opt))\n \t\t\treturn 0; /* Assume unmatch */\n \t\tbreak;\n \tcase GREP_BINARY_TEXT:\n+\tcase GREP_BINARY_TEXTCONV:\n \t\tbreak;\n \tdefault:\n \t\tdie(\"bug: unknown binary handling mode\");\n@@ -1372,7 +1375,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 (grep_source_load(gs, opt) < 0)\n \t\treturn 0;\n \n \tbol = gs->buf;\n@@ -1610,12 +1613,17 @@ void grep_source_clear_data(struct grep_source *gs)\n \t}\n }\n \n-static int grep_source_load_sha1(struct grep_source *gs)\n+static int grep_source_load_sha1(struct grep_source *gs, struct grep_opt *opt)\n {\n \tenum object_type type;\n-\n \tgrep_read_lock();\n-\tgs->buf = read_sha1_file(gs->identifier, &type, &gs->size);\n+\tif (opt->binary == GREP_BINARY_TEXTCONV) {\n+\t\tstruct diff_filespec *df = alloc_filespec(gs->name);\n+\t\tgs->size = fill_textconv(gs->driver, df, &gs->buf);\n+\t\tfree_filespec(df);\n+\t} else {\n+\t\tgs->buf = read_sha1_file(gs->identifier, &type, &gs->size);\n+\t}\n \tgrep_read_unlock();\n \n \tif (!gs->buf)\n@@ -1625,7 +1633,7 @@ static int grep_source_load_sha1(struct grep_source *gs)\n \treturn 0;\n }\n \n-static int grep_source_load_file(struct grep_source *gs)\n+static int grep_source_load_file(struct grep_source *gs, struct grep_opt *opt)\n {\n \tconst char *filename = gs->identifier;\n \tstruct stat st;\n@@ -1660,42 +1668,45 @@ static int grep_source_load_file(struct grep_source *gs)\n \treturn 0;\n }\n \n-static int grep_source_load(struct grep_source *gs)\n+static int grep_source_load(struct grep_source *gs, struct grep_opt *opt)\n {\n \tif (gs->buf)\n \t\treturn 0;\n \n \tswitch (gs->type) {\n \tcase GREP_SOURCE_FILE:\n-\t\treturn grep_source_load_file(gs);\n+\t\treturn grep_source_load_file(gs, opt);\n \tcase GREP_SOURCE_SHA1:\n-\t\treturn grep_source_load_sha1(gs);\n+\t\treturn grep_source_load_sha1(gs, opt);\n \tcase GREP_SOURCE_BUF:\n \t\treturn gs->buf ? 0 : -1;\n \t}\n \tdie(\"BUG: invalid grep_source type\");\n }\n \n-void grep_source_load_driver(struct grep_source *gs)\n+void grep_source_load_driver(struct grep_source *gs, struct grep_opt *opt)\n {\n \tif (gs->driver)\n \t\treturn;\n \n-\tgrep_attr_lock();\n+\tgrep_attr_lock(); //TODO\n+\tprintf(\"Looking up userdiff driver for: %s\", gs->path);\n \tif (gs->path)\n \t\tgs->driver = userdiff_find_by_path(gs->path);\n \tif (!gs->driver)\n \t\tgs->driver = userdiff_find_by_name(\"default\");\n+\tif (opt->binary == GREP_BINARY_TEXTCONV)\n+\t\tgs->driver = userdiff_get_textconv(gs->driver);\n \tgrep_attr_unlock();\n }\n \n-static int grep_source_is_binary(struct grep_source *gs)\n+static int grep_source_is_binary(struct grep_source *gs, struct grep_opt *opt)\n {\n-\tgrep_source_load_driver(gs);\n+\tgrep_source_load_driver(gs, opt);\n \tif (gs->driver->binary != -1)\n \t\treturn gs->driver->binary;\n \n-\tif (!grep_source_load(gs))\n+\tif (!grep_source_load(gs, opt))\n \t\treturn buffer_is_binary(gs->buf, gs->size);\n \n \treturn 0;\ndiff --git a/grep.h b/grep.h\nindex 8fc854f..d272d25 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -105,6 +105,7 @@ struct grep_opt {\n #define GREP_BINARY_DEFAULT\t0\n #define GREP_BINARY_NOMATCH\t1\n #define GREP_BINARY_TEXT\t2\n+#define GREP_BINARY_TEXTCONV\t3\n \tint binary;\n \tint extended;\n \tint use_reflog_filter;\n@@ -173,7 +174,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n \t\t const void *identifier);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n-void grep_source_load_driver(struct grep_source *gs);\n+void grep_source_load_driver(struct grep_source *gs, struct grep_opt *opt);\n \n \n int grep_source(struct grep_opt *opt, struct grep_source *gs);\n-- \n1.8.1.2.718.g9d378fc\n"},{"id":"208626","messageId":"7vtxps2eb2.fsf@alter.siamese.dyndns.org","threadId":"32824","inReplyTo":"2c0641ea4df6a872a4466efe0c0124f304f44c3e.1359991521.git.git@drmicha.warpmail.net","subject":"Re: [WIP/RFH/RFD/PATCH] grep: allow to use textconv filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-04T17:12:49Z","receivedAt":"2013-02-04T17:12:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\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 a binary mode \"--textconv\" (in addition to \"--text\" and \"-I\")\n> which makes git grep use any configured textconv filters for grepping\n> and output purposes.\n>\n> Signed-off-by: Michael J Gruber \n> ---\n>\n> Notes:\n> I'm somehow stuck in textconv/filespec/... hell, so I'm sending this out\n> in request for help. I'm sure there are people for whom it's a breeze to\n> get this right.\n\nLooks like the patch touches the right places in the codepaths that\nneed to be updated from a quick read.\n\n> The difficulty is in getting the different cases (blob/sha1 vs.\n> worktree) right, and in making the changes minimally invasive.\n\nI think grep_source abstraction was intended to be a way for that,\nand if what it gives you is not sufficient for your needs, extending\nit should not be seen as \"invasive\" at all.\n"},{"id":"208693","messageId":"5110C745.9050306@drmicha.warpmail.net","threadId":"32824","inReplyTo":"7vtxps2eb2.fsf@alter.siamese.dyndns.org","subject":"Re: [WIP/RFH/RFD/PATCH] grep: allow to use textconv filters","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-05T08:48:05Z","receivedAt":"2013-02-05T08:48:05Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 04.02.2013 18:12:\n> Michael J Gruber writes:\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 a binary mode \"--textconv\" (in addition to \"--text\" and \"-I\")\n>> which makes git grep use any configured textconv filters for grepping\n>> and output purposes.\n>>\n>> Signed-off-by: Michael J Gruber \n>> ---\n>>\n>> Notes:\n>> I'm somehow stuck in textconv/filespec/... hell, so I'm sending this out\n>> in request for help. I'm sure there are people for whom it's a breeze to\n>> get this right.\n> \n> Looks like the patch touches the right places in the codepaths that\n> need to be updated from a quick read.\n\nI should have added that this coredumps when used on blobs and that\ntextconv isn't invoked in any case (because it crashes on blobs, and I\nhaven't implemented tetxconv on worktree files yet).\n\nI'm somehow struggling to use just the right bits from the more diff\ncentric helpers like fill_textconv or fill_one (which is static so far).\n\n>> The difficulty is in getting the different cases (blob/sha1 vs.\n>> worktree) right, and in making the changes minimally invasive.\n> \n> I think grep_source abstraction was intended to be a way for that,\n> and if what it gives you is not sufficient for your needs, extending\n> it should not be seen as \"invasive\" at all.\n\nIt seems to me that we textconvified the diff versions of these\nabstractions, but not the grep_source abstractions. Doing it at the\nsource appears to be the right thing.\n\nMichael\n"},{"id":"208703","messageId":"20130205111353.GD24973@sigill.intra.peff.net","threadId":"32824","inReplyTo":"2c0641ea4df6a872a4466efe0c0124f304f44c3e.1359991521.git.git@drmicha.warpmail.net","subject":"Re: [WIP/RFH/RFD/PATCH] grep: allow to use textconv filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-05T11:13:54Z","receivedAt":"2013-02-05T11:13:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 04, 2013 at 04:27:31PM +0100, Michael J Gruber wrote:\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 a binary mode \"--textconv\" (in addition to \"--text\" and \"-I\")\n> which makes git grep use any configured textconv filters for grepping\n> and output purposes.\n\nSounds like a reasonable goal.\n\n> The difficulty is in getting the different cases (blob/sha1 vs.\n> worktree) right, and in making the changes minimally invasive. It seems\n> that some more refactoring could help: \"git show --textconv\" does not\n> use textconv filters when used on blobs either. (It does for diffs, of\n> course.) Most existing helper functions are tailored for diffs.\n\nI think \"git show\" with blobs originally did not because we have no\nfilename with which to look up the attributes. IIRC, the patches to\nsupport \"cat-file --textconv\" taught get_sha1_with_context to report the\npath at which we found a blob. I suspect it is mostly a matter of\nplumbing that information from the revision parser through to\nshow_blob_object.\n\n> Nota bene: --textconv does not affect \"diff --stat\" either...\n\nYeah, though I wonder if it should be on by default. The diffstat for a\nbinary file, unlike the diff, is already useful. The diffstat of the\ntextconv'd data may _also_ be useful, of course.\n\n> @@ -659,6 +659,9 @@ 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_SET_INT(0, \"textconv\", &opt.binary,\n> +\t\t\tN_(\"process binary files with textconv filters\"),\n> +\t\t\tGREP_BINARY_TEXTCONV),\n\nIs this really a new form of GREP_BINARY_*? What happens when a file\ndoes not have a textconv filter?\n\nI would expect this to be more like diff's \"--textconv\" flag, which is\nreally \"allow textconv to be respected\". Then you could do:\n\n git grep -I --textconv foo\n\nto grep in the text version of files which support it, and ignore the\nrest.\n\n> -static int grep_source_load(struct grep_source *gs);\n> -static int grep_source_is_binary(struct grep_source *gs);\n> +static int grep_source_load(struct grep_source *gs, struct grep_opt *opt);\n> +static int grep_source_is_binary(struct grep_source *gs, struct grep_opt *opt);\n\nHmm. grep_source_load is more or less the analogue of\ndiff_populate_filespec, which does not know about textconv at all. So I\nfeel like this might be going in at the wrong layer...\n\n> @@ -1354,14 +1356,15 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n> \n> \tswitch (opt->binary) {\n> \tcase GREP_BINARY_DEFAULT:\n> -\t\tif (grep_source_is_binary(gs))\n> +\t\tif (grep_source_is_binary(gs, opt))\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\tif (grep_source_is_binary(gs, opt))\n> \t\t\treturn 0; /* Assume unmatch */\n> \t\tbreak;\n\nThe is_binary function learned about \"opt\" so that it could pass it to\ngrep_source_load, which might do the textconv for us. But we _know_ that\nwe will not here, because we see that we have other GREP_BINARY flags.\n\nAnd when we do have _TEXTCONV:\n\n> \tcase GREP_BINARY_TEXT:\n> +\tcase GREP_BINARY_TEXTCONV:\n> \t\tbreak;\n\nWe do not call is_binary. :)\n\n> -static int grep_source_load_sha1(struct grep_source *gs)\n> +static int grep_source_load_sha1(struct grep_source *gs, struct grep_opt *opt)\n> {\n> \tenum object_type type;\n> -\n> \tgrep_read_lock();\n> -\tgs->buf = read_sha1_file(gs->identifier, &type, &gs->size);\n> +\tif (opt->binary == GREP_BINARY_TEXTCONV) {\n> +\t\tstruct diff_filespec *df = alloc_filespec(gs->name);\n> +\t\tgs->size = fill_textconv(gs->driver, df, &gs->buf);\n> +\t\tfree_filespec(df);\n> +\t} else {\n> +\t\tgs->buf = read_sha1_file(gs->identifier, &type, &gs->size);\n> +\t}\n\nSo here we do the textconv for the sha1 case. But what about file\nsources?\n\nThis is why I think the layer is wrong; you want the fill_textconv\nfunction to call your abstract _load function (in the diff_filespec\nworld, it is diff_filespec_populate, but it is the moral equivalent).\nAnd you want it to hold off as long as possible in case we can pull the\nvalue from cache, or feed the working tree version of a file straight to\nthe filter.\n\n> -void grep_source_load_driver(struct grep_source *gs)\n> +void grep_source_load_driver(struct grep_source *gs, struct grep_opt *opt)\n> {\n> \tif (gs->driver)\n> \t\treturn;\n> \n> -\tgrep_attr_lock();\n> +\tgrep_attr_lock(); //TODO\n> +\tprintf(\"Looking up userdiff driver for: %s\", gs->path);\n> \tif (gs->path)\n> \t\tgs->driver = userdiff_find_by_path(gs->path);\n> \tif (!gs->driver)\n> \t\tgs->driver = userdiff_find_by_name(\"default\");\n> +\tif (opt->binary == GREP_BINARY_TEXTCONV)\n> +\t\tgs->driver = userdiff_get_textconv(gs->driver);\n> \tgrep_attr_unlock();\n> }\n\nThis is wrong. The point of userdiff_get_textconv is that it will return\nNULL when we are not doing textconv for this path. So you can use it\nlike:\n\n struct userdiff_driver *textconv = userdiff_get_textconv(gs->driver);\n\n if (textconv) {\n /* ok, we are doing textconv. Call our fill_textconv\n * equivalent. */\n }\n else {\n /* nope, plain old file. */\n }\n\nBut by assigning it on top of gs->driver, you're going to end up with a\nNULL driver sometimes. And the post-condition of the load_driver\nfunction is that gs->driver always points to a valid driver (even if it\nis the default one). I wouldn't be surprised if this causes segfaults.\n\n\nSo I would do it more like the patch below. Only lightly tested by me.\nThere are some refactoring opportunities if you want to bring\ngrep_source and diff_filespec closer together.\n\n---\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 8025964..915c8ef 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 4bd1b8b..3880d64 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@@ -1321,6 +1323,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@@ -1331,6 +1385,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@@ -1352,19 +1407,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@@ -1372,7 +1444,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 8fc854f..94a7ac2 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -106,6 +106,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;\n"},{"id":"208742","messageId":"5111317E.8060906@drmicha.warpmail.net","threadId":"32824","inReplyTo":"20130205111353.GD24973@sigill.intra.peff.net","subject":"Re: [WIP/RFH/RFD/PATCH] grep: allow to use textconv filters","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-05T16:21:18Z","receivedAt":"2013-02-05T16:21:18Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 05.02.2013 12:13:\n> On Mon, Feb 04, 2013 at 04:27:31PM +0100, Michael J Gruber wrote:\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 a binary mode \"--textconv\" (in addition to \"--text\" and \"-I\")\n>> which makes git grep use any configured textconv filters for grepping\n>> and output purposes.\n> \n> Sounds like a reasonable goal.\n> \n>> The difficulty is in getting the different cases (blob/sha1 vs.\n>> worktree) right, and in making the changes minimally invasive. It seems\n>> that some more refactoring could help: \"git show --textconv\" does not\n>> use textconv filters when used on blobs either. (It does for diffs, of\n>> course.) Most existing helper functions are tailored for diffs.\n> \n> I think \"git show\" with blobs originally did not because we have no\n> filename with which to look up the attributes. IIRC, the patches to\n> support \"cat-file --textconv\" taught get_sha1_with_context to report the\n> path at which we found a blob. I suspect it is mostly a matter of\n> plumbing that information from the revision parser through to\n> show_blob_object.\n> \n>> Nota bene: --textconv does not affect \"diff --stat\" either...\n> \n> Yeah, though I wonder if it should be on by default. The diffstat for a\n> binary file, unlike the diff, is already useful. The diffstat of the\n> textconv'd data may _also_ be useful, of course.\n> \n>> @@ -659,6 +659,9 @@ 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_SET_INT(0, \"textconv\", &opt.binary,\n>> +\t\t\tN_(\"process binary files with textconv filters\"),\n>> +\t\t\tGREP_BINARY_TEXTCONV),\n> \n> Is this really a new form of GREP_BINARY_*? What happens when a file\n> does not have a textconv filter?\n> \n> I would expect this to be more like diff's \"--textconv\" flag, which is\n> really \"allow textconv to be respected\". Then you could do:\n> \n> git grep -I --textconv foo\n> \n> to grep in the text version of files which support it, and ignore the\n> rest.\n> \n>> -static int grep_source_load(struct grep_source *gs);\n>> -static int grep_source_is_binary(struct grep_source *gs);\n>> +static int grep_source_load(struct grep_source *gs, struct grep_opt *opt);\n>> +static int grep_source_is_binary(struct grep_source *gs, struct grep_opt *opt);\n> \n> Hmm. grep_source_load is more or less the analogue of\n> diff_populate_filespec, which does not know about textconv at all. So I\n> feel like this might be going in at the wrong layer...\n> \n>> @@ -1354,14 +1356,15 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n>> \n>> \tswitch (opt->binary) {\n>> \tcase GREP_BINARY_DEFAULT:\n>> -\t\tif (grep_source_is_binary(gs))\n>> +\t\tif (grep_source_is_binary(gs, opt))\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\tif (grep_source_is_binary(gs, opt))\n>> \t\t\treturn 0; /* Assume unmatch */\n>> \t\tbreak;\n> \n> The is_binary function learned about \"opt\" so that it could pass it to\n> grep_source_load, which might do the textconv for us. But we _know_ that\n> we will not here, because we see that we have other GREP_BINARY flags.\n> \n> And when we do have _TEXTCONV:\n> \n>> \tcase GREP_BINARY_TEXT:\n>> +\tcase GREP_BINARY_TEXTCONV:\n>> \t\tbreak;\n> \n> We do not call is_binary. :)\n> \n>> -static int grep_source_load_sha1(struct grep_source *gs)\n>> +static int grep_source_load_sha1(struct grep_source *gs, struct grep_opt *opt)\n>> {\n>> \tenum object_type type;\n>> -\n>> \tgrep_read_lock();\n>> -\tgs->buf = read_sha1_file(gs->identifier, &type, &gs->size);\n>> +\tif (opt->binary == GREP_BINARY_TEXTCONV) {\n>> +\t\tstruct diff_filespec *df = alloc_filespec(gs->name);\n>> +\t\tgs->size = fill_textconv(gs->driver, df, &gs->buf);\n>> +\t\tfree_filespec(df);\n>> +\t} else {\n>> +\t\tgs->buf = read_sha1_file(gs->identifier, &type, &gs->size);\n>> +\t}\n> \n> So here we do the textconv for the sha1 case. But what about file\n> sources?\n> \n> This is why I think the layer is wrong; you want the fill_textconv\n> function to call your abstract _load function (in the diff_filespec\n> world, it is diff_filespec_populate, but it is the moral equivalent).\n> And you want it to hold off as long as possible in case we can pull the\n> value from cache, or feed the working tree version of a file straight to\n> the filter.\n> \n>> -void grep_source_load_driver(struct grep_source *gs)\n>> +void grep_source_load_driver(struct grep_source *gs, struct grep_opt *opt)\n>> {\n>> \tif (gs->driver)\n>> \t\treturn;\n>> \n>> -\tgrep_attr_lock();\n>> +\tgrep_attr_lock(); //TODO\n>> +\tprintf(\"Looking up userdiff driver for: %s\", gs->path);\n>> \tif (gs->path)\n>> \t\tgs->driver = userdiff_find_by_path(gs->path);\n>> \tif (!gs->driver)\n>> \t\tgs->driver = userdiff_find_by_name(\"default\");\n>> +\tif (opt->binary == GREP_BINARY_TEXTCONV)\n>> +\t\tgs->driver = userdiff_get_textconv(gs->driver);\n>> \tgrep_attr_unlock();\n>> }\n> \n> This is wrong. The point of userdiff_get_textconv is that it will return\n> NULL when we are not doing textconv for this path. So you can use it\n> like:\n> \n> struct userdiff_driver *textconv = userdiff_get_textconv(gs->driver);\n> \n> if (textconv) {\n> /* ok, we are doing textconv. Call our fill_textconv\n> * equivalent. */\n> }\n> else {\n> /* nope, plain old file. */\n> }\n> \n> But by assigning it on top of gs->driver, you're going to end up with a\n> NULL driver sometimes. And the post-condition of the load_driver\n> function is that gs->driver always points to a valid driver (even if it\n> is the default one). I wouldn't be surprised if this causes segfaults.\n> \n> \n> So I would do it more like the patch below. Only lightly tested by me.\n> There are some refactoring opportunities if you want to bring\n> grep_source and diff_filespec closer together.\n> \n> ---\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 8025964..915c8ef 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 },\n> diff --git a/grep.c b/grep.c\n> index 4bd1b8b..3880d64 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> @@ -1321,6 +1323,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> @@ -1331,6 +1385,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> @@ -1352,19 +1407,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> @@ -1372,7 +1444,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;\n> diff --git a/grep.h b/grep.h\n> index 8fc854f..94a7ac2 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -106,6 +106,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;\n> \n\nThanks Jeff, that helps a lot! It covers \"grep expr\" and \"grep expr rev\n-- path\" just fine. I'll look into \"grep expr rev:path\" which does not\nwork yet because of an empty driver.\n\nI also have \"show --textconv\" covered and a suggestion for \"cat-file\n--textconv\" (to work without a textconv filter).\n\nExpect a mini-series soon :)\n\nMichael\n"},{"id":"208757","messageId":"20130205201106.GA29248@sigill.intra.peff.net","threadId":"32824","inReplyTo":"5111317E.8060906@drmicha.warpmail.net","subject":"Re: [WIP/RFH/RFD/PATCH] grep: allow to use textconv filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-05T20:11:06Z","receivedAt":"2013-02-05T20:11:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 05, 2013 at 05:21:18PM +0100, Michael J Gruber wrote:\n\n> Thanks Jeff, that helps a lot! It covers \"grep expr\" and \"grep expr rev\n> -- path\" just fine. I'll look into \"grep expr rev:path\" which does not\n> work yet because of an empty driver.\n> \n> I also have \"show --textconv\" covered and a suggestion for \"cat-file\n> --textconv\" (to work without a textconv filter).\n> \n> Expect a mini-series soon :)\n\nCool, I'm glad it helped. It would be great if diff_filespec and\ngrep_source could grow together into a unified object. One of the gross\nthings about the patch I posted is that we will now sometimes read the\nfile/blob data via grep_source_load, and sometimes via\ndiff_populate_filespec. They _should_ be equivalent, but in an ideal\nworld, they would be the same code path.\n\nThat may be too much to tackle for your series, though (I wanted to do\nit when I factored out grep_source, but backed off for the same reason).\n\nThe \"grep expr rev:path\" fix should look something like this:\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 915c8ef..cdc7d32 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -820,13 +820,17 @@ 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, sha1, &oc)) {\n \t\t\tstruct object *object = parse_object(sha1);\n \t\t\tif (!object)\n \t\t\t\tdie(_(\"bad object %s\"), arg);\n \t\t\tif (!seen_dashdash)\n \t\t\t\tverify_non_filename(prefix, arg);\n+\t\t\t/* oops, we need something that will remember oc.path\n+\t\t\t * here, so that we can pass it along to\n+\t\t\t * grep_source_init */\n \t\t\tadd_object_array(object, arg, &list);\n \t\t\tcontinue;\n \t\t}\n\nBut you'll have to replace the object_array with something more\nfeatureful, I think.\n\n-Peff\n"},{"id":"208803","messageId":"cover.1360162813.git.git@drmicha.warpmail.net","threadId":"32824","inReplyTo":"20130205201106.GA29248@sigill.intra.peff.net","subject":"[RFC/PATCH 0/4] textconv for show and grep","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-06T15:08:49Z","receivedAt":"2013-02-06T15:08:49Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"This min series aims at completing the textconv support of user facing\ncommands. It is RFC for lack of documentation and tests, to check\nwhether we really want to go in that direction (I do).\n\n1/4 covers the missing textconv support in the \"blob case\" of \"git\nshow\", which should be (and then is) analogous to \"cat-file\" and \"diff\".\n\n2/4 remedies an oddity of \"git cat-file --textconv\", which errored out\nwhen there is no textconv filter rather than giving an unfiltered blob\n(like every other textconv aware command).\n\n3/4 implements \"--textconv\" for \"git grep\" sans the blob case; the code\nis all Jeff's.\n\n4/4 adds blob support to 3/4 (the \"rev:path\" case).\n\n3 and 4 can be squashed, of course.\n\nNow I'm quite a happy differ/shower/grepper with my latin1 and OO files ;)\n\nJeff King (1):\n grep: allow to use textconv filters\n\nMichael J Gruber (3):\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 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/t8007-cat-file-textconv.sh | 20 +++------\n 8 files changed, 148 insertions(+), 47 deletions(-)\n\n-- \n1.8.1.2.752.g32d147e\n"},{"id":"208804","messageId":"883f0163cb732932061a368ea9bc187c13e4ecca.1360162813.git.git@drmicha.warpmail.net","threadId":"32824","inReplyTo":"20130205201106.GA29248@sigill.intra.peff.net","subject":"[RFC/PATCH 1/4] show: obey --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-06T15:08:50Z","receivedAt":"2013-02-06T15:08:50Z","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 1 file changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8f0b2e8..f83870d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -402,10 +402,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@@ -491,7 +509,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-- \n1.8.1.2.752.g32d147e\n"},{"id":"208805","messageId":"b20e91bc71e59b5390005f2e6428e69a467e80b5.1360162813.git.git@drmicha.warpmail.net","threadId":"32824","inReplyTo":"20130205201106.GA29248@sigill.intra.peff.net","subject":"[RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-06T15:08:51Z","receivedAt":"2013-02-06T15:08:51Z","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 00528dd..6912dc2 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.1.2.752.g32d147e\n"},{"id":"208807","messageId":"da8c01b918c94c84ab61859b1b1453885bff5b06.1360162813.git.git@drmicha.warpmail.net","threadId":"32824","inReplyTo":"20130205201106.GA29248@sigill.intra.peff.net","subject":"[RFC/PATCH 3/4] grep: allow to use textconv filters","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-06T15:08:52Z","receivedAt":"2013-02-06T15:08:52Z","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 3 files changed, 89 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 8025964..915c8ef 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 4bd1b8b..3880d64 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@@ -1321,6 +1323,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@@ -1331,6 +1385,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@@ -1352,19 +1407,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@@ -1372,7 +1444,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 8fc854f..94a7ac2 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -106,6 +106,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;\n-- \n1.8.1.2.752.g32d147e\n"},{"id":"208806","messageId":"a2808975f00bac5de0902e6830f254e4b064e8a5.1360162813.git.git@drmicha.warpmail.net","threadId":"32824","inReplyTo":"20130205201106.GA29248@sigill.intra.peff.net","subject":"[RFC/PATCH 4/4] grep: obey --textconv for the case rev:path","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-06T15:08:53Z","receivedAt":"2013-02-06T15:08:53Z","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 3 files changed, 28 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 915c8ef..0f3c4db 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,14 +820,15 @@ 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(sha1);\n \t\t\tif (!object)\n \t\t\t\tdie(_(\"bad object %s\"), 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 4af3451..1b796c7 100644\n--- a/object.c\n+++ b/object.c\n@@ -245,12 +245,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@@ -265,9 +260,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 6a97b6b..a11d719 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@@ -74,6 +75,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);\n-- \n1.8.1.2.752.g32d147e\n"},{"id":"208810","messageId":"vpqip65xyq5.fsf@grenoble-inp.fr","threadId":"32824","inReplyTo":"da8c01b918c94c84ab61859b1b1453885bff5b06.1360162813.git.git@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 3/4] grep: allow to use textconv filters","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-02-06T15:12:50Z","receivedAt":"2013-02-06T15:12:50Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Michael J Gruber writes:\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\nDon't forget to update Documentation/git-grep.txt too.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"208820","messageId":"7vr4ktmlt7.fsf@alter.siamese.dyndns.org","threadId":"32824","inReplyTo":"b20e91bc71e59b5390005f2e6428e69a467e80b5.1360162813.git.git@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-06T16:47:16Z","receivedAt":"2013-02-06T16:47:16Z","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\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 | 9 +++++----\n> t/t8007-cat-file-textconv.sh | 20 +++++---------------\n> 2 files changed, 10 insertions(+), 19 deletions(-)\n>\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 00528dd..6912dc2 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\nBut stepping back a bit.\n\nWhat happens when I say \"cat-file -c HEAD:Documentation\", and what\nshould happen when I do so?\n\nI think what we want to see in the ideal world might be:\n\n * If we have a textconv for tree objects at that path to format it\n specially, textconv_object() may be allowed to textualize it\n (even though it is not a blob, and textconv so far has always\n been about blobs; it needs to be considered carefully if it makes\n sense to allow such a usage) and show it;\n\n * If we don't, we act as if -c were -p; in other words, we treat\n the built-in \"human output\" implemented by \"cat-file -p\" as if\n that is a textconv.\n\nIn other words, you may want to fall-thru to case 'p', not case 0\nwith forced \"blob\" type.\n"},{"id":"208821","messageId":"7vmwvhmli7.fsf@alter.siamese.dyndns.org","threadId":"32824","inReplyTo":"883f0163cb732932061a368ea9bc187c13e4ecca.1360162813.git.git@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 1/4] show: obey --textconv for blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-06T16:53:52Z","receivedAt":"2013-02-06T16:53:52Z","isPatch":true,"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 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\nWhat does \"log -p\" do currently, and what should it do? Does/should\nit also use --textconv?\n\nThe --textconv is a natural extension of what --ext-diff provides us,\nso I think it should trigger the same way as how --ext-diff triggers.\n\nWe apply \"--ext-diff\" for \"diff\" by default but not for \"log -p\" and\n\"show\"; I suspect this may have been for a good reason but I do not\nrecall the discussion that led to the current behaviour offhand.\n\n> Signed-off-by: Michael J Gruber \n> ---\n> builtin/log.c | 24 +++++++++++++++++++++---\n> 1 file changed, 21 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 8f0b2e8..f83870d 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -402,10 +402,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> @@ -491,7 +509,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"},{"id":"208822","messageId":"7vip65mlfp.fsf@alter.siamese.dyndns.org","threadId":"32824","inReplyTo":"cover.1360162813.git.git@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 0/4] textconv for show and grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-06T16:55:22Z","receivedAt":"2013-02-06T16:55:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The parts for \"grep\" in the series makes tons of sense to me. I am\nnot yet convinced about the other two, though.\n\nThanks.\n"},{"id":"208870","messageId":"20130206220644.GB27507@sigill.intra.peff.net","threadId":"32824","inReplyTo":"883f0163cb732932061a368ea9bc187c13e4ecca.1360162813.git.git@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 1/4] show: obey --textconv for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-06T22:06:44Z","receivedAt":"2013-02-06T22:06:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 06, 2013 at 04:08:50PM +0100, Michael J Gruber wrote:\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 8f0b2e8..f83870d 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -402,10 +402,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\nShould this maybe just take the whole object_array_entry as a cleanup?\n\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\nIt seems a little hacky that we have to look up the sha1 again. What\nshould happen in the off chance that \"hashcmp(sha1, sha1c) != 0\" due to\na race with a simultaneous update of a ref?\n\nWould it be better if object_array_entry replaced its \"mode\" member with\nan object_context? The only downside I see is that we might waste a\nsignificant amount of memory (each context has a PATH_MAX buffer in it).\n\n-Peff\n"},{"id":"208872","messageId":"20130206221240.GC27507@sigill.intra.peff.net","threadId":"32824","inReplyTo":"7vmwvhmli7.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 1/4] show: obey --textconv for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-06T22:12:40Z","receivedAt":"2013-02-06T22:12:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 06, 2013 at 08:53:52AM -0800, Junio C Hamano wrote:\n\n> Michael J Gruber writes:\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> What does \"log -p\" do currently, and what should it do? Does/should\n> it also use --textconv?\n> \n> The --textconv is a natural extension of what --ext-diff provides us,\n> so I think it should trigger the same way as how --ext-diff triggers.\n> \n> We apply \"--ext-diff\" for \"diff\" by default but not for \"log -p\" and\n> \"show\"; I suspect this may have been for a good reason but I do not\n> recall the discussion that led to the current behaviour offhand.\n\nI think Michael's commit message explains the situation badly.\n--textconv is already on for \"git show\" (and for \"git log\") by default.\nDiffs already use it.\n\nThis is more about the fact that when showing a single blob, we do not\nbother to remember the context of the sha1 lookup, including its\npathname. Therefore we were not previously able to apply any\n.gitattributes to the output. So this patch really does two things:\n\n 1. Pass the information along to show_blob_object so that it can\n look up .gitattributes.\n\n 2. Apply the textconv attribute (if ALLOW_TEXTCONV is on, of course).\n\nAnd stating it that way makes it clear that there may be other missing\nsteps (3 and up) to apply other gitattributes. For example, should \"git\nshow $blob\" respect crlf attributes? Smudge filters?\n\n-Peff\n"},{"id":"208874","messageId":"20130206221912.GD27507@sigill.intra.peff.net","threadId":"32824","inReplyTo":"b20e91bc71e59b5390005f2e6428e69a467e80b5.1360162813.git.git@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-06T22:19:12Z","receivedAt":"2013-02-06T22:19:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 06, 2013 at 04:08:51PM +0100, Michael J Gruber wrote:\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\nMakes sense.\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\nThe implication here is that textconv_object should be handling its own\nerrors and dying, and the return is always \"yes, I converted\" or \"no, I\ndid not\". Which I think is the case.\n\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 wondered at first why we needed to set exp_type here; shouldn't we\nalready be expecting a blob if we are doing textconv? But then I see\nthis is really about the fall-through in the switch (which we might want\nan explicit comment for).\n\nWhich made me wonder: what happens with:\n\n git cat-file --textconv HEAD\n\nIt looks like we die just before textconv-ing, because we have no\nobj_context.path. But that is also unlike all of the other --textconv\nswitches, which mean \"turn on textconv if you are showing a blob that\nsupports it\" and not \"the specific operation is --textconv, apply it to\nthis blob\". I don't know if that is worth changing or not.\n\n-Peff\n"},{"id":"208876","messageId":"7vvca59j4n.fsf@alter.siamese.dyndns.org","threadId":"32824","inReplyTo":"20130206221912.GD27507@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-06T22:23:36Z","receivedAt":"2013-02-06T22:23:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Which made me wonder: what happens with:\n>\n> git cat-file --textconv HEAD\n>\n> It looks like we die just before textconv-ing, because we have no\n> obj_context.path. But that is also unlike all of the other --textconv\n> switches, which mean \"turn on textconv if you are showing a blob that\n> supports it\" and not \"the specific operation is --textconv, apply it to\n> this blob\". I don't know if that is worth changing or not.\n\nOK, so in that sense, \"cat-file --textconv HEAD\" (or HEAD:) should\ndie with \"Hey, that is not a blob\", in other words, Michael's patch\ndoes what we want without further tweaks, right?\n\nBy the way are we sure textconv_object() barfs and dies if fed a non\nblob? Otherwise the above does not hold, and the semantics become\n\"turn on textconv if the object you are showing supports it,\notherwise it has to be a blob.\", I think.\n"},{"id":"208875","messageId":"20130206222344.GE27507@sigill.intra.peff.net","threadId":"32824","inReplyTo":"da8c01b918c94c84ab61859b1b1453885bff5b06.1360162813.git.git@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 3/4] grep: allow to use textconv filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-06T22:23:44Z","receivedAt":"2013-02-06T22:23:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 06, 2013 at 04:08:52PM +0100, 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\nSigned-off-by: Jeff King \n\nI'd really love to see the refactoring I talked about in my earlier\nmessage. But as I'm not willing to devote the time to do it right now,\nand I do not think this patch has any particular bugs, I think it is OK\nas it gets the job done, and does not make the later refactoring any\nharder.\n\nThe one ugliness that still remains is:\n\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\nWe lock/unlock the grep_attr_lock twice here: once in\ngrep_source_load_driver, and then immediately again to call\nuserdiff_get_textconv. I don't know if it is worth doing the two under\nthe same lock or not (I guess it should not increase lock contention,\nsince we do the same amount of work, so it is really just the extra lock\ninstructions).\n\n-Peff\n"},{"id":"208878","messageId":"20130206223656.GF27507@sigill.intra.peff.net","threadId":"32824","inReplyTo":"a2808975f00bac5de0902e6830f254e4b064e8a5.1360162813.git.git@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 4/4] grep: obey --textconv for the case rev:path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-06T22:36:56Z","receivedAt":"2013-02-06T22:36:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 06, 2013 at 04:08:53PM +0100, Michael J Gruber wrote:\n\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\nIf we go this route, this new _with_context variant should be used in\npatch 1, too.\n\n> @@ -265,9 +260,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\nThis seems a little gross. Who is responsible for allocating the\ncontext? Who frees it? It looks like we duplicate it in cmd_grep. Which\nI think is OK, but it means all of this context infrastructure in\nobject.[ch] is just bolted-on junk waiting for somebody to use it wrong\nor get confused. It does not get set, for example, by the regular\nsetup_revisions code path.\n\nIt would be nice if we could just always have the context available,\nthen setup_revisions could set it up by default (and replace the \"mode\"\nparameter entirely). But we'd need to do something to avoid the\nPATH_MAX-sized buffer for each entry, as some code paths may have a\nlarge number of pending objects.\n\n-Peff\n"},{"id":"208879","messageId":"20130206224353.GG27507@sigill.intra.peff.net","threadId":"32824","inReplyTo":"7vvca59j4n.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-06T22:43:53Z","receivedAt":"2013-02-06T22:43:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 06, 2013 at 02:23:36PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Which made me wonder: what happens with:\n> >\n> > git cat-file --textconv HEAD\n> >\n> > It looks like we die just before textconv-ing, because we have no\n> > obj_context.path. But that is also unlike all of the other --textconv\n> > switches, which mean \"turn on textconv if you are showing a blob that\n> > supports it\" and not \"the specific operation is --textconv, apply it to\n> > this blob\". I don't know if that is worth changing or not.\n> \n> OK, so in that sense, \"cat-file --textconv HEAD\" (or HEAD:) should\n> die with \"Hey, that is not a blob\", in other words, Michael's patch\n> does what we want without further tweaks, right?\n\nRight, it will die because we do not find a path in the object_context.\nFor the same reason that \"cat-file --textconv $sha1\" would die.\n\nA more interesting case is \"cat-file --textconv HEAD:Documentation\",\nwhich does have a path, but not a blob. And I think that speaks to your\npoint that we want to fall-through to the pretty-print case, not the\nblob case.\n\n> By the way are we sure textconv_object() barfs and dies if fed a non\n> blob? Otherwise the above does not hold, and the semantics become\n> \"turn on textconv if the object you are showing supports it,\n> otherwise it has to be a blob.\", I think.\n\nI'm not sure. The sha1 would get passed all the way down to\nfill_textconv. I think because sha1_valid is set, it will not try to\nreuse the working tree file, so we will end up in\ndiff_populate_filespec, and we could actually textconv the tree object\nitself.\n\nSo yeah, I think we want a check that makes sure we are working with a\nblob before we even call that function, and \"--textconv\" should just be\n\"you must feed me a blob of the form $treeish:$path\". In practice nobody\nwants to do anything else anyway, so let's keep the code paths simple;\nwe can always loosen it later if there is a good reason to do so.\n\n-Peff\n"},{"id":"208887","messageId":"7vr4kt9f53.fsf@alter.siamese.dyndns.org","threadId":"32824","inReplyTo":"20130206221240.GC27507@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 1/4] show: obey --textconv for blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-06T23:49:44Z","receivedAt":"2013-02-06T23:49:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> And stating it that way makes it clear that there may be other missing\n> steps (3 and up) to apply other gitattributes. For example, should \"git\n> show $blob\" respect crlf attributes? Smudge filters?\n\nYeah, I think applying these when path is availble may make sense.\n\nAre we going to teach cat-file to honor them with \"--textconv\" and\npossibly other new options?\n\nOr should the \"--textconv\" imply application of these other filters?\nI am tempted to say yes, but I haven't thought things through...\n"},{"id":"208888","messageId":"20130207001013.GA28970@sigill.intra.peff.net","threadId":"32824","inReplyTo":"7vr4kt9f53.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 1/4] show: obey --textconv for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-07T00:10:13Z","receivedAt":"2013-02-07T00:10:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 06, 2013 at 03:49:44PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > And stating it that way makes it clear that there may be other missing\n> > steps (3 and up) to apply other gitattributes. For example, should \"git\n> > show $blob\" respect crlf attributes? Smudge filters?\n> \n> Yeah, I think applying these when path is availble may make sense.\n> \n> Are we going to teach cat-file to honor them with \"--textconv\" and\n> possibly other new options?\n> \n> Or should the \"--textconv\" imply application of these other filters?\n> I am tempted to say yes, but I haven't thought things through...\n\nFor diff, we should already recognize them (because we feed the diff\nmachinery the path name, and it does the usual attributes lookup). Of\ncourse it only uses things like funcname, not any of the\nconvert-to-filesystem options (hmm, actually, I guess it may use\nconvert-from-filesystem, but in such a case it would not be reading from\na blob anyway, but from a filesystem path, so it is not related to this\ncode).\n\nFor cat-file, I don't think --textconv should necessarily imply it. The\noriginal motivation for \"cat-file --textconv\" was for external blame to\nbe able to access the converted contents. But it would not want to do\nfilesystem-level conversion; it wants the canonical version of the file.\nI think a better option name for smudge/crlf would be \"--to-filesystem\"\nor something like that. And probably it should not be the default.\n\ngit-show should probably have the same option. I doubt it should be on\nby default, but I can see it being useful for:\n\n git show --to-filesystem HEAD:Makefile >Makefile\n\nor whatever. I don't think those features necessarily need to come as\npart of Michael's series. They can wait for people who care to implement\nthem. But I do think the refactoring of passing along the path from the\nobject_context should keep in mind that we will probably want to go that\nway eventually.\n\nAre there other attributes that we might care about when showing a blob?\nThe only ones I can think of are the ones for converting to the\nfilesystem representation.\n\n-Peff\n"},{"id":"208892","messageId":"7vbobx9dfs.fsf@alter.siamese.dyndns.org","threadId":"32824","inReplyTo":"20130207001013.GA28970@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 1/4] show: obey --textconv for blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-07T00:26:31Z","receivedAt":"2013-02-07T00:26:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> git-show should probably have the same option. I doubt it should be on\n> by default, but I can see it being useful for:\n>\n> git show --to-filesystem HEAD:Makefile >Makefile\n>\n> or whatever. I don't think those features necessarily need to come as\n> part of Michael's series.\n\nYeah, all makes sense (including the part that it might be useful\nbut it does not belong to this series).\n\nThanks for sanity checking.\n"},{"id":"208901","messageId":"51136A55.7060805@drmicha.warpmail.net","threadId":"32824","inReplyTo":"7vmwvhmli7.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 1/4] show: obey --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-07T08:48:21Z","receivedAt":"2013-02-07T08:48:21Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 06.02.2013 17:53:\n> Michael J Gruber writes:\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> What does \"log -p\" do currently, and what should it do? Does/should\n> it also use --textconv?\n\nIt invokes \"--textconv\" by default, and allows to override with\n\"--no-textconv. That's what I meant to say but seem to have failed to.\n\nI think at some point we decided to have \"human output\" on by default\nfor all things diff (porcelain only, of course) unless the diff is meant\nfor machine consumption, such as in the case of format-patch.\n\n> The --textconv is a natural extension of what --ext-diff provides us,\n> so I think it should trigger the same way as how --ext-diff triggers.\n\nThis series' aim is to make the textconv behavior the same for all\n\"human output\" commands. I don't see textconv and ext-diff to be\nstrongly related, and if then the other way round:\n\ntextconv is about converting blobs to a (possibly lossy) human\nconsumable text.\n\next-diff is about computing diffs with an external diff \"tool\" (not in\nthe sense of diff-tool).\n\nOne way of diffing is textconving blobs then using internal diff on the\nresulting text blobs, but ext-diff is more general and, really,\northogonal in a way. (It used to be used often for what textconv does\nwhen that didn't exist.)\n\n> We apply \"--ext-diff\" for \"diff\" by default but not for \"log -p\" and\n> \"show\"; I suspect this may have been for a good reason but I do not\n> recall the discussion that led to the current behaviour offhand.\n\nI don't think I changed anything ext-diff related, did I? 1/4 is about\nshowing blobs, not diffs.\n\n>> Signed-off-by: Michael J Gruber \n>> ---\n>> builtin/log.c | 24 +++++++++++++++++++++---\n>> 1 file changed, 21 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/builtin/log.c b/builtin/log.c\n>> index 8f0b2e8..f83870d 100644\n>> --- a/builtin/log.c\n>> +++ b/builtin/log.c\n>> @@ -402,10 +402,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>> @@ -491,7 +509,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"},{"id":"208902","messageId":"51136E56.7060703@drmicha.warpmail.net","threadId":"32824","inReplyTo":"20130206220644.GB27507@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 1/4] show: obey --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-07T09:05:26Z","receivedAt":"2013-02-07T09:05:26Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 06.02.2013 23:06:\n> On Wed, Feb 06, 2013 at 04:08:50PM +0100, Michael J Gruber wrote:\n> \n>> diff --git a/builtin/log.c b/builtin/log.c\n>> index 8f0b2e8..f83870d 100644\n>> --- a/builtin/log.c\n>> +++ b/builtin/log.c\n>> @@ -402,10 +402,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> Should this maybe just take the whole object_array_entry as a cleanup?\n\nIt's just a question of one or two/three pointers (I can't count), but\nyes, that would be possible.\n\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> \n> It seems a little hacky that we have to look up the sha1 again. What\n> should happen in the off chance that \"hashcmp(sha1, sha1c) != 0\" due to\n> a race with a simultaneous update of a ref?\n\nI thought about a check here but didn't bother to because I knew the\nrefactoring would come up again...\n\n> Would it be better if object_array_entry replaced its \"mode\" member with\n> an object_context?\n\nDo all callers/users want to deal with object_context?\n\nI'm wondering why o_c has a mode at all, since it is mostly used in\nconjunction with an object, isn't it?\n\n> The only downside I see is that we might waste a\n> significant amount of memory (each context has a PATH_MAX buffer in it).\n\nThat's why I used a reference to the struct, see my other reply.\n\nMichael\n"},{"id":"208903","messageId":"51136E75.5060002@drmicha.warpmail.net","threadId":"32824","inReplyTo":"20130206223656.GF27507@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 4/4] grep: obey --textconv for the case rev:path","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-07T09:05:57Z","receivedAt":"2013-02-07T09:05:57Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 06.02.2013 23:36:\n> On Wed, Feb 06, 2013 at 04:08:53PM +0100, Michael J Gruber wrote:\n> \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> If we go this route, this new _with_context variant should be used in\n> patch 1, too.\n> \n>> @@ -265,9 +260,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> This seems a little gross. Who is responsible for allocating the\n> context? Who frees it? It looks like we duplicate it in cmd_grep. Which\n\nWell, who is responsible for allocating and freeing name and item? I\ndidn't want to introduce a new member which is a struct when all other\ncomplex members are pointers. Wouldn't that be confusing?\n\n> I think is OK, but it means all of this context infrastructure in\n> object.[ch] is just bolted-on junk waiting for somebody to use it wrong\n> or get confused. It does not get set, for example, by the regular\n> setup_revisions code path.\n\nSure, it's NULL when there is no context info, just like in many other\ncases.\n\n> It would be nice if we could just always have the context available,\n> then setup_revisions could set it up by default (and replace the \"mode\"\n> parameter entirely). But we'd need to do something to avoid the\n> PATH_MAX-sized buffer for each entry, as some code paths may have a\n> large number of pending objects.\n\nIf the information is always available even if we don't need it then it\nalways takes space. The only way out would be pointing into a pool of\npath names rather having a copy in each entry. It's not like I hadn't\ntalked about providing virtual (blob) objects for path names keyed by\ntheir sha1 before... It's just that I want my grep --textconv now ;)\n\nMichael\n"},{"id":"208904","messageId":"20130207091116.GB15727@sigill.intra.peff.net","threadId":"32824","inReplyTo":"51136E56.7060703@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 1/4] show: obey --textconv for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-07T09:11:16Z","receivedAt":"2013-02-07T09:11:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 07, 2013 at 10:05:26AM +0100, Michael J Gruber wrote:\n\n> > Would it be better if object_array_entry replaced its \"mode\" member with\n> > an object_context?\n> \n> Do all callers/users want to deal with object_context?\n\nWouldn't it just mean replacing \"entry->mode\" with \"entry->oc.mode\" at\neach user?\n\n> I'm wondering why o_c has a mode at all, since it is mostly used in\n> conjunction with an object, isn't it?\n\nJust as we record the path from the surrounding tree, we record the\nmode. It's that mode which gets put into the pending object list by the\nrevision parser (see the very end of handle_revision_arg). Storing an\nobject_context instead of the mode would be a strict superset of what we\nstore now (right now we just throw the rest away).\n\n-Peff\n"},{"id":"208905","messageId":"20130207092640.GC15727@sigill.intra.peff.net","threadId":"32824","inReplyTo":"51136E75.5060002@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 4/4] grep: obey --textconv for the case rev:path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-07T09:26:40Z","receivedAt":"2013-02-07T09:26:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 07, 2013 at 10:05:57AM +0100, Michael J Gruber wrote:\n\n> >> @@ -265,9 +260,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> > This seems a little gross. Who is responsible for allocating the\n> > context? Who frees it? It looks like we duplicate it in cmd_grep. Which\n> \n> Well, who is responsible for allocating and freeing name and item? I\n> didn't want to introduce a new member which is a struct when all other\n> complex members are pointers. Wouldn't that be confusing?\n\nWe cheat on those two. \"item\" is always a pointer to a \"struct object\",\nwhich lasts forever and never gets freed. When \"name\" is set by\nsetup_revisions, it comes from the argv list, which is assumed to last\nforever (and when we add pending blobs for a \"--objects\" traversal, it\nis the empty string (literal).\n\nI'd be OK if we had an exterior object_context that could be handled\nin the same way. But how do we tell setup_revisions that we are\ninterested in seeing the object_context from each parsed item, where\ndoes the allocation come from (is it malloc'd by setup_revisions?), and\nwho is responsible for freeing it when we pop pending objects in\nget_revisions and similar?\n\nI don't think it's as clear cut.\n\nI wonder, though...what we really care about here is just the pathname.\nBut if it is a pending object that comes from a blob revision argument,\nwon't it always be of the form \"treeish:path\"? Could we not even resolve\nthe sha1 again, but instead just parse out the \":path\" bit?\n\nThat is sort of like what the repeated call to get_sha1_with_context\ndoes in your first patch. Except that we do not actually want to lookup\nthe sha1, and it is harmful to do so (e.g., if the ref had moved on to a\nnew tree that does not have that path, get_sha1 would fail, but we do\nnot even care what is in the tree; we only want the parsing side effects\nof get_sha1).\n\nHmm.\n\n-Peff\n\nPS By the way, while looking at the object_array code (which I have not\n really used much before), I noticed that add_pending_commit_list sets\n the \"name\" field to the result of sha1_to_hex. Which means that it is\n likely to be completely bogus by the time you read it. I'm not even\n sure where it gets read or if this matters. And obviously it's\n completely unrelated to what we were discussing; just something I\n noticed.\n"},{"id":"208906","messageId":"51137522.3010005@drmicha.warpmail.net","threadId":"32824","inReplyTo":"20130207091116.GB15727@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 1/4] show: obey --textconv for blobs","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-07T09:34:26Z","receivedAt":"2013-02-07T09:34:26Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 07.02.2013 10:11:\n> On Thu, Feb 07, 2013 at 10:05:26AM +0100, Michael J Gruber wrote:\n> \n>>> Would it be better if object_array_entry replaced its \"mode\" member with\n>>> an object_context?\n>>\n>> Do all callers/users want to deal with object_context?\n> \n> Wouldn't it just mean replacing \"entry->mode\" with \"entry->oc.mode\" at\n> each user?\n\nYes, I meant at the time of creation, i.e. when someone has to create\nand pass an o_a_e and maybe only knows a mode, and thus would have to\nset the path to NULL or \"\".\n\n>> I'm wondering why o_c has a mode at all, since it is mostly used in\n>> conjunction with an object, isn't it?\n> \n> Just as we record the path from the surrounding tree, we record the\n> mode. It's that mode which gets put into the pending object list by the\n> revision parser (see the very end of handle_revision_arg). Storing an\n> object_context instead of the mode would be a strict superset of what we\n> store now (right now we just throw the rest away).\n\nSure. But why does object_context have a mode member at all? Maybe it is\nnot alway used together with another struct which has the mode already,\nthen that's a reason.\n\nMichael\n"},{"id":"208908","messageId":"20130207094314.GC16832@sigio.peff.net","threadId":"32824","inReplyTo":"51137522.3010005@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 1/4] show: obey --textconv for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-07T09:43:14Z","receivedAt":"2013-02-07T09:43:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 07, 2013 at 10:34:26AM +0100, Michael J Gruber wrote:\n\n> > Just as we record the path from the surrounding tree, we record the\n> > mode. It's that mode which gets put into the pending object list by the\n> > revision parser (see the very end of handle_revision_arg). Storing an\n> > object_context instead of the mode would be a strict superset of what we\n> > store now (right now we just throw the rest away).\n> \n> Sure. But why does object_context have a mode member at all? Maybe it is\n> not alway used together with another struct which has the mode already,\n> then that's a reason.\n\nExactly. It's purely for pulling information out of\nget_sha1_with_context, and does not know that you are going to put its\noutput into an object_array_entry (and many call sites do not).\n\n-Peff\n"},{"id":"208909","messageId":"5113784B.10103@drmicha.warpmail.net","threadId":"32824","inReplyTo":"20130207092640.GC15727@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 4/4] grep: obey --textconv for the case rev:path","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-07T09:47:55Z","receivedAt":"2013-02-07T09:47:55Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 07.02.2013 10:26:\n> On Thu, Feb 07, 2013 at 10:05:57AM +0100, Michael J Gruber wrote:\n> \n>>>> @@ -265,9 +260,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>>> This seems a little gross. Who is responsible for allocating the\n>>> context? Who frees it? It looks like we duplicate it in cmd_grep. Which\n>>\n>> Well, who is responsible for allocating and freeing name and item? I\n>> didn't want to introduce a new member which is a struct when all other\n>> complex members are pointers. Wouldn't that be confusing?\n> \n> We cheat on those two. \"item\" is always a pointer to a \"struct object\",\n> which lasts forever and never gets freed. When \"name\" is set by\n> setup_revisions, it comes from the argv list, which is assumed to last\n> forever (and when we add pending blobs for a \"--objects\" traversal, it\n> is the empty string (literal).\n\nI see, so they are really different.\n\n> I'd be OK if we had an exterior object_context that could be handled\n> in the same way. But how do we tell setup_revisions that we are\n> interested in seeing the object_context from each parsed item, where\n> does the allocation come from (is it malloc'd by setup_revisions?), and\n> who is responsible for freeing it when we pop pending objects in\n> get_revisions and similar?\n\nDo we really need all of tree, path and mode in object_context (I mean\nnot just here, but other users), or only the path? I'd try and resurrect\nthe virtual path name objects then, they would be just like \"item\"\nstorage-wise.\n\n> I don't think it's as clear cut.\n> \n> I wonder, though...what we really care about here is just the pathname.\n> But if it is a pending object that comes from a blob revision argument,\n> won't it always be of the form \"treeish:path\"? Could we not even resolve\n> the sha1 again, but instead just parse out the \":path\" bit?\n\nDo we have that, and in what form (e.g. magic expanded etc.)?\n\n> That is sort of like what the repeated call to get_sha1_with_context\n> does in your first patch. Except that we do not actually want to lookup\n> the sha1, and it is harmful to do so (e.g., if the ref had moved on to a\n> new tree that does not have that path, get_sha1 would fail, but we do\n> not even care what is in the tree; we only want the parsing side effects\n> of get_sha1).\n> \n> Hmm.\n> \n> -Peff\n> \n> PS By the way, while looking at the object_array code (which I have not\n> really used much before), I noticed that add_pending_commit_list sets\n> the \"name\" field to the result of sha1_to_hex. Which means that it is\n> likely to be completely bogus by the time you read it. I'm not even\n> sure where it gets read or if this matters. And obviously it's\n> completely unrelated to what we were discussing; just something I\n> noticed.\n\nAnother thing I noted is that our path mangling at least for grep has\nsome issues:\n\n(cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)\n../HEAD:../cache.h:#define GET_SHA1_QUIETLY 01\n\nTaking everything right of \":\" could still work.\n\nMichael\n"},{"id":"208910","messageId":"20130207095533.GA17110@sigio.peff.net","threadId":"32824","inReplyTo":"5113784B.10103@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 4/4] grep: obey --textconv for the case rev:path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-07T09:55:35Z","receivedAt":"2013-02-07T09:55:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 07, 2013 at 10:47:55AM +0100, Michael J Gruber wrote:\n\n> > I'd be OK if we had an exterior object_context that could be handled\n> > in the same way. But how do we tell setup_revisions that we are\n> > interested in seeing the object_context from each parsed item, where\n> > does the allocation come from (is it malloc'd by setup_revisions?), and\n> > who is responsible for freeing it when we pop pending objects in\n> > get_revisions and similar?\n> \n> Do we really need all of tree, path and mode in object_context (I mean\n> not just here, but other users), or only the path? I'd try and resurrect\n> the virtual path name objects then, they would be just like \"item\"\n> storage-wise.\n\nWe need at least mode, since that is how the mode parameter of\nobject_array_entry gets set. I do not know off-hand who uses \"tree\". I\nsuspect the intent was to do .gitattributes lookups inside that tree,\nbut I do not think we actually do in-tree lookups currently.\n\n> > I don't think it's as clear cut.\n> > \n> > I wonder, though...what we really care about here is just the pathname.\n> > But if it is a pending object that comes from a blob revision argument,\n> > won't it always be of the form \"treeish:path\"? Could we not even resolve\n> > the sha1 again, but instead just parse out the \":path\" bit?\n> \n> Do we have that, and in what form (e.g. magic expanded etc.)?\n\nAh, I should have mentioned that. :) We should have the original rev\nname in the object_array_entry's name field, shouldn't we? It's just a\nmatter of re-parsing it.\n\n> Another thing I noted is that our path mangling at least for grep has\n> some issues:\n> \n> (cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)\n> ../HEAD:../cache.h:#define GET_SHA1_QUIETLY 01\n\nYuck.\n\n-Peff\n"},{"id":"208912","messageId":"5113827A.40801@drmicha.warpmail.net","threadId":"32824","inReplyTo":"20130207095533.GA17110@sigio.peff.net","subject":"Re: [RFC/PATCH 4/4] grep: obey --textconv for the case rev:path","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-07T10:31:22Z","receivedAt":"2013-02-07T10:31:22Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 07.02.2013 10:55:\n> On Thu, Feb 07, 2013 at 10:47:55AM +0100, Michael J Gruber wrote:\n> \n>>> I'd be OK if we had an exterior object_context that could be handled\n>>> in the same way. But how do we tell setup_revisions that we are\n>>> interested in seeing the object_context from each parsed item, where\n>>> does the allocation come from (is it malloc'd by setup_revisions?), and\n>>> who is responsible for freeing it when we pop pending objects in\n>>> get_revisions and similar?\n>>\n>> Do we really need all of tree, path and mode in object_context (I mean\n>> not just here, but other users), or only the path? I'd try and resurrect\n>> the virtual path name objects then, they would be just like \"item\"\n>> storage-wise.\n> \n> We need at least mode, since that is how the mode parameter of\n> object_array_entry gets set. I do not know off-hand who uses \"tree\". I\n> suspect the intent was to do .gitattributes lookups inside that tree,\n> but I do not think we actually do in-tree lookups currently.\n> \n>>> I don't think it's as clear cut.\n>>>\n>>> I wonder, though...what we really care about here is just the pathname.\n>>> But if it is a pending object that comes from a blob revision argument,\n>>> won't it always be of the form \"treeish:path\"? Could we not even resolve\n>>> the sha1 again, but instead just parse out the \":path\" bit?\n>>\n>> Do we have that, and in what form (e.g. magic expanded etc.)?\n> \n> Ah, I should have mentioned that. :) We should have the original rev\n> name in the object_array_entry's name field, shouldn't we? It's just a\n> matter of re-parsing it.\n> \n>> Another thing I noted is that our path mangling at least for grep has\n>> some issues:\n>>\n>> (cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)\n>> ../HEAD:../cache.h:#define GET_SHA1_QUIETLY 01\n> \n> Yuck.\n\nAnd even more yuck:\n\n(cd t && git grep --full-name GET_SHA1_QUIETLY HEAD:../cache.h)\nHEAD:../cache.h:#define GET_SHA1_QUIETLY 01\n\nSomeone does not expect a \"rev:\" to be in there, it seems ;)\n\nMichael\n"},{"id":"208925","messageId":"7v4nho9f28.fsf@alter.siamese.dyndns.org","threadId":"32824","inReplyTo":"5113827A.40801@drmicha.warpmail.net","subject":"Re: [RFC/PATCH 4/4] grep: obey --textconv for the case rev:path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-07T18:03:43Z","receivedAt":"2013-02-07T18:03:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n>>> (cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)\n>>> ../HEAD:../cache.h:#define GET_SHA1_QUIETLY 01\n>> \n>> Yuck.\n>\n> And even more yuck:\n>\n> (cd t && git grep --full-name GET_SHA1_QUIETLY HEAD:../cache.h)\n> HEAD:../cache.h:#define GET_SHA1_QUIETLY 01\n>\n> Someone does not expect a \"rev:\" to be in there, it seems ;)\n\nI think stepping outside of $(cwd) is an afterthought the code does\nnot anticipate.\n"},{"id":"208993","messageId":"5114E130.6060902@drmicha.warpmail.net","threadId":"32824","inReplyTo":"7v4nho9f28.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 4/4] grep: obey --textconv for the case rev:path","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-08T11:27:44Z","receivedAt":"2013-02-08T11:27:44Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 07.02.2013 19:03:\n> Michael J Gruber writes:\n> \n>>>> (cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)\n>>>> ../HEAD:../cache.h:#define GET_SHA1_QUIETLY 01\n>>>\n>>> Yuck.\n>>\n>> And even more yuck:\n>>\n>> (cd t && git grep --full-name GET_SHA1_QUIETLY HEAD:../cache.h)\n>> HEAD:../cache.h:#define GET_SHA1_QUIETLY 01\n>>\n>> Someone does not expect a \"rev:\" to be in there, it seems ;)\n> \n> I think stepping outside of $(cwd) is an afterthought the code does\n> not anticipate.\n> \n\nWell, we do resolve relative paths correctly, and there are even some\n\"chdir\" in the code path. It's just that the output label is incorrect.\n\nMichael\n"}]}