{"thread":{"id":"25145","subject":"[BUG, PATCH 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","startedAt":"2010-09-18T17:25:03Z","lastAt":"2010-09-24T16:43:59Z","messageCount":35,"participants":["Kirill Smelkov","Matthieu Moy","Junio C Hamano","Jeff King","Johannes Sixt","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"150959","messageId":"cover.1284830388.git.kirr@landau.phys.spbu.ru","threadId":"25145","inReplyTo":null,"subject":"[BUG, PATCH 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-18T17:25:03Z","receivedAt":"2010-09-18T17:25:03Z","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\nFirst 2 patches demonstrate the problem (and are ready to go into git.git),\nthough third, while working for me, is only an RFC.\n\nThanks,\nKirill\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 RFC: blame,cat-file --textconv: Don't assume mode is ``S_IFREF |\n 0664''\n\n builtin.h | 2 +-\n builtin/blame.c | 33 ++++++++++++++-------\n builtin/cat-file.c | 2 +-\n sha1_name.c | 3 +-\n t/t4042-diff-textconv-caching.sh | 25 ++++++++--------\n t/t8006-blame-textconv.sh | 57 +++++++++++++++++++++++++++++++++----\n t/t8007-cat-file-textconv.sh | 38 ++++++++++++++++++++++---\n 7 files changed, 122 insertions(+), 38 deletions(-)\n\n-- \n1.7.3.rc2\n"},{"id":"150961","messageId":"26d0544dac2515e76bee0608881cfd8c23bf1ebf.1284830388.git.kirr@landau.phys.spbu.ru","threadId":"25145","inReplyTo":"cover.1284830388.git.kirr@landau.phys.spbu.ru","subject":"[PATCH 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-18T17:25:04Z","receivedAt":"2010-09-18T17:25:04Z","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\nSo git-blame is wrong here, and I'm going to write problem-demonstration\ntests + try to fix it, but first we have to convert existing textconv\nconverter, so it will mimic pdftext behaviour -- if there is no '^bin:'\nin input -- it's not a \"binary\" file and helper exits with error.\n\nNo other semantic changes at this stage.\n\nCc: Axel Bonnet \nCc: Clément Poulain \nCc: Diane Gasselin \nCc: Jeff King \nSigned-off-by: Kirill Smelkov \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 \nSigned-off-by: Kirill Smelkov \n---\n t/t8006-blame-textconv.sh | 44 ++++++++++++++++++++++++++++++++++++++++++\n t/t8007-cat-file-textconv.sh | 29 +++++++++++++++++++++++++++\n 2 files changed, 73 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh\nindex d0f8d62..3ed6155 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@@ -78,4 +80,46 @@ 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+\techo \"bin: test number 2\" >three.bin &&\n+\techo \"bin: test number 2 version 2\" >>three.bin &&\n+\techo \"bin: test number 2 version 3\" >>three.bin &&\n+\techo \"bin: test number 3\" >>three.bin &&\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+cat >expected <blame &&\n+\tfind_blame result &&\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..dfb2b04 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,32 @@ 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+echo -n \"one.bin\" >expected\n+\n+test_expect_success 'cat-file without --textconv (symlink)' '\n+\tgit cat-file blob :symlink.bin >result &&\n+\ttest_cmp expected result\n+'\n+\n+cat >expected <result &&\n+\ttest_cmp expected result\n+'\n+\n+cat >expected <result &&\n+\ttest_cmp expected result\n+'\n+\n test_done\n-- \n1.7.3.rc2\n"},{"id":"150962","messageId":"7ab60e32582447ad792602ff405dcee464ef1414.1284830388.git.kirr@landau.phys.spbu.ru","threadId":"25145","inReplyTo":"cover.1284830388.git.kirr@landau.phys.spbu.ru","subject":"[PATCH 3/3] RFC: blame,cat-file --textconv: Don't assume mode is ``S_IFREF | 0664''","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-18T17:25:06Z","receivedAt":"2010-09-18T17:25:06Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Instead get the mode from either worktree, index, .git, or origin\nentries when blaming and pass it to textconv_object() as context.\n\nThe reason to do it is not to run textconv filters on symlinks.\n\n~~~~\n\nI don't know blame code well, and I'm not sure I'm doing it right or\notherwise without mistakes. Thus an RFC.\n\nCc: Axel Bonnet \nCc: Clément Poulain \nCc: Diane Gasselin \nSigned-off-by: Kirill Smelkov \n---\n builtin.h | 2 +-\n builtin/blame.c | 33 ++++++++++++++++++++++-----------\n builtin/cat-file.c | 2 +-\n sha1_name.c | 3 ++-\n t/t8006-blame-textconv.sh | 6 ++----\n t/t8007-cat-file-textconv.sh | 6 ++----\n 6 files changed, 30 insertions(+), 22 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..cfb7d30 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 correspoinding 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..5470a69 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1068,7 +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\treturn get_sha1_oneline(name + 2, sha1);\n+\t\t\treturn get_sha1_oneline(name + 2, sha1); /* XXX also mode? */\n \t\tif (namelen < 3 ||\n \t\t name[2] != ':' ||\n \t\t name[1] < '0' || '3' < name[1])\n@@ -1095,6 +1095,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; /* XXX ok? */\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 3ed6155..43074d1 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@@ -115,8 +114,7 @@ cat >expected <blame &&\n \tfind_blame result &&\n \ttest_cmp expected result\ndiff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh\nindex dfb2b04..2b3d56a 100755\n--- a/t/t8007-cat-file-textconv.sh\n+++ b/t/t8007-cat-file-textconv.sh\n@@ -81,8 +81,7 @@ cat >expected <result &&\n \ttest_cmp expected result\n '\n@@ -91,8 +90,7 @@ cat >expected <result &&\n \ttest_cmp expected result\n '\n-- \n1.7.3.rc2\n"},{"id":"150963","messageId":"vpqhbhmx6tg.fsf@bauges.imag.fr","threadId":"25145","inReplyTo":"cover.1284830388.git.kirr@landau.phys.spbu.ru","subject":"Re: [BUG, PATCH 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-09-18T18:08:59Z","receivedAt":"2010-09-18T18:08:59Z","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> Recently I've spot a bug in git blame --textconv, which was wrongly\n> calling pdftotext (my *.pdf conversion program) on a symlink.pdf,\n\nNo time for a detailed review now, but textconv is also called by diff\n(that's the original implementation). Does \"git diff\" work properly on\nsymlinks in the same case?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"150964","messageId":"vpqmxrevppb.fsf@bauges.imag.fr","threadId":"25145","inReplyTo":"7ab60e32582447ad792602ff405dcee464ef1414.1284830388.git.kirr@landau.phys.spbu.ru","subject":"Re: [PATCH 3/3] RFC: blame,cat-file --textconv: Don't assume mode is ``S_IFREF | 0664''","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-09-18T19:04:00Z","receivedAt":"2010-09-18T19:04:00Z","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 don't know blame code well, and I'm not sure I'm doing it right or\n> otherwise without mistakes. Thus an RFC.\n\nI don't know the code well either, but I didn't see any obvious\nproblem in your code.\n\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -1068,7 +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\treturn get_sha1_oneline(name + 2, sha1);\n> +\t\t\treturn get_sha1_oneline(name + 2, sha1); /* XXX also mode? */\n\n(This is the case where we're parsing \":/foo\")\n\nCurrently, the mode is set a few lines above:\n\n\toc->mode = S_IFINVALID;\n\nI guess this is OK since :/foo will return a commit sha1, not a file\nsha1.\n\n> @@ -1095,6 +1095,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; /* XXX ok? */\n\nI'd say this is OK, you're setting the mode from the index. What was\nthe reason for your question mark?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"150965","messageId":"vpqiq22vp72.fsf@bauges.imag.fr","threadId":"25145","inReplyTo":"26d0544dac2515e76bee0608881cfd8c23bf1ebf.1284830388.git.kirr@landau.phys.spbu.ru","subject":"Re: [PATCH 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-09-18T19:14:57Z","receivedAt":"2010-09-18T19:14:57Z","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> Recently I've spot a bug\n\nWe usually avoid the first person in commit messages. The cover letter\nis a good place to tell about your personal story, but the commit\nmessage is what will remain, what people will read after a blame or\nbisect. They won't care whether you've \"recently\" found a bug, or in\nwhich circumstances you've found it.\n\nI'd write stg like (which would probably go to PATCH 2/3 instead of\nhere):\n\n-----8<----\ngit blame --textconv is wrongly calling the textconv filter on\nsymlinks: symlinks are stored as blobs whose content is the target of\nthe link, and blame calls the textconv filter on a temporary file\nfilled-in with the content of this blob.\n\nFor example:\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-----8<----\n\n> So git-blame is wrong here, and I'm going to write problem-demonstration\n> tests + try to fix it, but first we have to convert existing textconv\n> converter, so it will mimic pdftext behaviour -- if there is no '^bin:'\n> in input -- it's not a \"binary\" file and helper exits with error.\n\nWhat's interesting here is not that you mimick pdftext behavior, but\nthat you allow to easily distinguish file content and symlink target.\n\nHere's my try:\n\n-----8<----\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-----8<----\n\n> No other semantic changes at this stage.\n\nOtherwise, the code looks OK.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"150966","messageId":"vpq8w2yvonu.fsf@bauges.imag.fr","threadId":"25145","inReplyTo":"000c991c7a0673e26ee3ecc19ea3c8a7b437fecf.1284830388.git.kirr@landau.phys.spbu.ru","subject":"Re: [PATCH 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-18T19:26:29Z","receivedAt":"2010-09-18T19:26:29Z","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> Subject: Re: [PATCH 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks\n\nWe try to keep the subject lines short (<80 chars, and as much as\npossible less so that \"git log --oneline\" be pretty).\n\nHow about\n\nblame,cat-file: add failing tests for --textconv on symlinks\n\n> Because as described in previous patch - it should not.\n\nSince the actual problem is exhibited here, I think it is the best\nplace to actually describe it.\n\n> +test_expect_success 'make another new commit' '\n> +\techo \"bin: test number 2\" >three.bin &&\n> +\techo \"bin: test number 2 version 2\" >>three.bin &&\n> +\techo \"bin: test number 2 version 3\" >>three.bin &&\n> +\techo \"bin: test number 3\" >>three.bin &&\n\ncat >three.bin < +cat >expected < +(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\nThese days, it's recommanded to put this kind of code within the\ntest_expect_success/test_expect_failure.\n\n> +\n> +echo -n \"one.bin\" >expected\n\necho -n is not very portable (and doesn't seem to be used in git's t/\ndirectory). Better use\n\nprintf \"%s\" \"one.bin\" >expected\n\n(again, within text_expect_failure if possible)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"150967","messageId":"7vpqwa254i.fsf@alter.siamese.dyndns.org","threadId":"25145","inReplyTo":"vpqhbhmx6tg.fsf@bauges.imag.fr","subject":"Re: [BUG, PATCH 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-18T20:01:17Z","receivedAt":"2010-09-18T20:01:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Kirill Smelkov writes:\n>\n>> Recently I've spot a bug in git blame --textconv, which was wrongly\n>> calling pdftotext (my *.pdf conversion program) on a symlink.pdf,\n>\n> No time for a detailed review now, but textconv is also called by diff\n> (that's the original implementation). Does \"git diff\" work properly on\n> symlinks in the same case?\n\ndiff knows symlinks and regular files are different, and produces \"delete\nold then add new\" if you changed a regular file to a symlink.\n\nThat said, if you changed a symlink from pointing at A to pointing at B,\nit does run the textual diff between the string we get from readlink(3).\n\nI happen to think that textconv, if specified, for such a path should be\nhonored, so that people can keep doing whatever munging they have been\ndoing in their existing textconv filters.\n\nI didn't look at the thread or problem description, but are we running the\ntextconv filter on the file that symlink points at, instead of the\npathname stored in the symlink? If so I'd call that a bug.\n\nOn the other hand, if we are running the textconv filter on the pathname,\nthen I don't think we are doing anything wrong. If you have a filter that\nis meant to munge a PDF file to some other format, and if you do not want\nto apply that filter to munge a pathname a symlink that happens to be\nnamed \"foo.pdf\", either the filter itself or the attributes pattern you\nare using to choose what paths to apply that filter might want to be\nwritten more carefully, that's all.\n"},{"id":"150980","messageId":"vpq8w2yt8hc.fsf@bauges.imag.fr","threadId":"25145","inReplyTo":"7vpqwa254i.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG, PATCH 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-09-19T08:58:55Z","receivedAt":"2010-09-19T08:58:55Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> That said, if you changed a symlink from pointing at A to pointing at B,\n> it does run the textual diff between the string we get from readlink(3).\n\nYes, and my question was to make sure we don't run the textconv filter\non these strings.\n\nI've just checked, and from my little tests, git diff doesn't try to\ntextconv the pathnames, it runs the textual diff without textconv,\nwhich is the expected behavior.\n\n> I didn't look at the thread or problem description, but are we running the\n> textconv filter on the file that symlink points at, instead of the\n> pathname stored in the symlink? If so I'd call that a bug.\n\nNo, we don't do that. And yes, it's good not to do that: AFAIK, Git\nnever looks at the file symlinks points to (which allows one to store\nbroken symlinks or symlinks pointing outside the repository).\n\n> On the other hand, if we are running the textconv filter on the pathname,\n> then I don't think we are doing anything wrong. If you have a filter that\n> is meant to munge a PDF file to some other format, and if you do not want\n> to apply that filter to munge a pathname a symlink that happens to be\n> named \"foo.pdf\", either the filter itself or the attributes pattern you\n> are using to choose what paths to apply that filter might want to be\n> written more carefully, that's all.\n\nIf a symlink points to \"foo.pdf\", then you really want to call the\nstring \"foo.pdf\" a pathname, not a \"content\", although it happens to\nbe stored in a blob content.\n\nIf you have a program that can textconv PDF files, then you really\nwant to say\n\n*.pdf textconv=the-driver-for-that-program\n\nin your .gitattributes file, and let Git call the program on _files_,\nnot pathnames, and hence not symlink target (i.e. result of readlink).\n\nIf you think Git should call the textconv filter on pathnames, and let\nthe driver decide whether to call it depending on whether it's a file\ncontent or a symlink target, then you should 1) give a use-case where\nit's usefull to textconv symlink targets (I really don't see any, and\neven on highly artificial examples, I can't imagine one where the same\ndriver should textconv file content and pathnames), and 2) tell us the\nsyntax to use in .gitattributes or in the driver to call it only on\nfile content (I don't know any, and the .gitattribute manpage doesn't\ncontaint either \"link\" or \"mode\", so I don't think there is actually\nany).\n\nSo, I'm pretty sure either I misunderstood you or you misunderstood\nsomething ;-).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"150999","messageId":"7vd3s9y4w7.fsf@alter.siamese.dyndns.org","threadId":"25145","inReplyTo":"vpq8w2yt8hc.fsf@bauges.imag.fr","subject":"Re: [BUG, PATCH 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-19T18:17:28Z","receivedAt":"2010-09-19T18:17:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Junio C Hamano writes:\n>\n>> That said, if you changed a symlink from pointing at A to pointing at B,\n>> it does run the textual diff between the string we get from readlink(3).\n>\n> Yes, and my question was to make sure we don't run the textconv filter\n> on these strings.\n>\n> I've just checked, and from my little tests, git diff doesn't try to\n> textconv the pathnames, it runs the textual diff without textconv,\n> which is the expected behavior.\n\nOk, then I think it makes sense to make sure we don't run the textconv\nfilter on them.\n"},{"id":"151138","messageId":"20100920180046.GA1790@sigill.intra.peff.net","threadId":"25145","inReplyTo":"7vpqwa254i.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG, PATCH 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-20T18:00:46Z","receivedAt":"2010-09-20T18:00:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 18, 2010 at 01:01:17PM -0700, Junio C Hamano wrote:\n\n> diff knows symlinks and regular files are different, and produces \"delete\n> old then add new\" if you changed a regular file to a symlink.\n> \n> That said, if you changed a symlink from pointing at A to pointing at B,\n> it does run the textual diff between the string we get from readlink(3).\n> \n> I happen to think that textconv, if specified, for such a path should be\n> honored, so that people can keep doing whatever munging they have been\n> doing in their existing textconv filters.\n\nI think you came to the conclusion later in the thread that this is a\nbad idea, if only because it is not how \"git diff\" works, but I wanted\nto make one additional point.\n\nI think that git, being symlink aware, needs to behave similarly to\n\"lstat\". That is, we should never dereference symlinks transparently\nwhen diffing or analyzing content, because otherwise there is no way to\nactually look at the symlink data itself. It is the user's\nresponsibility to dereference symlinks in their diffs (e.g., I can get\neither the symlink data _or_ the actual file data by doing \"git diff\nsymlink-to-foo.bin\" or \"git diff foo.bin\". If git dereferenced for me, I\nwould get file data for _both_). Not to mention that we can't always\ndereference anyway because of broken links or links outside the repo, as\nMatthieu pointed out.\n\nSo doing anything but a straight text diff for the pathnames in symlink\nblobs is, IMHO, a bug.\n\nThe one thing this does not enable is using a special diff driver on the\n_pathnames_ of symlinks. Since these are by-definition text, I don't\nknow why anyone would want to do that. But it is an orthogonal problem,\nanyway. We would need some way in the .gitattributes or the .gitconfig\nto say \"this is the diff driver to use not based on pathname matching,\nbut based on the file's mode\". E.g., a special \"SYMLINK\" diff driver\nlike:\n\n [diff \"SYMLINK\"]\n textconv = pointless-munge\n\nBut again, I have no idea why anyone would want such a feature, so it is\nnot worth thinking too hard about it.\n\n-Peff\n"},{"id":"151140","messageId":"20100920182128.GB1790@sigill.intra.peff.net","threadId":"25145","inReplyTo":"7ab60e32582447ad792602ff405dcee464ef1414.1284830388.git.kirr@landau.phys.spbu.ru","subject":"Re: [PATCH 3/3] RFC: blame,cat-file --textconv: Don't assume mode is ``S_IFREF | 0664''","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-20T18:21:28Z","receivedAt":"2010-09-20T18:21:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 18, 2010 at 09:25:06PM +0400, Kirill Smelkov wrote:\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\nI think this is absolutely a bug, and your solution is definitely in the\nright direction. We obviously can't just ignore the mode when deciding\nwhether to textconv. I suspect there is similar breakage for S_IFGITLINK\nfiles, though they are perhaps less likely in practice to match another\nfiletype's extension.\n\nSo all three patches look sane to me, with the caveat that I also don't\nknow the blame code very well.\n\nI agree with Matthieu's points on cleaning up the commit messages, and\nthere is a small comment typo in this third patch:\n\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 correspoinding tree path.\n\nTypo: s/poind/pond\n\n-Peff\n"},{"id":"151151","messageId":"201009202218.25558.j6t@kdbg.org","threadId":"25145","inReplyTo":"20100920180046.GA1790@sigill.intra.peff.net","subject":"Re: [BUG, PATCH 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-09-20T20:18:25Z","receivedAt":"2010-09-20T20:18:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Montag, 20. September 2010, Jeff King wrote:\n> [diff \"SYMLINK\"]\n> textconv = pointless-munge\n>\n> But again, I have no idea why anyone would want such a feature, so it is\n> not worth thinking too hard about it.\n\nSome people dislike \"\\ No newline at end of file\" for symlinks. This would be \nthe opportunity to hook a suitable diff driver.\n\n-- Hannes\n"},{"id":"151153","messageId":"20100920203501.GB23533@landau.phys.spbu.ru","threadId":"25145","inReplyTo":"20100920182128.GB1790@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-20T20:35:01Z","receivedAt":"2010-09-20T20:35:01Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Matthieu, Jeff, Junio, thanks for your review and input.\n\nComments below inline...\n\nOn Sat, Sep 18, 2010 at 09:14:57PM +0200, Matthieu Moy wrote:\n> Kirill Smelkov writes:\n> \n> > Recently I've spot a bug\n> \n> We usually avoid the first person in commit messages. The cover letter\n> is a good place to tell about your personal story, but the commit\n> message is what will remain, what people will read after a blame or\n> bisect. They won't care whether you've \"recently\" found a bug, or in\n> which circumstances you've found it.\n> \n> I'd write stg like (which would probably go to PATCH 2/3 instead of\n> here):\n> \n> -----8<----\n> git blame --textconv is wrongly calling the textconv filter on\n> symlinks: symlinks are stored as blobs whose content is the target of\n> the link, and blame calls the textconv filter on a temporary file\n> filled-in with the content of this blob.\n> \n> For example:\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> -----8<----\n\nAgree, your wording is better.\n\n> > So git-blame is wrong here, and I'm going to write problem-demonstration\n> > tests + try to fix it, but first we have to convert existing textconv\n> > converter, so it will mimic pdftext behaviour -- if there is no '^bin:'\n> > in input -- it's not a \"binary\" file and helper exits with error.\n> \n> What's interesting here is not that you mimick pdftext behavior, but\n> that you allow to easily distinguish file content and symlink target.\n> \n> Here's my try:\n> \n> -----8<----\n> The textconv filter is sometimes incorrectly ran on a temporary file\n> whose content is the target of a symbolic link, instead of actual file\n> content. Prepare to test this by marking the content of the file to\n> convert with \"bin:\", and let the helper die if \"bin:\" is not found in\n> the file content.\n> -----8<----\n\nAgree\n\n> > No other semantic changes at this stage.\n> \n> Otherwise, the code looks OK.\n\nThanks\n\n\nOn Sat, Sep 18, 2010 at 09:26:29PM +0200, Matthieu Moy wrote:\n> Kirill Smelkov writes:\n> \n> > Subject: Re: [PATCH 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks\n> \n> We try to keep the subject lines short (<80 chars, and as much as\n> possible less so that \"git log --oneline\" be pretty).\n> \n> How about\n> \n> blame,cat-file: add failing tests for --textconv on symlinks\n\nI'd like to shorten it, but \"add failing tests\" is not as descriptive as\n\"Demonstrate --textconv misbehaves in such-and-such way\", and I can't\ncome up with a shorter subject without making it more cryptic. And btw,\nI've looked at log --oneline output, and (surprise, surprise)\n\n479a56 4fccc04 etc ...\n\nSo if that's not a major obstacle, I'd leave it as is.\n\n> > Because as described in previous patch - it should not.\n> \n> Since the actual problem is exhibited here, I think it is the best\n> place to actually describe it.\n\nAgree - will move description here.\n\n> > +test_expect_success 'make another new commit' '\n> > +\techo \"bin: test number 2\" >three.bin &&\n> > +\techo \"bin: test number 2 version 2\" >>three.bin &&\n> > +\techo \"bin: test number 2 version 3\" >>three.bin &&\n> > +\techo \"bin: test number 3\" >>three.bin &&\n> \n> cat >three.bin < 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> \n> ?\n\nYes, thanks.\n\n> > +cat >expected < > +(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> \n> These days, it's recommanded to put this kind of code within the\n> test_expect_success/test_expect_failure.\n\nI see, thanks.\n\nI've moved other >expected preparation inside test_expect_*, but this\nexpect is special in that it is used in subsequent two tests. So I'd\nleave this one outside.\n\nAnd btw, I've originally copied in-style what was already there in t8006\nand t8007 which date to Jun 2010.\n\n> > +\n> > +echo -n \"one.bin\" >expected\n> \n> echo -n is not very portable (and doesn't seem to be used in git's t/\n> directory). Better use\n> \n> printf \"%s\" \"one.bin\" >expected\n> \n> (again, within text_expect_failure if possible)\n\nI didn't knew echo -n is not portable, thanks. Converted to printf and\nmoved inside test.\n\n\nOn Sat, Sep 18, 2010 at 09:04:00PM +0200, Matthieu Moy wrote:\n> Kirill Smelkov writes:\n> \n> > I don't know blame code well, and I'm not sure I'm doing it right or\n> > otherwise without mistakes. Thus an RFC.\n> \n> I don't know the code well either, but I didn't see any obvious\n> problem in your code.\n> \n> > --- a/sha1_name.c\n> > +++ b/sha1_name.c\n> > @@ -1068,7 +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\treturn get_sha1_oneline(name + 2, sha1);\n> > +\t\t\treturn get_sha1_oneline(name + 2, sha1); /* XXX also mode? */\n> \n> (This is the case where we're parsing \":/foo\")\n> \n> Currently, the mode is set a few lines above:\n> \n> \toc->mode = S_IFINVALID;\n> \n> I guess this is OK since :/foo will return a commit sha1, not a file\n> sha1.\n\nAh, yes, thanks. I forgot ':/text' means `commit which log is text' -\nyes, mode should be S_IFINVALID here then.\n\n> > @@ -1095,6 +1095,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; /* XXX ok? */\n> \n> I'd say this is OK, you're setting the mode from the index. What was\n> the reason for your question mark?\n\nYes, this turned out to be needed - without this copy, one of cat-file\ntests does not pass.\n\nI was worrying because initially I've also tried to propagete\norigin->mode into ce->ce_mode in fake_working_tree_commit():\n\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2133,9 +2133,9 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \tsize = cache_entry_size(len);\n \tce = xcalloc(1, size);\n \thashcpy(ce->sha1, origin->blob_sha1);\n+\tce->ce_mode = create_ce_mode(origin->mode);\n \tmemcpy(ce->name, path, len);\n \tce->ce_flags = create_ce_flags(len, 0);\n-\tce->ce_mode = create_ce_mode(mode);\n \tadd_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n \n \t/*\n\nand it caused test failures, e.g.\n\n --- expected 2010-09-20 20:26:53.000000000 +0000\n +++ result 2010-09-20 20:26:53.000000000 +0000\n @@ -1 +1 @@\n -(Number2 2010-01-01 20:00:00 +0000 1) two.bin\n +(Not Committed Yet 2010-09-20 20:26:53 +0000 1) two.bin\n not ok - 9 blame with --no-textconv (on symlink)\n #\n # git blame --no-textconv symlink.bin >blame &&\n # find_blame result &&\n # test_cmp expected result\n #\n\nin t8006-blame-textconv.sh\n\nSo I dummily conclude I'de better not touch ce_mode...\n\nBut now I read comments in there \n\n /*\n * Read the current index, replace the path entry with\n * origin->blob_sha1 without mucking with its mode or type\n * bits; we are not going to write this index out -- we just\n * want to run \"diff-index --cached\".\n */\n\nAnd guess I probably should not touch mode here. Not 100% sure, but more\nconfident (call it heuristic programming :)), so I'm removing my XXX.\n\n\nOn Mon, Sep 20, 2010 at 02:21:28PM -0400, Jeff King wrote:\n> On Sat, Sep 18, 2010 at 09:25:06PM +0400, Kirill Smelkov wrote:\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> \n> I think this is absolutely a bug, and your solution is definitely in the\n> right direction. We obviously can't just ignore the mode when deciding\n> whether to textconv. I suspect there is similar breakage for S_IFGITLINK\n> files, though they are perhaps less likely in practice to match another\n> filetype's extension.\n> \n> So all three patches look sane to me, with the caveat that I also don't\n> know the blame code very well.\n> \n> I agree with Matthieu's points on cleaning up the commit messages, and\n> there is a small comment typo in this third patch:\n\nJeff, thanks for you comments - appreciated.\n\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 correspoinding tree path.\n> \n> Typo: s/poind/pond\n\nGood eyes, thanks.\n\nI should sleep more :)\n\n\n\nThanks again to everyone. I'll repost this series as v2 shortly.\n\nKirill\n"},{"id":"151161","messageId":"1285016477-22222-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"25145","inReplyTo":"7ab60e32582447ad792602ff405dcee464ef1414.1284830388.git.kirr@landau.phys.spbu.ru","subject":"[PATCH] sha1_name.c: update comment to mention :/foo syntax","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2010-09-20T21:01:17Z","receivedAt":"2010-09-20T21:01:17Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"\nSigned-off-by: Matthieu Moy \n---\nNoticed while reviewing the patch serie about textconv and symlinks.\nIf we have comments, better have them up-to-date ;-).\n\n sha1_name.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 7b7e617..d7ab72a 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1062,6 +1062,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t/* sha1:path --> object name of path in ent sha1\n \t * :path -> object name of path in index\n \t * :[0-3]:path -> object name of path in index at stage\n+\t * :/foo -> last commit whose subject starts with foo\n \t */\n \tif (name[0] == ':') {\n \t\tint stage = 0;\n-- \n1.7.3.2.g257b5f\n"},{"id":"151162","messageId":"vpq1v8o9lg8.fsf@bauges.imag.fr","threadId":"25145","inReplyTo":"20100920203501.GB23533@landau.phys.spbu.ru","subject":"Re: [PATCH 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-09-20T21:03:35Z","receivedAt":"2010-09-20T21:03:35Z","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> Matthieu, Jeff, Junio, thanks for your review and input.\n>\n> Comments below inline...\n\nYou've replied to several messages in one. That's OK here since the\ncontent of your message is essentially uncontroversial, but I'd\nsuggest keeping separate threads for separate patches for next time.\n(for example, my reply below looks strange since I'm commenting on\nPATCH 2/3 in a message titled PATCH 1/3 ...). No big deal.\n\n>> > Subject: Re: [PATCH 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks\n>>\n>> We try to keep the subject lines short (<80 chars, and as much as\n>> possible less so that \"git log --oneline\" be pretty).\n>>\n>> How about\n>>\n>> blame,cat-file: add failing tests for --textconv on symlinks\n>\n> I'd like to shorten it, but \"add failing tests\" is not as descriptive as\n> \"Demonstrate --textconv misbehaves in such-and-such way\",\n\nRight, but your \"such-and-such way\" is not really precise either. One\ncannot know whether it's running, but the wrong way, whether it's\nrunning and should not, etc. The subject line is here to be a summary,\nyou don't need details. And I actually find \"add failing tests\" more\nprecise than \"demonstrate that ... is behaving wrongly\".\n\n> and I can't come up with a shorter subject without making it more\n> cryptic. And btw, I've looked at log --oneline output, and\n> (surprise, surprise)\n>\n> 479a56 4fccc04 etc ...\n\nI don't understand what you mean. After applying your patches, I get:\n\n6998f9a RFC: blame,cat-file --textconv: Don't assume mode is ``S_IFREF | 0664''\n025aaac blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks\n3646401 tests: Prepare --textconv tests for correctly-failing conversion program\n\nwhich do not fit on a 80-columns terminal.\n\n> So if that's not a major obstacle, I'd leave it as is.\n\nI don't consider it \"major\" (but other may have different opinions)\nthough.\n\n>> These days, it's recommanded to put this kind of code within the\n>> test_expect_success/test_expect_failure.\n>\n> I see, thanks.\n>\n> I've moved other >expected preparation inside test_expect_*, but this\n> expect is special in that it is used in subsequent two tests. So I'd\n> leave this one outside.\n>\n> And btw, I've originally copied in-style what was already there in t8006\n> and t8007 which date to Jun 2010.\n\nYes, the guideline is new, and not widely applied (yet).\n\n> Ah, yes, thanks. I forgot ':/text' means `commit which log is text' -\n\nA patch to update the comment above follows.\n\n> Thanks again to everyone.\n\nThat \"everyone\" includes you ;-). Thanks for the patch, good job both\nfor testing and for the actual fix!\n\n--\nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"151227","messageId":"7vaanbuggr.fsf@alter.siamese.dyndns.org","threadId":"25145","inReplyTo":"20100920180046.GA1790@sigill.intra.peff.net","subject":"Re: [BUG, PATCH 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-21T17:57:56Z","receivedAt":"2010-09-21T17:57:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The one thing this does not enable is using a special diff driver on the\n> _pathnames_ of symlinks. Since these are by-definition text, I don't\n> know why anyone would want to do that. But it is an orthogonal problem,\n> anyway. We would need some way in the .gitattributes or the .gitconfig\n> to say \"this is the diff driver to use not based on pathname matching,\n> but based on the file's mode\". E.g., a special \"SYMLINK\" diff driver\n> like:\n>\n> [diff \"SYMLINK\"]\n> textconv = pointless-munge\n>\n> But again, I have no idea why anyone would want such a feature, so it is\n> not worth thinking too hard about it.\n\nI agree with you; pointless-munge would just be 'printf \"%s\\n\"' ;-)\n"},{"id":"151228","messageId":"7v62xzug94.fsf@alter.siamese.dyndns.org","threadId":"25145","inReplyTo":"1285016477-22222-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH] sha1_name.c: update comment to mention :/foo syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-21T18:02:31Z","receivedAt":"2010-09-21T18:02:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Signed-off-by: Matthieu Moy \n> ---\n> Noticed while reviewing the patch serie about textconv and symlinks.\n> If we have comments, better have them up-to-date ;-).\n>\n> sha1_name.c | 1 +\n> 1 files changed, 1 insertions(+), 0 deletions(-)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 7b7e617..d7ab72a 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -1062,6 +1062,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n> \t/* sha1:path --> object name of path in ent sha1\n> \t * :path -> object name of path in index\n> \t * :[0-3]:path -> object name of path in index at stage\n> +\t * :/foo -> last commit whose subject starts with foo\n\nDocumenting what hasn't been is a good thing, but is it really up-to-date?\n\nIsn't it \"a randomly chosen recent commit whose subject contains regexp\nfoo\" these days?\n"},{"id":"151230","messageId":"20100921183950.GA4390@landau.phys.spbu.ru","threadId":"25145","inReplyTo":"vpq1v8o9lg8.fsf@bauges.imag.fr","subject":"Re: [PATCH 1/3] tests: Prepare --textconv tests for correctly-failing conversion program","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-09-21T18:39:50Z","receivedAt":"2010-09-21T18:39:50Z","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:03:35PM +0200, Matthieu Moy wrote:\n> Kirill Smelkov writes:\n> \n> > Matthieu, Jeff, Junio, thanks for your review and input.\n> >\n> > Comments below inline...\n> \n> You've replied to several messages in one. That's OK here since the\n> content of your message is essentially uncontroversial, but I'd\n> suggest keeping separate threads for separate patches for next time.\n> (for example, my reply below looks strange since I'm commenting on\n> PATCH 2/3 in a message titled PATCH 1/3 ...). No big deal.\n\nI knew someone will say this :)\n\nBut let's try to think about it from a bit different point of view - we\nhad 3 patches, and description for at least two of them were intermixed.\nIn this situation the series is better considered as a whole, and if we\nconsider each patch git branch, what would you do if you need to get\ncumulative result of all the branches?\n\nYes, merge them - so did I. And the reply actually had all the\nreferences to parent messages in it's In-Reply-To header. It's just that\nusually MUA's don't show merges graphically (please correct me if I'm\nwrong), but otherwise isn't it ok?\n\nI agree, subject should be better corrected somehow, but main idea still\nstays valid!\n\nClearly I did lots of giting recently, so I project it's concepts on\nthe world :)\n\n\n> >> > Subject: Re: [PATCH 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks\n> >>\n> >> We try to keep the subject lines short (<80 chars, and as much as\n> >> possible less so that \"git log --oneline\" be pretty).\n> >>\n> >> How about\n> >>\n> >> blame,cat-file: add failing tests for --textconv on symlinks\n> >\n> > I'd like to shorten it, but \"add failing tests\" is not as descriptive as\n> > \"Demonstrate --textconv misbehaves in such-and-such way\",\n> \n> Right, but your \"such-and-such way\" is not really precise either. One\n> cannot know whether it's running, but the wrong way, whether it's\n> running and should not, etc. The subject line is here to be a summary,\n> you don't need details. And I actually find \"add failing tests\" more\n> precise than \"demonstrate that ... is behaving wrongly\".\n\nHmm, maybe my wording is not that good, but to me \"Demonstrate\n--textconv misbehaves ...\" is closer to being more descriptive than \"add\nfailing tests\".\n\nThis is maybe just a matter of preferences, so I'm ok with either way.\n\n> > and I can't come up with a shorter subject without making it more\n> > cryptic. And btw, I've looked at log --oneline output, and\n> > (surprise, surprise)\n> >\n> > 479a56 4fccc04 etc ...\n> \n> I don't understand what you mean. After applying your patches, I get:\n> \n> 6998f9a RFC: blame,cat-file --textconv: Don't assume mode is ``S_IFREF | 0664''\n> 025aaac blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks\n> 3646401 tests: Prepare --textconv tests for correctly-failing conversion program\n> \n> which do not fit on a 80-columns terminal.\n\nI meant do git log on those sha's, but late at night I've ommitted one\nletter. Here you go:\n\n$ git log -1 --oneline a479a56\na479a56 Documentation/Makefile: allow man.base.url.for.relative.link to be set from Make\n$ git log -1 --oneline 4fccc04\n4fccc04 Documentation: remove stray backslashes from \"Fighting regressions\" article\n\n\nThat's 88 & 83 characters. I mean to say maybe it's not that big deal to\nsacrifice understandability for 80 chars, especially when git log output\nis paged, usually through less -S.\n\nThat said I also try to keep subjects short when possible.\n\n> > So if that's not a major obstacle, I'd leave it as is.\n> \n> I don't consider it \"major\" (but other may have different opinions)\n> though.\n\nSo what would be conclusion here?\n\n> >> These days, it's recommanded to put this kind of code within the\n> >> test_expect_success/test_expect_failure.\n> >\n> > I see, thanks.\n> >\n> > I've moved other >expected preparation inside test_expect_*, but this\n> > expect is special in that it is used in subsequent two tests. So I'd\n> > leave this one outside.\n> >\n> > And btw, I've originally copied in-style what was already there in t8006\n> > and t8007 which date to Jun 2010.\n> \n> Yes, the guideline is new, and not widely applied (yet).\n> \n> > Ah, yes, thanks. I forgot ':/text' means `commit which log is text' -\n> \n> A patch to update the comment above follows.\n\nthanks\n\n> > Thanks again to everyone.\n> \n> That \"everyone\" includes you ;-). Thanks for the patch, good job both\n> for testing and for the actual fix!\n\nCome on, Matthieu, it's just one small correction :)\n\nThanks,\nKirill\n"},{"id":"151232","messageId":"20100921184241.GA28567@sigill.intra.peff.net","threadId":"25145","inReplyTo":"7vaanbuggr.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG, PATCH 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-21T18:42:41Z","receivedAt":"2010-09-21T18:42:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2010 at 10:57:56AM -0700, Junio C Hamano wrote:\n\n> > [diff \"SYMLINK\"]\n> > textconv = pointless-munge\n> >\n> > But again, I have no idea why anyone would want such a feature, so it is\n> > not worth thinking too hard about it.\n> \n> I agree with you; pointless-munge would just be 'printf \"%s\\n\"' ;-)\n\nAlmost. That would print the name of the tempfile; the actual pathname\nis the contents of the tempfile (unless you are proposing totally\nalternate semantics for the symlink diff section. :) ).\n\nJust for fun, the patch to make this work is as tiny as:\n\ndiff --git a/diff.c b/diff.c\nindex 6fb18ae..58c4477 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1771,8 +1771,14 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two, char *pre\n \n static void diff_filespec_load_driver(struct diff_filespec *one)\n {\n-\tif (!one->driver)\n+\tif (one->driver)\n+\t\treturn;\n+\n+\tif (S_ISLNK(one->mode))\n+\t\tone->driver = userdiff_find_by_name(\"SYMLINK\");\n+\telse\n \t\tone->driver = userdiff_find_by_path(one->path);\n+\n \tif (!one->driver)\n \t\tone->driver = userdiff_find_by_name(\"default\");\n }\n@@ -1820,7 +1826,7 @@ struct userdiff_driver *get_textconv(struct diff_filespec *one)\n {\n \tif (!DIFF_FILE_VALID(one))\n \t\treturn NULL;\n-\tif (!S_ISREG(one->mode))\n+\tif (!S_ISREG(one->mode) && !S_ISLNK(one->mode))\n \t\treturn NULL;\n \tdiff_filespec_load_driver(one);\n \tif (!one->driver->textconv)\n\nafter which I successfully tested with:\n\n git init repo && cd repo &&\n echo content >file.txt &&\n ln -s file.txt link.txt &&\n echo '*.txt diff=txt' >.gitattributes &&\n git add . && git commit -m foo &&\n git config diff.txt.textconv \"sed 's/^/converted: /'\" &&\n git config diff.SYMLINK.textconv \"perl -pe 's/$/\\n/'\" &&\n git show\n\nIt works with the whole range of diff config, so you could do something\nas awesomely stupid as:\n\n $ git config diff.SYMLINK.binary true\n $ git show link.txt\n ...\n diff --git a/link.txt b/link.txt\n new file mode 120000\n index 0000000..4c33073\n Binary files /dev/null and b/link.txt differ\n\nIf you really wanted the \"dereference my symlinks\" behavior, you could\ndo:\n\n git config diff.SYMLINK.textconv 'sh -c \"cat `cat $1`\" -'\n\nbut that is not quite right; you would actually need to dereference\nrelative symlinks with respect to the link itself, which textconv never\ngets (plus you would probably want to handle broken links more\ngracefully).\n\nAnyway, as I said at the beginning, for me this was just for fun. I find\nthe intended use rather silly, but maybe somebody else is interested. I\ndo think it's the right way of implementing such a feature, because we\nalready turn off textconv when making patches that are meant to be\napplied rather than viewed. However, I didn't do any testing or give\nmuch thought to whether this would affect any unintended code paths.\n\n-Peff\n"},{"id":"151233","messageId":"20100921185613.GB28567@sigill.intra.peff.net","threadId":"25145","inReplyTo":"20100921184241.GA28567@sigill.intra.peff.net","subject":"Re: [BUG, PATCH 0/3] Fix {blame,cat-file} --textconv for cases with symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-21T18:56:13Z","receivedAt":"2010-09-21T18:56:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2010 at 02:42:41PM -0400, Jeff King wrote:\n\n> diff --git a/diff.c b/diff.c\n> index 6fb18ae..58c4477 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1771,8 +1771,14 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two, char *pre\n> \n> static void diff_filespec_load_driver(struct diff_filespec *one)\n> {\n> -\tif (!one->driver)\n> +\tif (one->driver)\n> +\t\treturn;\n> +\n> +\tif (S_ISLNK(one->mode))\n> +\t\tone->driver = userdiff_find_by_name(\"SYMLINK\");\n> +\telse\n> \t\tone->driver = userdiff_find_by_path(one->path);\n> +\n> \tif (!one->driver)\n> \t\tone->driver = userdiff_find_by_name(\"default\");\n> }\n> @@ -1820,7 +1826,7 @@ struct userdiff_driver *get_textconv(struct diff_filespec *one)\n> {\n> \tif (!DIFF_FILE_VALID(one))\n> \t\treturn NULL;\n> -\tif (!S_ISREG(one->mode))\n> +\tif (!S_ISREG(one->mode) && !S_ISLNK(one->mode))\n> \t\treturn NULL;\n> \tdiff_filespec_load_driver(one);\n> \tif (!one->driver->textconv)\n\nActually, thinking about this a little more, I wonder if the test for\nS_ISREG should perhaps have been in diff_filespec_load_driver from the\nvery beginning. With the current code, I can do:\n\n $ echo 'binary content' >file.bin\n $ ln -s file.bin link.bin\n $ echo '*.bin diff=bin' >.gitattributes\n $ git config diff.bin.binary true\n $ git add -N .\n $ git diff\n diff --git a/.gitattributes b/.gitattributes\n index e69de29..d38ad2e 100644\n --- a/.gitattributes\n +++ b/.gitattributes\n @@ -0,0 +1 @@\n +*.bin diff=bin\n diff --git a/file.bin b/file.bin\n index e69de29..f55142d 100644\n Binary files a/file.bin and b/file.bin differ\n diff --git a/link.bin b/link.bin\n index e69de29..dce41ec 120000\n Binary files a/link.bin and b/link.bin differ\n\nwhich does not seem right. We are again acting on the symlink's\ncontents, which are a text pathname, as if they represented the content\nof that pathname.\n\nSo I think that is a bug, albeit one that is relatively uncommon to\ntrigger. S_ISGITLINK files may have the same issues, but perhaps not, as\nI think they get routed out of the regular diff codepath early.\n\n-Peff\n"},{"id":"151239","messageId":"vpq8w2ug8u0.fsf@bauges.imag.fr","threadId":"25145","inReplyTo":"7v62xzug94.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] sha1_name.c: update comment to mention :/foo syntax","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-09-21T20:06:31Z","receivedAt":"2010-09-21T20:06:31Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> Signed-off-by: Matthieu Moy \n>> ---\n>> Noticed while reviewing the patch serie about textconv and symlinks.\n>> If we have comments, better have them up-to-date ;-).\n>>\n>> sha1_name.c | 1 +\n>> 1 files changed, 1 insertions(+), 0 deletions(-)\n>>\n>> diff --git a/sha1_name.c b/sha1_name.c\n>> index 7b7e617..d7ab72a 100644\n>> --- a/sha1_name.c\n>> +++ b/sha1_name.c\n>> @@ -1062,6 +1062,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n>> \t/* sha1:path --> object name of path in ent sha1\n>> \t * :path -> object name of path in index\n>> \t * :[0-3]:path -> object name of path in index at stage\n>> +\t * :/foo -> last commit whose subject starts with foo\n>\n> Documenting what hasn't been is a good thing, but is it really up-to-date?\n>\n> Isn't it \"a randomly chosen recent commit whose subject contains regexp\n> foo\" these days?\n\nI don't know.\n\nI just tried to summarize what man git-rev-parse says:\n\n · A colon, followed by a slash, followed by a text (e.g. :/fix nasty bug):\n this names a commit whose commit message starts with the specified text.\n This name returns the youngest matching commit which is reachable from any\n ref. If the commit message starts with a !, you have to repeat that; the\n special sequence :/!, followed by something else than ! is reserved for\n now.\n\nIf your description is more accurate than mine (especially about\n\"randomly\" Vs \"last\" and \"regexp\"), then the man should be updated.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"151251","messageId":"20100921205914.GA1166@sigill.intra.peff.net","threadId":"25145","inReplyTo":"20100921185613.GB28567@sigill.intra.peff.net","subject":"[PATCH 0/2] better userdiff behavior for symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-21T20:59:14Z","receivedAt":"2010-09-21T20:59:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2010 at 02:56:13PM -0400, Jeff King wrote:\n\n> We are again acting on the symlink's contents, which are a text\n> pathname, as if they represented the content of that pathname.\n> \n> So I think that is a bug, albeit one that is relatively uncommon to\n> trigger. S_ISGITLINK files may have the same issues, but perhaps not,\n> as I think they get routed out of the regular diff codepath early.\n\nHere is a patch series to address this:\n\n [1/2]: diff: don't use pathname-based diff drivers for symlinks\n [2/2]: diff: add a special SYMLINK user-diff driver\n\nThe first one fixes the bug. The second one is similar to what I posted\nbefore, but it may actually be more useful now, as it re-enables some\ndubious functionality that the first patch dropped (but in a more sane\nway). See the comments in each patch for details.\n\n-Peff\n"},{"id":"151252","messageId":"20100921210124.GA1188@sigill.intra.peff.net","threadId":"25145","inReplyTo":"20100921205914.GA1166@sigill.intra.peff.net","subject":"[PATCH 1/2] diff: don't use pathname-based diff drivers for symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-21T21:01:24Z","receivedAt":"2010-09-21T21:01:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we're diffing symlinks, we consider the contents to be\nthe pathname that the symlink points to. When a user sets up\na userdiff driver like \"*.pdf diff=pdf\", their \"diff.pdf.*\"\nconfig generally tells us what to do with the content of\npdf files.\n\nWith the current code, we will actually process a symlink\nlike \"link.pdf\" using a configured pdf driver, meaning we\nare using contents which consist of a pathname with\nconfiguration that is expecting contents that consist of an\nactual pdf file.\n\nThe most noticeable example of this would have been\ntextconv; however, it was already protected in its own\ntextconv-specific code path. We can still see the breakage\nwith something like \"diff.*.binary\", though. You could\nalso see it with diff.*.funcname, though it is a bit harder\nto trigger accidentally there.\n\nThis patch adds a check for S_ISREG lower in the callstack\nthan the textconv-specific check, which should block use of\nany userdiff config for non-regular files. We can drop the\ncheck in the textconv code, which is now redundant.\n\nSigned-off-by: Jeff King \n---\nTechnically, this could be breaking somebody's setup if:\n\n 1. They found some use for userdiff config on symlinks. Textconv is\n already disabled. A custom diff driver might work.\n\nand\n\n 2. They were willing to specify symlinks individually in\n .gitattributes, or name them according to some symlink-specific\n pattern. Attribute patterns that matched both regular files and\n symlinks were broken, as shown in the test.\n\nI find it unlikely, and given the potential breakage, it seems more like\nexploiting a bug to get what you want. A more sane way of doing the same\nthing is provided in patch 2/2.\n\n diff.c | 11 ++++++++---\n t/t4011-diff-symlink.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9a5c77c..276e029 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1771,8 +1771,14 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two, char *pre\n \n static void diff_filespec_load_driver(struct diff_filespec *one)\n {\n-\tif (!one->driver)\n+\t/* Use already-loaded driver */\n+\tif (one->driver)\n+\t\treturn;\n+\n+\tif (S_ISREG(one->mode))\n \t\tone->driver = userdiff_find_by_path(one->path);\n+\n+\t/* Fallback to default settings */\n \tif (!one->driver)\n \t\tone->driver = userdiff_find_by_name(\"default\");\n }\n@@ -1820,8 +1826,7 @@ struct userdiff_driver *get_textconv(struct diff_filespec *one)\n {\n \tif (!DIFF_FILE_VALID(one))\n \t\treturn NULL;\n-\tif (!S_ISREG(one->mode))\n-\t\treturn NULL;\n+\n \tdiff_filespec_load_driver(one);\n \tif (!one->driver->textconv)\n \t\treturn NULL;\ndiff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh\nindex 6f69489..408a19c 100755\n--- a/t/t4011-diff-symlink.sh\n+++ b/t/t4011-diff-symlink.sh\n@@ -88,4 +88,30 @@ test_expect_success SYMLINKS \\\n test_must_fail git diff --no-index pinky brain > output 2> output.err &&\n grep narf output &&\n ! grep error output.err'\n+\n+test_expect_success SYMLINKS 'setup symlinks with attributes' '\n+\techo \"*.bin diff=bin\" >>.gitattributes &&\n+\techo content >file.bin &&\n+\tln -s file.bin link.bin &&\n+\tgit add -N file.bin link.bin\n+'\n+\n+cat >expect <<'EOF'\n+diff --git a/file.bin b/file.bin\n+index e69de29..d95f3ad 100644\n+Binary files a/file.bin and b/file.bin differ\n+diff --git a/link.bin b/link.bin\n+index e69de29..dce41ec 120000\n+--- a/link.bin\n++++ b/link.bin\n+@@ -0,0 +1 @@\n++file.bin\n+\\ No newline at end of file\n+EOF\n+test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '\n+\tgit config diff.bin.binary true &&\n+\tgit diff file.bin link.bin >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.3.256.gb9713\n"},{"id":"151254","messageId":"20100921211303.GB1188@sigill.intra.peff.net","threadId":"25145","inReplyTo":"20100921205914.GA1166@sigill.intra.peff.net","subject":"[PATCH 2/2] diff: add a special SYMLINK user-diff driver","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-21T21:13:03Z","receivedAt":"2010-09-21T21:13:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As of the previous patch, there is no way to impact the diff\ncharacteristics of a symbolic link. So if you wanted to use\na custom external driver or textconv for a symbolic link,\nyou can't. This patch enables that. See the added\ndocumentation for an example use.\n\nSigned-off-by: Jeff King \n---\nAnd this provides any people who really were using the symlink behavior\nfixed in patch 1/2 with a less buggy way to do the same thing. In\naddition, it now allows them to use textconv, which can be used to\naddress the \"no newline at end of file\" complaint.\n\nWriting this, though, made me wonder if diff is alone in having this\nbug. If I have \"*.pdf merge=pdf\" in my .gitattributes file, will we try\nto merge a symoblic link to a pdf like a pdf? That seems just as wrong\nas diffing.\n\nSo I wonder if the problem really is not in the userdiff code at all,\nand we should be disabling gitattribute lookup entirely for non-regular\nfiles.\n\nAlso, this special SYMLINK driver assumes you want to treat all symlinks\nthe same. Which is probably good enough in practice. But if\ngitattributes were aware of file modes, it would be nice to be able to\nbe able to apply multiple criteria (i.e., something besides just a\npathname match) to matching files. And then you could do something like\nthis in your gitattributes file:\n\n # all symlinks match this\n * is_symlink diff=symlink-diff-driver\n # only a symlink that matches this filename will get this\n some-particular-symlink is_symlink diff=other-symlink-diff-driver\n # and here we can explicitly say that only regular files get this\n # treatment\n *.pdf is_regular diff=pdf\n # and since that's what you want 99% of the time, we can imply\n # is_regular, which is backwards compatible and syntactically nicer.\n # So this is equivalent to the above\n *.pdf diff=pdf\n\nwhich is more flexible, and neatly solves the \"attributes shouldn't\naffect symlinks at all\" issue.\n\nIt does feel like overengineering, though. I really don't think people\nare going to want to treat various symlinks differently. There is also\nthe question of submodule links. In theory they could match\ngitattributes, too, and maybe there is something more useful people\ncould do there. I dunno.\n\n Documentation/gitattributes.txt | 32 ++++++++++++++++++++++++++++++++\n diff.c | 2 ++\n t/t4011-diff-symlink.sh | 11 +++++++++++\n 3 files changed, 45 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex e5a27d8..cc3bddf 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -405,6 +405,8 @@ String::\n \tby the configuration variables in the \"diff.foo\" section of the\n \tgit config file.\n \n+NOTE: Diff attributes apply only to regular files, not symlinks or\n+submodules. See the section on special diff drivers below.\n \n Defining an external diff driver\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n@@ -572,6 +574,36 @@ and now produces better output), you can remove the cache\n manually with `git update-ref -d refs/notes/textconv/jpg` (where\n \"jpg\" is the name of the diff driver, as in the example above).\n \n+Special diff drivers\n+^^^^^^^^^^^^^^^^^^^^\n+\n+Diff drivers matched by attributes are used only for regular files. They\n+do not apply to symbolic links, as a diff of symbolic links is not\n+performed on the content of the files the links point to, but rather\n+on the names of the files pointed to by the links. This makes a\n+configuration like the following work as expected:\n+\n+-----------------------------\n+$ echo '*.pdf diff=pdf' >.gitattributes\n+$ git config diff.pdf.textconv pdftotext\n+$ ln -s file.pdf link.pdf\n+-----------------------------\n+\n+The regular file \"file.pdf\" will be converted to text for diffing, but\n+the symlink \"link.pdf\", whose diffed contents are not PDF at all but\n+rather the pathname \"file.pdf\", will not be affected.\n+\n+However, it may happen that you do want to change the diff parameters\n+specifically for symlinks. You can do that by configuring the special\n+\"SYMLINK\" diff driver. For example, to add a newline to the end of\n+the symlink contents (and suppress the usual \"no newline at end of file\"\n+warning), you could configure:\n+\n+-----------------------------\n+[diff \"SYMLINK\"]\n+\ttextconv = perl -pe 's/$/\\n/'\n+-----------------------------\n+\n Performing a three-way merge\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/diff.c b/diff.c\nindex 276e029..281b74a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1777,6 +1777,8 @@ static void diff_filespec_load_driver(struct diff_filespec *one)\n \n \tif (S_ISREG(one->mode))\n \t\tone->driver = userdiff_find_by_path(one->path);\n+\telse if(S_ISLNK(one->mode))\n+\t\tone->driver = userdiff_find_by_name(\"SYMLINK\");\n \n \t/* Fallback to default settings */\n \tif (!one->driver)\ndiff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh\nindex 408a19c..aa1daa2 100755\n--- a/t/t4011-diff-symlink.sh\n+++ b/t/t4011-diff-symlink.sh\n@@ -114,4 +114,15 @@ test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '\n \ttest_cmp expect actual\n '\n \n+cat >expect <<'EOF'\n+diff --git a/link.bin b/link.bin\n+index e69de29..dce41ec 120000\n+Binary files a/link.bin and b/link.bin differ\n+EOF\n+test_expect_success SYMLINKS 'symlinks do respect SYMLINK userdiff config' '\n+\tgit config diff.SYMLINK.binary true &&\n+\tgit diff link.bin >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.3.256.gb9713\n"},{"id":"151260","messageId":"7vpqw6u142.fsf@alter.siamese.dyndns.org","threadId":"25145","inReplyTo":"vpq8w2ug8u0.fsf@bauges.imag.fr","subject":"Re: [PATCH] sha1_name.c: update comment to mention :/foo syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-21T23:29:33Z","receivedAt":"2010-09-21T23:29:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n>>> +\t * :/foo -> last commit whose subject starts with foo\n>>\n>> Documenting what hasn't been is a good thing, but is it really up-to-date?\n>>\n>> Isn't it \"a randomly chosen recent commit whose subject contains regexp\n>> foo\" these days?\n>\n> I don't know.\n\nI was vaguely recalling this one when I wrote the above.\n\ncommit 57895105c4ff083d7c9bc59b2b88b9b5176c1915\nAuthor: Linus Torvalds \nDate: Fri Apr 23 08:20:20 2010 -0700\n\n Make :/ accept a regex rather than a fixed pattern\n \n This also makes it trigger anywhere in the commit message, rather than\n just at the beginning. Which tends to be a lot more useful.\n \n Signed-off-by: Linus Torvalds \n Signed-off-by: Junio C Hamano \n"},{"id":"151262","messageId":"AANLkTi=zSPegcPNY8jr4S5qeXWHrxuC+0SY-h5EhyA+6@mail.gmail.com","threadId":"25145","inReplyTo":"20100921211303.GB1188@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] diff: add a special SYMLINK user-diff driver","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-22T00:12:30Z","receivedAt":"2010-09-22T00:12:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"2010/9/21 Jeff King :\n> +However, it may happen that you do want to change the diff parameters\n> +specifically for symlinks. You can do that by configuring the special\n> +\"SYMLINK\" diff driver. For example, to add a newline to the end of\n> +the symlink contents (and suppress the usual \"no newline at end of file\"\n> +warning), you could configure:\n> +\n> +-----------------------------\n> +[diff \"SYMLINK\"]\n> +       textconv = perl -pe 's/$/\\n/'\n> +-----------------------------\n\nThat'll turn every \\n in the stream into \\n\\n, not add a newline to\nthe end of the file. Don't you mean:\n\n perl -0666 -pe 's/$/\\n/'\n\nOr, more efficiently:\n\n perl -ple 'END { print }'\n\n?\n"},{"id":"151266","messageId":"20100922003049.GA23931@sigill.intra.peff.net","threadId":"25145","inReplyTo":"AANLkTi=zSPegcPNY8jr4S5qeXWHrxuC+0SY-h5EhyA+6@mail.gmail.com","subject":"Re: [PATCH 2/2] diff: add a special SYMLINK user-diff driver","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-22T00:30:49Z","receivedAt":"2010-09-22T00:30:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 22, 2010 at 12:12:30AM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> 2010/9/21 Jeff King :\n> > +However, it may happen that you do want to change the diff parameters\n> > +specifically for symlinks. You can do that by configuring the special\n> > +\"SYMLINK\" diff driver. For example, to add a newline to the end of\n> > +the symlink contents (and suppress the usual \"no newline at end of file\"\n> > +warning), you could configure:\n> > +\n> > +-----------------------------\n> > +[diff \"SYMLINK\"]\n> > +       textconv = perl -pe 's/$/\\n/'\n> > +-----------------------------\n> \n> That'll turn every \\n in the stream into \\n\\n, not add a newline to\n> the end of the file. Don't you mean:\n> \n> perl -0666 -pe 's/$/\\n/'\n\nYeah, it will add a newline to the end of file, but it will also double\nexisting newlines. I wanted something short and clear so the reader\ncould understand what was going on, and symlink paths don't generally\ncontain an extra newline.\n\nJust \"perl -0pe 's/$/\\n/'\" would work, as symlink targets can't possibly\nhave embedded NULs.\n\nBut:\n\n> Or, more efficiently:\n> \n> perl -ple 'END { print }'\n\nThat one is even more readable, IMHO. But it isn't right. :)\nThe automatic line-handling actually adds the missing newline, so I\nthink it would have to be:\n\n perl -ple ''\n\n-Peff\n"},{"id":"151269","messageId":"AANLkTiktGT6aLN261FmXa4DeXX9+DNdFLX4_nFvNHCHW@mail.gmail.com","threadId":"25145","inReplyTo":"20100922003049.GA23931@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] diff: add a special SYMLINK user-diff driver","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-22T00:39:11Z","receivedAt":"2010-09-22T00:39:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Sep 22, 2010 at 00:30, Jeff King wrote:\n> On Wed, Sep 22, 2010 at 12:12:30AM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> 2010/9/21 Jeff King :\n>> > +However, it may happen that you do want to change the diff parameters\n>> > +specifically for symlinks. You can do that by configuring the special\n>> > +\"SYMLINK\" diff driver. For example, to add a newline to the end of\n>> > +the symlink contents (and suppress the usual \"no newline at end of file\"\n>> > +warning), you could configure:\n>> > +\n>> > +-----------------------------\n>> > +[diff \"SYMLINK\"]\n>> > +       textconv = perl -pe 's/$/\\n/'\n>> > +-----------------------------\n>>\n>> That'll turn every \\n in the stream into \\n\\n, not add a newline to\n>> the end of the file. Don't you mean:\n>>\n>>     perl -0666 -pe 's/$/\\n/'\n>\n> Yeah, it will add a newline to the end of file, but it will also double\n> existing newlines. I wanted something short and clear so the reader\n> could understand what was going on, and symlink paths don't generally\n> contain an extra newline.\n>\n> Just \"perl -0pe 's/$/\\n/'\" would work, as symlink targets can't possibly\n> have embedded NULs.\n>\n> But:\n>\n>> Or, more efficiently:\n>>\n>>     perl -ple 'END { print }'\n>\n> That one is even more readable, IMHO. But it isn't right. :)\n> The automatic line-handling actually adds the missing newline, so I\n> think it would have to be:\n>\n>  perl -ple ''\n\n perl -ple1\n\n:)\n"},{"id":"151273","messageId":"vpqvd5ycp3w.fsf@bauges.imag.fr","threadId":"25145","inReplyTo":"20100921210124.GA1188@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] diff: don't use pathname-based diff drivers for symlinks","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-09-22T05:40:51Z","receivedAt":"2010-09-22T05:40:51Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King writes:\n\n> We can drop the check in the textconv code, which is now redundant.\n\nAm I correct if I say that this makes \"[PATCH 3/3] RFC: blame,cat-file\n--textconv: Don't assume mode is ``S_IFREF | 0664''\" obsolete?\n\n(but patches 1 and 2 are still useful to check the behavior)\n\n> Technically, this could be breaking somebody's setup if:\n>\n> 1. They found some use for userdiff config on symlinks. Textconv is\n> already disabled. A custom diff driver might work.\n\nI'm wondering about cases other than regular files and symlinks here.\nDirectories are not a problem, since for Git, they somehow don't\nexist, but for example, \"git diff\" shows diff for subprojects too.\nAfter little testing, it seem the textconv is never applied on\nsubprojects, but I couldn't say why.\n\n> I find it unlikely, and given the potential breakage, it seems more like\n> exploiting a bug to get what you want.\n\nAgreed.\n\n> +cat >expect <<'EOF'\n> +diff --git a/file.bin b/file.bin\n> +index e69de29..d95f3ad 100644\n> +Binary files a/file.bin and b/file.bin differ\n> +diff --git a/link.bin b/link.bin\n> +index e69de29..dce41ec 120000\n> +--- a/link.bin\n> ++++ b/link.bin\n> +@@ -0,0 +1 @@\n> ++file.bin\n> +\\ No newline at end of file\n> +EOF\n> +test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '\n> +\tgit config diff.bin.binary true &&\n> +\tgit diff file.bin link.bin >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n\nIt's recommanded to put these cat <<'EOF' within the\ntest_expect_success, but otherwise, the code looks good.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"151274","messageId":"20100922055006.GA2980@sigill.intra.peff.net","threadId":"25145","inReplyTo":"vpqvd5ycp3w.fsf@bauges.imag.fr","subject":"Re: [PATCH 1/2] diff: don't use pathname-based diff drivers for symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-22T05:50:06Z","receivedAt":"2010-09-22T05:50:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 22, 2010 at 07:40:51AM +0200, Matthieu Moy wrote:\n\n> Jeff King writes:\n> \n> > We can drop the check in the textconv code, which is now redundant.\n> \n> Am I correct if I say that this makes \"[PATCH 3/3] RFC: blame,cat-file\n> --textconv: Don't assume mode is ``S_IFREF | 0664''\" obsolete?\n> \n> (but patches 1 and 2 are still useful to check the behavior)\n\nNo. This just moves the check of the mode further down in the callstack.\nBut the problem that Kirill's patch fixes is that we were feeding a\nbogus mode in the first place. So we still need to start actually\nsending the correct mode.\n\n> > Technically, this could be breaking somebody's setup if:\n> >\n> > 1. They found some use for userdiff config on symlinks. Textconv is\n> > already disabled. A custom diff driver might work.\n> \n> I'm wondering about cases other than regular files and symlinks here.\n> Directories are not a problem, since for Git, they somehow don't\n> exist, but for example, \"git diff\" shows diff for subprojects too.\n> After little testing, it seem the textconv is never applied on\n> subprojects, but I couldn't say why.\n\nYeah, gitlinks were the only example I could come up with, too. The\ncurrent textconv code already checked for S_ISREG, so you couldn't\ntextconv them. I'm not sure if you could do an external diff, or mark\nthem as binary. I suspect not, since we tend to treat them specially in\nthe diff code, but I didn't actually test. At any rate, those things are\nexplicitly disallowed by my change.\n\n> > +cat >expect <<'EOF'\n> [...]\n> It's recommanded to put these cat <<'EOF' within the\n> test_expect_success, but otherwise, the code looks good.\n\nI didn't think we had actually reached consensus on that.\n\n-Peff\n"},{"id":"151275","messageId":"vpqpqw6coia.fsf@bauges.imag.fr","threadId":"25145","inReplyTo":"20100921211303.GB1188@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] diff: add a special SYMLINK user-diff driver","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-09-22T05:53:49Z","receivedAt":"2010-09-22T05:53:49Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King writes:\n\n> So I wonder if the problem really is not in the userdiff code at all,\n> and we should be disabling gitattribute lookup entirely for non-regular\n> files.\n\nActually, I'd also say that what needs to be changed is on the\n.gitattributes side, not on the .gitconfig side.\n\nAnother option would be zsh-style globbing, like:\n\n# any symbolic link\n*(@) diff=link\n\n# Symbolic links named *.pdf\n*.pdf(@)\n\n(except that * means \"all\" to zsh, and should mean \"regular\" to Git).\n\nBut OTOH, that's a lot of work to implement, for very little benefit.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"151301","messageId":"vpq62xxg1eb.fsf@bauges.imag.fr","threadId":"25145","inReplyTo":"20100921211303.GB1188@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] diff: add a special SYMLINK user-diff driver","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-09-22T16:59:24Z","receivedAt":"2010-09-22T16:59:24Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King writes:\n\n> Also, this special SYMLINK driver assumes you want to treat all symlinks\n> the same. Which is probably good enough in practice. But if\n> gitattributes were aware of file modes, it would be nice to be able to\n> be able to apply multiple criteria (i.e., something besides just a\n> pathname match) to matching files. And then you could do something like\n> this in your gitattributes file:\n>\n> # all symlinks match this\n> * is_symlink diff=symlink-diff-driver\n> # only a symlink that matches this filename will get this\n> some-particular-symlink is_symlink diff=other-symlink-diff-driver\n> # and here we can explicitly say that only regular files get this\n> # treatment\n> *.pdf is_regular diff=pdf\n> # and since that's what you want 99% of the time, we can imply\n> # is_regular, which is backwards compatible and syntactically nicer.\n> # So this is equivalent to the above\n> *.pdf diff=pdf\n>\n> which is more flexible, and neatly solves the \"attributes shouldn't\n> affect symlinks at all\" issue.\n>\n> It does feel like overengineering, though.\n\nAfter thinking a bit more, I don't feel this is so much\noverengineering. Your initial SYMLINK driver proposal was somehow a\nsolution looking for a problem, and now we've found just one problem.\nSo I'd say the SYMLINK driver is overengineering for the potential\nproblem it solves: instead, people would probably prefer a\ndiff.showFinalNewlineForSymlink config option or so --- btw, wasn't\nthere a patch for that recently?\n\nOTOH, if the SYMLINK driver is ever used for something other than the\n\"no newline at end of file\", I'm pretty sure it will not be general\nenough. So, to me, the SYMLINK driver is a rather bad compromise\nbetween complexity and flexibility.\n\n(but the counter-argument \"that's a lot of work for very little\nbenefit\" still holds...)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"151512","messageId":"1285346639-6583-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"25145","inReplyTo":"7vpqw6u142.fsf@alter.siamese.dyndns.org","subject":"[PATCH] update comment and documentation for :/foo syntax","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2010-09-24T16:43:59Z","receivedAt":"2010-09-24T16:43:59Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"The documentation in revisions.txt did not match the implementation, and\nthe comment in sha1_name.c was incomplete.\n\nSigned-off-by: Matthieu Moy \n---\nJunio wrote:\n\n> Isn't it \"a randomly chosen recent commit whose subject contains regexp\n> foo\" these days?\n\nSo, that's right. Here's an updated version. I've kept the comment\nshort and vague, to avoid duplicating the doc, it should be sufficient\nas a reminder.\n\n Documentation/revisions.txt | 4 +++-\n sha1_name.c | 1 +\n 2 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\nindex fe846f0..3d4b79c 100644\n--- a/Documentation/revisions.txt\n+++ b/Documentation/revisions.txt\n@@ -107,11 +107,13 @@ the `$GIT_DIR/refs` directory or from the `$GIT_DIR/packed-refs` file.\n found.\n \n * A colon, followed by a slash, followed by a text (e.g. `:/fix nasty bug`): this names\n- a commit whose commit message starts with the specified text.\n+ a commit whose commit message matches the specified regular expression.\n This name returns the youngest matching commit which is\n reachable from any ref. If the commit message starts with a\n '!', you have to repeat that; the special sequence ':/!',\n followed by something else than '!' is reserved for now.\n+ The regular expression can match any part of the commit message. To\n+ match messages starting with a string, one can use e.g. `:/^foo`.\n \n * A suffix ':' followed by a path (e.g. `HEAD:README`); this names the blob or tree\n at the given path in the tree-ish object named by the part\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 5470a69..ba95a79 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1062,6 +1062,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t/* sha1:path --> object name of path in ent sha1\n \t * :path -> object name of path in index\n \t * :[0-3]:path -> object name of path in index at stage\n+\t * :/foo -> recent commit matching foo\n \t */\n \tif (name[0] == ':') {\n \t\tint stage = 0;\n-- \n1.7.3.2.g257b5f\n"}]}