[BUG, PATCH v3 0/3] Fix {blame,cat-file} --textconv for cases with symlinks

[BUG, PATCH v3 0/3] Fix {blame,cat-file} --textconv for cases with symlinks

[Kirill Smelkov]

Recently I've spot a bug in git blame --textconv, which was wrongly
calling pdftotext (my *.pdf conversion program) on a symlink.pdf, and I
was getting something like

    $ git blame -C -C regular-file.pdf
    Error: May not be a PDF file (continuing anyway)
    Error: PDF file is damaged - attempting to reconstruct xref table...
    Error: Couldn't find trailer dictionary
    Error: Couldn't read xref table
    Warning: program returned non-zero exit code #1
    fatal: unable to read files to diff

That errors come from pdftotext run on symlink.pdf being extracted to
/tmp/ with one-line plain-text content pointing to link destination.


Please apply and thanks,
Kirill


v3:

 o Slightly changed patches descriptions as per comment by Matthieu, and added
   Matthieu's Reviewed-by.

v2:

 o Incorporated suggestions by Matthieu and Jeff (details in each patch)


Kirill Smelkov (3):
  tests: Prepare --textconv tests for correctly-failing conversion
    program
  blame,cat-file: Demonstrate --textconv is wrongly running converter
    on symlinks
  blame,cat-file --textconv: Don't assume mode is ``S_IFREF | 0664''

 builtin.h                        |    2 +-
 builtin/blame.c                  |   33 ++++++++++++++-------
 builtin/cat-file.c               |    2 +-
 sha1_name.c                      |    2 +
 t/t4042-diff-textconv-caching.sh |   25 ++++++++--------
 t/t8006-blame-textconv.sh        |   58 +++++++++++++++++++++++++++++++++----
 t/t8007-cat-file-textconv.sh     |   36 ++++++++++++++++++++---
 7 files changed, 121 insertions(+), 37 deletions(-)

-- 
1.7.3.rc2

[PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Kirill Smelkov]

The textconv filter is sometimes incorrectly ran on a temporary file
whose content is the target of a symbolic link, instead of actual file
content. Prepare to test this by marking the content of the file to
convert with "bin:", and let the helper die if "bin:" is not found in
the file content.

(Description by Matthieu Moy)

Cc: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
Cc: Clément Poulain <clement.poulain@ensimag.imag.fr>
Cc: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Cc: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Cc: Jeff King <peff@peff.net>
Signed-off-by: Kirill Smelkov <kirr@landau.phys.spbu.ru>
Reviewed-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---

v3:

 o Add Matthieu's Reviewed-by, and move secondary note to the end to avoid
   distracting an intrested reader.

v2:

 o Changed patch description as suggested by Matthieu


 t/t4042-diff-textconv-caching.sh |   25 +++++++++++++------------
 t/t8006-blame-textconv.sh        |   15 ++++++++-------
 t/t8007-cat-file-textconv.sh     |   11 ++++++-----
 3 files changed, 27 insertions(+), 24 deletions(-)

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' '
-- 
1.7.3.rc2

[PATCH v3 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks

[Kirill Smelkov]

git blame --textconv is wrongly calling the textconv filter on
symlinks: symlinks are stored as blobs whose content is the target of
the link, and blame calls the textconv filter on a temporary file
filled-in with the content of this blob.

For example:

    $ git blame -C -C regular-file.pdf
    Error: May not be a PDF file (continuing anyway)
    Error: PDF file is damaged - attempting to reconstruct xref table...
    Error: Couldn't find trailer dictionary
    Error: Couldn't read xref table
    Warning: program returned non-zero exit code #1
    fatal: unable to read files to diff

That errors come from pdftotext run on symlink.pdf being extracted to
/tmp/ with one-line plain-text content pointing to link destination.

So several failures are demonstrated here:

  - git cat-file --textconv :symlink.bin    # also HEAD:symlink.bin
  - git blame --textconv symlink.bin
  - git blame -C -C --textconv regular-file # but also looks on symlink.bin

At present they all fail with something like.

    E: /tmp/j3ELEs_symlink.bin is not "binary" file

NOTE: git diff doesn't try to textconv the pathnames, it runs the
textual diff without textconv, which is the expected behavior.

(Description partly by Matthieu Moy)

Cc: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
Cc: Clément Poulain <clement.poulain@ensimag.imag.fr>
Cc: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Cc: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Cc: Jeff King <peff@peff.net>
Signed-off-by: Kirill Smelkov <kirr@landau.phys.spbu.ru>
Reviewed-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---

v3:

 o Slight cleanup of the description
 o Reviewed-by: Matthieu

v2:

 (As suggested by Matthieu)
 o Changed patch descriptio 
 o Moved most of >expected preparation into test_expect_*
 o Changed multiple echo'es into cat <<EOF
 o Use printf "%s" instead echo -n, since latter is said to be not very
   portable


 t/t8006-blame-textconv.sh    |   45 ++++++++++++++++++++++++++++++++++++++++++
 t/t8007-cat-file-textconv.sh |   27 +++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index d0f8d62..7d42e96 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,47 @@ 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' '
+	cat >three.bin <<\EOF &&
+bin: test number 2
+bin: test number 2 version 2
+bin: test number 2 version 3
+bin: test number 3
+EOF
+	git add three.bin &&
+	GIT_AUTHOR_NAME=Number4 git commit -a -m Fourth --date="2010-01-01 23:00:00"
+'
+
+# 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 &&
+	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
+	test_cmp expected result
+'
+
 test_done
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index 413d623..f747c05 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,30 @@ test_expect_success 'cat-file --textconv on previous commit' '
 	git cat-file --textconv HEAD^:one.bin >result &&
 	test_cmp expected result
 '
+
+test_expect_success 'cat-file without --textconv (symlink)' '
+	git cat-file blob :symlink.bin >result &&
+	printf "%s" "one.bin" >expected
+	test_cmp expected result
+'
+
+
+# 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 &&
+	cat >expected <<\EOF &&
+fatal: git cat-file --textconv: unable to run textconv on :symlink.bin
+EOF
+	test_cmp expected result
+'
+
+# 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 &&
+	cat >expected <<EOF &&
+fatal: git cat-file --textconv: unable to run textconv on HEAD:symlink.bin
+EOF
+	test_cmp expected result
+'
+
 test_done
-- 
1.7.3.rc2

[PATCH v3 3/3] blame,cat-file --textconv: Don't assume mode is ``S_IFREF | 0664''

[Kirill Smelkov]

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.

Cc: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
Cc: Clément Poulain <clement.poulain@ensimag.imag.fr>
Cc: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Cc: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Cc: Jeff King <peff@peff.net>
Signed-off-by: Kirill Smelkov <kirr@landau.phys.spbu.ru>
Reviewed-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---

v3:

 o Reviewed-by: Matthieu

v2:

 o Thanks to Matthieu and Jeff got a bit more sure I'm not doing stupid things,
   so
 o My XXX were removed, and the patch is no longer an RFC

 builtin.h                    |    2 +-
 builtin/blame.c              |   33 ++++++++++++++++++++++-----------
 builtin/cat-file.c           |    2 +-
 sha1_name.c                  |    2 ++
 t/t8006-blame-textconv.sh    |    6 ++----
 t/t8007-cat-file-textconv.sh |    6 ++----
 6 files changed, 30 insertions(+), 21 deletions(-)

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..f5fccc1 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 corresponding 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..4458fe8 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1068,6 +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] == '/')
+			/* don't need mode for commit */
 			return get_sha1_oneline(name + 2, sha1);
 		if (namelen < 3 ||
 		    name[2] != ':' ||
@@ -1095,6 +1096,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;
 				return 0;
 			}
 			pos++;
diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index 7d42e96..400d9f1 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
@@ -110,8 +109,7 @@ EOF
 	GIT_AUTHOR_NAME=Number4 git commit -a -m Fourth --date="2010-01-01 23:00:00"
 '
 
-# 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 &&
 	cat >expected <<\EOF &&
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index f747c05..40e4af5 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -77,8 +77,7 @@ test_expect_success 'cat-file without --textconv (symlink)' '
 '
 
 
-# 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 &&
 	cat >expected <<\EOF &&
 fatal: git cat-file --textconv: unable to run textconv on :symlink.bin
@@ -86,8 +85,7 @@ EOF
 	test_cmp expected result
 '
 
-# 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 &&
 	cat >expected <<EOF &&
 fatal: git cat-file --textconv: unable to run textconv on HEAD:symlink.bin
-- 
1.7.3.rc2

Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Junio C Hamano]

Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:

> 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; }

You are not feeding arguments you think you are to the above "echo":

        $ cat >/var/tmp/j.sh <<\EOF
        #!/bin/sh
        e () {
                i=0
                for s
                do
                        i=$(( $i + 1 ))
                        echo "$i: $s"
                done
        }
        f () {
                e "E: $@ is not binary"
        }
        f 1 "2 3" 4
        EOF
        $ sh /var/tmp/j.sh
        1: E: 1
        2: 2 3
        3: 4 is not binary

Granted, echo is forgiving and will concatenate the arguments it gets with
a space in between, but you would either want to either:

 (1) make it more explicit that helper gets only one argument, by saying
     "$1" instead of "$@", in all places in the helper script; or

 (2) if you are planning to make 'helper' capable of handling multiple
     input files, show the error message for the ones that are not binary
     (you would probably need a loop for that).

I think (1) would be sufficient in this case.

> +sed 's/^bin:/converted:/' "$@" >helper.out

Minor nit: this is inconsistent with the check done with grep above that
insists that the colon is followed by a SP.

Re: [PATCH v3 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks

[Junio C Hamano]

Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:

> diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
> index d0f8d62..7d42e96 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"
>  '

Doesn't this need some test prereq marker?

Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Kirill Smelkov]

On Mon, Sep 27, 2010 at 11:23:35AM -0700, Junio C Hamano wrote:
> Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:


> > +sed 's/^bin:/converted:/' "$@" >helper.out
> 
> Minor nit: this is inconsistent with the check done with grep above that
> insists that the colon is followed by a SP.

Yes, you are right. I'll amend it.

> > 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; }
> 
> You are not feeding arguments you think you are to the above "echo":
> 
>         $ cat >/var/tmp/j.sh <<\EOF
>         #!/bin/sh
>         e () {
>                 i=0
>                 for s
>                 do
>                         i=$(( $i + 1 ))
>                         echo "$i: $s"
>                 done
>         }
>         f () {
>                 e "E: $@ is not binary"
>         }
>         f 1 "2 3" 4
>         EOF
>         $ sh /var/tmp/j.sh
>         1: E: 1
>         2: 2 3
>         3: 4 is not binary
> 
> Granted, echo is forgiving and will concatenate the arguments it gets with
> a space in between, but you would either want to either:
> 
>  (1) make it more explicit that helper gets only one argument, by saying
>      "$1" instead of "$@", in all places in the helper script; or
> 
>  (2) if you are planning to make 'helper' capable of handling multiple
>      input files, show the error message for the ones that are not binary
>      (you would probably need a loop for that).
> 
> I think (1) would be sufficient in this case.

I too think (1) is right. It was just that originally there was $@
(which I now understand was wrong). So ok to apply the following patch
on top of this series? (I assume it's ok, and will repost the whole
thing)

---- 8< ----

From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Tue, 28 Sep 2010 15:34:48 +0400
Subject: [PATCH 4/4] t4042,t8006,t8007: Textconv converter should take only one argument

Textconv helper in this tests was incorrectly referencing $@, which goes
agains textconv "spec". I quote Documentation/gitattributes.txt

""" The `textconv` config option is used to define a program for
    performing such a conversion. The program should take a single
    argument, the name of a file to convert, and produce the
    resulting text on stdout. """

So correct textconv helpers to use $1 instead.

Noticed-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 t/t4042-diff-textconv-caching.sh |    4 ++--
 t/t8006-blame-textconv.sh        |    4 ++--
 t/t8007-cat-file-textconv.sh     |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 68fee12..6aaa10b 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -5,8 +5,8 @@ test_description='test textconv caching'
 
 cat >helper <<'EOF'
 #!/bin/sh
-grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
-sed 's/^bin: /converted: /' "$@" >helper.out
+grep -q '^bin: ' "$1" || { echo "E: $1 is not \"binary\" file" 1>&2; exit 1; }
+sed 's/^bin: /converted: /' "$1" >helper.out
 cat helper.out
 EOF
 chmod +x helper
diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index a8c311f..dbf623b 100755
--- a/t/t8006-blame-textconv.sh
+++ b/t/t8006-blame-textconv.sh
@@ -9,8 +9,8 @@ find_blame() {
 
 cat >helper <<'EOF'
 #!/bin/sh
-grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
-sed 's/^bin: /converted: /' "$@"
+grep -q '^bin: ' "$1" || { echo "E: $1 is not \"binary\" file" 1>&2; exit 1; }
+sed 's/^bin: /converted: /' "$1"
 EOF
 chmod +x helper
 
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index 5e2e0d2..78a0085 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -5,8 +5,8 @@ test_description='git cat-file textconv support'
 
 cat >helper <<'EOF'
 #!/bin/sh
-grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
-sed 's/^bin: /converted: /' "$@"
+grep -q '^bin: ' "$1" || { echo "E: $1 is not \"binary\" file" 1>&2; exit 1; }
+sed 's/^bin: /converted: /' "$1"
 EOF
 chmod +x helper
 
-- 
1.7.3.19.g3fe0a

Re: [PATCH v3 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks

[Kirill Smelkov]

On Mon, Sep 27, 2010 at 11:27:47AM -0700, Junio C Hamano wrote:
> Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:
> 
> > diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
> > index d0f8d62..7d42e96 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"
> >  '
> 
> Doesn't this need some test prereq marker?

Yes, I forgot about symlink prereqs.

Below is interdiff of the fixes to this patch. Updated series with all
queued fixes will be reposted shortly.
 

Thanks,
Kirill


---- 8< ----

diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index 917d362..324f127 100755
--- a/t/t8006-blame-textconv.sh
+++ b/t/t8006-blame-textconv.sh
@@ -17,12 +17,16 @@ 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 &&
+	if test_have_prereq SYMLINKS; then
+		ln -s one.bin symlink.bin
+	fi &&
 	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 &&
+	if test_have_prereq SYMLINKS; then
+		ln -sf two.bin symlink.bin
+	fi &&
 	GIT_AUTHOR_NAME=Number2 git commit -a -m Second --date="2010-01-01 20:00:00"
 '
 
@@ -84,14 +88,14 @@ cat >expected <<EOF
 (Number2 2010-01-01 20:00:00 +0000 1) two.bin
 EOF
 
-test_expect_success 'blame with --no-textconv (on symlink)' '
+test_expect_success SYMLINKS '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)' '
+test_expect_failure SYMLINKS 'blame --textconv (on symlink)' '
 	git blame --textconv symlink.bin >blame &&
 	find_blame <blame >result &&
 	test_cmp expected result
@@ -99,7 +103,7 @@ test_expect_failure 'blame --textconv (on symlink)' '
 
 # 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' '
+test_expect_success SYMLINKS 'make another new commit' '
 	cat >three.bin <<\EOF &&
 bin: test number 2
 bin: test number 2 version 2
@@ -111,7 +115,7 @@ EOF
 '
 
 # fails with '...symlink.bin is not "binary" file'
-test_expect_failure 'blame on last commit (-C -C, symlink)' '
+test_expect_failure SYMLINKS 'blame on last commit (-C -C, symlink)' '
 	git blame -C -C three.bin >blame &&
 	find_blame <blame >result &&
 	cat >expected <<\EOF &&
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index 7bec265..03af7ae 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -12,7 +12,9 @@ chmod +x helper
 
 test_expect_success 'setup ' '
 	echo "bin: test" >one.bin &&
-	ln -s one.bin symlink.bin &&
+	if test_have_prereq SYMLINKS; then
+		ln -s one.bin symlink.bin
+	fi &&
 	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 &&
@@ -70,7 +72,7 @@ test_expect_success 'cat-file --textconv on previous commit' '
 	test_cmp expected result
 '
 
-test_expect_success 'cat-file without --textconv (symlink)' '
+test_expect_success SYMLINKS 'cat-file without --textconv (symlink)' '
 	git cat-file blob :symlink.bin >result &&
 	printf "%s" "one.bin" >expected
 	test_cmp expected result
@@ -78,7 +80,7 @@ test_expect_success 'cat-file without --textconv (symlink)' '
 
 
 # fails because cat-file tries to run converter on symlink.bin
-test_expect_failure 'cat-file --textconv on index (symlink)' '
+test_expect_failure SYMLINKS 'cat-file --textconv on index (symlink)' '
 	! git cat-file --textconv :symlink.bin 2>result &&
 	cat >expected <<\EOF &&
 fatal: git cat-file --textconv: unable to run textconv on :symlink.bin
@@ -87,7 +89,7 @@ EOF
 '
 
 # fails because cat-file tries to run converter on symlink.bin
-test_expect_failure 'cat-file --textconv on HEAD (symlink)' '
+test_expect_failure SYMLINKS 'cat-file --textconv on HEAD (symlink)' '
 	! git cat-file --textconv HEAD:symlink.bin 2>result &&
 	cat >expected <<EOF &&
 fatal: git cat-file --textconv: unable to run textconv on HEAD:symlink.bin

Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Kirill Smelkov]

On Tue, Sep 28, 2010 at 04:07:22PM +0400, Kirill Smelkov wrote:
> On Mon, Sep 27, 2010 at 11:23:35AM -0700, Junio C Hamano wrote:
> > Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:
> 
> 
> > > +sed 's/^bin:/converted:/' "$@" >helper.out
> > 
> > Minor nit: this is inconsistent with the check done with grep above that
> > insists that the colon is followed by a SP.
> 
> Yes, you are right. I'll amend it.
> 
> > > 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; }
> > 
> > You are not feeding arguments you think you are to the above "echo":
> > 
> >         $ cat >/var/tmp/j.sh <<\EOF
> >         #!/bin/sh
> >         e () {
> >                 i=0
> >                 for s
> >                 do
> >                         i=$(( $i + 1 ))
> >                         echo "$i: $s"
> >                 done
> >         }
> >         f () {
> >                 e "E: $@ is not binary"
> >         }
> >         f 1 "2 3" 4
> >         EOF
> >         $ sh /var/tmp/j.sh
> >         1: E: 1
> >         2: 2 3
> >         3: 4 is not binary
> > 
> > Granted, echo is forgiving and will concatenate the arguments it gets with
> > a space in between, but you would either want to either:
> > 
> >  (1) make it more explicit that helper gets only one argument, by saying
> >      "$1" instead of "$@", in all places in the helper script; or
> > 
> >  (2) if you are planning to make 'helper' capable of handling multiple
> >      input files, show the error message for the ones that are not binary
> >      (you would probably need a loop for that).
> > 
> > I think (1) would be sufficient in this case.
> 
> I too think (1) is right. It was just that originally there was $@
> (which I now understand was wrong). So ok to apply the following patch
> on top of this series? (I assume it's ok, and will repost the whole
> thing)
> 
> ---- 8< ----
> 
> From: Kirill Smelkov <kirr@mns.spb.ru>
> Date: Tue, 28 Sep 2010 15:34:48 +0400
> Subject: [PATCH 4/4] t4042,t8006,t8007: Textconv converter should take only one argument
> 
> Textconv helper in this tests was incorrectly referencing $@, which goes
> agains textconv "spec". I quote Documentation/gitattributes.txt
> 
> """ The `textconv` config option is used to define a program for
>     performing such a conversion. The program should take a single
>     argument, the name of a file to convert, and produce the
>     resulting text on stdout. """
> 
> So correct textconv helpers to use $1 instead.
> 
> Noticed-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> ---
>  t/t4042-diff-textconv-caching.sh |    4 ++--
>  t/t8006-blame-textconv.sh        |    4 ++--
>  t/t8007-cat-file-textconv.sh     |    4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
> index 68fee12..6aaa10b 100755
> --- a/t/t4042-diff-textconv-caching.sh
> +++ b/t/t4042-diff-textconv-caching.sh
> @@ -5,8 +5,8 @@ test_description='test textconv caching'
>  
>  cat >helper <<'EOF'
>  #!/bin/sh
> -grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
> -sed 's/^bin: /converted: /' "$@" >helper.out
> +grep -q '^bin: ' "$1" || { echo "E: $1 is not \"binary\" file" 1>&2; exit 1; }
> +sed 's/^bin: /converted: /' "$1" >helper.out
>  cat helper.out
>  EOF
>  chmod +x helper

Please ignore this - changing so breaks textconv cache tests in this
file.

I'll come up with updated patch.

Sorry for the noise...

Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Kirill Smelkov]

On Tue, Sep 28, 2010 at 04:20:31PM +0400, Kirill Smelkov wrote:
> On Tue, Sep 28, 2010 at 04:07:22PM +0400, Kirill Smelkov wrote:
> > diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
> > index 68fee12..6aaa10b 100755
> > --- a/t/t4042-diff-textconv-caching.sh
> > +++ b/t/t4042-diff-textconv-caching.sh
> > @@ -5,8 +5,8 @@ test_description='test textconv caching'
> >  
> >  cat >helper <<'EOF'
> >  #!/bin/sh
> > -grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
> > -sed 's/^bin: /converted: /' "$@" >helper.out
> > +grep -q '^bin: ' "$1" || { echo "E: $1 is not \"binary\" file" 1>&2; exit 1; }
> > +sed 's/^bin: /converted: /' "$1" >helper.out
> >  cat helper.out
> >  EOF
> >  chmod +x helper
> 
> Please ignore this - changing so breaks textconv cache tests in this
> file.

Ok, here is what also is needed here

---- 8< ----

    NOTE: in t4042, I had to correct args hack, and just roll second
    textconv helper for cache-invalidates-on-textconv-change test.

    ...

diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 6aaa10b..c362261 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -9,7 +9,12 @@ grep -q '^bin: ' "$1" || { echo "E: $1 is not \"binary\" file" 1>&2; exit 1; }
 sed 's/^bin: /converted: /' "$1" >helper.out
 cat helper.out
 EOF
-chmod +x helper
+cat >helper2 <<'EOF'
+#!/bin/sh
+echo "converted: other"
+./helper "$1"
+EOF
+chmod +x helper helper2
 
 test_expect_success 'setup' '
 	echo "bin: foo content 1" >foo.bin &&
@@ -77,8 +82,7 @@ index 255496b..ad450ff 100644
 +converted: foo content 2
 EOF
 test_expect_success 'changing textconv invalidates cache' '
-	echo "bin: other" >other &&
-	git config diff.magic.textconv "./helper other" &&
+	git config diff.magic.textconv ./helper2 &&
 	git diff HEAD^ HEAD >actual &&
 	test_cmp expect actual
 '

Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Jeff King]

On Tue, Sep 28, 2010 at 04:07:22PM +0400, Kirill Smelkov wrote:

> > Granted, echo is forgiving and will concatenate the arguments it gets with
> > a space in between, but you would either want to either:
> > 
> >  (1) make it more explicit that helper gets only one argument, by saying
> >      "$1" instead of "$@", in all places in the helper script; or
> > 
> >  (2) if you are planning to make 'helper' capable of handling multiple
> >      input files, show the error message for the ones that are not binary
> >      (you would probably need a loop for that).
> > 
> > I think (1) would be sufficient in this case.
> 
> I too think (1) is right. It was just that originally there was $@
> (which I now understand was wrong). So ok to apply the following patch
> on top of this series? (I assume it's ok, and will repost the whole
> thing)

No, "helper" is supposed to be able to take multiple arguments, at least
in t4042. See the "changing textconv invalidates cache" test. The extra
argument comes from the user, not from git.

>  t/t4042-diff-textconv-caching.sh |    4 ++--

Why are we touching t4042 at all in this series? We are not actually
adding any tests to it, AFAICT.

>  t/t8006-blame-textconv.sh        |    4 ++--
>  t/t8007-cat-file-textconv.sh     |    4 ++--

These ones simply copied the helper script, and could be switched to use
$1.

-Peff

Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Kirill Smelkov]

On Tue, Sep 28, 2010 at 09:23:57AM -0400, Jeff King wrote:
> On Tue, Sep 28, 2010 at 04:07:22PM +0400, Kirill Smelkov wrote:
> 
> > > Granted, echo is forgiving and will concatenate the arguments it gets with
> > > a space in between, but you would either want to either:
> > > 
> > >  (1) make it more explicit that helper gets only one argument, by saying
> > >      "$1" instead of "$@", in all places in the helper script; or
> > > 
> > >  (2) if you are planning to make 'helper' capable of handling multiple
> > >      input files, show the error message for the ones that are not binary
> > >      (you would probably need a loop for that).
> > > 
> > > I think (1) would be sufficient in this case.
> > 
> > I too think (1) is right. It was just that originally there was $@
> > (which I now understand was wrong). So ok to apply the following patch
> > on top of this series? (I assume it's ok, and will repost the whole
> > thing)
> 
> No, "helper" is supposed to be able to take multiple arguments, at least
> in t4042. See the "changing textconv invalidates cache" test. The extra
> argument comes from the user, not from git.

Yes, I've reworked that in my v4/patch4 post.

> >  t/t4042-diff-textconv-caching.sh |    4 ++--
> 
> Why are we touching t4042 at all in this series? We are not actually
> adding any tests to it, AFAICT.

Because we want to catch potential wrong textconv invocation on non
"bin: " files there too?


Thanks,
Kirill

Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Jeff King]

On Tue, Sep 28, 2010 at 06:35:40PM +0400, Kirill Smelkov wrote:

> > >  t/t4042-diff-textconv-caching.sh |    4 ++--
> > 
> > Why are we touching t4042 at all in this series? We are not actually
> > adding any tests to it, AFAICT.
> 
> Because we want to catch potential wrong textconv invocation on non
> "bin: " files there too?

But we don't actually add any tests that display the problem there, do
we? And even if we wanted to test the diff implementation, wouldn't
t4030 be the write place to do that? t4042 is specifically about
textconv caching.

-Peff

Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Kirill Smelkov]

On Tue, Sep 28, 2010 at 10:39:28AM -0400, Jeff King wrote:
> On Tue, Sep 28, 2010 at 06:35:40PM +0400, Kirill Smelkov wrote:
> 
> > > >  t/t4042-diff-textconv-caching.sh |    4 ++--
> > > 
> > > Why are we touching t4042 at all in this series? We are not actually
> > > adding any tests to it, AFAICT.
> > 
> > Because we want to catch potential wrong textconv invocation on non
> > "bin: " files there too?
> 
> But we don't actually add any tests that display the problem there, do
> we? And even if we wanted to test the diff implementation, wouldn't
> t4030 be the write place to do that? t4042 is specifically about
> textconv caching.

Yes, I hadn't added new tests there, but at least I've changed
already-in-there helper to bail out if it is called on non "binary"
files, so a small step, but still step forward, no?

And I've changed that helper after doing `git grep` for textconv, and
ideally we shouldn't keep those copy-pasted helpers in several tests (we
have 3 at present - in t4042, t8006 and t8007), but move them into one
common place some day.

So in order to at least keep copies consistent between each other, I've
changed them all in uniform manner.

That was my rationale.


Thanks,
Kirill

Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Junio C Hamano]

Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:

> On Mon, Sep 27, 2010 at 11:23:35AM -0700, Junio C Hamano wrote:
>> Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:
>
>
>> > +sed 's/^bin:/converted:/' "$@" >helper.out
>> 
>> Minor nit: this is inconsistent with the check done with grep above that
>> insists that the colon is followed by a SP.
>
> Yes, you are right. I'll amend it.
>
>> > 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; }
>>  ...
> ...
> I too think (1) is right. It was just that originally there was $@
> (which I now understand was wrong).

Well, the original's use of "$@" is perfectly fine; it would do the right
thing with one argument, of course, but it would do the right thing with
more than one, too.  On the other hand, your use inside "echo" is not.

Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Jeff King]

On Tue, Sep 28, 2010 at 08:36:46AM -0700, Junio C Hamano wrote:

> >> >  cat >helper <<'EOF'
> >> >  #!/bin/sh
> >> > -sed 's/^/converted: /' "$@" >helper.out
> >> > +grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
> >>  ...
> > ...
> > I too think (1) is right. It was just that originally there was $@
> > (which I now understand was wrong).
> 
> Well, the original's use of "$@" is perfectly fine; it would do the right
> thing with one argument, of course, but it would do the right thing with
> more than one, too.  On the other hand, your use inside "echo" is not.

Moreover, the use of "grep" is wrong. Giving it two files, one of which
has "^bin: " and one of which doesn't, will silently accept the latter.
If it's going to handle multiple files, it must be a for-loop (or you
could invert "grep -qv", but I think that might be getting too clever to
remain readable).

Which is why I suggested just dropping the t4042 bit, which is the only
part that actually needs to handle multiple arguments. The other ones
can just switch to using "$1". The helper script is simple enough that
there is no need for them to share the same code.

-Peff

Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Kirill Smelkov]

On Tue, Sep 28, 2010 at 11:58:45AM -0400, Jeff King wrote:
> On Tue, Sep 28, 2010 at 08:36:46AM -0700, Junio C Hamano wrote:
> 
> > >> >  cat >helper <<'EOF'
> > >> >  #!/bin/sh
> > >> > -sed 's/^/converted: /' "$@" >helper.out
> > >> > +grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
> > >>  ...
> > > ...
> > > I too think (1) is right. It was just that originally there was $@
> > > (which I now understand was wrong).
> > 
> > Well, the original's use of "$@" is perfectly fine; it would do the right
> > thing with one argument, of course, but it would do the right thing with
> > more than one, too.  On the other hand, your use inside "echo" is not.
> 
> Moreover, the use of "grep" is wrong. Giving it two files, one of which
> has "^bin: " and one of which doesn't, will silently accept the latter.
> If it's going to handle multiple files, it must be a for-loop (or you
> could invert "grep -qv", but I think that might be getting too clever to
> remain readable).
> 
> Which is why I suggested just dropping the t4042 bit, which is the only
> part that actually needs to handle multiple arguments. The other ones
> can just switch to using "$1". The helper script is simple enough that
> there is no need for them to share the same code.

Let's do whatever is appropriate. I just wanted to be consitent, but I'm
doing mistakes becase I'm overworked and have time pressure on other
tasks. (sorry).

If it would be more convenient to just drop t4042 - let's do it.

I could amend the patch, or is it ok to just kill that portion on top of
what is already in pu?


Sorry for dummy me,
Kirill

Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

[Kirill Smelkov]

On Tue, Sep 28, 2010 at 08:12:10PM +0400, Kirill Smelkov wrote:
> On Tue, Sep 28, 2010 at 11:58:45AM -0400, Jeff King wrote:
> > On Tue, Sep 28, 2010 at 08:36:46AM -0700, Junio C Hamano wrote:
> > 
> > > >> >  cat >helper <<'EOF'
> > > >> >  #!/bin/sh
> > > >> > -sed 's/^/converted: /' "$@" >helper.out
> > > >> > +grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
> > > >>  ...
> > > > ...
> > > > I too think (1) is right. It was just that originally there was $@
> > > > (which I now understand was wrong).
> > > 
> > > Well, the original's use of "$@" is perfectly fine; it would do the right
> > > thing with one argument, of course, but it would do the right thing with
> > > more than one, too.  On the other hand, your use inside "echo" is not.
> > 
> > Moreover, the use of "grep" is wrong. Giving it two files, one of which
> > has "^bin: " and one of which doesn't, will silently accept the latter.
> > If it's going to handle multiple files, it must be a for-loop (or you
> > could invert "grep -qv", but I think that might be getting too clever to
> > remain readable).
> > 
> > Which is why I suggested just dropping the t4042 bit, which is the only
> > part that actually needs to handle multiple arguments. The other ones
> > can just switch to using "$1". The helper script is simple enough that
> > there is no need for them to share the same code.
> 
> Let's do whatever is appropriate. I just wanted to be consitent, but I'm
> doing mistakes becase I'm overworked and have time pressure on other
> tasks. (sorry).
> 
> If it would be more convenient to just drop t4042 - let's do it.

Update:

I've removed t4042 bits from this series, and also squashed $@ -> $1
conversion in the first patch. The whole thing is reposted as v5.

Sorry if maybe there are some bugs still left...
Kirill