{"thread":{"id":"25225","subject":"[PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","startedAt":"2010-09-24T18:24:07Z","lastAt":"2010-09-29T11:44:19Z","messageCount":18,"participants":["Kirill Smelkov","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"151521","messageId":"cover.1285351816.git.kirr@landau.phys.spbu.ru","threadId":"25225","inReplyTo":null,"subject":"[BUG, PATCH v3 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-24T18:24:07Z","receivedAt":"2010-09-24T18:24:07Z","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\n\nPlease apply and thanks,\nKirill\n\n\nv3:\n\n o Slightly changed patches descriptions as per comment by Matthieu, and added\n Matthieu's Reviewed-by.\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":"151520","messageId":"66d4603c7e21561557f612690d6196e7ae0b38f9.1285351816.git.kirr@landau.phys.spbu.ru","threadId":"25225","inReplyTo":"cover.1285351816.git.kirr@landau.phys.spbu.ru","subject":"[PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-24T18:24:08Z","receivedAt":"2010-09-24T18:24:08Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"The 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\n(Description by Matthieu Moy)\n\nCc: Axel Bonnet \nCc: Clément Poulain \nCc: Diane Gasselin \nCc: Matthieu Moy \nCc: Jeff King \nSigned-off-by: Kirill Smelkov \nReviewed-by: Matthieu Moy \n---\n\nv3:\n\n o Add Matthieu's Reviewed-by, and move secondary note to the end to avoid\n distracting an intrested reader.\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 \nReviewed-by: Matthieu Moy \n---\n\nv3:\n\n o Slight cleanup of the description\n o Reviewed-by: Matthieu\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 \nReviewed-by: Matthieu Moy \n---\n\nv3:\n\n o Reviewed-by: Matthieu\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\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> diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\n> index 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\nYou are not feeding arguments you think you are to the above \"echo\":\n\n $ cat >/var/tmp/j.sh <<\\EOF\n #!/bin/sh\n e () {\n i=0\n for s\n do\n i=$(( $i + 1 ))\n echo \"$i: $s\"\n done\n }\n f () {\n e \"E: $@ is not binary\"\n }\n f 1 \"2 3\" 4\n EOF\n $ sh /var/tmp/j.sh\n 1: E: 1\n 2: 2 3\n 3: 4 is not binary\n\nGranted, echo is forgiving and will concatenate the arguments it gets with\na space in between, but you would either want to either:\n\n (1) make it more explicit that helper gets only one argument, by saying\n \"$1\" instead of \"$@\", in all places in the helper script; or\n\n (2) if you are planning to make 'helper' capable of handling multiple\n input files, show the error message for the ones that are not binary\n (you would probably need a loop for that).\n\nI think (1) would be sufficient in this case.\n\n> +sed 's/^bin:/converted:/' \"$@\" >helper.out\n\nMinor nit: this is inconsistent with the check done with grep above that\ninsists that the colon is followed by a SP.\n"},{"id":"151851","messageId":"7vocbjyrbw.fsf@alter.siamese.dyndns.org","threadId":"25225","inReplyTo":"4805c18603a79e265c1bb1a1858ece8c33b17e2b.1285351816.git.kirr@landau.phys.spbu.ru","subject":"Re: [PATCH v3 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-27T18:27:47Z","receivedAt":"2010-09-27T18:27:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov writes:\n\n> diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh\n> index d0f8d62..7d42e96 100755\n> --- a/t/t8006-blame-textconv.sh\n> +++ b/t/t8006-blame-textconv.sh\n> @@ -17,10 +17,12 @@ chmod +x helper\n> test_expect_success 'setup ' '\n> \techo \"bin: test 1\" >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\nDoesn't this need some test prereq marker?\n"},{"id":"151901","messageId":"20100928120722.GA29525@landau.phys.spbu.ru","threadId":"25225","inReplyTo":"7vsk0vyriw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-28T12:07:22Z","receivedAt":"2010-09-28T12:07:22Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Mon, Sep 27, 2010 at 11:23:35AM -0700, Junio C Hamano wrote:\n> Kirill Smelkov writes:\n\n\n> > +sed 's/^bin:/converted:/' \"$@\" >helper.out\n> \n> Minor nit: this is inconsistent with the check done with grep above that\n> insists that the colon is followed by a SP.\n\nYes, you are right. I'll amend it.\n\n> > diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\n> > index 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> \n> You are not feeding arguments you think you are to the above \"echo\":\n> \n> $ cat >/var/tmp/j.sh <<\\EOF\n> #!/bin/sh\n> e () {\n> i=0\n> for s\n> do\n> i=$(( $i + 1 ))\n> echo \"$i: $s\"\n> done\n> }\n> f () {\n> e \"E: $@ is not binary\"\n> }\n> f 1 \"2 3\" 4\n> EOF\n> $ sh /var/tmp/j.sh\n> 1: E: 1\n> 2: 2 3\n> 3: 4 is not binary\n> \n> Granted, echo is forgiving and will concatenate the arguments it gets with\n> a space in between, but you would either want to either:\n> \n> (1) make it more explicit that helper gets only one argument, by saying\n> \"$1\" instead of \"$@\", in all places in the helper script; or\n> \n> (2) if you are planning to make 'helper' capable of handling multiple\n> input files, show the error message for the ones that are not binary\n> (you would probably need a loop for that).\n> \n> I think (1) would be sufficient in this case.\n\nI too think (1) is right. It was just that originally there was $@\n(which I now understand was wrong). So ok to apply the following patch\non top of this series? (I assume it's ok, and will repost the whole\nthing)\n\n---- 8< ----\n\nFrom: Kirill Smelkov \nDate: Tue, 28 Sep 2010 15:34:48 +0400\nSubject: [PATCH 4/4] t4042,t8006,t8007: Textconv converter should take only one argument\n\nTextconv helper in this tests was incorrectly referencing $@, which goes\nagains textconv \"spec\". I quote Documentation/gitattributes.txt\n\n\"\"\" The `textconv` config option is used to define a program for\n performing such a conversion. The program should take a single\n argument, the name of a file to convert, and produce the\n resulting text on stdout. \"\"\"\n\nSo correct textconv helpers to use $1 instead.\n\nNoticed-by: Junio C Hamano \nSigned-off-by: Kirill Smelkov \n---\n t/t4042-diff-textconv-caching.sh | 4 ++--\n t/t8006-blame-textconv.sh | 4 ++--\n t/t8007-cat-file-textconv.sh | 4 ++--\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\nindex 68fee12..6aaa10b 100755\n--- a/t/t4042-diff-textconv-caching.sh\n+++ b/t/t4042-diff-textconv-caching.sh\n@@ -5,8 +5,8 @@ test_description='test textconv caching'\n \n cat >helper <<'EOF'\n #!/bin/sh\n-grep -q '^bin: ' \"$@\" || { echo \"E: $@ is not \\\"binary\\\" file\" 1>&2; exit 1; }\n-sed 's/^bin: /converted: /' \"$@\" >helper.out\n+grep -q '^bin: ' \"$1\" || { echo \"E: $1 is not \\\"binary\\\" file\" 1>&2; exit 1; }\n+sed 's/^bin: /converted: /' \"$1\" >helper.out\n cat helper.out\n EOF\n chmod +x helper\ndiff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh\nindex a8c311f..dbf623b 100755\n--- a/t/t8006-blame-textconv.sh\n+++ b/t/t8006-blame-textconv.sh\n@@ -9,8 +9,8 @@ find_blame() {\n \n cat >helper <<'EOF'\n #!/bin/sh\n-grep -q '^bin: ' \"$@\" || { echo \"E: $@ is not \\\"binary\\\" file\" 1>&2; exit 1; }\n-sed 's/^bin: /converted: /' \"$@\"\n+grep -q '^bin: ' \"$1\" || { echo \"E: $1 is not \\\"binary\\\" file\" 1>&2; exit 1; }\n+sed 's/^bin: /converted: /' \"$1\"\n EOF\n chmod +x helper\n \ndiff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh\nindex 5e2e0d2..78a0085 100755\n--- a/t/t8007-cat-file-textconv.sh\n+++ b/t/t8007-cat-file-textconv.sh\n@@ -5,8 +5,8 @@ test_description='git cat-file textconv support'\n \n cat >helper <<'EOF'\n #!/bin/sh\n-grep -q '^bin: ' \"$@\" || { echo \"E: $@ is not \\\"binary\\\" file\" 1>&2; exit 1; }\n-sed 's/^bin: /converted: /' \"$@\"\n+grep -q '^bin: ' \"$1\" || { echo \"E: $1 is not \\\"binary\\\" file\" 1>&2; exit 1; }\n+sed 's/^bin: /converted: /' \"$1\"\n EOF\n chmod +x helper\n \n-- \n1.7.3.19.g3fe0a\n"},{"id":"151902","messageId":"20100928120730.GB29525@landau.phys.spbu.ru","threadId":"25225","inReplyTo":"7vocbjyrbw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 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-28T12:07:30Z","receivedAt":"2010-09-28T12:07:30Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Mon, Sep 27, 2010 at 11:27:47AM -0700, Junio C Hamano wrote:\n> Kirill Smelkov writes:\n> \n> > diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh\n> > index d0f8d62..7d42e96 100755\n> > --- a/t/t8006-blame-textconv.sh\n> > +++ b/t/t8006-blame-textconv.sh\n> > @@ -17,10 +17,12 @@ chmod +x helper\n> > test_expect_success 'setup ' '\n> > \techo \"bin: test 1\" >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> Doesn't this need some test prereq marker?\n\nYes, I forgot about symlink prereqs.\n\nBelow is interdiff of the fixes to this patch. Updated series with all\nqueued fixes will be reposted shortly.\n \n\nThanks,\nKirill\n\n\n---- 8< ----\n\ndiff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh\nindex 917d362..324f127 100755\n--- a/t/t8006-blame-textconv.sh\n+++ b/t/t8006-blame-textconv.sh\n@@ -17,12 +17,16 @@ chmod +x helper\n test_expect_success 'setup ' '\n \techo \"bin: test 1\" >one.bin &&\n \techo \"bin: test number 2\" >two.bin &&\n-\tln -s one.bin symlink.bin &&\n+\tif test_have_prereq SYMLINKS; then\n+\t\tln -s one.bin symlink.bin\n+\tfi &&\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+\tif test_have_prereq SYMLINKS; then\n+\t\tln -sf two.bin symlink.bin\n+\tfi &&\n \tGIT_AUTHOR_NAME=Number2 git commit -a -m Second --date=\"2010-01-01 20:00:00\"\n '\n \n@@ -84,14 +88,14 @@ 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+test_expect_failure SYMLINKS 'blame --textconv (on symlink)' '\n \tgit blame --textconv symlink.bin >blame &&\n \tfind_blame result &&\n \ttest_cmp expected result\n@@ -99,7 +103,7 @@ test_expect_failure 'blame --textconv (on symlink)' '\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+test_expect_success SYMLINKS 'make another new commit' '\n \tcat >three.bin <<\\EOF &&\n bin: test number 2\n bin: test number 2 version 2\n@@ -111,7 +115,7 @@ EOF\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_failure SYMLINKS '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 7bec265..03af7ae 100755\n--- a/t/t8007-cat-file-textconv.sh\n+++ b/t/t8007-cat-file-textconv.sh\n@@ -12,7 +12,9 @@ chmod +x helper\n \n test_expect_success 'setup ' '\n \techo \"bin: test\" >one.bin &&\n-\tln -s one.bin symlink.bin &&\n+\tif test_have_prereq SYMLINKS; then\n+\t\tln -s one.bin symlink.bin\n+\tfi &&\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@@ -70,7 +72,7 @@ test_expect_success 'cat-file --textconv on previous commit' '\n \ttest_cmp expected result\n '\n \n-test_expect_success 'cat-file without --textconv (symlink)' '\n+test_expect_success SYMLINKS '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@@ -78,7 +80,7 @@ test_expect_success 'cat-file without --textconv (symlink)' '\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_failure 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@@ -87,7 +89,7 @@ EOF\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_failure SYMLINKS 'cat-file --textconv on HEAD (symlink)' '\n \t! git cat-file --textconv HEAD:symlink.bin 2>result &&\n \tcat >expected < On Mon, Sep 27, 2010 at 11:23:35AM -0700, Junio C Hamano wrote:\n> > Kirill Smelkov writes:\n> \n> \n> > > +sed 's/^bin:/converted:/' \"$@\" >helper.out\n> > \n> > Minor nit: this is inconsistent with the check done with grep above that\n> > insists that the colon is followed by a SP.\n> \n> Yes, you are right. I'll amend it.\n> \n> > > diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\n> > > index 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> > \n> > You are not feeding arguments you think you are to the above \"echo\":\n> > \n> > $ cat >/var/tmp/j.sh <<\\EOF\n> > #!/bin/sh\n> > e () {\n> > i=0\n> > for s\n> > do\n> > i=$(( $i + 1 ))\n> > echo \"$i: $s\"\n> > done\n> > }\n> > f () {\n> > e \"E: $@ is not binary\"\n> > }\n> > f 1 \"2 3\" 4\n> > EOF\n> > $ sh /var/tmp/j.sh\n> > 1: E: 1\n> > 2: 2 3\n> > 3: 4 is not binary\n> > \n> > Granted, echo is forgiving and will concatenate the arguments it gets with\n> > a space in between, but you would either want to either:\n> > \n> > (1) make it more explicit that helper gets only one argument, by saying\n> > \"$1\" instead of \"$@\", in all places in the helper script; or\n> > \n> > (2) if you are planning to make 'helper' capable of handling multiple\n> > input files, show the error message for the ones that are not binary\n> > (you would probably need a loop for that).\n> > \n> > I think (1) would be sufficient in this case.\n> \n> I too think (1) is right. It was just that originally there was $@\n> (which I now understand was wrong). So ok to apply the following patch\n> on top of this series? (I assume it's ok, and will repost the whole\n> thing)\n> \n> ---- 8< ----\n> \n> From: Kirill Smelkov \n> Date: Tue, 28 Sep 2010 15:34:48 +0400\n> Subject: [PATCH 4/4] t4042,t8006,t8007: Textconv converter should take only one argument\n> \n> Textconv helper in this tests was incorrectly referencing $@, which goes\n> agains textconv \"spec\". I quote Documentation/gitattributes.txt\n> \n> \"\"\" The `textconv` config option is used to define a program for\n> performing such a conversion. The program should take a single\n> argument, the name of a file to convert, and produce the\n> resulting text on stdout. \"\"\"\n> \n> So correct textconv helpers to use $1 instead.\n> \n> Noticed-by: Junio C Hamano \n> Signed-off-by: Kirill Smelkov \n> ---\n> t/t4042-diff-textconv-caching.sh | 4 ++--\n> t/t8006-blame-textconv.sh | 4 ++--\n> t/t8007-cat-file-textconv.sh | 4 ++--\n> 3 files changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\n> index 68fee12..6aaa10b 100755\n> --- a/t/t4042-diff-textconv-caching.sh\n> +++ b/t/t4042-diff-textconv-caching.sh\n> @@ -5,8 +5,8 @@ test_description='test textconv caching'\n> \n> cat >helper <<'EOF'\n> #!/bin/sh\n> -grep -q '^bin: ' \"$@\" || { echo \"E: $@ is not \\\"binary\\\" file\" 1>&2; exit 1; }\n> -sed 's/^bin: /converted: /' \"$@\" >helper.out\n> +grep -q '^bin: ' \"$1\" || { echo \"E: $1 is not \\\"binary\\\" file\" 1>&2; exit 1; }\n> +sed 's/^bin: /converted: /' \"$1\" >helper.out\n> cat helper.out\n> EOF\n> chmod +x helper\n\nPlease ignore this - changing so breaks textconv cache tests in this\nfile.\n\nI'll come up with updated patch.\n\nSorry for the noise...\n"},{"id":"151906","messageId":"20100928124148.GA29882@landau.phys.spbu.ru","threadId":"25225","inReplyTo":"20100928122031.GC29525@landau.phys.spbu.ru","subject":"Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-28T12:41:48Z","receivedAt":"2010-09-28T12:41:48Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Tue, Sep 28, 2010 at 04:20:31PM +0400, Kirill Smelkov wrote:\n> On Tue, Sep 28, 2010 at 04:07:22PM +0400, Kirill Smelkov wrote:\n> > diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\n> > index 68fee12..6aaa10b 100755\n> > --- a/t/t4042-diff-textconv-caching.sh\n> > +++ b/t/t4042-diff-textconv-caching.sh\n> > @@ -5,8 +5,8 @@ test_description='test textconv caching'\n> > \n> > cat >helper <<'EOF'\n> > #!/bin/sh\n> > -grep -q '^bin: ' \"$@\" || { echo \"E: $@ is not \\\"binary\\\" file\" 1>&2; exit 1; }\n> > -sed 's/^bin: /converted: /' \"$@\" >helper.out\n> > +grep -q '^bin: ' \"$1\" || { echo \"E: $1 is not \\\"binary\\\" file\" 1>&2; exit 1; }\n> > +sed 's/^bin: /converted: /' \"$1\" >helper.out\n> > cat helper.out\n> > EOF\n> > chmod +x helper\n> \n> Please ignore this - changing so breaks textconv cache tests in this\n> file.\n\nOk, here is what also is needed here\n\n---- 8< ----\n\n NOTE: in t4042, I had to correct args hack, and just roll second\n textconv helper for cache-invalidates-on-textconv-change test.\n\n ...\n\ndiff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\nindex 6aaa10b..c362261 100755\n--- a/t/t4042-diff-textconv-caching.sh\n+++ b/t/t4042-diff-textconv-caching.sh\n@@ -9,7 +9,12 @@ grep -q '^bin: ' \"$1\" || { echo \"E: $1 is not \\\"binary\\\" file\" 1>&2; exit 1; }\n sed 's/^bin: /converted: /' \"$1\" >helper.out\n cat helper.out\n EOF\n-chmod +x helper\n+cat >helper2 <<'EOF'\n+#!/bin/sh\n+echo \"converted: other\"\n+./helper \"$1\"\n+EOF\n+chmod +x helper helper2\n \n test_expect_success 'setup' '\n \techo \"bin: foo content 1\" >foo.bin &&\n@@ -77,8 +82,7 @@ index 255496b..ad450ff 100644\n +converted: foo content 2\n EOF\n test_expect_success 'changing textconv invalidates cache' '\n-\techo \"bin: other\" >other &&\n-\tgit config diff.magic.textconv \"./helper other\" &&\n+\tgit config diff.magic.textconv ./helper2 &&\n \tgit diff HEAD^ HEAD >actual &&\n \ttest_cmp expect actual\n '\n"},{"id":"151911","messageId":"20100928132356.GA5829@sigill.intra.peff.net","threadId":"25225","inReplyTo":"20100928120722.GA29525@landau.phys.spbu.ru","subject":"Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-28T13:23:57Z","receivedAt":"2010-09-28T13:23:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 28, 2010 at 04:07:22PM +0400, Kirill Smelkov wrote:\n\n> > Granted, echo is forgiving and will concatenate the arguments it gets with\n> > a space in between, but you would either want to either:\n> > \n> > (1) make it more explicit that helper gets only one argument, by saying\n> > \"$1\" instead of \"$@\", in all places in the helper script; or\n> > \n> > (2) if you are planning to make 'helper' capable of handling multiple\n> > input files, show the error message for the ones that are not binary\n> > (you would probably need a loop for that).\n> > \n> > I think (1) would be sufficient in this case.\n> \n> I too think (1) is right. It was just that originally there was $@\n> (which I now understand was wrong). So ok to apply the following patch\n> on top of this series? (I assume it's ok, and will repost the whole\n> thing)\n\nNo, \"helper\" is supposed to be able to take multiple arguments, at least\nin t4042. See the \"changing textconv invalidates cache\" test. The extra\nargument comes from the user, not from git.\n\n> t/t4042-diff-textconv-caching.sh | 4 ++--\n\nWhy are we touching t4042 at all in this series? We are not actually\nadding any tests to it, AFAICT.\n\n> t/t8006-blame-textconv.sh | 4 ++--\n> t/t8007-cat-file-textconv.sh | 4 ++--\n\nThese ones simply copied the helper script, and could be switched to use\n$1.\n\n-Peff\n"},{"id":"151915","messageId":"20100928143540.GA30357@landau.phys.spbu.ru","threadId":"25225","inReplyTo":"20100928132356.GA5829@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-28T14:35:40Z","receivedAt":"2010-09-28T14:35:40Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Tue, Sep 28, 2010 at 09:23:57AM -0400, Jeff King wrote:\n> On Tue, Sep 28, 2010 at 04:07:22PM +0400, Kirill Smelkov wrote:\n> \n> > > Granted, echo is forgiving and will concatenate the arguments it gets with\n> > > a space in between, but you would either want to either:\n> > > \n> > > (1) make it more explicit that helper gets only one argument, by saying\n> > > \"$1\" instead of \"$@\", in all places in the helper script; or\n> > > \n> > > (2) if you are planning to make 'helper' capable of handling multiple\n> > > input files, show the error message for the ones that are not binary\n> > > (you would probably need a loop for that).\n> > > \n> > > I think (1) would be sufficient in this case.\n> > \n> > I too think (1) is right. It was just that originally there was $@\n> > (which I now understand was wrong). So ok to apply the following patch\n> > on top of this series? (I assume it's ok, and will repost the whole\n> > thing)\n> \n> No, \"helper\" is supposed to be able to take multiple arguments, at least\n> in t4042. See the \"changing textconv invalidates cache\" test. The extra\n> argument comes from the user, not from git.\n\nYes, I've reworked that in my v4/patch4 post.\n\n> > t/t4042-diff-textconv-caching.sh | 4 ++--\n> \n> Why are we touching t4042 at all in this series? We are not actually\n> adding any tests to it, AFAICT.\n\nBecause we want to catch potential wrong textconv invocation on non\n\"bin: \" files there too?\n\n\nThanks,\nKirill\n"},{"id":"151916","messageId":"20100928143928.GA8918@sigill.intra.peff.net","threadId":"25225","inReplyTo":"20100928143540.GA30357@landau.phys.spbu.ru","subject":"Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-28T14:39:28Z","receivedAt":"2010-09-28T14:39:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 28, 2010 at 06:35:40PM +0400, Kirill Smelkov wrote:\n\n> > > t/t4042-diff-textconv-caching.sh | 4 ++--\n> > \n> > Why are we touching t4042 at all in this series? We are not actually\n> > adding any tests to it, AFAICT.\n> \n> Because we want to catch potential wrong textconv invocation on non\n> \"bin: \" files there too?\n\nBut we don't actually add any tests that display the problem there, do\nwe? And even if we wanted to test the diff implementation, wouldn't\nt4030 be the write place to do that? t4042 is specifically about\ntextconv caching.\n\n-Peff\n"},{"id":"151922","messageId":"20100928150910.GA30544@landau.phys.spbu.ru","threadId":"25225","inReplyTo":"20100928143928.GA8918@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-28T15:09:10Z","receivedAt":"2010-09-28T15:09:10Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Tue, Sep 28, 2010 at 10:39:28AM -0400, Jeff King wrote:\n> On Tue, Sep 28, 2010 at 06:35:40PM +0400, Kirill Smelkov wrote:\n> \n> > > > t/t4042-diff-textconv-caching.sh | 4 ++--\n> > > \n> > > Why are we touching t4042 at all in this series? We are not actually\n> > > adding any tests to it, AFAICT.\n> > \n> > Because we want to catch potential wrong textconv invocation on non\n> > \"bin: \" files there too?\n> \n> But we don't actually add any tests that display the problem there, do\n> we? And even if we wanted to test the diff implementation, wouldn't\n> t4030 be the write place to do that? t4042 is specifically about\n> textconv caching.\n\nYes, I hadn't added new tests there, but at least I've changed\nalready-in-there helper to bail out if it is called on non \"binary\"\nfiles, so a small step, but still step forward, no?\n\nAnd I've changed that helper after doing `git grep` for textconv, and\nideally we shouldn't keep those copy-pasted helpers in several tests (we\nhave 3 at present - in t4042, t8006 and t8007), but move them into one\ncommon place some day.\n\nSo in order to at least keep copies consistent between each other, I've\nchanged them all in uniform manner.\n\nThat was my rationale.\n\n\nThanks,\nKirill\n"},{"id":"151924","messageId":"7vlj6lubg1.fsf@alter.siamese.dyndns.org","threadId":"25225","inReplyTo":"20100928120722.GA29525@landau.phys.spbu.ru","subject":"Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-28T15:36:46Z","receivedAt":"2010-09-28T15:36:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov writes:\n\n> On Mon, Sep 27, 2010 at 11:23:35AM -0700, Junio C Hamano wrote:\n>> Kirill Smelkov writes:\n>\n>\n>> > +sed 's/^bin:/converted:/' \"$@\" >helper.out\n>> \n>> Minor nit: this is inconsistent with the check done with grep above that\n>> insists that the colon is followed by a SP.\n>\n> Yes, you are right. I'll amend it.\n>\n>> > diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\n>> > index 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>> ...\n> ...\n> I too think (1) is right. It was just that originally there was $@\n> (which I now understand was wrong).\n\nWell, the original's use of \"$@\" is perfectly fine; it would do the right\nthing with one argument, of course, but it would do the right thing with\nmore than one, too. On the other hand, your use inside \"echo\" is not.\n"},{"id":"151927","messageId":"20100928155845.GA10271@sigill.intra.peff.net","threadId":"25225","inReplyTo":"7vlj6lubg1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-28T15:58:45Z","receivedAt":"2010-09-28T15:58:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 28, 2010 at 08:36:46AM -0700, Junio C Hamano wrote:\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> >> ...\n> > ...\n> > I too think (1) is right. It was just that originally there was $@\n> > (which I now understand was wrong).\n> \n> Well, the original's use of \"$@\" is perfectly fine; it would do the right\n> thing with one argument, of course, but it would do the right thing with\n> more than one, too. On the other hand, your use inside \"echo\" is not.\n\nMoreover, the use of \"grep\" is wrong. Giving it two files, one of which\nhas \"^bin: \" and one of which doesn't, will silently accept the latter.\nIf it's going to handle multiple files, it must be a for-loop (or you\ncould invert \"grep -qv\", but I think that might be getting too clever to\nremain readable).\n\nWhich is why I suggested just dropping the t4042 bit, which is the only\npart that actually needs to handle multiple arguments. The other ones\ncan just switch to using \"$1\". The helper script is simple enough that\nthere is no need for them to share the same code.\n\n-Peff\n"},{"id":"151930","messageId":"20100928161210.GA31036@landau.phys.spbu.ru","threadId":"25225","inReplyTo":"20100928155845.GA10271@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-28T16:12:10Z","receivedAt":"2010-09-28T16:12:10Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Tue, Sep 28, 2010 at 11:58:45AM -0400, Jeff King wrote:\n> On Tue, Sep 28, 2010 at 08:36:46AM -0700, Junio C Hamano wrote:\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> > >> ...\n> > > ...\n> > > I too think (1) is right. It was just that originally there was $@\n> > > (which I now understand was wrong).\n> > \n> > Well, the original's use of \"$@\" is perfectly fine; it would do the right\n> > thing with one argument, of course, but it would do the right thing with\n> > more than one, too. On the other hand, your use inside \"echo\" is not.\n> \n> Moreover, the use of \"grep\" is wrong. Giving it two files, one of which\n> has \"^bin: \" and one of which doesn't, will silently accept the latter.\n> If it's going to handle multiple files, it must be a for-loop (or you\n> could invert \"grep -qv\", but I think that might be getting too clever to\n> remain readable).\n> \n> Which is why I suggested just dropping the t4042 bit, which is the only\n> part that actually needs to handle multiple arguments. The other ones\n> can just switch to using \"$1\". The helper script is simple enough that\n> there is no need for them to share the same code.\n\nLet's do whatever is appropriate. I just wanted to be consitent, but I'm\ndoing mistakes becase I'm overworked and have time pressure on other\ntasks. (sorry).\n\nIf it would be more convenient to just drop t4042 - let's do it.\n\nI could amend the patch, or is it ok to just kill that portion on top of\nwhat is already in pu?\n\n\nSorry for dummy me,\nKirill\n"},{"id":"152045","messageId":"20100929114419.GA11819@landau.phys.spbu.ru","threadId":"25225","inReplyTo":"20100928161210.GA31036@landau.phys.spbu.ru","subject":"Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-29T11:44:19Z","receivedAt":"2010-09-29T11:44:19Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Tue, Sep 28, 2010 at 08:12:10PM +0400, Kirill Smelkov wrote:\n> On Tue, Sep 28, 2010 at 11:58:45AM -0400, Jeff King wrote:\n> > On Tue, Sep 28, 2010 at 08:36:46AM -0700, Junio C Hamano wrote:\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> > > >> ...\n> > > > ...\n> > > > I too think (1) is right. It was just that originally there was $@\n> > > > (which I now understand was wrong).\n> > > \n> > > Well, the original's use of \"$@\" is perfectly fine; it would do the right\n> > > thing with one argument, of course, but it would do the right thing with\n> > > more than one, too. On the other hand, your use inside \"echo\" is not.\n> > \n> > Moreover, the use of \"grep\" is wrong. Giving it two files, one of which\n> > has \"^bin: \" and one of which doesn't, will silently accept the latter.\n> > If it's going to handle multiple files, it must be a for-loop (or you\n> > could invert \"grep -qv\", but I think that might be getting too clever to\n> > remain readable).\n> > \n> > Which is why I suggested just dropping the t4042 bit, which is the only\n> > part that actually needs to handle multiple arguments. The other ones\n> > can just switch to using \"$1\". The helper script is simple enough that\n> > there is no need for them to share the same code.\n> \n> Let's do whatever is appropriate. I just wanted to be consitent, but I'm\n> doing mistakes becase I'm overworked and have time pressure on other\n> tasks. (sorry).\n> \n> If it would be more convenient to just drop t4042 - let's do it.\n\nUpdate:\n\nI've removed t4042 bits from this series, and also squashed $@ -> $1\nconversion in the first patch. The whole thing is reposted as v5.\n\nSorry if maybe there are some bugs still left...\nKirill\n"}]}