{"thread":{"id":"25173","subject":"[PATCH v2 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","startedAt":"2010-09-20T20:39:43Z","lastAt":"2010-09-24T18:20:41Z","messageCount":9,"participants":["Kirill Smelkov","Matthieu Moy","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"151157","messageId":"cover.1285013802.git.kirr@landau.phys.spbu.ru","threadId":"25173","inReplyTo":null,"subject":"[BUG, PATCH v2 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-20T20:39:43Z","receivedAt":"2010-09-20T20:39:43Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Recently I've spot a bug in git blame --textconv, which was wrongly\ncalling pdftotext (my *.pdf conversion program) on a symlink.pdf, and I\nwas getting something like\n\n $ git blame -C -C regular-file.pdf\n Error: May not be a PDF file (continuing anyway)\n Error: PDF file is damaged - attempting to reconstruct xref table...\n Error: Couldn't find trailer dictionary\n Error: Couldn't read xref table\n Warning: program returned non-zero exit code #1\n fatal: unable to read files to diff\n\nThat errors come from pdftotext run on symlink.pdf being extracted to\n/tmp/ with one-line plain-text content pointing to link destination.\n\nPlease apply and thanks,\nKirill\n\nv2:\n\n o Incorporated suggestions by Matthieu and Jeff (details in each patch)\n\n\nKirill Smelkov (3):\n tests: Prepare --textconv tests for correctly-failing conversion\n program\n blame,cat-file: Demonstrate --textconv is wrongly running converter\n on symlinks\n blame,cat-file --textconv: Don't assume mode is ``S_IFREF | 0664''\n\n builtin.h | 2 +-\n builtin/blame.c | 33 ++++++++++++++-------\n builtin/cat-file.c | 2 +-\n sha1_name.c | 2 +\n t/t4042-diff-textconv-caching.sh | 25 ++++++++--------\n t/t8006-blame-textconv.sh | 58 +++++++++++++++++++++++++++++++++----\n t/t8007-cat-file-textconv.sh | 36 ++++++++++++++++++++---\n 7 files changed, 121 insertions(+), 37 deletions(-)\n\n-- \n1.7.3.rc2\n"},{"id":"151155","messageId":"6b2704af93283143d7756a1fa0fd52411e689292.1285013802.git.kirr@landau.phys.spbu.ru","threadId":"25173","inReplyTo":"cover.1285013802.git.kirr@landau.phys.spbu.ru","subject":"[PATCH v2 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-20T20:39:44Z","receivedAt":"2010-09-20T20:39:44Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"(Description by Matthieu Moy)\n\nThe textconv filter is sometimes incorrectly ran on a temporary file\nwhose content is the target of a symbolic link, instead of actual file\ncontent. Prepare to test this by marking the content of the file to\nconvert with \"bin:\", and let the helper die if \"bin:\" is not found in\nthe file content.\n\nCc: Axel Bonnet \nCc: Clément Poulain \nCc: Diane Gasselin \nCc: Matthieu Moy \nCc: Jeff King \nSigned-off-by: Kirill Smelkov \n---\n\nv2:\n\n o Changed patch description as suggested by Matthieu\n\n\n t/t4042-diff-textconv-caching.sh | 25 +++++++++++++------------\n t/t8006-blame-textconv.sh | 15 ++++++++-------\n t/t8007-cat-file-textconv.sh | 11 ++++++-----\n 3 files changed, 27 insertions(+), 24 deletions(-)\n\ndiff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\nindex 91f8198..7668099 100755\n--- a/t/t4042-diff-textconv-caching.sh\n+++ b/t/t4042-diff-textconv-caching.sh\n@@ -5,18 +5,19 @@ test_description='test textconv caching'\n \n cat >helper <<'EOF'\n #!/bin/sh\n-sed 's/^/converted: /' \"$@\" >helper.out\n+grep -q '^bin: ' \"$@\" || { echo \"E: $@ is not \\\"binary\\\" file\" 1>&2; exit 1; }\n+sed 's/^bin:/converted:/' \"$@\" >helper.out\n cat helper.out\n EOF\n chmod +x helper\n \n test_expect_success 'setup' '\n-\techo foo content 1 >foo.bin &&\n-\techo bar content 1 >bar.bin &&\n+\techo \"bin: foo content 1\" >foo.bin &&\n+\techo \"bin: bar content 1\" >bar.bin &&\n \tgit add . &&\n \tgit commit -m one &&\n-\techo foo content 2 >foo.bin &&\n-\techo bar content 2 >bar.bin &&\n+\techo \"bin: foo content 2\" >foo.bin &&\n+\techo \"bin: bar content 2\" >bar.bin &&\n \tgit commit -a -m two &&\n \techo \"*.bin diff=magic\" >.gitattributes &&\n \tgit config diff.magic.textconv ./helper &&\n@@ -25,14 +26,14 @@ test_expect_success 'setup' '\n \n cat >expect <expect <other &&\n+\techo \"bin: other\" >other &&\n \tgit config diff.magic.textconv \"./helper other\" &&\n \tgit diff HEAD^ HEAD >actual &&\n \ttest_cmp expect actual\n@@ -84,7 +85,7 @@ test_expect_success 'changing textconv invalidates cache' '\n \n cat >expect <helper <<'EOF'\n #!/bin/sh\n-sed 's/^/converted: /' \"$@\"\n+grep -q '^bin: ' \"$@\" || { echo \"E: $@ is not \\\"binary\\\" file\" 1>&2; exit 1; }\n+sed 's/^bin:/converted:/' \"$@\"\n EOF\n chmod +x helper\n \n test_expect_success 'setup ' '\n-\techo test 1 >one.bin &&\n-\techo test number 2 >two.bin &&\n+\techo \"bin: test 1\" >one.bin &&\n+\techo \"bin: test number 2\" >two.bin &&\n \tgit add . &&\n \tGIT_AUTHOR_NAME=Number1 git commit -a -m First --date=\"2010-01-01 18:00:00\" &&\n-\techo test 1 version 2 >one.bin &&\n-\techo test number 2 version 2 >>two.bin &&\n+\techo \"bin: test 1 version 2\" >one.bin &&\n+\techo \"bin: test number 2 version 2\" >>two.bin &&\n \tGIT_AUTHOR_NAME=Number2 git commit -a -m Second --date=\"2010-01-01 20:00:00\"\n '\n \n cat >expected <>two.bin &&\n+\techo \"bin: test number 2 version 3\" >>two.bin &&\n \tGIT_AUTHOR_NAME=Number3 git commit -a -m Third --date=\"2010-01-01 22:00:00\"\n '\n \ndiff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh\nindex 38ac05e..413d623 100755\n--- a/t/t8007-cat-file-textconv.sh\n+++ b/t/t8007-cat-file-textconv.sh\n@@ -5,15 +5,16 @@ test_description='git cat-file textconv support'\n \n cat >helper <<'EOF'\n #!/bin/sh\n-sed 's/^/converted: /' \"$@\"\n+grep -q '^bin: ' \"$@\" || { echo \"E: $@ is not \\\"binary\\\" file\" 1>&2; exit 1; }\n+sed 's/^bin:/converted:/' \"$@\"\n EOF\n chmod +x helper\n \n test_expect_success 'setup ' '\n-\techo test >one.bin &&\n+\techo \"bin: test\" >one.bin &&\n \tgit add . &&\n \tGIT_AUTHOR_NAME=Number1 git commit -a -m First --date=\"2010-01-01 18:00:00\" &&\n-\techo test version 2 >one.bin &&\n+\techo \"bin: test version 2\" >one.bin &&\n \tGIT_AUTHOR_NAME=Number2 git commit -a -m Second --date=\"2010-01-01 20:00:00\"\n '\n \n@@ -33,7 +34,7 @@ test_expect_success 'setup textconv filters' '\n '\n \n cat >expected <expected <\nCc: Clément Poulain \nCc: Diane Gasselin \nCc: Matthieu Moy \nCc: Jeff King \nSigned-off-by: Kirill Smelkov \n---\n\nv2:\n\n (As suggested by Matthieu)\n o Changed patch descriptio \n o Moved most of >expected preparation into test_expect_*\n o Changed multiple echo'es into cat <one.bin &&\n \techo \"bin: test number 2\" >two.bin &&\n+\tln -s one.bin symlink.bin &&\n \tgit add . &&\n \tGIT_AUTHOR_NAME=Number1 git commit -a -m First --date=\"2010-01-01 18:00:00\" &&\n \techo \"bin: test 1 version 2\" >one.bin &&\n \techo \"bin: test number 2 version 2\" >>two.bin &&\n+\tln -sf two.bin symlink.bin &&\n \tGIT_AUTHOR_NAME=Number2 git commit -a -m Second --date=\"2010-01-01 20:00:00\"\n '\n \n@@ -78,4 +80,47 @@ test_expect_success 'blame from previous revision' '\n \ttest_cmp expected result\n '\n \n+cat >expected <blame &&\n+\tfind_blame result &&\n+\ttest_cmp expected result\n+'\n+\n+# fails with '...symlink.bin is not \"binary\" file'\n+test_expect_failure 'blame --textconv (on symlink)' '\n+\tgit blame --textconv symlink.bin >blame &&\n+\tfind_blame result &&\n+\ttest_cmp expected result\n+'\n+\n+# cp two.bin three.bin and make small tweak\n+# (this will direct blame -C -C three.bin to consider two.bin and symlink.bin)\n+test_expect_success 'make another new commit' '\n+\tcat >three.bin <<\\EOF &&\n+bin: test number 2\n+bin: test number 2 version 2\n+bin: test number 2 version 3\n+bin: test number 3\n+EOF\n+\tgit add three.bin &&\n+\tGIT_AUTHOR_NAME=Number4 git commit -a -m Fourth --date=\"2010-01-01 23:00:00\"\n+'\n+\n+# fails with '...symlink.bin is not \"binary\" file'\n+test_expect_failure 'blame on last commit (-C -C, symlink)' '\n+\tgit blame -C -C three.bin >blame &&\n+\tfind_blame result &&\n+\tcat >expected <<\\EOF &&\n+(Number1 2010-01-01 18:00:00 +0000 1) converted: test number 2\n+(Number2 2010-01-01 20:00:00 +0000 2) converted: test number 2 version 2\n+(Number3 2010-01-01 22:00:00 +0000 3) converted: test number 2 version 3\n+(Number4 2010-01-01 23:00:00 +0000 4) converted: test number 3\n+EOF\n+\ttest_cmp expected result\n+'\n+\n test_done\ndiff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh\nindex 413d623..f747c05 100755\n--- a/t/t8007-cat-file-textconv.sh\n+++ b/t/t8007-cat-file-textconv.sh\n@@ -12,6 +12,7 @@ chmod +x helper\n \n test_expect_success 'setup ' '\n \techo \"bin: test\" >one.bin &&\n+\tln -s one.bin symlink.bin &&\n \tgit add . &&\n \tGIT_AUTHOR_NAME=Number1 git commit -a -m First --date=\"2010-01-01 18:00:00\" &&\n \techo \"bin: test version 2\" >one.bin &&\n@@ -68,4 +69,30 @@ test_expect_success 'cat-file --textconv on previous commit' '\n \tgit cat-file --textconv HEAD^:one.bin >result &&\n \ttest_cmp expected result\n '\n+\n+test_expect_success 'cat-file without --textconv (symlink)' '\n+\tgit cat-file blob :symlink.bin >result &&\n+\tprintf \"%s\" \"one.bin\" >expected\n+\ttest_cmp expected result\n+'\n+\n+\n+# fails because cat-file tries to run converter on symlink.bin\n+test_expect_failure '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+\ttest_cmp expected result\n+'\n+\n+# fails because cat-file tries to run converter on symlink.bin\n+test_expect_failure 'cat-file --textconv on HEAD (symlink)' '\n+\t! git cat-file --textconv HEAD:symlink.bin 2>result &&\n+\tcat >expected <\nCc: Clément Poulain \nCc: Diane Gasselin \nCc: Matthieu Moy \nCc: Jeff King \nSigned-off-by: Kirill Smelkov \n---\n\nv2:\n\n o Thanks to Matthieu and Jeff got a bit more sure I'm not doing stupid things,\n so\n o My XXX were removed, and the patch is no longer an RFC\n\nOtherwise no changes.\n\n builtin.h | 2 +-\n builtin/blame.c | 33 ++++++++++++++++++++++-----------\n builtin/cat-file.c | 2 +-\n sha1_name.c | 2 ++\n t/t8006-blame-textconv.sh | 6 ++----\n t/t8007-cat-file-textconv.sh | 6 ++----\n 6 files changed, 30 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin.h b/builtin.h\nindex 0398d24..9bf69ee 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -35,7 +35,7 @@ void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c);\n \n extern int check_pager_config(const char *cmd);\n \n-extern int textconv_object(const char *path, const unsigned char *sha1, char **buf, unsigned long *buf_size);\n+extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, char **buf, unsigned long *buf_size);\n \n extern int cmd_add(int argc, const char **argv, const char *prefix);\n extern int cmd_annotate(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 1015354..f5fccc1 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -83,6 +83,7 @@ struct origin {\n \tstruct commit *commit;\n \tmmfile_t file;\n \tunsigned char blob_sha1[20];\n+\tunsigned mode;\n \tchar path[FLEX_ARRAY];\n };\n \n@@ -92,6 +93,7 @@ struct origin {\n * Return 1 if the conversion succeeds, 0 otherwise.\n */\n int textconv_object(const char *path,\n+\t\t unsigned mode,\n \t\t const unsigned char *sha1,\n \t\t char **buf,\n \t\t unsigned long *buf_size)\n@@ -100,7 +102,7 @@ int textconv_object(const char *path,\n \tstruct userdiff_driver *textconv;\n \n \tdf = alloc_filespec(path);\n-\tfill_filespec(df, sha1, S_IFREG | 0664);\n+\tfill_filespec(df, sha1, mode);\n \ttextconv = get_textconv(df);\n \tif (!textconv) {\n \t\tfree_filespec(df);\n@@ -125,7 +127,7 @@ static void fill_origin_blob(struct diff_options *opt,\n \n \t\tnum_read_blob++;\n \t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n-\t\t textconv_object(o->path, o->blob_sha1, &file->ptr, &file_size))\n+\t\t textconv_object(o->path, o->mode, o->blob_sha1, &file->ptr, &file_size))\n \t\t\t;\n \t\telse\n \t\t\tfile->ptr = read_sha1_file(o->blob_sha1, &type, &file_size);\n@@ -313,21 +315,23 @@ static struct origin *get_origin(struct scoreboard *sb,\n * for an origin is also used to pass the blame for the entire file to\n * the parent to detect the case where a child's blob is identical to\n * that of its parent's.\n+ *\n+ * This also fills origin->mode for corresponding tree path.\n */\n-static int fill_blob_sha1(struct origin *origin)\n+static int fill_blob_sha1_and_mode(struct origin *origin)\n {\n-\tunsigned mode;\n \tif (!is_null_sha1(origin->blob_sha1))\n \t\treturn 0;\n \tif (get_tree_entry(origin->commit->object.sha1,\n \t\t\t origin->path,\n-\t\t\t origin->blob_sha1, &mode))\n+\t\t\t origin->blob_sha1, &origin->mode))\n \t\tgoto error_out;\n \tif (sha1_object_info(origin->blob_sha1, NULL) != OBJ_BLOB)\n \t\tgoto error_out;\n \treturn 0;\n error_out:\n \thashclr(origin->blob_sha1);\n+\torigin->mode = S_IFINVALID;\n \treturn -1;\n }\n \n@@ -360,12 +364,14 @@ static struct origin *find_origin(struct scoreboard *sb,\n \t\t\t/*\n \t\t\t * If the origin was newly created (i.e. get_origin\n \t\t\t * would call make_origin if none is found in the\n-\t\t\t * scoreboard), it does not know the blob_sha1,\n+\t\t\t * scoreboard), it does not know the blob_sha1/mode,\n \t\t\t * so copy it. Otherwise porigin was in the\n-\t\t\t * scoreboard and already knows blob_sha1.\n+\t\t\t * scoreboard and already knows blob_sha1/mode.\n \t\t\t */\n-\t\t\tif (porigin->refcnt == 1)\n+\t\t\tif (porigin->refcnt == 1) {\n \t\t\t\thashcpy(porigin->blob_sha1, cached->blob_sha1);\n+\t\t\t\tporigin->mode = cached->mode;\n+\t\t\t}\n \t\t\treturn porigin;\n \t\t}\n \t\t/* otherwise it was not very useful; free it */\n@@ -400,6 +406,7 @@ static struct origin *find_origin(struct scoreboard *sb,\n \t\t/* The path is the same as parent */\n \t\tporigin = get_origin(sb, parent, origin->path);\n \t\thashcpy(porigin->blob_sha1, origin->blob_sha1);\n+\t\tporigin->mode = origin->mode;\n \t} else {\n \t\t/*\n \t\t * Since origin->path is a pathspec, if the parent\n@@ -425,6 +432,7 @@ static struct origin *find_origin(struct scoreboard *sb,\n \t\tcase 'M':\n \t\t\tporigin = get_origin(sb, parent, origin->path);\n \t\t\thashcpy(porigin->blob_sha1, p->one->sha1);\n+\t\t\tporigin->mode = p->one->mode;\n \t\t\tbreak;\n \t\tcase 'A':\n \t\tcase 'T':\n@@ -444,6 +452,7 @@ static struct origin *find_origin(struct scoreboard *sb,\n \n \t\tcached = make_origin(porigin->commit, porigin->path);\n \t\thashcpy(cached->blob_sha1, porigin->blob_sha1);\n+\t\tcached->mode = porigin->mode;\n \t\tparent->util = cached;\n \t}\n \treturn porigin;\n@@ -486,6 +495,7 @@ static struct origin *find_rename(struct scoreboard *sb,\n \t\t !strcmp(p->two->path, origin->path)) {\n \t\t\tporigin = get_origin(sb, parent, p->one->path);\n \t\t\thashcpy(porigin->blob_sha1, p->one->sha1);\n+\t\t\tporigin->mode = p->one->mode;\n \t\t\tbreak;\n \t\t}\n \t}\n@@ -1099,6 +1109,7 @@ static int find_copy_in_parent(struct scoreboard *sb,\n \n \t\t\tnorigin = get_origin(sb, parent, p->one->path);\n \t\t\thashcpy(norigin->blob_sha1, p->one->sha1);\n+\t\t\tnorigin->mode = p->one->mode;\n \t\t\tfill_origin_blob(&sb->revs->diffopt, norigin, &file_p);\n \t\t\tif (!file_p.ptr)\n \t\t\t\tcontinue;\n@@ -2075,7 +2086,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t\tswitch (st.st_mode & S_IFMT) {\n \t\tcase S_IFREG:\n \t\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n-\t\t\t textconv_object(read_from, null_sha1, &buf.buf, &buf_len))\n+\t\t\t textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len))\n \t\t\t\tbuf.len = buf_len;\n \t\t\telse if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)\n \t\t\t\tdie_errno(\"cannot open or read '%s'\", read_from);\n@@ -2455,11 +2466,11 @@ parse_done:\n \t}\n \telse {\n \t\to = get_origin(&sb, sb.final, path);\n-\t\tif (fill_blob_sha1(o))\n+\t\tif (fill_blob_sha1_and_mode(o))\n \t\t\tdie(\"no such path %s in %s\", path, final_commit_name);\n \n \t\tif (DIFF_OPT_TST(&sb.revs->diffopt, ALLOW_TEXTCONV) &&\n-\t\t textconv_object(path, o->blob_sha1, (char **) &sb.final_buf,\n+\t\t textconv_object(path, o->mode, o->blob_sha1, (char **) &sb.final_buf,\n \t\t\t\t &sb.final_buf_size))\n \t\t\t;\n \t\telse\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 76ec3fe..94632db 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -143,7 +143,7 @@ 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, sha1, &buf, &size))\n+\t\tif (!textconv_object(obj_context.path, obj_context.mode, sha1, &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;\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 7b7e617..4458fe8 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1068,6 +1068,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\tstruct cache_entry *ce;\n \t\tint pos;\n \t\tif (namelen > 2 && name[1] == '/')\n+\t\t\t/* don't need mode for commit */\n \t\t\treturn get_sha1_oneline(name + 2, sha1);\n \t\tif (namelen < 3 ||\n \t\t name[2] != ':' ||\n@@ -1095,6 +1096,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\t\t\tbreak;\n \t\t\tif (ce_stage(ce) == stage) {\n \t\t\t\thashcpy(sha1, ce->sha1);\n+\t\t\t\toc->mode = ce->ce_mode;\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t\tpos++;\ndiff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh\nindex 7d42e96..400d9f1 100755\n--- a/t/t8006-blame-textconv.sh\n+++ b/t/t8006-blame-textconv.sh\n@@ -90,8 +90,7 @@ test_expect_success 'blame with --no-textconv (on symlink)' '\n \ttest_cmp expected result\n '\n \n-# fails with '...symlink.bin is not \"binary\" file'\n-test_expect_failure 'blame --textconv (on symlink)' '\n+test_expect_success 'blame --textconv (on symlink)' '\n \tgit blame --textconv symlink.bin >blame &&\n \tfind_blame result &&\n \ttest_cmp expected result\n@@ -110,8 +109,7 @@ EOF\n \tGIT_AUTHOR_NAME=Number4 git commit -a -m Fourth --date=\"2010-01-01 23:00:00\"\n '\n \n-# fails with '...symlink.bin is not \"binary\" file'\n-test_expect_failure 'blame on last commit (-C -C, symlink)' '\n+test_expect_success 'blame on last commit (-C -C, symlink)' '\n \tgit blame -C -C three.bin >blame &&\n \tfind_blame result &&\n \tcat >expected <<\\EOF &&\ndiff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh\nindex f747c05..40e4af5 100755\n--- a/t/t8007-cat-file-textconv.sh\n+++ b/t/t8007-cat-file-textconv.sh\n@@ -77,8 +77,7 @@ test_expect_success 'cat-file without --textconv (symlink)' '\n '\n \n \n-# fails because cat-file tries to run converter on symlink.bin\n-test_expect_failure 'cat-file --textconv on index (symlink)' '\n+test_expect_success '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@@ -86,8 +85,7 @@ EOF\n \ttest_cmp expected result\n '\n \n-# fails because cat-file tries to run converter on symlink.bin\n-test_expect_failure 'cat-file --textconv on HEAD (symlink)' '\n+test_expect_success 'cat-file --textconv on HEAD (symlink)' '\n \t! git cat-file --textconv HEAD:symlink.bin 2>result &&\n \tcat >expected < writes:\n\n> (Description partly by Matthieu Moy)\n\nBetter put such statements at the end, to avoid distracting the reader.\n\n> ~~~~\n>\n> NOTE: git diff doesn't try to textconv the pathnames, it runs the\n> textual diff without textconv, which is the expected behavior.\n\nIt's not clear whether this is intended to stay in the commit message.\nIf not, it should go below the ---. If yes, then I'd incorporate this\ninto the message itself. The ~~~~ and NOTE look odd.\n\nFor example (in next patch):\n\n| Instead get the mode from either worktree, index, .git, or origin\n| entries when blaming and pass it to textconv_object() as context.\n| \n| The reason to do it is not to run textconv filters on symlinks\n+ (just like \"git diff\" already does).\n\nAnyway, I'm bikeshedding. With or without these remarks,\n\nReviewed-by: Matthieu Moy \n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"151231","messageId":"20100921183959.GB4390@landau.phys.spbu.ru","threadId":"25173","inReplyTo":"vpqvd6086fq.fsf@bauges.imag.fr","subject":"Re: [PATCH v2 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-21T18:39:59Z","receivedAt":"2010-09-21T18:39:59Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Mon, Sep 20, 2010 at 11:13:13PM +0200, Matthieu Moy wrote:\n> Kirill Smelkov writes:\n> \n> > (Description partly by Matthieu Moy)\n> \n> Better put such statements at the end, to avoid distracting the reader.\n\nOk\n\n> > ~~~~\n> >\n> > NOTE: git diff doesn't try to textconv the pathnames, it runs the\n> > textual diff without textconv, which is the expected behavior.\n> \n> It's not clear whether this is intended to stay in the commit message.\n> If not, it should go below the ---. If yes, then I'd incorporate this\n> into the message itself. The ~~~~ and NOTE look odd.\n\nI know about --- and that content after it and up to patch itself goes\nto /dev/null. The text here was intended to stay in the commit message,\nand ~~~~ served as a separator in that message (git commit hook merges\nseveral blank lines into one, so one can't separate text parts with\nseveral empty lines, that's why I used this separator).\n\nIf it's ugly, let's omit it - I don't insist, but i don't understand why\n'NOTE:' looks odd?\n\n\nAnd also, do you remember your first question to this series? It was\nabout does git diff --textconv misbehaves too or not. So I consider this\nnote is important to put into description, and isn't the right place for\nsuch a note this patch, where show tests that demonstrate fauilures?\n\nThis note clearly says \"git diff is not affected, that's why we don't\nwrite new tests for it\".\n\nSomething like that...\n\n> \n> For example (in next patch):\n> \n> | Instead get the mode from either worktree, index, .git, or origin\n> | entries when blaming and pass it to textconv_object() as context.\n> | \n> | The reason to do it is not to run textconv filters on symlinks\n> + (just like \"git diff\" already does).\n\nSee above about my rationale on the note place.\n\n> Anyway, I'm bikeshedding. With or without these remarks,\n> \n> Reviewed-by: Matthieu Moy \n\nThanks :)\n\nIs it for this one patch, or does it apply to the whole series?\n\n\nThanks,\nKirill\n"},{"id":"151240","messageId":"vpqzkvaetq4.fsf@bauges.imag.fr","threadId":"25173","inReplyTo":"20100921183959.GB4390@landau.phys.spbu.ru","subject":"Re: [PATCH v2 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-09-21T20:18:11Z","receivedAt":"2010-09-21T20:18:11Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Kirill Smelkov writes:\n\n> I know about --- and that content after it and up to patch itself goes\n> to /dev/null. The text here was intended to stay in the commit message,\n> and ~~~~ served as a separator in that message (git commit hook merges\n> several blank lines into one, so one can't separate text parts with\n> several empty lines, that's why I used this separator).\n>\n> If it's ugly, let's omit it - I don't insist, but i don't understand why\n> 'NOTE:' looks odd?\n\nThat was especially about the combination of both, but I don't really\ncare. Consider my remarks as food for thoughts, not real objections.\n\n> This note clearly says \"git diff is not affected, that's why we don't\n> write new tests for it\".\n\nI disagree with the implication. Git diff is not affected because it\nwas done right, but behaving correctly doesn't mean you don't need\ntests. Checking the behavior of diff with tests wouldn't harm (but\nthat's not serious not to do it).\n\n>> Reviewed-by: Matthieu Moy \n>\n> Thanks :)\n>\n> Is it for this one patch, or does it apply to the whole series?\n\nTo the serie, yes.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"151248","messageId":"20100921204312.GC395@sigill.intra.peff.net","threadId":"25173","inReplyTo":"vpqzkvaetq4.fsf@bauges.imag.fr","subject":"Re: [PATCH v2 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-21T20:43:12Z","receivedAt":"2010-09-21T20:43:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2010 at 10:18:11PM +0200, Matthieu Moy wrote:\n\n> > This note clearly says \"git diff is not affected, that's why we don't\n> > write new tests for it\".\n> \n> I disagree with the implication. Git diff is not affected because it\n> was done right, but behaving correctly doesn't mean you don't need\n> tests. Checking the behavior of diff with tests wouldn't harm (but\n> that's not serious not to do it).\n\nActually, there is some breakage in \"git diff\" with userdiffs, just not\nfor textconv (but the fix covers both, so the problem is sort of a\nsuperset). I'm about to send a patch that has some tests, so don't\nworry about them for this series.\n\n-Peff\n"},{"id":"151519","messageId":"20100924182041.GA13943@landau.phys.spbu.ru","threadId":"25173","inReplyTo":"vpqzkvaetq4.fsf@bauges.imag.fr","subject":"Re: [PATCH v2 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-24T18:20:41Z","receivedAt":"2010-09-24T18:20:41Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Tue, Sep 21, 2010 at 10:18:11PM +0200, Matthieu Moy wrote:\n> Kirill Smelkov writes:\n> \n> > I know about --- and that content after it and up to patch itself goes\n> > to /dev/null. The text here was intended to stay in the commit message,\n> > and ~~~~ served as a separator in that message (git commit hook merges\n> > several blank lines into one, so one can't separate text parts with\n> > several empty lines, that's why I used this separator).\n> >\n> > If it's ugly, let's omit it - I don't insist, but i don't understand why\n> > 'NOTE:' looks odd?\n> \n> That was especially about the combination of both, but I don't really\n> care. Consider my remarks as food for thoughts, not real objections.\n\nOk\n\n> > This note clearly says \"git diff is not affected, that's why we don't\n> > write new tests for it\".\n> \n> I disagree with the implication. Git diff is not affected because it\n> was done right, but behaving correctly doesn't mean you don't need\n> tests. Checking the behavior of diff with tests wouldn't harm (but\n> that's not serious not to do it).\n\nYes, my implication was not correct, and I agree tests woudn't harm. But\nas Jeff already wrote in another mail, he'll test-cover the diff case,\nso I'll let myself to be lazy here (thanks Jeff!) :)\n\n> >> Reviewed-by: Matthieu Moy \n> >\n> > Thanks :)\n> >\n> > Is it for this one patch, or does it apply to the whole series?\n> \n> To the serie, yes.\n\nThanks. Will repost v3 soon. Hopefuly it will be ok to merge it then.\n\nKirill\n"}]}