# [RFC/PATCH 0/4] git-gui blame: use textconv 8 messages from 2010-06-06 to 2010-06-07. Participants: Clément Poulain, Matthieu Moy, Jeff King. Thread: https://gitlist.dev/t/24022 ## Clément Poulain, 2010-06-06 11:30 Subject: [RFC/PATCH 0/4] git-gui blame: use textconv Message-ID: <1275823848-7151-1-git-send-email-clement.poulain@ensimag.imag.fr> URL: https://gitlist.dev/e/1275823848-7151-1-git-send-email-clement.poulain%40ensimag.imag.fr ``` 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, 2010-06-06 11:30 Subject: [RFC/PATCH 1/4] sha1_name: creating context cache Message-ID: <1275823848-7151-2-git-send-email-clement.poulain@ensimag.imag.fr> URL: https://gitlist.dev/e/1275823848-7151-2-git-send-email-clement.poulain%40ensimag.imag.fr In-Reply-To: <1275823848-7151-1-git-send-email-clement.poulain@ensimag.imag.fr> ``` 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 Signed-off-by: Diane Gasselin Signed-off-by: Axel Bonnet --- builtin/cat-file.c | 3 +++ cache.h | 8 ++++++++ sha1_name.c | 19 +++++++++++++++++++ 3 files changed, 30 insertions(+), 0 deletions(-) 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, 2010-06-06 11:30 Subject: [RFC/PATCH 2/4] textconv: support for cat-file Message-ID: <1275823848-7151-3-git-send-email-clement.poulain@ensimag.imag.fr> URL: https://gitlist.dev/e/1275823848-7151-3-git-send-email-clement.poulain%40ensimag.imag.fr In-Reply-To: <1275823848-7151-2-git-send-email-clement.poulain@ensimag.imag.fr> ``` 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 Signed-off-by: Diane Gasselin Signed-off-by: Axel Bonnet --- builtin/blame.c | 8 ++++---- builtin/cat-file.c | 21 ++++++++++++++++++++- 2 files changed, 24 insertions(+), 5 deletions(-) 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: must be ", + 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|) ", + "git cat-file (-t|-s|-e|-p||--textconv) ", "git cat-file (--batch|--batch-check) < ", 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, 2010-06-06 11:30 Subject: [RFC/PATCH 3/4] git-gui: use textconv filter for diff and blame Message-ID: <1275823848-7151-4-git-send-email-clement.poulain@ensimag.imag.fr> URL: https://gitlist.dev/e/1275823848-7151-4-git-send-email-clement.poulain%40ensimag.imag.fr In-Reply-To: <1275823848-7151-3-git-send-email-clement.poulain@ensimag.imag.fr> ``` 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 Signed-off-by: Diane Gasselin Signed-off-by: Axel Bonnet --- 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(-) 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, 2010-06-06 11:30 Subject: [RFC/PATCH 4/4] t/t8007: test textconv support for cat-file Message-ID: <1275823848-7151-5-git-send-email-clement.poulain@ensimag.imag.fr> URL: https://gitlist.dev/e/1275823848-7151-5-git-send-email-clement.poulain%40ensimag.imag.fr In-Reply-To: <1275823848-7151-4-git-send-email-clement.poulain@ensimag.imag.fr> ``` Test the correct functionning of textconv with cat-file and cat-file HEAD^ . Test the case when no driver is specified. Signed-off-by: Clément Poulain Signed-off-by: Diane Gasselin Signed-off-by: Axel Bonnet --- t/t8007-cat-file-textconv.sh | 66 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 66 insertions(+), 0 deletions(-) create mode 100755 t/t8007-cat-file-textconv.sh 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 <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 <result && + test_cmp expected result +' + +cat >expected <result && + test_cmp expected result +' + +cat >expected <result && + test_cmp expected result +' +test_done -- 1.6.6.7.ga5fe3 ``` ## Matthieu Moy, 2010-06-06 15:56 Subject: Re: [RFC/PATCH 1/4] sha1_name: creating context cache Message-ID: URL: https://gitlist.dev/e/vpqpr045f46.fsf%40bauges.imag.fr In-Reply-To: <1275823848-7151-2-git-send-email-clement.poulain@ensimag.imag.fr> ``` Clément Poulain 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. > 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, 2010-06-06 22:15 Subject: Re: [RFC/PATCH 1/4] sha1_name: creating context cache Message-ID: <20100606221557.GF6993@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100606221557.GF6993%40coredump.intra.peff.net In-Reply-To: ``` 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 ``` ## Clément Poulain, 2010-06-07 08:34 Subject: Re: [RFC/PATCH 1/4] sha1_name: creating context cache Message-ID: URL: https://gitlist.dev/e/AANLkTimLzfmF060d_8mNjrXwTV72A9KudtpEeJUkMWJt%40mail.gmail.com In-Reply-To: <20100606221557.GF6993@coredump.intra.peff.net> ``` 2010/6/7 Jeff King : > 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 ```