patch, 3 partsFix {blame,cat-file} --textconv for cases with symlinks

t/t4042-diff-textconv-caching.sh, t/t8006-blame-textconv.sh, t/t8007-cat-file-textconv.sh

diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 91f8198..7668099 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -5,18 +5,19 @@ test_description='test textconv caching'
 
 cat >helper <<'EOF'
 #!/bin/sh
-sed 's/^/converted: /' "$@" >helper.out
+grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
+sed 's/^bin:/converted:/' "$@" >helper.out
 cat helper.out
 EOF
 chmod +x helper
 
 test_expect_success 'setup' '
-	echo foo content 1 >foo.bin &&
-	echo bar content 1 >bar.bin &&
+	echo "bin: foo content 1" >foo.bin &&
+	echo "bin: bar content 1" >bar.bin &&
 	git add . &&
 	git commit -m one &&
-	echo foo content 2 >foo.bin &&
-	echo bar content 2 >bar.bin &&
+	echo "bin: foo content 2" >foo.bin &&
+	echo "bin: bar content 2" >bar.bin &&
 	git commit -a -m two &&
 	echo "*.bin diff=magic" >.gitattributes &&
 	git config diff.magic.textconv ./helper &&
@@ -25,14 +26,14 @@ test_expect_success 'setup' '
 
 cat >expect <<EOF
 diff --git a/bar.bin b/bar.bin
-index fcf9166..28283d5 100644
+index 628fb83..f64d847 100644
 --- a/bar.bin
 +++ b/bar.bin
 @@ -1 +1 @@
 -converted: bar content 1
 +converted: bar content 2
 diff --git a/foo.bin b/foo.bin
-index d5b9fe3..1345db2 100644
+index 255496b..ad450ff 100644
 --- a/foo.bin
 +++ b/foo.bin
 @@ -1 +1 @@
@@ -59,7 +60,7 @@ test_expect_success 'cached textconv does not run helper' '
 
 cat >expect <<EOF
 diff --git a/bar.bin b/bar.bin
-index fcf9166..28283d5 100644
+index 628fb83..f64d847 100644
 --- a/bar.bin
 +++ b/bar.bin
 @@ -1,2 +1,2 @@
@@ -67,7 +68,7 @@ index fcf9166..28283d5 100644
 -converted: bar content 1
 +converted: bar content 2
 diff --git a/foo.bin b/foo.bin
-index d5b9fe3..1345db2 100644
+index 255496b..ad450ff 100644
 --- a/foo.bin
 +++ b/foo.bin
 @@ -1,2 +1,2 @@
@@ -76,7 +77,7 @@ index d5b9fe3..1345db2 100644
 +converted: foo content 2
 EOF
 test_expect_success 'changing textconv invalidates cache' '
-	echo other >other &&
+	echo "bin: other" >other &&
 	git config diff.magic.textconv "./helper other" &&
 	git diff HEAD^ HEAD >actual &&
 	test_cmp expect actual
@@ -84,7 +85,7 @@ test_expect_success 'changing textconv invalidates cache' '
 
 cat >expect <<EOF
 diff --git a/bar.bin b/bar.bin
-index fcf9166..28283d5 100644
+index 628fb83..f64d847 100644
 --- a/bar.bin
 +++ b/bar.bin
 @@ -1,2 +1,2 @@
@@ -92,7 +93,7 @@ index fcf9166..28283d5 100644
 -converted: bar content 1
 +converted: bar content 2
 diff --git a/foo.bin b/foo.bin
-index d5b9fe3..1345db2 100644
+index 255496b..ad450ff 100644
 --- a/foo.bin
 +++ b/foo.bin
 @@ -1 +1 @@
diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index 9ad96d4..d0f8d62 100755
--- a/t/t8006-blame-textconv.sh
+++ b/t/t8006-blame-textconv.sh
@@ -9,22 +9,23 @@ find_blame() {
 
 cat >helper <<'EOF'
 #!/bin/sh
-sed 's/^/converted: /' "$@"
+grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
+sed 's/^bin:/converted:/' "$@"
 EOF
 chmod +x helper
 
 test_expect_success 'setup ' '
-	echo test 1 >one.bin &&
-	echo test number 2 >two.bin &&
+	echo "bin: test 1" >one.bin &&
+	echo "bin: test number 2" >two.bin &&
 	git add . &&
 	GIT_AUTHOR_NAME=Number1 git commit -a -m First --date="2010-01-01 18:00:00" &&
-	echo test 1 version 2 >one.bin &&
-	echo test number 2 version 2 >>two.bin &&
+	echo "bin: test 1 version 2" >one.bin &&
+	echo "bin: test number 2 version 2" >>two.bin &&
 	GIT_AUTHOR_NAME=Number2 git commit -a -m Second --date="2010-01-01 20:00:00"
 '
 
 cat >expected <<EOF
-(Number2 2010-01-01 20:00:00 +0000 1) test 1 version 2
+(Number2 2010-01-01 20:00:00 +0000 1) bin: test 1 version 2
 EOF
 
 test_expect_success 'no filter specified' '
@@ -67,7 +68,7 @@ test_expect_success 'blame --textconv going through revisions' '
 '
 
 test_expect_success 'make a new commit' '
-	echo "test number 2 version 3" >>two.bin &&
+	echo "bin: test number 2 version 3" >>two.bin &&
 	GIT_AUTHOR_NAME=Number3 git commit -a -m Third --date="2010-01-01 22:00:00"
 '
 
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index 38ac05e..413d623 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -5,15 +5,16 @@ test_description='git cat-file textconv support'
 
 cat >helper <<'EOF'
 #!/bin/sh
-sed 's/^/converted: /' "$@"
+grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
+sed 's/^bin:/converted:/' "$@"
 EOF
 chmod +x helper
 
 test_expect_success 'setup ' '
-	echo test >one.bin &&
+	echo "bin: test" >one.bin &&
 	git add . &&
 	GIT_AUTHOR_NAME=Number1 git commit -a -m First --date="2010-01-01 18:00:00" &&
-	echo test version 2 >one.bin &&
+	echo "bin: test version 2" >one.bin &&
 	GIT_AUTHOR_NAME=Number2 git commit -a -m Second --date="2010-01-01 20:00:00"
 '
 
@@ -33,7 +34,7 @@ test_expect_success 'setup textconv filters' '
 '
 
 cat >expected <<EOF
-test version 2
+bin: test version 2
 EOF
 
 test_expect_success 'cat-file without --textconv' '
@@ -42,7 +43,7 @@ test_expect_success 'cat-file without --textconv' '
 '
 
 cat >expected <<EOF
-test
+bin: test
 EOF
 
 test_expect_success 'cat-file without --textconv on previous commit' '

t/t8006-blame-textconv.sh, t/t8007-cat-file-textconv.sh

diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index d0f8d62..3ed6155 100755
--- a/t/t8006-blame-textconv.sh
+++ b/t/t8006-blame-textconv.sh
@@ -17,10 +17,12 @@ chmod +x helper
 test_expect_success 'setup ' '
 	echo "bin: test 1" >one.bin &&
 	echo "bin: test number 2" >two.bin &&
+	ln -s one.bin symlink.bin &&
 	git add . &&
 	GIT_AUTHOR_NAME=Number1 git commit -a -m First --date="2010-01-01 18:00:00" &&
 	echo "bin: test 1 version 2" >one.bin &&
 	echo "bin: test number 2 version 2" >>two.bin &&
+	ln -sf two.bin symlink.bin &&
 	GIT_AUTHOR_NAME=Number2 git commit -a -m Second --date="2010-01-01 20:00:00"
 '
 
@@ -78,4 +80,46 @@ test_expect_success 'blame from previous revision' '
 	test_cmp expected result
 '
 
+cat >expected <<EOF
+(Number2 2010-01-01 20:00:00 +0000 1) two.bin
+EOF
+
+test_expect_success 'blame with --no-textconv (on symlink)' '
+	git blame --no-textconv symlink.bin >blame &&
+	find_blame <blame >result &&
+	test_cmp expected result
+'
+
+# fails with '...symlink.bin is not "binary" file'
+test_expect_failure 'blame --textconv (on symlink)' '
+	git blame --textconv symlink.bin >blame &&
+	find_blame <blame >result &&
+	test_cmp expected result
+'
+
+# cp two.bin three.bin  and make small tweak
+# (this will direct blame -C -C three.bin to consider two.bin and symlink.bin)
+test_expect_success 'make another new commit' '
+	echo "bin: test number 2" >three.bin &&
+	echo "bin: test number 2 version 2" >>three.bin &&
+	echo "bin: test number 2 version 3" >>three.bin &&
+	echo "bin: test number 3" >>three.bin &&
+	git add three.bin &&
+	GIT_AUTHOR_NAME=Number4 git commit -a -m Fourth --date="2010-01-01 23:00:00"
+'
+
+cat >expected <<EOF
+(Number1 2010-01-01 18:00:00 +0000 1) converted: test number 2
+(Number2 2010-01-01 20:00:00 +0000 2) converted: test number 2 version 2
+(Number3 2010-01-01 22:00:00 +0000 3) converted: test number 2 version 3
+(Number4 2010-01-01 23:00:00 +0000 4) converted: test number 3
+EOF
+
+# fails with '...symlink.bin is not "binary" file'
+test_expect_failure 'blame on last commit (-C -C, symlink)' '
+	git blame -C -C three.bin >blame &&
+	find_blame <blame >result &&
+	test_cmp expected result
+'
+
 test_done
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index 413d623..dfb2b04 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -12,6 +12,7 @@ chmod +x helper
 
 test_expect_success 'setup ' '
 	echo "bin: test" >one.bin &&
+	ln -s one.bin symlink.bin &&
 	git add . &&
 	GIT_AUTHOR_NAME=Number1 git commit -a -m First --date="2010-01-01 18:00:00" &&
 	echo "bin: test version 2" >one.bin &&
@@ -68,4 +69,32 @@ test_expect_success 'cat-file --textconv on previous commit' '
 	git cat-file --textconv HEAD^:one.bin >result &&
 	test_cmp expected result
 '
+
+echo -n "one.bin" >expected
+
+test_expect_success 'cat-file without --textconv (symlink)' '
+	git cat-file blob :symlink.bin >result &&
+	test_cmp expected result
+'
+
+cat >expected <<EOF
+fatal: git cat-file --textconv: unable to run textconv on :symlink.bin
+EOF
+
+# fails because cat-file tries to run converter on symlink.bin
+test_expect_failure 'cat-file --textconv on index (symlink)' '
+	! git cat-file --textconv :symlink.bin 2>result &&
+	test_cmp expected result
+'
+
+cat >expected <<EOF
+fatal: git cat-file --textconv: unable to run textconv on HEAD:symlink.bin
+EOF
+
+# fails because cat-file tries to run converter on symlink.bin
+test_expect_failure 'cat-file --textconv on HEAD (symlink)' '
+	! git cat-file --textconv HEAD:symlink.bin 2>result &&
+	test_cmp expected result
+'
+
 test_done

builtin.h, builtin/blame.c, builtin/cat-file.c, sha1_name.c, t/t8006-blame-textconv.sh, t/t8007-cat-file-textconv.sh

diff --git a/builtin.h b/builtin.h
index 0398d24..9bf69ee 100644
--- a/builtin.h
+++ b/builtin.h
@@ -35,7 +35,7 @@ void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c);
 
 extern int check_pager_config(const char *cmd);
 
-extern int textconv_object(const char *path, const unsigned char *sha1, char **buf, unsigned long *buf_size);
+extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, char **buf, unsigned long *buf_size);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);
 extern int cmd_annotate(int argc, const char **argv, const char *prefix);
diff --git a/builtin/blame.c b/builtin/blame.c
index 1015354..cfb7d30 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -83,6 +83,7 @@ struct origin {
 	struct commit *commit;
 	mmfile_t file;
 	unsigned char blob_sha1[20];
+	unsigned mode;
 	char path[FLEX_ARRAY];
 };
 
@@ -92,6 +93,7 @@ struct origin {
  * Return 1 if the conversion succeeds, 0 otherwise.
  */
 int textconv_object(const char *path,
+		    unsigned mode,
 		    const unsigned char *sha1,
 		    char **buf,
 		    unsigned long *buf_size)
@@ -100,7 +102,7 @@ int textconv_object(const char *path,
 	struct userdiff_driver *textconv;
 
 	df = alloc_filespec(path);
-	fill_filespec(df, sha1, S_IFREG | 0664);
+	fill_filespec(df, sha1, mode);
 	textconv = get_textconv(df);
 	if (!textconv) {
 		free_filespec(df);
@@ -125,7 +127,7 @@ static void fill_origin_blob(struct diff_options *opt,
 
 		num_read_blob++;
 		if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
-		    textconv_object(o->path, o->blob_sha1, &file->ptr, &file_size))
+		    textconv_object(o->path, o->mode, o->blob_sha1, &file->ptr, &file_size))
 			;
 		else
 			file->ptr = read_sha1_file(o->blob_sha1, &type, &file_size);
@@ -313,21 +315,23 @@ static struct origin *get_origin(struct scoreboard *sb,
  * for an origin is also used to pass the blame for the entire file to
  * the parent to detect the case where a child's blob is identical to
  * that of its parent's.
+ *
+ * This also fills origin->mode for correspoinding tree path.
  */
-static int fill_blob_sha1(struct origin *origin)
+static int fill_blob_sha1_and_mode(struct origin *origin)
 {
-	unsigned mode;
 	if (!is_null_sha1(origin->blob_sha1))
 		return 0;
 	if (get_tree_entry(origin->commit->object.sha1,
 			   origin->path,
-			   origin->blob_sha1, &mode))
+			   origin->blob_sha1, &origin->mode))
 		goto error_out;
 	if (sha1_object_info(origin->blob_sha1, NULL) != OBJ_BLOB)
 		goto error_out;
 	return 0;
  error_out:
 	hashclr(origin->blob_sha1);
+	origin->mode = S_IFINVALID;
 	return -1;
 }
 
@@ -360,12 +364,14 @@ static struct origin *find_origin(struct scoreboard *sb,
 			/*
 			 * If the origin was newly created (i.e. get_origin
 			 * would call make_origin if none is found in the
-			 * scoreboard), it does not know the blob_sha1,
+			 * scoreboard), it does not know the blob_sha1/mode,
 			 * so copy it.  Otherwise porigin was in the
-			 * scoreboard and already knows blob_sha1.
+			 * scoreboard and already knows blob_sha1/mode.
 			 */
-			if (porigin->refcnt == 1)
+			if (porigin->refcnt == 1) {
 				hashcpy(porigin->blob_sha1, cached->blob_sha1);
+				porigin->mode = cached->mode;
+			}
 			return porigin;
 		}
 		/* otherwise it was not very useful; free it */
@@ -400,6 +406,7 @@ static struct origin *find_origin(struct scoreboard *sb,
 		/* The path is the same as parent */
 		porigin = get_origin(sb, parent, origin->path);
 		hashcpy(porigin->blob_sha1, origin->blob_sha1);
+		porigin->mode = origin->mode;
 	} else {
 		/*
 		 * Since origin->path is a pathspec, if the parent
@@ -425,6 +432,7 @@ static struct origin *find_origin(struct scoreboard *sb,
 		case 'M':
 			porigin = get_origin(sb, parent, origin->path);
 			hashcpy(porigin->blob_sha1, p->one->sha1);
+			porigin->mode = p->one->mode;
 			break;
 		case 'A':
 		case 'T':
@@ -444,6 +452,7 @@ static struct origin *find_origin(struct scoreboard *sb,
 
 		cached = make_origin(porigin->commit, porigin->path);
 		hashcpy(cached->blob_sha1, porigin->blob_sha1);
+		cached->mode = porigin->mode;
 		parent->util = cached;
 	}
 	return porigin;
@@ -486,6 +495,7 @@ static struct origin *find_rename(struct scoreboard *sb,
 		    !strcmp(p->two->path, origin->path)) {
 			porigin = get_origin(sb, parent, p->one->path);
 			hashcpy(porigin->blob_sha1, p->one->sha1);
+			porigin->mode = p->one->mode;
 			break;
 		}
 	}
@@ -1099,6 +1109,7 @@ static int find_copy_in_parent(struct scoreboard *sb,
 
 			norigin = get_origin(sb, parent, p->one->path);
 			hashcpy(norigin->blob_sha1, p->one->sha1);
+			norigin->mode = p->one->mode;
 			fill_origin_blob(&sb->revs->diffopt, norigin, &file_p);
 			if (!file_p.ptr)
 				continue;
@@ -2075,7 +2086,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 		switch (st.st_mode & S_IFMT) {
 		case S_IFREG:
 			if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
-			    textconv_object(read_from, null_sha1, &buf.buf, &buf_len))
+			    textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len))
 				buf.len = buf_len;
 			else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)
 				die_errno("cannot open or read '%s'", read_from);
@@ -2455,11 +2466,11 @@ parse_done:
 	}
 	else {
 		o = get_origin(&sb, sb.final, path);
-		if (fill_blob_sha1(o))
+		if (fill_blob_sha1_and_mode(o))
 			die("no such path %s in %s", path, final_commit_name);
 
 		if (DIFF_OPT_TST(&sb.revs->diffopt, ALLOW_TEXTCONV) &&
-		    textconv_object(path, o->blob_sha1, (char **) &sb.final_buf,
+		    textconv_object(path, o->mode, o->blob_sha1, (char **) &sb.final_buf,
 				    &sb.final_buf_size))
 			;
 		else
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 76ec3fe..94632db 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -143,7 +143,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 			die("git cat-file --textconv %s: <object> must be <sha1:path>",
 			    obj_name);
 
-		if (!textconv_object(obj_context.path, sha1, &buf, &size))
+		if (!textconv_object(obj_context.path, obj_context.mode, sha1, &buf, &size))
 			die("git cat-file --textconv: unable to run textconv on %s",
 			    obj_name);
 		break;
diff --git a/sha1_name.c b/sha1_name.c
index 7b7e617..5470a69 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1068,7 +1068,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 		struct cache_entry *ce;
 		int pos;
 		if (namelen > 2 && name[1] == '/')
-			return get_sha1_oneline(name + 2, sha1);
+			return get_sha1_oneline(name + 2, sha1);    /* XXX also mode? */
 		if (namelen < 3 ||
 		    name[2] != ':' ||
 		    name[1] < '0' || '3' < name[1])
@@ -1095,6 +1095,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 				break;
 			if (ce_stage(ce) == stage) {
 				hashcpy(sha1, ce->sha1);
+				oc->mode = ce->ce_mode; /* XXX ok? */
 				return 0;
 			}
 			pos++;
diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index 3ed6155..43074d1 100755
--- a/t/t8006-blame-textconv.sh
+++ b/t/t8006-blame-textconv.sh
@@ -90,8 +90,7 @@ test_expect_success 'blame with --no-textconv (on symlink)' '
 	test_cmp expected result
 '
 
-# fails with '...symlink.bin is not "binary" file'
-test_expect_failure 'blame --textconv (on symlink)' '
+test_expect_success 'blame --textconv (on symlink)' '
 	git blame --textconv symlink.bin >blame &&
 	find_blame <blame >result &&
 	test_cmp expected result
@@ -115,8 +114,7 @@ cat >expected <<EOF
 (Number4 2010-01-01 23:00:00 +0000 4) converted: test number 3
 EOF
 
-# fails with '...symlink.bin is not "binary" file'
-test_expect_failure 'blame on last commit (-C -C, symlink)' '
+test_expect_success 'blame on last commit (-C -C, symlink)' '
 	git blame -C -C three.bin >blame &&
 	find_blame <blame >result &&
 	test_cmp expected result
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index dfb2b04..2b3d56a 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -81,8 +81,7 @@ cat >expected <<EOF
 fatal: git cat-file --textconv: unable to run textconv on :symlink.bin
 EOF
 
-# fails because cat-file tries to run converter on symlink.bin
-test_expect_failure 'cat-file --textconv on index (symlink)' '
+test_expect_success 'cat-file --textconv on index (symlink)' '
 	! git cat-file --textconv :symlink.bin 2>result &&
 	test_cmp expected result
 '
@@ -91,8 +90,7 @@ cat >expected <<EOF
 fatal: git cat-file --textconv: unable to run textconv on HEAD:symlink.bin
 EOF
 
-# fails because cat-file tries to run converter on symlink.bin
-test_expect_failure 'cat-file --textconv on HEAD (symlink)' '
+test_expect_success 'cat-file --textconv on HEAD (symlink)' '
 	! git cat-file --textconv HEAD:symlink.bin 2>result &&
 	test_cmp expected result
 '

--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2133,9 +2133,9 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	size = cache_entry_size(len);
 	ce = xcalloc(1, size);
 	hashcpy(ce->sha1, origin->blob_sha1);
+	ce->ce_mode = create_ce_mode(origin->mode);
 	memcpy(ce->name, path, len);
 	ce->ce_flags = create_ce_flags(len, 0);
-	ce->ce_mode = create_ce_mode(mode);
 	add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
 
 	/*

and it caused test failures, e.g.

    --- expected    2010-09-20 20:26:53.000000000 +0000
    +++ result      2010-09-20 20:26:53.000000000 +0000
    @@ -1 +1 @@
    -(Number2 2010-01-01 20:00:00 +0000 1) two.bin
    +(Not Committed Yet 2010-09-20 20:26:53 +0000 1) two.bin
    not ok - 9 blame with --no-textconv (on symlink)
    #
    #               git blame --no-textconv symlink.bin >blame &&
    #               find_blame <blame >result &&
    #               test_cmp expected result
    #

in t8006-blame-textconv.sh

So I dummily conclude I'de better not touch ce_mode...

But now I read comments in there 

        /*
         * Read the current index, replace the path entry with
         * origin->blob_sha1 without mucking with its mode or type
         * bits; we are not going to write this index out -- we just
         * want to run "diff-index --cached".
         */

And guess I probably should not touch mode here. Not 100% sure, but more
confident (call it heuristic programming :)), so I'm removing my XXX.


On Mon, Sep 20, 2010 at 02:21:28PM -0400, Jeff King wrote:
> On Sat, Sep 18, 2010 at 09:25:06PM +0400, Kirill Smelkov wrote:
> 
> > Instead get the mode from either worktree, index, .git, or origin
> > entries when blaming and pass it to textconv_object() as context.
> > 
> > The reason to do it is not to run textconv filters on symlinks.
> 
> I think this is absolutely a bug, and your solution is definitely in the
> right direction. We obviously can't just ignore the mode when deciding
> whether to textconv. I suspect there is similar breakage for S_IFGITLINK
> files, though they are perhaps less likely in practice to match another
> filetype's extension.
> 
> So all three patches look sane to me, with the caveat that I also don't
> know the blame code very well.
> 
> I agree with Matthieu's points on cleaning up the commit messages, and
> there is a small comment typo in this third patch:

Jeff, thanks for you comments - appreciated.

> > @@ -313,21 +315,23 @@ static struct origin *get_origin(struct scoreboard *sb,
> >   * for an origin is also used to pass the blame for the entire file to
> >   * the parent to detect the case where a child's blob is identical to
> >   * that of its parent's.
> > + *
> > + * This also fills origin->mode for correspoinding tree path.
> 
> Typo: s/poind/pond

Good eyes, thanks.

I should sleep more :)



Thanks again to everyone. I'll repost this series as v2 shortly.

Kirill

diff --git a/sha1_name.c b/sha1_name.c
index 7b7e617..d7ab72a 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1062,6 +1062,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 	/* sha1:path --> object name of path in ent sha1
 	 * :path -> object name of path in index
 	 * :[0-3]:path -> object name of path in index at stage
+	 * :/foo -> last commit whose subject starts with foo
 	 */
 	if (name[0] == ':') {
 		int stage = 0;

Documentation/revisions.txt, sha1_name.c

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index fe846f0..3d4b79c 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -107,11 +107,13 @@ the `$GIT_DIR/refs` directory or from the `$GIT_DIR/packed-refs` file.
   found.
 
 * A colon, followed by a slash, followed by a text (e.g. `:/fix nasty bug`): this names
-  a commit whose commit message starts with the specified text.
+  a commit whose commit message matches the specified regular expression.
   This name returns the youngest matching commit which is
   reachable from any ref.  If the commit message starts with a
   '!', you have to repeat that;  the special sequence ':/!',
   followed by something else than '!' is reserved for now.
+  The regular expression can match any part of the commit message. To
+  match messages starting with a string, one can use e.g. `:/^foo`.
 
 * A suffix ':' followed by a path (e.g. `HEAD:README`); this names the blob or tree
   at the given path in the tree-ish object named by the part
diff --git a/sha1_name.c b/sha1_name.c
index 5470a69..ba95a79 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1062,6 +1062,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 	/* sha1:path --> object name of path in ent sha1
 	 * :path -> object name of path in index
 	 * :[0-3]:path -> object name of path in index at stage
+	 * :/foo -> recent commit matching foo
 	 */
 	if (name[0] == ':') {
 		int stage = 0;

diff --git a/diff.c b/diff.c
index 6fb18ae..58c4477 100644
--- a/diff.c
+++ b/diff.c
@@ -1771,8 +1771,14 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two, char *pre
 
 static void diff_filespec_load_driver(struct diff_filespec *one)
 {
-	if (!one->driver)
+	if (one->driver)
+		return;
+
+	if (S_ISLNK(one->mode))
+		one->driver = userdiff_find_by_name("SYMLINK");
+	else
 		one->driver = userdiff_find_by_path(one->path);
+
 	if (!one->driver)
 		one->driver = userdiff_find_by_name("default");
 }
@@ -1820,7 +1826,7 @@ struct userdiff_driver *get_textconv(struct diff_filespec *one)
 {
 	if (!DIFF_FILE_VALID(one))
 		return NULL;
-	if (!S_ISREG(one->mode))
+	if (!S_ISREG(one->mode) && !S_ISLNK(one->mode))
 		return NULL;
 	diff_filespec_load_driver(one);
 	if (!one->driver->textconv)

after which I successfully tested with:

  git init repo && cd repo &&
  echo content >file.txt &&
  ln -s file.txt link.txt &&
  echo '*.txt diff=txt' >.gitattributes &&
  git add . && git commit -m foo &&
  git config diff.txt.textconv "sed 's/^/converted: /'" &&
  git config diff.SYMLINK.textconv "perl -pe 's/$/\n/'" &&
  git show

It works with the whole range of diff config, so you could do something
as awesomely stupid as:

  $ git config diff.SYMLINK.binary true
  $ git show link.txt
  ...
  diff --git a/link.txt b/link.txt
  new file mode 120000
  index 0000000..4c33073
  Binary files /dev/null and b/link.txt differ

If you really wanted the "dereference my symlinks" behavior, you could
do:

  git config diff.SYMLINK.textconv 'sh -c "cat `cat $1`" -'

but that is not quite right; you would actually need to dereference
relative symlinks with respect to the link itself, which textconv never
gets (plus you would probably want to handle broken links more
gracefully).

Anyway, as I said at the beginning, for me this was just for fun. I find
the intended use rather silly, but maybe somebody else is interested. I
do think it's the right way of implementing such a feature, because we
already turn off textconv when making patches that are meant to be
applied rather than viewed. However, I didn't do any testing or give
much thought to whether this would affect any unintended code paths.

-Peff

diff.c, t/t4011-diff-symlink.sh

diff --git a/diff.c b/diff.c
index 9a5c77c..276e029 100644
--- a/diff.c
+++ b/diff.c
@@ -1771,8 +1771,14 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two, char *pre
 
 static void diff_filespec_load_driver(struct diff_filespec *one)
 {
-	if (!one->driver)
+	/* Use already-loaded driver */
+	if (one->driver)
+		return;
+
+	if (S_ISREG(one->mode))
 		one->driver = userdiff_find_by_path(one->path);
+
+	/* Fallback to default settings */
 	if (!one->driver)
 		one->driver = userdiff_find_by_name("default");
 }
@@ -1820,8 +1826,7 @@ struct userdiff_driver *get_textconv(struct diff_filespec *one)
 {
 	if (!DIFF_FILE_VALID(one))
 		return NULL;
-	if (!S_ISREG(one->mode))
-		return NULL;
+
 	diff_filespec_load_driver(one);
 	if (!one->driver->textconv)
 		return NULL;
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index 6f69489..408a19c 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -88,4 +88,30 @@ test_expect_success SYMLINKS \
     test_must_fail git diff --no-index pinky brain > output 2> output.err &&
     grep narf output &&
     ! grep error output.err'
+
+test_expect_success SYMLINKS 'setup symlinks with attributes' '
+	echo "*.bin diff=bin" >>.gitattributes &&
+	echo content >file.bin &&
+	ln -s file.bin link.bin &&
+	git add -N file.bin link.bin
+'
+
+cat >expect <<'EOF'
+diff --git a/file.bin b/file.bin
+index e69de29..d95f3ad 100644
+Binary files a/file.bin and b/file.bin differ
+diff --git a/link.bin b/link.bin
+index e69de29..dce41ec 120000
+--- a/link.bin
++++ b/link.bin
+@@ -0,0 +1 @@
++file.bin
+\ No newline at end of file
+EOF
+test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '
+	git config diff.bin.binary true &&
+	git diff file.bin link.bin >actual &&
+	test_cmp expect actual
+'
+
 test_done

Documentation/gitattributes.txt, diff.c, t/t4011-diff-symlink.sh

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e5a27d8..cc3bddf 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -405,6 +405,8 @@ String::
 	by the configuration variables in the "diff.foo" section of the
 	git config file.
 
+NOTE: Diff attributes apply only to regular files, not symlinks or
+submodules. See the section on special diff drivers below.
 
 Defining an external diff driver
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -572,6 +574,36 @@ and now produces better output), you can remove the cache
 manually with `git update-ref -d refs/notes/textconv/jpg` (where
 "jpg" is the name of the diff driver, as in the example above).
 
+Special diff drivers
+^^^^^^^^^^^^^^^^^^^^
+
+Diff drivers matched by attributes are used only for regular files. They
+do not apply to symbolic links, as a diff of symbolic links is not
+performed on the content of the files the links point to, but rather
+on the names of the files pointed to by the links. This makes a
+configuration like the following work as expected:
+
+-----------------------------
+$ echo '*.pdf diff=pdf' >.gitattributes
+$ git config diff.pdf.textconv pdftotext
+$ ln -s file.pdf link.pdf
+-----------------------------
+
+The regular file "file.pdf" will be converted to text for diffing, but
+the symlink "link.pdf", whose diffed contents are not PDF at all but
+rather the pathname "file.pdf", will not be affected.
+
+However, it may happen that you do want to change the diff parameters
+specifically for symlinks. You can do that by configuring the special
+"SYMLINK" diff driver. For example, to add a newline to the end of
+the symlink contents (and suppress the usual "no newline at end of file"
+warning), you could configure:
+
+-----------------------------
+[diff "SYMLINK"]
+	textconv = perl -pe 's/$/\n/'
+-----------------------------
+
 Performing a three-way merge
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/diff.c b/diff.c
index 276e029..281b74a 100644
--- a/diff.c
+++ b/diff.c
@@ -1777,6 +1777,8 @@ static void diff_filespec_load_driver(struct diff_filespec *one)
 
 	if (S_ISREG(one->mode))
 		one->driver = userdiff_find_by_path(one->path);
+	else if(S_ISLNK(one->mode))
+		one->driver = userdiff_find_by_name("SYMLINK");
 
 	/* Fallback to default settings */
 	if (!one->driver)
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index 408a19c..aa1daa2 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -114,4 +114,15 @@ test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '
 	test_cmp expect actual
 '
 
+cat >expect <<'EOF'
+diff --git a/link.bin b/link.bin
+index e69de29..dce41ec 120000
+Binary files a/link.bin and b/link.bin differ
+EOF
+test_expect_success SYMLINKS 'symlinks do respect SYMLINK userdiff config' '
+	git config diff.SYMLINK.binary true &&
+	git diff link.bin >actual &&
+	test_cmp expect actual
+'
+
 test_done