threads / rfc / 24022

RFC patch, 4 partsgit-gui blame: use textconv

Subject: [RFC/PATCH 0/4] git-gui blame: use textconv

## tl;dr

8 messages between Jun 6, 2010 and Jun 7, 2010. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Clément Poulain· Jun 6, 2010, 11:30 UTC · lore
This patch adds support of textconv to git-gui blame. It is based on the work already done here: http://mid.gmane.org/1275562038-7468-1-git-send-email-axel.bonnet@ensimag.imag.fr, and uses a git-gui patch done by Clemens Buchacher (http://mid.gmane.org/20100415193944.GA5848@localhost) which adds textconv support to git-gui diff.
git-gui blame is based on cat-file to get the content of the file in different revisions. So the patch adds textconv support to cat-file.
After a discussion with Jeff King and Matthieu Moy, it appears that creating a context cache is a good way to know the pathname of the concerned blob, as textconv needs a pathname to work. The first part of the patch adds this cache
Clément Poulain (4):
  sha1_name: creating context cache
  textconv: support for cat-file
  git-gui: use textconv filter for diff and blame
  t/t8007: test textconv support for cat-file
 builtin/blame.c              |    8 ++--
 builtin/cat-file.c           |   24 ++++++++++++++-
 cache.h                      |    8 +++++
 git-gui/git-gui.sh           |   28 +++++++++++++++++-
 git-gui/lib/blame.tcl        |   18 ++++++++++-
 git-gui/lib/diff.tcl         |    5 ++-
 git-gui/lib/option.tcl       |    1 +
 sha1_name.c                  |   19 ++++++++++++
 t/t8007-cat-file-textconv.sh |   66 ++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 168 insertions(+), 9 deletions(-)
 create mode 100755 t/t8007-cat-file-textconv.sh
Clément Poulain· Jun 6, 2010, 11:30 UTC · re: Clément Poulain · lore

[RFC/PATCH 1/4] sha1_name: creating context cache

This cache keeps the global context for the last sha1 looked up, especially its pathname. The textconv is indeed defined by the diff driver, which is associated with a pathname, not a blob
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 builtin/cat-file.c |    3 +++
 cache.h            |    8 ++++++++
 sha1_name.c        |   19 +++++++++++++++++++
 3 files changed, 30 insertions(+), 0 deletions(-)
Show changes to 3 files +30 −0

builtin/cat-file.c, cache.h, sha1_name.c

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index a933eaa..124e0a9 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -87,6 +87,9 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 	void *buf;
 	unsigned long size;
 
+	struct strbuf sbuf = STRBUF_INIT;
+
+	object_resolve_context_init(&object_context);
 	if (get_sha1(obj_name, sha1))
 		die("Not a valid object name %s", obj_name);
 
diff --git a/cache.h b/cache.h
index 0f4263c..26a1faf 100644
--- a/cache.h
+++ b/cache.h
@@ -730,6 +730,14 @@ static inline unsigned int hexval(unsigned char c)
 #define MINIMUM_ABBREV 4
 #define DEFAULT_ABBREV 7
 
+struct object_resolve_context {
+	unsigned char tree[20];
+	char path[PATH_MAX];
+	unsigned mode;
+};
+extern struct object_resolve_context object_context;
+void object_resolve_context_init(struct object_resolve_context *orc);
+
 extern int get_sha1(const char *str, unsigned char *sha1);
 extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, const char *prefix);
 static inline int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
diff --git a/sha1_name.c b/sha1_name.c
index bf92417..093d71a 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -7,6 +7,13 @@
 #include "refs.h"
 #include "remote.h"
 
+struct object_resolve_context object_context;
+
+void object_resolve_context_init(struct object_resolve_context *orc)
+{
+	memset(orc, 0, sizeof(*orc));
+}
+
 static int find_short_object_filename(int len, const char *name, unsigned char *sha1)
 {
 	struct alternate_object_database *alt;
@@ -1059,6 +1066,11 @@ int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,
 			cp = name + 3;
 		}
 		namelen = namelen - (cp - name);
+
+		strncpy(object_context.path, cp,
+			sizeof(object_context.path));
+		object_context.path[sizeof(object_context.path)] = '\0';
+
 		if (!active_cache)
 			read_cache();
 		pos = cache_name_pos(cp, namelen);
@@ -1072,6 +1084,7 @@ int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,
 			if (ce_stage(ce) == stage) {
 				hashcpy(sha1, ce->sha1);
 				*mode = ce->ce_mode;
+				object_context.mode = *mode;
 				return 0;
 			}
 			pos++;
@@ -1104,6 +1117,12 @@ int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,
 							   tree_sha1, object_name);
 				free(object_name);
 			}
+			hashcpy(object_context.tree, tree_sha1);
+			strncpy(object_context.path, filename,
+				sizeof(object_context.path));
+			object_context.path[sizeof(object_context.path)] = '\0';
+			object_context.mode = *mode;
+
 			return ret;
 		} else {
 			if (!gently)
-- 
1.6.6.7.ga5fe3
Clément Poulain· Jun 6, 2010, 11:30 UTC · re: Clément Poulain · lore

[RFC/PATCH 2/4] textconv: support for cat-file

Make the textconv_object function public, and add --textconv option to cat-file to perform conversion on blob objects. Using --textconv implies that we are working on a blob
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 builtin/blame.c    |    8 ++++----
 builtin/cat-file.c |   21 ++++++++++++++++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)
Show changes to 2 files +24 −5

builtin/blame.c, builtin/cat-file.c

diff --git a/builtin/blame.c b/builtin/blame.c
index 4679fd9..869be1d 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -91,10 +91,10 @@ struct origin {
  * 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)
+int textconv_object(const char *path,
+		    const unsigned char *sha1,
+		    unsigned short mode,
+		    struct strbuf *buf)
 {
 	struct diff_filespec *df;
 
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 124e0a9..fde5fb9 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -9,6 +9,7 @@
 #include "tree.h"
 #include "builtin.h"
 #include "parse-options.h"
+#include "diff.h"
 
 #define BATCH 1
 #define BATCH_CHECK 2
@@ -135,6 +136,21 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 
 		/* otherwise just spit out the data */
 		break;
+
+	case 'c':
+		if (!object_context.path[0])
+			die("git cat-file --textconv %s: <object> must be <sha1:path>",
+			    obj_name);
+
+		if(textconv_object(object_context.path, sha1,
+				   object_context.mode, &sbuf))
+			buf = strbuf_detach(&sbuf, (size_t *) &size);
+		else
+			buf = read_object_with_reference(sha1, "blob", &size, NULL);
+
+		strbuf_release(&sbuf);
+		break;
+
 	case 0:
 		buf = read_object_with_reference(sha1, exp_type, &size, NULL);
 		break;
@@ -204,7 +220,7 @@ static int batch_objects(int print_contents)
 }
 
 static const char * const cat_file_usage[] = {
-	"git cat-file (-t|-s|-e|-p|<type>) <object>",
+	"git cat-file (-t|-s|-e|-p|<type>|--textconv) <object>",
 	"git cat-file (--batch|--batch-check) < <list_of_objects>",
 	NULL
 };
@@ -221,6 +237,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT('e', NULL, &opt,
 			    "exit with zero when there's no error", 'e'),
 		OPT_SET_INT('p', NULL, &opt, "pretty-print object's content", 'p'),
+		OPT_SET_INT(0, "textconv", &opt,
+				"for blob objects, run textconv on object's content", 'c'),
 		OPT_SET_INT(0, "batch", &batch,
 			    "show info and content of objects fed from the standard input",
 			    BATCH),
@@ -231,6 +249,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_default_config, NULL);
+	git_config(git_diff_basic_config, NULL);
 
 	if (argc != 3 && argc != 2)
 		usage_with_options(cat_file_usage, options);
-- 
1.6.6.7.ga5fe3
Clément Poulain· Jun 6, 2010, 11:30 UTC · re: Clément Poulain · lore

[RFC/PATCH 3/4] git-gui: use textconv filter for diff and blame

Create a checkbox "Use Textconv For Diffs and Blame" in git-gui options. If checked, git-gui calls diff and blame with --textconv option
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 git-gui/git-gui.sh     |   28 +++++++++++++++++++++++++++-
 git-gui/lib/blame.tcl  |   18 ++++++++++++++++--
 git-gui/lib/diff.tcl   |    5 ++++-
 git-gui/lib/option.tcl |    1 +
 4 files changed, 48 insertions(+), 4 deletions(-)
Show changes to 4 files +48 −4

git-gui/git-gui.sh, git-gui/lib/blame.tcl, git-gui/lib/diff.tcl, git-gui/lib/option.tcl

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 7d54511..59edf39 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -269,6 +269,17 @@ proc is_config_true {name} {
 	}
 }
 
+proc is_config_false {name} {
+	global repo_config
+	if {[catch {set v $repo_config($name)}]} {
+		return 0
+	} elseif {$v eq {false} || $v eq {0} || $v eq {no}} {
+		return 1
+	} else {
+		return 0
+	}
+}
+
 proc get_config {name} {
 	global repo_config
 	if {[catch {set v $repo_config($name)}]} {
@@ -782,6 +793,7 @@ set default_config(user.email) {}
 
 set default_config(gui.encoding) [encoding system]
 set default_config(gui.matchtrackingbranch) false
+set default_config(gui.textconv) true
 set default_config(gui.pruneduringfetch) false
 set default_config(gui.trustmtime) false
 set default_config(gui.fastcopyblame) false
@@ -3405,6 +3417,19 @@ lappend diff_actions [list $ctxmsm entryconf [$ctxmsm index last] -state]
 $ctxmsm add separator
 create_common_diff_popup $ctxmsm
 
+proc has_textconv {path} {
+	if {[is_config_false gui.textconv]} {
+		return 0
+	}
+	set filter [gitattr $path diff set]
+	set textconv [get_config [join [list diff $filter textconv] .]]
+	if {$filter ne {set} && $textconv ne {}} {
+		return 1
+	} else {
+		return 0
+	}
+}
+
 proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 	global current_diff_path file_states
 	set ::cursorX $x
@@ -3440,7 +3465,8 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 			|| {__} eq $state
 			|| {_O} eq $state
 			|| {_T} eq $state
-			|| {T_} eq $state} {
+			|| {T_} eq $state
+			|| [has_textconv $current_diff_path]} {
 			set s disabled
 		} else {
 			set s normal
diff --git a/git-gui/lib/blame.tcl b/git-gui/lib/blame.tcl
index 786b50b..45adf64 100644
--- a/git-gui/lib/blame.tcl
+++ b/git-gui/lib/blame.tcl
@@ -450,10 +450,24 @@ method _load {jump} {
 	$status show [mc "Reading %s..." "$commit:[escape_path $path]"]
 	$w_path conf -text [escape_path $path]
 	if {$commit eq {}} {
-		set fd [open $path r]
+		if {![is_config_false gui.textconv]} {
+			set filter [gitattr $path diff set]
+			set textconv [get_config [join [list diff $filter textconv] .]]
+			if {$filter ne {set} && $textconv ne {}} {
+				set fd [open "|$textconv $path" r]
+			} else {
+				set fd [open $path r]
+			}
+		} else {
+			set fd [open $path r]
+		}
 		fconfigure $fd -eofchar {}
 	} else {
-		set fd [git_read cat-file blob "$commit:$path"]
+		if {![is_config_false gui.textconv]} {
+			set fd [git_read cat-file --textconv "$commit:$path"]
+		} else {
+			set fd [git_read cat-file blob "$commit:$path"]
+		}
 	}
 	fconfigure $fd \
 		-blocking 0 \
diff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl
index ec8c11e..c628750 100644
--- a/git-gui/lib/diff.tcl
+++ b/git-gui/lib/diff.tcl
@@ -55,7 +55,7 @@ proc handle_empty_diff {} {
 
 	set path $current_diff_path
 	set s $file_states($path)
-	if {[lindex $s 0] ne {_M}} return
+	if {[lindex $s 0] ne {_M} || [has_textconv $path]} return
 
 	# Prevent infinite rescan loops
 	incr diff_empty_count
@@ -280,6 +280,9 @@ proc start_show_diff {cont_info {add_opts {}}} {
 			lappend cmd diff-files
 		}
 	}
+	if {![is_config_false gui.textconv] && [git-version >= 1.6.1]} {
+		lappend cmd --textconv
+	}
 
 	if {[string match {160000 *} [lindex $s 2]]
 	 || [string match {160000 *} [lindex $s 3]]} {
diff --git a/git-gui/lib/option.tcl b/git-gui/lib/option.tcl
index d4c5e45..3807c8d 100644
--- a/git-gui/lib/option.tcl
+++ b/git-gui/lib/option.tcl
@@ -148,6 +148,7 @@ proc do_options {} {
 		{b gui.trustmtime  {mc "Trust File Modification Timestamps"}}
 		{b gui.pruneduringfetch {mc "Prune Tracking Branches During Fetch"}}
 		{b gui.matchtrackingbranch {mc "Match Tracking Branches"}}
+		{b gui.textconv {mc "Use Textconv For Diffs and Blames"}}
 		{b gui.fastcopyblame {mc "Blame Copy Only On Changed Files"}}
 		{i-20..200 gui.copyblamethreshold {mc "Minimum Letters To Blame Copy On"}}
 		{i-0..300 gui.blamehistoryctx {mc "Blame History Context Radius (days)"}}
-- 
1.6.6.7.ga5fe3
Clément Poulain· Jun 6, 2010, 11:30 UTC · re: Clément Poulain · lore

[RFC/PATCH 4/4] t/t8007: test textconv support for cat-file

Test the correct functionning of textconv with cat-file <sha1:blob> and cat-file HEAD^ <file>. Test the case when no driver is specified.

Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 t/t8007-cat-file-textconv.sh |   66 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)
 create mode 100755 t/t8007-cat-file-textconv.sh
Show changes to t/t8007-cat-file-textconv.sh +66 −0
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
new file mode 100755
index 0000000..0789912
--- /dev/null
+++ b/t/t8007-cat-file-textconv.sh
@@ -0,0 +1,66 @@
+#!/bin/sh
+
+test_description='git cat-file textconv support'
+. ./test-lib.sh
+
+cat >helper <<'EOF'
+#!/bin/sh
+sed 's/^/converted: /' "$@"
+EOF
+chmod +x helper
+
+test_expect_success 'setup ' '
+	echo 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 &&
+	GIT_AUTHOR_NAME=Number2 git commit -a -m Second --date="2010-01-01 20:00:00"
+'
+
+cat >expected <<EOF
+test version 2
+EOF
+
+test_expect_success 'no filter specified' '
+	git cat-file --textconv :one.bin >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 'cat-file without --textconv' '
+	git cat-file blob :one.bin >result &&
+	test_cmp expected result
+'
+
+cat >expected <<EOF
+test
+EOF
+
+test_expect_success 'cat-file without --textconv on previous commit' '
+	git cat-file -p HEAD^:one.bin >result &&
+	test_cmp expected result
+'
+
+cat >expected <<EOF
+converted: test version 2
+EOF
+
+test_expect_success 'cat-file --textconv on last commit' '
+	git cat-file --textconv :one.bin >result &&
+	test_cmp expected result
+'
+
+cat >expected <<EOF
+converted: test
+EOF
+
+test_expect_success 'cat-file --textconv on previous commit' '
+	git cat-file --textconv HEAD^:one.bin >result &&
+	test_cmp expected result
+'
+test_done
-- 
1.6.6.7.ga5fe3
Matthieu Moy· Jun 6, 2010, 15:56 UTC · re: Clément Poulain · lore

Re: [RFC/PATCH 1/4] sha1_name: creating context cache

Clément Poulain <clement.poulain@ensimag.imag.fr> writes:
> This cache keeps the global context for the last sha1 looked up, especially its pathname. The textconv is indeed defined by the diff driver, which is associated with a pathname, not a blob

Please, wrap your message to make it fit in 80 characters (M-q under Emacs can help). This applies to the cover email too, but is particularly important for this one, since it'll become the commit message if the patch is accepted.

Show 14 quoted lines
> diff --git a/cache.h b/cache.h
> index 0f4263c..26a1faf 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -730,6 +730,14 @@ static inline unsigned int hexval(unsigned char c)
>  #define MINIMUM_ABBREV 4
>  #define DEFAULT_ABBREV 7
>  
> +struct object_resolve_context {
> +	unsigned char tree[20];
> +	char path[PATH_MAX];
> +	unsigned mode;
> +};
> +extern struct object_resolve_context object_context;

Is it really a good idea to make this a global variable? As I understand it, the semantics of this variable is that it contains information on the last sha1 name parsed (BTW, you probably want to add a comment here explaining that). Wouldn't it be more robust to have this value returned by the function doing the parsing?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Jeff King· Jun 6, 2010, 22:15 UTC · re: Matthieu Moy · lore

Re: [RFC/PATCH 1/4] sha1_name: creating context cache

On Sun, Jun 06, 2010 at 05:56:41PM +0200, Matthieu Moy wrote:
Show 12 quoted lines
> > +struct object_resolve_context {
> > +	unsigned char tree[20];
> > +	char path[PATH_MAX];
> > +	unsigned mode;
> > +};
> > +extern struct object_resolve_context object_context;
> 
> Is it really a good idea to make this a global variable? As I
> understand it, the semantics of this variable is that it contains
> information on the last sha1 name parsed (BTW, you probably want to
> add a comment here explaining that). Wouldn't it be more robust to
> have this value returned by the function doing the parsing?

This is straight from my earlier patch, but I think I said in that thread that it could use some cleaning up. You added the 'mode' field, but it probably makes sense to then get rid of the distinction between get_sha1 and get_sha1_with_mode, and just have:

  get_sha1_with_context(const char *name, unsigned char *sha1, struct
                        object_resolve_context *orc) {
     /* basically get_sha1_with_mode, but set mode in orc */
  }
  get_sha1(const char *name, unsigned char *sha1) {
         struct object_resolve_context orc; /* to be thrown away */
         return get_sha1_with_context(name, sha1, &orc);
  }
-Peff
Clément Poulain· Jun 7, 2010, 08:34 UTC · re: Jeff King · lore

Re: [RFC/PATCH 1/4] sha1_name: creating context cache

2010/6/7 Jeff King <peff@peff.net>:
Show 31 quoted lines
> On Sun, Jun 06, 2010 at 05:56:41PM +0200, Matthieu Moy wrote:
>
>> > +struct object_resolve_context {
>> > +   unsigned char tree[20];
>> > +   char path[PATH_MAX];
>> > +   unsigned mode;
>> > +};
>> > +extern struct object_resolve_context object_context;
>>
>> Is it really a good idea to make this a global variable? As I
>> understand it, the semantics of this variable is that it contains
>> information on the last sha1 name parsed (BTW, you probably want to
>> add a comment here explaining that). Wouldn't it be more robust to
>> have this value returned by the function doing the parsing?
>
> This is straight from my earlier patch, but I think I said in that
> thread that it could use some cleaning up. You added the 'mode' field,
> but it probably makes sense to then get rid of the distinction between
> get_sha1 and get_sha1_with_mode, and just have:
>
>  get_sha1_with_context(const char *name, unsigned char *sha1, struct
>                        object_resolve_context *orc) {
>     /* basically get_sha1_with_mode, but set mode in orc */
>  }
>  get_sha1(const char *name, unsigned char *sha1) {
>         struct object_resolve_context orc; /* to be thrown away */
>         return get_sha1_with_context(name, sha1, &orc);
>  }
>
> -Peff
>

Yes, we've already thought about it (you've said it, Jeff ;-) ), and we have actually changed it with something very close to what you say. We'll soon propose a v2 for this part of the patch.

Thank you

← back to recent threads