Re: [PATCH v2 2/3] textconv: support for blame

Axel Bonnet [off-list ref] writes:

This cast is not correct, as there is no guarantee that your size_t and
typeof(mmfile_t.size) are compatible.  Depending on the gcc version, you
would get "dereferencing type-punned pointer will break strict-aliasing
rules" error.

The same issue exists in Clément's patch to builtin/cat-file.c.

On Mon, 14 Jun 2010 13:40:21 -0700, Junio C Hamano [off-list ref]
wrote:

Axel Bonnet [off-list ref] writes:

quoted

@@ -86,16 +87,49 @@ struct origin {

...
+static void fill_origin_blob(struct diff_options *opt,
+			     struct origin *o, mmfile_t *file)
 {
 	if (!o->file.ptr) {
 		enum object_type type;
 		num_read_blob++;
-		file->ptr = read_sha1_file(o->blob_sha1, &type,
-					   (unsigned long *)(&(file->size)));
+
+		if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
+		    textconv_object(o->path, o->blob_sha1, &file->ptr,
+				    (size_t *) &file->size))

This cast is not correct, as there is no guarantee that your size_t and
typeof(mmfile_t.size) are compatible.  Depending on the gcc version, you
would get "dereferencing type-punned pointer will break strict-aliasing
rules" error.

The same issue exists in Clément's patch to builtin/cat-file.c.

We did this way because we found a similar cast in prep_temp_blob(),
diff.c:

	if (convert_to_working_tree(path,
			(const char *)blob, (size_t)size, &buf)) {

where size is an unsigned long.
Is it the same issue ? Or is it different because it's not a pointer cast?

Otherwise, we thought of reversing the conversion. That is to say, instead
of casting "long *" in "size_t *" when calling textconv_object(), is it
better to cast size_t in "unsigned long" in textconv_object():

	*buf_size = (unsigned long) fill_textconv(textconv, df, buf); ?

On Tue, Jun 15, 2010 at 11:29:57AM +0200, Clément Poulain wrote:

quoted

The same issue exists in Clément's patch to builtin/cat-file.c.

We did this way because we found a similar cast in prep_temp_blob(),
diff.c:

	if (convert_to_working_tree(path,
			(const char *)blob, (size_t)size, &buf)) {

where size is an unsigned long.
Is it the same issue ? Or is it different because it's not a pointer cast?

Right. The compiler will handle conversion between integer types during
assignment itself, converting representations as necessary (in fact,
that cast looks useless to me, as implicit conversions are allowed in
C). The only problem is dereferencing a pointer to X as something other
than X.

Otherwise, we thought of reversing the conversion. That is to say, instead
of casting "long *" in "size_t *" when calling textconv_object(), is it
better to cast size_t in "unsigned long" in textconv_object():

	*buf_size = (unsigned long) fill_textconv(textconv, df, buf); ?

You shouldn't even have to cast there, for the same reason as above.
That is why I wrote fill_textconv to return the size parameter, rather
than writing to a passed-in pointer. It avoids the annoying
size_t / unsigned long casting caused by different usage (in an ideal
world, all of our sizes would be the same type, but the strbuf and diff
code obviously differ).

-Peff

On Tue, 15 Jun 2010 05:54:53 -0400, Jeff King [off-list ref] wrote:

On Tue, Jun 15, 2010 at 11:29:57AM +0200, Clément Poulain wrote:

quoted

quoted

The same issue exists in Clément's patch to builtin/cat-file.c.

We did this way because we found a similar cast in prep_temp_blob(),
diff.c:

	if (convert_to_working_tree(path,
			(const char *)blob, (size_t)size, &buf)) {

where size is an unsigned long.
Is it the same issue ? Or is it different because it's not a pointer
cast?

Right. The compiler will handle conversion between integer types during
assignment itself, converting representations as necessary (in fact,
that cast looks useless to me, as implicit conversions are allowed in
C). The only problem is dereferencing a pointer to X as something other
than X.

quoted

Otherwise, we thought of reversing the conversion. That is to say,
instead
of casting "long *" in "size_t *" when calling textconv_object(), is it
better to cast size_t in "unsigned long" in textconv_object():

	*buf_size = (unsigned long) fill_textconv(textconv, df, buf); ?

You shouldn't even have to cast there, for the same reason as above.
That is why I wrote fill_textconv to return the size parameter, rather
than writing to a passed-in pointer. It avoids the annoying
size_t / unsigned long casting caused by different usage (in an ideal
world, all of our sizes would be the same type, but the strbuf and diff
code obviously differ).

Thanks for your answer.

We have changed the declaration of textconv_object() to:

static int textconv_object(const char *path,
                           const unsigned char *sha1,
                           char **buf,
                           unsigned long *buf_size)

And now we can do:
*buf_size = fill_textconv(textconv, df, buf);
without any cast.

But we have to do:
textconv_object(read_from, null_sha1, &buf.buf, (unsigned long *)
&buf.len))
where buf.len is size_t.

Is that ok?
Our gcc doesn't report any strict-aliasing problem, so we don't know if it
is better than the initial version or not...

This patches enables to perform textconv with blame if a textconv driver is
available for the file.

The main task is performed by the textconv_object function which prepares
diff_filespec and if possible converts the file using diff textconv API.
Only regular files are converted, so the mode of diff_filespec is faked.

Textconv conversion is enabled by default (equivalent to the option
--textconv), since blaming binary files is useless in most cases.
The option --no-textconv is used to disable textconv conversion.

The declarations of several functions are modified to give access to a
diff_options, in order to know whether the textconv option is activated or not.

Signed-off-by: Axel Bonnet <redacted>
Signed-off-by: Clément Poulain <redacted>
Signed-off-by: Diane Gasselin <redacted>
---

The problem with cast between size_t and unsigned long is fixed.
The style problem with the case is fixed.

 builtin/blame.c |   86 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 72 insertions(+), 14 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 8506286..5b61067 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c

@@ -20,6 +20,7 @@
 #include "mailmap.h"
 #include "parse-options.h"
 #include "utf8.h"
+#include "userdiff.h"
 
 static char blame_usage[] = "git blame [options] [rev-opts] [rev] [--] file";
 

@@ -86,16 +87,49 @@ struct origin {
 };
 
 /*
+ * Prepare diff_filespec and convert it using diff textconv API
+ * if the textconv driver exists.
+ * Return 1 if the conversion succeeds, 0 otherwise.
+ */
+static int textconv_object(const char *path,
+			   const unsigned char *sha1,
+			   char **buf,
+			   unsigned long *buf_size)
+{
+	struct diff_filespec *df;
+	struct userdiff_driver *textconv;
+
+	df = alloc_filespec(path);
+	fill_filespec(df, sha1, S_IFREG | 0664);
+	textconv = get_textconv(df);
+	if (!textconv) {
+		free_filespec(df);
+		return 0;
+	}
+
+	*buf_size = fill_textconv(textconv, df, buf);
+	free_filespec(df);
+	return 1;
+}
+
+/*
  * Given an origin, prepare mmfile_t structure to be used by the
  * diff machinery
  */
-static void fill_origin_blob(struct origin *o, mmfile_t *file)
+static void fill_origin_blob(struct diff_options *opt,
+			     struct origin *o, mmfile_t *file)
 {
 	if (!o->file.ptr) {
 		enum object_type type;
 		num_read_blob++;
-		file->ptr = read_sha1_file(o->blob_sha1, &type,
-					   (unsigned long *)(&(file->size)));
+
+		if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
+		    textconv_object(o->path, o->blob_sha1, &file->ptr,
+				    (unsigned long *)(&file->size)))
+			;
+		else
+			file->ptr = read_sha1_file(o->blob_sha1, &type,
+						   (unsigned long *)(&(file->size)));
 		if (!file->ptr)
 			die("Cannot read blob %s for path %s",
 			    sha1_to_hex(o->blob_sha1),

@@ -282,7 +316,6 @@ static struct origin *get_origin(struct scoreboard *sb,
 static int fill_blob_sha1(struct origin *origin)
 {
 	unsigned mode;
-
 	if (!is_null_sha1(origin->blob_sha1))
 		return 0;
 	if (get_tree_entry(origin->commit->object.sha1,

@@ -741,8 +774,8 @@ static int pass_blame_to_parent(struct scoreboard *sb,
 	if (last_in_target < 0)
 		return 1; /* nothing remains for this target */
 
-	fill_origin_blob(parent, &file_p);
-	fill_origin_blob(target, &file_o);
+	fill_origin_blob(&sb->revs->diffopt, parent, &file_p);
+	fill_origin_blob(&sb->revs->diffopt, target, &file_o);
 	num_get_patch++;
 
 	memset(&xpp, 0, sizeof(xpp));

@@ -922,7 +955,7 @@ static int find_move_in_parent(struct scoreboard *sb,
 	if (last_in_target < 0)
 		return 1; /* nothing remains for this target */
 
-	fill_origin_blob(parent, &file_p);
+	fill_origin_blob(&sb->revs->diffopt, parent, &file_p);
 	if (!file_p.ptr)
 		return 0;
 

@@ -1063,7 +1096,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);
-			fill_origin_blob(norigin, &file_p);
+			fill_origin_blob(&sb->revs->diffopt, norigin, &file_p);
 			if (!file_p.ptr)
 				continue;
 

@@ -1983,6 +2016,16 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 		blame_date_mode = parse_date_format(value);
 		return 0;
 	}
+
+	switch (userdiff_config(var, value)) {
+	case 0:
+		break;
+	case -1:
+		return -1;
+	default:
+		return 0;
+	}
+
 	return git_default_config(var, value, cb);
 }
 

@@ -1990,7 +2033,9 @@ static int git_blame_config(const char *var, const char *value, void *cb)
  * Prepare a dummy commit that represents the work tree (or staged) item.
  * Note that annotating work tree item never works in the reverse.
  */
-static struct commit *fake_working_tree_commit(const char *path, const char *contents_from)
+static struct commit *fake_working_tree_commit(struct diff_options *opt,
+					       const char *path,
+					       const char *contents_from)
 {
 	struct commit *commit;
 	struct origin *origin;

@@ -2018,6 +2063,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 	if (!contents_from || strcmp("-", contents_from)) {
 		struct stat st;
 		const char *read_from;
+		unsigned long buf_len;
 
 		if (contents_from) {
 			if (stat(contents_from, &st) < 0)

@@ -2030,10 +2076,14 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 			read_from = path;
 		}
 		mode = canon_mode(st.st_mode);
+
 		switch (st.st_mode & S_IFMT) {
 		case S_IFREG:
-			if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)
-				die_errno("cannot open or read '%s'", read_from);
+			if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
+			    textconv_object(read_from, 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);
 			break;
 		case S_IFLNK:
 			if (strbuf_readlink(&buf, read_from, st.st_size) < 0)

@@ -2248,6 +2298,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	git_config(git_blame_config, NULL);
 	init_revisions(&revs, NULL);
 	revs.date_mode = blame_date_mode;
+	DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
 
 	save_commit_buffer = 0;
 	dashdash_pos = 0;

@@ -2384,7 +2435,8 @@ parse_done:
 		 * or "--contents".
 		 */
 		setup_work_tree();
-		sb.final = fake_working_tree_commit(path, contents_from);
+		sb.final = fake_working_tree_commit(&sb.revs->diffopt,
+						    path, contents_from);
 		add_pending_object(&revs, &(sb.final->object), ":");
 	}
 	else if (contents_from)

@@ -2411,8 +2463,14 @@ parse_done:
 		if (fill_blob_sha1(o))
 			die("no such path %s in %s", path, final_commit_name);
 
-		sb.final_buf = read_sha1_file(o->blob_sha1, &type,
-					      &sb.final_buf_size);
+		if (DIFF_OPT_TST(&sb.revs->diffopt, ALLOW_TEXTCONV) &&
+		    textconv_object(path, o->blob_sha1, (char **) &sb.final_buf,
+				    &sb.final_buf_size))
+			;
+		else
+			sb.final_buf = read_sha1_file(o->blob_sha1, &type,
+						      &sb.final_buf_size);
+
 		if (!sb.final_buf)
 			die("Cannot read blob %s for path %s",
 			    sha1_to_hex(o->blob_sha1),

-- 
1.6.6.7.ga5fe3