[RFC/PATCH 0/4] textconv support for blame

[RFC/PATCH 0/4] textconv support for blame

[Axel Bonnet]

This is a patch series to implement textconv support for git blame. As textconv support has already been added to git diff, so we use textconv methods of diff.
Here are the different changes:
- make the diff textconv API public
- add diff_options to blame (--textconv and --no-textconv)
- perform textconv when we meet an object with textconv driver
- t8006-blame-textconv.sh tests conversion works

Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>

Axel Bonnet (4):
  textconv: make the API public
  textconv: make diff_options accessible from blame
  textconv: support for blame
  t/t8006: test textconv support for blame

 builtin/blame.c           |   92 ++++++++++++++++++++++++++++++++++--------
 diff.c                    |   12 ++----
 diff.h                    |    8 ++++
 t/t8006-blame-textconv.sh |   98 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 184 insertions(+), 26 deletions(-)
 create mode 100755 t/t8006-blame-textconv.sh

[RFC/PATCH 1/4] textconv: make the API public

[Axel Bonnet]

The textconv functionality allows one to convert a file into text before
running diff. But this functionality can be useful to other features
such as blame.

Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 diff.c |   12 ++++--------
 diff.h |    8 ++++++++
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index 494f560..b4a830f 100644
--- a/diff.c
+++ b/diff.c
@@ -43,10 +43,6 @@ static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NORMAL,	/* FUNCINFO */
 };
 
-static void diff_filespec_load_driver(struct diff_filespec *one);
-static size_t fill_textconv(struct userdiff_driver *driver,
-			    struct diff_filespec *df, char **outbuf);
-
 static int parse_diff_color_slot(const char *var, int ofs)
 {
 	if (!strcasecmp(var+ofs, "plain"))
@@ -1629,7 +1625,7 @@ void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const
 		options->b_prefix = b;
 }
 
-static struct userdiff_driver *get_textconv(struct diff_filespec *one)
+struct userdiff_driver *get_textconv(struct diff_filespec *one)
 {
 	if (!DIFF_FILE_VALID(one))
 		return NULL;
@@ -4002,9 +3998,9 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,
 	return strbuf_detach(&buf, outsize);
 }
 
-static size_t fill_textconv(struct userdiff_driver *driver,
-			    struct diff_filespec *df,
-			    char **outbuf)
+size_t fill_textconv(struct userdiff_driver *driver,
+		     struct diff_filespec *df,
+		     char **outbuf)
 {
 	size_t size;
 
diff --git a/diff.h b/diff.h
index 9ace08c..2a0e36d 100644
--- a/diff.h
+++ b/diff.h
@@ -9,6 +9,8 @@
 struct rev_info;
 struct diff_options;
 struct diff_queue_struct;
+struct diff_filespec;
+struct userdiff_driver;
 
 typedef void (*change_fn_t)(struct diff_options *options,
 		 unsigned old_mode, unsigned new_mode,
@@ -287,4 +289,10 @@ extern void diff_no_index(struct rev_info *, int, const char **, int, const char
 
 extern int index_differs_from(const char *def, int diff_flags);
 
+extern size_t fill_textconv(struct userdiff_driver *driver,
+			    struct diff_filespec *df,
+			    char **outbuf);
+
+extern struct userdiff_driver *get_textconv(struct diff_filespec *one);
+
 #endif /* DIFF_H */
-- 
1.6.6.7.ga5fe3

[RFC/PATCH 2/4] textconv: make diff_options accessible from blame

[Axel Bonnet]

Diff_options specify whether conversion is activated or not. Blame needs
to access these options in order to concert files with external drivers

Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 builtin/blame.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index fc15863..63b497c 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -89,7 +89,8 @@ struct origin {
  * 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;
@@ -741,8 +742,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 +923,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 +1064,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;
 
@@ -1990,7 +1991,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;
@@ -2384,7 +2387,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)
-- 
1.6.6.7.ga5fe3

[RFC/PATCH 3/4] textconv: support for blame

[Axel Bonnet]

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.

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 declaration of fill_blob_sha1 declaration is modified to get back
the mode the function was getting.

Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 builtin/blame.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 63 insertions(+), 11 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 63b497c..4679fd9 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,17 +87,57 @@ 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,
+			   unsigned short mode,
+			   struct strbuf *buf)
+{
+	struct diff_filespec *df;
+
+	df = alloc_filespec(path);
+	fill_filespec(df, sha1, mode);
+	get_textconv(df);
+
+	if (!df->driver|| !df->driver->textconv) {
+		free_filespec(df);
+		return 0;
+	}
+
+	buf->len = fill_textconv(df->driver, df, &buf->buf);
+	buf->alloc = 1;
+	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 diff_options opt,
 			     struct origin *o, mmfile_t *file)
 {
+	unsigned mode;
+
 	if (!o->file.ptr) {
+		struct strbuf buf = STRBUF_INIT;
 		enum object_type type;
 		num_read_blob++;
-		file->ptr = read_sha1_file(o->blob_sha1, &type,
-					   (unsigned long *)(&(file->size)));
+
+		get_tree_entry(o->commit->object.sha1,
+			       o->path,
+			       o->blob_sha1, &mode);
+		if (DIFF_OPT_TST(&opt, ALLOW_TEXTCONV) &&
+		    textconv_object(o->path, o->blob_sha1, mode, &buf))
+			file->ptr = strbuf_detach(&buf, (size_t *) &file->size);
+		else
+			file->ptr = read_sha1_file(o->blob_sha1, &type,
+						   (unsigned long *)(&(file->size)));
+		strbuf_release(&buf);
+
 		if (!file->ptr)
 			die("Cannot read blob %s for path %s",
 			    sha1_to_hex(o->blob_sha1),
@@ -280,15 +321,13 @@ static struct origin *get_origin(struct scoreboard *sb,
  * the parent to detect the case where a child's blob is identical to
  * that of its parent's.
  */
-static int fill_blob_sha1(struct origin *origin)
+static int fill_blob_sha1(struct origin *origin, unsigned *mode)
 {
-	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, mode))
 		goto error_out;
 	if (sha1_object_info(origin->blob_sha1, NULL) != OBJ_BLOB)
 		goto error_out;
@@ -2033,10 +2072,13 @@ static struct commit *fake_working_tree_commit(struct diff_options opt,
 			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, mode, &buf))
+				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)
@@ -2249,8 +2291,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	int cmd_is_annotate = !strcmp(argv[0], "annotate");
 
 	git_config(git_blame_config, NULL);
+	git_config(git_diff_ui_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;
@@ -2411,12 +2455,20 @@ parse_done:
 		sb.final_buf_size = o->file.size;
 	}
 	else {
+		struct strbuf buf = STRBUF_INIT;
+		unsigned mode;
 		o = get_origin(&sb, sb.final, path);
-		if (fill_blob_sha1(o))
+		if (fill_blob_sha1(o, &mode))
 			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, mode, &buf))
+			sb.final_buf= strbuf_detach(&buf, (size_t *) &sb.final_buf_size);
+		else
+			sb.final_buf = read_sha1_file(o->blob_sha1, &type,
+						      &sb.final_buf_size);
+
+		strbuf_release(&buf);
 		if (!sb.final_buf)
 			die("Cannot read blob %s for path %s",
 			    sha1_to_hex(o->blob_sha1),
-- 
1.6.6.7.ga5fe3

[RFC/PATCH 4/4] t/t8006: test textconv support for blame

[Axel Bonnet]

Test the correct functionning of textconv with blame <file|link> and
blame HEAD^ <file>.
Test the case when no driver is specified.

Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 t/t8006-blame-textconv.sh |   98 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 98 insertions(+), 0 deletions(-)
 create mode 100755 t/t8006-blame-textconv.sh

diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
new file mode 100755
index 0000000..d3780ed
--- /dev/null
+++ b/t/t8006-blame-textconv.sh
@@ -0,0 +1,98 @@
+#!/bin/sh
+
+test_description='git blame textconv support'
+. ./test-lib.sh
+
+find_blame() {
+	sed -e 's/^.*(/(/g'
+}
+
+cat >helper <<'EOF'
+#!/bin/sh
+sed 's/^/converted: /' "$@" >helper.out
+cat helper.out
+EOF
+chmod +x helper
+
+test_expect_success 'setup ' '
+	echo test 1 >one.bin &&
+	echo test number 2 >two.bin &&
+	ln one.bin link.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 &&
+	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
+EOF
+
+test_expect_success 'no filter specified' '
+	git blame one.bin | grep Number2 >blame
+	find_blame <blame >result
+	test_cmp expected result
+'
+
+test_expect_success 'setup textconv filters' '
+	echo "*.bin diff=test" >.gitattributes &&
+	git config diff.test.textconv ./helper &&
+	git config diff.test.cachetextconv false
+'
+
+test_expect_success 'blame with --no-textconv' '
+	git blame --no-textconv one.bin | grep Number2 >blame
+	find_blame <blame >result
+	test_cmp expected result
+'
+
+cat >expected <<EOF
+(Number2 2010-01-01 20:00:00 +0000 1) converted: test 1 version 2
+EOF
+
+test_expect_success 'basic blame textconv on last commit' '
+	git blame one.bin | grep Number2 >blame
+	find_blame <blame >result
+	test_cmp expected 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
+EOF
+
+test_expect_success 'blame textconv going through revisions' '
+	git blame two.bin >blame
+	find_blame <blame >result
+	test_cmp expected result
+'
+
+test_expect_success 'make a new commit' '
+	echo "test number 2 version 3" >>two.bin &&
+	GIT_AUTHOR_NAME=Number3 git commit -a -m Third --date="2010-01-01 22:00:00"
+'
+
+test_expect_success 'textconv with blame from previous revision' '
+	git blame HEAD^ two.bin >blame
+	find_blame <blame >result
+	test_cmp expected result
+'
+
+cat >expected <<EOF
+(Number2 2010-01-01 20:00:00 +0000 1) converted: test 1 version 2
+(Number3 2010-01-01 20:00:00 +0000 2) converted: test link
+EOF
+
+test_expect_success 'setup with links' '
+	echo test link >>link.bin &&
+	GIT_AUTHOR_NAME=Number3 git commit -a -m Third --date="2010-01-01 20:00:00"
+'
+
+test_expect_success 'blame textconv on links' '
+	git blame link.bin >blame
+	find_blame <blame >result
+	test_cmp expected result
+'
+
+test_done
-- 
1.6.6.7.ga5fe3

Re: [RFC/PATCH 4/4] t/t8006: test textconv support for blame

[Johannes Sixt]

On Donnerstag, 3. Juni 2010, Axel Bonnet wrote:
> +cat >helper <<'EOF'
> +#!/bin/sh
> +sed 's/^/converted: /' "$@" >helper.out
> +cat helper.out
> +EOF

You don't need an intermediate file here, do you? Without it, this textconv 
script is a one-liner; now, isn't it possible to configure a shell command as 
textconv command, i.e., without this helper script?

> +test_expect_success 'setup ' '
> +	echo test 1 >one.bin &&
> +	echo test number 2 >two.bin &&
> +	ln one.bin link.bin &&

Do you need a hard link? Can't you just copy the file at the right time?

> +test_expect_success 'blame with --no-textconv' '
> +	git blame --no-textconv one.bin | grep Number2 >blame
> +	find_blame <blame >result

It would be nice if you could write this like, e.g.,

	git blame --no-textconv one.bin >blame &&
	find_blame Number2 <blame >result

so that the git command is not part of a pipeline (otherwise, unexpected exit 
codes would go undetected).

Please look for missing '&&', you forgot it in many places.

-- Hannes

Re: [RFC/PATCH 4/4] t/t8006: test textconv support for blame

[Diane Gasselin]

Le 3 juin 2010 17:44, Johannes Sixt <j6t@kdbg.org> a écrit :
> On Donnerstag, 3. Juni 2010, Axel Bonnet wrote:
>> +cat >helper <<'EOF'
>> +#!/bin/sh
>> +sed 's/^/converted: /' "$@" >helper.out
>> +cat helper.out
>> +EOF
>
> You don't need an intermediate file here, do you? Without it, this textconv
> script is a one-liner; now, isn't it possible to configure a shell command as
> textconv command, i.e., without this helper script?
>

Ok. We don't use the intermediate file anymore. Actually, we used what
has been done for textconv test for diff.
I didn't find a way to directly specify the sed command as textconv
command without using ./helper though.

cat >helper <<'EOF'
#!/bin/sh
sed 's/^/converted: /' "$@"
EOF
chmod +x helper

>> +test_expect_success 'setup ' '
>> +     echo test 1 >one.bin &&
>> +     echo test number 2 >two.bin &&
>> +     ln one.bin link.bin &&
>
> Do you need a hard link? Can't you just copy the file at the right time?
>

At first, we wanted to test how links handle textconv but it behaves
as regular file so the test is not really relevant. It will be
deleted.

>> +test_expect_success 'blame with --no-textconv' '
>> +     git blame --no-textconv one.bin | grep Number2 >blame
>> +     find_blame <blame >result
>
> It would be nice if you could write this like, e.g.,
>
>        git blame --no-textconv one.bin >blame &&
>        find_blame Number2 <blame >result
>
> so that the git command is not part of a pipeline (otherwise, unexpected exit
> codes would go undetected).
>
> Please look for missing '&&', you forgot it in many places.
>

We did the appropriate changes.
Thanks a lot for your comments!

Diane

> -- Hannes
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC/PATCH 4/4] t/t8006: test textconv support for blame

[Matthieu Moy]

Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:

>>> +test_expect_success 'setup ' '
>>> +     echo test 1 >one.bin &&
>>> +     echo test number 2 >two.bin &&
>>> +     ln one.bin link.bin &&
>>
>> Do you need a hard link? Can't you just copy the file at the right time?
>>
>
> At first, we wanted to test how links handle textconv but it behaves
> as regular file so the test is not really relevant. It will be
> deleted.

You do want to test what happens for _symbolic_ links (stored as blob
whose content is the target of the link IIRC). You probably don't want
to run the textconv filter on link targets. I guess you meant "ln -s"
here.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH 4/4] t/t8006: test textconv support for blame

[Matthieu Moy]

Axel Bonnet <axel.bonnet@ensimag.imag.fr> writes:

> +test_expect_success 'blame with --no-textconv' '
> +	git blame --no-textconv one.bin | grep Number2 >blame
> +	find_blame <blame >result
> +	test_cmp expected result
> +'

Don't you want to add && at each end of line, to make sure you catch
potential failures of git blame on the first line (e.g. git blame
producing the correct output and then segfaulting for example)?

Actually, to really catch such failures, you should not run git on the
left hand side of a | (otherwise, you look for failures of the right
hand side), and do this instead:

test_expect_success 'no filter specified' '
	git blame one.bin >to-grep &&
	grep Number2 to-grep >blame &&
	find_blame <blame >result &&
	test_cmp expected result
'

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH 3/4] textconv: support for blame

[Junio C Hamano]

Axel Bonnet <axel.bonnet@ensimag.imag.fr> writes:

> @@ -2033,10 +2072,13 @@ static struct commit *fake_working_tree_commit(struct diff_options opt,
>  			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, mode, &buf))
> +				if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)
> +					die_errno("cannot open or read '%s'", read_from);

This is just a style thing but it would probably be easier to read if you
structured it like:

	if (! we are allowed to use textconv ||
	    do textconv and we did get the converted data in the buffer)
		; /* happy */
	else if (! successfully read the blob into buffer)
		die;

By the way, can't textconv_object() ever fail?  I see the function has its
own die() but it looks a bit funny to see one branch of an "if" statement
calls a function that lets the caller decide to die while the function
called by the other branch unconditionally dies on failure at the API
design level.

An alternative would be to encapsulate the whole of the above logic in one
helper function perhaps.

> @@ -2249,8 +2291,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  	int cmd_is_annotate = !strcmp(argv[0], "annotate");
>  
>  	git_config(git_blame_config, NULL);
> +	git_config(git_diff_ui_config, NULL);

What configuration are we pulling into the system with this call?  Would
they ever affect the internal diff machinery in a negative way?  I am
especially wondering about "diff.renames" here.

>  	init_revisions(&revs, NULL);
>  	revs.date_mode = blame_date_mode;
> +	DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);

As an RFC patch, I would have preferred if we didn't have this line to
force --textconv on by default, but instead you merely allowed the
mechanism to be used by giving the option explicitly from the command
line.

Other than these points, the series looked quite sane to me.

Re: [RFC/PATCH 3/4] textconv: support for blame

[Diane Gasselin]

Le 4 juin 2010 08:00, Junio C Hamano <gitster@pobox.com> a écrit :
> Axel Bonnet <axel.bonnet@ensimag.imag.fr> writes:
>
>> @@ -2033,10 +2072,13 @@ static struct commit *fake_working_tree_commit(struct diff_options opt,
>>                       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, mode, &buf))
>> +                             if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)
>> +                                     die_errno("cannot open or read '%s'", read_from);
>
> This is just a style thing but it would probably be easier to read if you
> structured it like:
>
>        if (! we are allowed to use textconv ||
>            do textconv and we did get the converted data in the buffer)
>                ; /* happy */
>        else if (! successfully read the blob into buffer)
>                die;
>

We changed the structure, we now have something like:

case S_IFREG:
	if (DIFF_OPT_TST(&opt, ALLOW_TEXTCONV) &&
	    textconv_object(read_from, null_sha1, mode, &buf))
		;
	else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)
		die_errno("cannot open or read '%s'", read_from);
break;

> By the way, can't textconv_object() ever fail?  I see the function has its
> own die()

I don't really see which die you are talking about, there is no direct
die in textconv_object().

> but it looks a bit funny to see one branch of an "if" statement
> calls a function that lets the caller decide to die while the function
> called by the other branch unconditionally dies on failure at the API
> design level.
>

The function fill_textconv() called by textconv_object() can fail if
there is a problem during the textconv conversion. But
textconv_object() tests before if we have to and if we can peform a
conversion.
We did not add some die but only let the existing die.

> An alternative would be to encapsulate the whole of the above logic in one
> helper function perhaps.
>
>> @@ -2249,8 +2291,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>>       int cmd_is_annotate = !strcmp(argv[0], "annotate");
>>
>>       git_config(git_blame_config, NULL);
>> +     git_config(git_diff_ui_config, NULL);
>
> What configuration are we pulling into the system with this call?  Would
> they ever affect the internal diff machinery in a negative way?  I am
> especially wondering about "diff.renames" here.
>

Actually, we call git_diff_ui_config in order to get the drivers. But
we could call a more specific configuration.
Here are others solutions:
1) We could add:

switch (userdiff_config(var, value)) {
	case 0: break;
	case -1: return -1;
	default: return 0;
}
to git_blame_config

2) We could call
git_config(git_diff_basic_config, NULL);
or git_config((config_fn_t)userdiff_config, NULL);
instead of git_config(git_diff_ui_config, NULL);

>>       init_revisions(&revs, NULL);
>>       revs.date_mode = blame_date_mode;
>> +     DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
>
> As an RFC patch, I would have preferred if we didn't have this line to
> force --textconv on by default, but instead you merely allowed the
> mechanism to be used by giving the option explicitly from the command
> line.
>
> Other than these points, the series looked quite sane to me.
>

Thanks a lot for your time and comments.

Re: [RFC/PATCH 3/4] textconv: support for blame

[Jeff King]

On Thu, Jun 03, 2010 at 12:47:17PM +0200, Axel Bonnet wrote:

>  /*
> + * 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,
> +			   unsigned short mode,
> +			   struct strbuf *buf)
> +{
> +	struct diff_filespec *df;
> +
> +	df = alloc_filespec(path);
> +	fill_filespec(df, sha1, mode);
> +	get_textconv(df);
> +
> +	if (!df->driver|| !df->driver->textconv) {
> +		free_filespec(df);
> +		return 0;
> +	}

df->driver is always non-NULL these days, isn't it? Also, why not just
use the return value of get_textconv, which does handles this
conditional for you already (and avoids peeking directly at df->driver,
which was what get_textconv was meant to abstract). I.e.:

  struct userdiff_driver *textconv;
  ...
  textconv = get_textconv(df);
  if (!textconv)
     ... free and return ...

> +	buf->len = fill_textconv(df->driver, df, &buf->buf);
> +	buf->alloc = 1;
> +	free_filespec(df);
> +	return 1;

Shoving the allocated buffer into a strbuf really feels like an abuse of
strbuf. I don't think there is any bug here (the original buffer
actually comes from a strbuf, and by setting alloc to 1, you indicate
that any further appending would need to realloc), but it just seems
like violating the boundary of the strbuf API.

And it isn't really necessary because:

> +		if (DIFF_OPT_TST(&opt, ALLOW_TEXTCONV) &&
> +		    textconv_object(o->path, o->blob_sha1, mode, &buf))
> +			file->ptr = strbuf_detach(&buf, (size_t *) &file->size);

You just end up pulling it out into an mmfile_t, anyway. So why involve
strbuf at all?

>  static void fill_origin_blob(struct diff_options opt,
>  			     struct origin *o, mmfile_t *file)
>  {
> +	unsigned mode;
> +
>  	if (!o->file.ptr) {
> +		struct strbuf buf = STRBUF_INIT;
>  		enum object_type type;
>  		num_read_blob++;
> -		file->ptr = read_sha1_file(o->blob_sha1, &type,
> -					   (unsigned long *)(&(file->size)));
> +
> +		get_tree_entry(o->commit->object.sha1,
> +			       o->path,
> +			       o->blob_sha1, &mode);
> +		if (DIFF_OPT_TST(&opt, ALLOW_TEXTCONV) &&
> +		    textconv_object(o->path, o->blob_sha1, mode, &buf))
> +			file->ptr = strbuf_detach(&buf, (size_t *) &file->size);
> +		else
> +			file->ptr = read_sha1_file(o->blob_sha1, &type,
> +						   (unsigned long *)(&(file->size)));
> +		strbuf_release(&buf);

I don't understand why there's a get_tree_entry call here. Don't we
already have the path and blob_sha1 fields? Is the mode actually
relevant (i.e., can we just fake it for the purposes of the
diff_filespec we will create)?

Even if we do need the get_tree_entry call, shouldn't it happen only if
we are allowing textconv, so non-textconv users don't pay for the call?

> @@ -2249,8 +2291,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  	int cmd_is_annotate = !strcmp(argv[0], "annotate");
>  
>  	git_config(git_blame_config, NULL);
> +	git_config(git_diff_ui_config, NULL);

Like Junio, I am worried about the unintended consequences of
git_diff_ui_config. The userdiff drivers are parsed as part of
git_diff_basic_config, and you should use that.

Also, you are better to just add the call to git_blame_config (either
git_diff_basic_config, or looking directly to userdiff_config) instead
of calling git_config again, which will avoid parsing the config files
twice.

-Peff

Re: [RFC/PATCH 2/4] textconv: make diff_options accessible from blame

[Junio C Hamano]

Axel Bonnet <axel.bonnet@ensimag.imag.fr> writes:

> Diff_options specify whether conversion is activated or not. Blame needs
> to access these options in order to concert files with external drivers
>
> Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
> Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
> Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>

The name of Clément is spelled correctly on the mail header while S-o-b
line is corrupt.  Perhaps you have recorded your commits in UTF-8 but
allowed your MUA to send in 8859-1?  This comment applies to all the
patches in the series.

> diff --git a/builtin/blame.c b/builtin/blame.c
> index fc15863..63b497c 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -89,7 +89,8 @@ struct origin {
>   * 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;

Two points.

 * Generally we do not want to pass structures by value.  It is especially
   true when the structure is bigger than one word, and accesses to the
   variable in the callee is read-only.

 * The callee does not seem to use the new parameter yet.  You might want
   to defer this change until fill-origin-blob actually starts using it.

Re: [RFC/PATCH 2/4] textconv: make diff_options accessible from blame

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Axel Bonnet <axel.bonnet@ensimag.imag.fr> writes:
>
>> Diff_options specify whether conversion is activated or not. Blame needs
>> to access these options in order to concert files with external drivers
>>
>> Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
>> Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
>> Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
>
> The name of Clément is spelled correctly on the mail header while S-o-b
> line is corrupt.

Actually, it's valid UTF-8, but there's no header specifying the
encoding in the email, therefore, the reader's default applies. My
mailer displays it correctly, but yours doesn't.

> Perhaps you have recorded your commits in UTF-8 but allowed your MUA
> to send in 8859-1?

The MUA seems to be git-send-email. According to the source (I didn't
find it in the doc), git-send-email looks at the patch's headers to
specify the encoding.

On my machine, the patch applies well, and if I re-export it using
format-patch, I do get the headers:

Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If I send myself the patch with git-send-email, I also get the headers
in the email (I tried from ensibm, which is the machine which sent the
patch serie). So, it doesn't look like a bug in git, but rather a
miss-use.

Axel, can you give us the exact command(s) you used to send the patch?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH 2/4] textconv: make diff_options accessible from blame

[bonneta]

On Fri, 04 Jun 2010 09:59:47 +0200, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Axel Bonnet <axel.bonnet@ensimag.imag.fr> writes:
>>
>>> Diff_options specify whether conversion is activated or not. Blame
needs
>>> to access these options in order to concert files with external
drivers
>>>
>>> Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
>>> Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
>>> Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
>>
>> The name of Clément is spelled correctly on the mail header while S-o-b
>> line is corrupt.
> 
> Actually, it's valid UTF-8, but there's no header specifying the
> encoding in the email, therefore, the reader's default applies. My
> mailer displays it correctly, but yours doesn't.
> 
>> Perhaps you have recorded your commits in UTF-8 but allowed your MUA
>> to send in 8859-1?
> 
> The MUA seems to be git-send-email. According to the source (I didn't
> find it in the doc), git-send-email looks at the patch's headers to
> specify the encoding.
> 
> On my machine, the patch applies well, and if I re-export it using
> format-patch, I do get the headers:
> 
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> If I send myself the patch with git-send-email, I also get the headers
> in the email (I tried from ensibm, which is the machine which sent the
> patch serie). So, it doesn't look like a bug in git, but rather a
> miss-use.
> 
> Axel, can you give us the exact command(s) you used to send the patch?

I made the patch with "git send-email --cover --annotate", and then edited
the messages with vim.

I added the S-o-b lines by copy-pasting them from the test mail I
had send to Matthieu (from thunderbird). I saw there was a problem of
encoding with the "é" of
Clément, so I modified it.

I think I should have written the S-o-b lines directly in vim.

Re: [RFC/PATCH 2/4] textconv: make diff_options accessible from blame

[Matthieu Moy]

bonneta <bonneta@ensimag.fr> writes:

> On Fri, 04 Jun 2010 09:59:47 +0200, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>>> Axel Bonnet <axel.bonnet@ensimag.imag.fr> writes:

>>>> Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
>>>
>>> The name of Clément is spelled correctly on the mail header while S-o-b
>>> line is corrupt.
>> 
>> Actually, it's valid UTF-8, but there's no header specifying the
>> encoding in the email,
[...]
>> Axel, can you give us the exact command(s) you used to send the patch?
>
> I made the patch with "git send-email --cover --annotate", and then edited
> the messages with vim.
>
> I added the S-o-b lines by copy-pasting them 

OK, I got it. You ran "git format-patch", and it didn't find any
non-ascii characters, so it didn't add any encoding header. Then you
added UTF-8, and the header still wasn't there.

You can use "git rebase -i" to edit the commit messages directly, and
add the s-o-b there, then git format-patch will DRT.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/