git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Kirill Smelkov <kirr@landau.phys.spbu.ru>
Date
Sep 20, 2010, 20:39 UTC
Message-ID
<a92e184e2345c3e8d8908425bb3b03765beed304.1285013803.git.kirr@landau.phys.spbu.ru>
In-Reply-To
<cover.1285013802.git.kirr@landau.phys.spbu.ru>

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>
---
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
Otherwise no changes.
 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
Previous: Kirill Smelkov
Message 9 of 9 in “Fix {blame,cat-file} --textconv for cases with symlinks”
  1. 0/3 Fix {blame,cat-file} --textconv for cases with symlinksKirill Smelkov, Sep 20, 2010
  2. 1/3 tests: Prepare --textconv tests for correctly-failing conversion programKirill Smelkov, Sep 20, 2010
  3. 2/3 blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinksKirill Smelkov, Sep 20, 2010
  4. Matthieu MoySep 20, 2010
  5. Kirill SmelkovSep 21, 2010
  6. Matthieu MoySep 21, 2010
  7. Jeff KingSep 21, 2010
  8. Kirill SmelkovSep 24, 2010
  9. 3/3 blame,cat-file --textconv: Don't assume mode is ``S_IFREF | 0664''Kirill Smelkov, Sep 20, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.