{"thread":{"id":"24022","subject":"[RFC/PATCH 0/4] git-gui blame: use textconv","startedAt":"2010-06-06T11:30:44Z","lastAt":"2010-06-07T08:34:01Z","messageCount":8,"participants":["Clément Poulain","Matthieu Moy","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"143077","messageId":"1275823848-7151-1-git-send-email-clement.poulain@ensimag.imag.fr","threadId":"24022","inReplyTo":null,"subject":"[RFC/PATCH 0/4] git-gui blame: use textconv","fromName":"Clément Poulain","fromEmail":"clement.poulain@ensimag.imag.fr","sentAt":"2010-06-06T11:30:44Z","receivedAt":"2010-06-06T11:30:44Z","isPatch":true,"sender":{"key":"clement.poulain@ensimag.imag.fr","avatar":null},"body":"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.\n\ngit-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.\n\nAfter 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\n\nClément Poulain (4):\n sha1_name: creating context cache\n textconv: support for cat-file\n git-gui: use textconv filter for diff and blame\n t/t8007: test textconv support for cat-file\n\n builtin/blame.c | 8 ++--\n builtin/cat-file.c | 24 ++++++++++++++-\n cache.h | 8 +++++\n git-gui/git-gui.sh | 28 +++++++++++++++++-\n git-gui/lib/blame.tcl | 18 ++++++++++-\n git-gui/lib/diff.tcl | 5 ++-\n git-gui/lib/option.tcl | 1 +\n sha1_name.c | 19 ++++++++++++\n t/t8007-cat-file-textconv.sh | 66 ++++++++++++++++++++++++++++++++++++++++++\n 9 files changed, 168 insertions(+), 9 deletions(-)\n create mode 100755 t/t8007-cat-file-textconv.sh\n"},{"id":"143078","messageId":"1275823848-7151-2-git-send-email-clement.poulain@ensimag.imag.fr","threadId":"24022","inReplyTo":"1275823848-7151-1-git-send-email-clement.poulain@ensimag.imag.fr","subject":"[RFC/PATCH 1/4] sha1_name: creating context cache","fromName":"Clément Poulain","fromEmail":"clement.poulain@ensimag.imag.fr","sentAt":"2010-06-06T11:30:45Z","receivedAt":"2010-06-06T11:30:45Z","isPatch":true,"sender":{"key":"clement.poulain@ensimag.imag.fr","avatar":null},"body":"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\n\nSigned-off-by: Clément Poulain \nSigned-off-by: Diane Gasselin \nSigned-off-by: Axel Bonnet \n---\n builtin/cat-file.c | 3 +++\n cache.h | 8 ++++++++\n sha1_name.c | 19 +++++++++++++++++++\n 3 files changed, 30 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex a933eaa..124e0a9 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -87,6 +87,9 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \tvoid *buf;\n \tunsigned long size;\n \n+\tstruct strbuf sbuf = STRBUF_INIT;\n+\n+\tobject_resolve_context_init(&object_context);\n \tif (get_sha1(obj_name, sha1))\n \t\tdie(\"Not a valid object name %s\", obj_name);\n \ndiff --git a/cache.h b/cache.h\nindex 0f4263c..26a1faf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -730,6 +730,14 @@ static inline unsigned int hexval(unsigned char c)\n #define MINIMUM_ABBREV 4\n #define DEFAULT_ABBREV 7\n \n+struct object_resolve_context {\n+\tunsigned char tree[20];\n+\tchar path[PATH_MAX];\n+\tunsigned mode;\n+};\n+extern struct object_resolve_context object_context;\n+void object_resolve_context_init(struct object_resolve_context *orc);\n+\n extern int get_sha1(const char *str, unsigned char *sha1);\n extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, const char *prefix);\n static inline int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)\ndiff --git a/sha1_name.c b/sha1_name.c\nindex bf92417..093d71a 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -7,6 +7,13 @@\n #include \"refs.h\"\n #include \"remote.h\"\n \n+struct object_resolve_context object_context;\n+\n+void object_resolve_context_init(struct object_resolve_context *orc)\n+{\n+\tmemset(orc, 0, sizeof(*orc));\n+}\n+\n static int find_short_object_filename(int len, const char *name, unsigned char *sha1)\n {\n \tstruct alternate_object_database *alt;\n@@ -1059,6 +1066,11 @@ int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,\n \t\t\tcp = name + 3;\n \t\t}\n \t\tnamelen = namelen - (cp - name);\n+\n+\t\tstrncpy(object_context.path, cp,\n+\t\t\tsizeof(object_context.path));\n+\t\tobject_context.path[sizeof(object_context.path)] = '\\0';\n+\n \t\tif (!active_cache)\n \t\t\tread_cache();\n \t\tpos = cache_name_pos(cp, namelen);\n@@ -1072,6 +1084,7 @@ int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,\n \t\t\tif (ce_stage(ce) == stage) {\n \t\t\t\thashcpy(sha1, ce->sha1);\n \t\t\t\t*mode = ce->ce_mode;\n+\t\t\t\tobject_context.mode = *mode;\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t\tpos++;\n@@ -1104,6 +1117,12 @@ int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,\n \t\t\t\t\t\t\t tree_sha1, object_name);\n \t\t\t\tfree(object_name);\n \t\t\t}\n+\t\t\thashcpy(object_context.tree, tree_sha1);\n+\t\t\tstrncpy(object_context.path, filename,\n+\t\t\t\tsizeof(object_context.path));\n+\t\t\tobject_context.path[sizeof(object_context.path)] = '\\0';\n+\t\t\tobject_context.mode = *mode;\n+\n \t\t\treturn ret;\n \t\t} else {\n \t\t\tif (!gently)\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143079","messageId":"1275823848-7151-3-git-send-email-clement.poulain@ensimag.imag.fr","threadId":"24022","inReplyTo":"1275823848-7151-2-git-send-email-clement.poulain@ensimag.imag.fr","subject":"[RFC/PATCH 2/4] textconv: support for cat-file","fromName":"Clément Poulain","fromEmail":"clement.poulain@ensimag.imag.fr","sentAt":"2010-06-06T11:30:46Z","receivedAt":"2010-06-06T11:30:46Z","isPatch":true,"sender":{"key":"clement.poulain@ensimag.imag.fr","avatar":null},"body":"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\n\nSigned-off-by: Clément Poulain \nSigned-off-by: Diane Gasselin \nSigned-off-by: Axel Bonnet \n---\n builtin/blame.c | 8 ++++----\n builtin/cat-file.c | 21 ++++++++++++++++++++-\n 2 files changed, 24 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 4679fd9..869be1d 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -91,10 +91,10 @@ struct origin {\n * if the textconv driver exists.\n * Return 1 if the conversion succeeds, 0 otherwise.\n */\n-static int textconv_object(const char *path,\n-\t\t\t const unsigned char *sha1,\n-\t\t\t unsigned short mode,\n-\t\t\t struct strbuf *buf)\n+int textconv_object(const char *path,\n+\t\t const unsigned char *sha1,\n+\t\t unsigned short mode,\n+\t\t struct strbuf *buf)\n {\n \tstruct diff_filespec *df;\n \ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 124e0a9..fde5fb9 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -9,6 +9,7 @@\n #include \"tree.h\"\n #include \"builtin.h\"\n #include \"parse-options.h\"\n+#include \"diff.h\"\n \n #define BATCH 1\n #define BATCH_CHECK 2\n@@ -135,6 +136,21 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \n \t\t/* otherwise just spit out the data */\n \t\tbreak;\n+\n+\tcase 'c':\n+\t\tif (!object_context.path[0])\n+\t\t\tdie(\"git cat-file --textconv %s: must be \",\n+\t\t\t obj_name);\n+\n+\t\tif(textconv_object(object_context.path, sha1,\n+\t\t\t\t object_context.mode, &sbuf))\n+\t\t\tbuf = strbuf_detach(&sbuf, (size_t *) &size);\n+\t\telse\n+\t\t\tbuf = read_object_with_reference(sha1, \"blob\", &size, NULL);\n+\n+\t\tstrbuf_release(&sbuf);\n+\t\tbreak;\n+\n \tcase 0:\n \t\tbuf = read_object_with_reference(sha1, exp_type, &size, NULL);\n \t\tbreak;\n@@ -204,7 +220,7 @@ static int batch_objects(int print_contents)\n }\n \n static const char * const cat_file_usage[] = {\n-\t\"git cat-file (-t|-s|-e|-p|) \",\n+\t\"git cat-file (-t|-s|-e|-p||--textconv) \",\n \t\"git cat-file (--batch|--batch-check) < \",\n \tNULL\n };\n@@ -221,6 +237,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\tOPT_SET_INT('e', NULL, &opt,\n \t\t\t \"exit with zero when there's no error\", 'e'),\n \t\tOPT_SET_INT('p', NULL, &opt, \"pretty-print object's content\", 'p'),\n+\t\tOPT_SET_INT(0, \"textconv\", &opt,\n+\t\t\t\t\"for blob objects, run textconv on object's content\", 'c'),\n \t\tOPT_SET_INT(0, \"batch\", &batch,\n \t\t\t \"show info and content of objects fed from the standard input\",\n \t\t\t BATCH),\n@@ -231,6 +249,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t};\n \n \tgit_config(git_default_config, NULL);\n+\tgit_config(git_diff_basic_config, NULL);\n \n \tif (argc != 3 && argc != 2)\n \t\tusage_with_options(cat_file_usage, options);\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143080","messageId":"1275823848-7151-4-git-send-email-clement.poulain@ensimag.imag.fr","threadId":"24022","inReplyTo":"1275823848-7151-3-git-send-email-clement.poulain@ensimag.imag.fr","subject":"[RFC/PATCH 3/4] git-gui: use textconv filter for diff and blame","fromName":"Clément Poulain","fromEmail":"clement.poulain@ensimag.imag.fr","sentAt":"2010-06-06T11:30:47Z","receivedAt":"2010-06-06T11:30:47Z","isPatch":true,"sender":{"key":"clement.poulain@ensimag.imag.fr","avatar":null},"body":"Create a checkbox \"Use Textconv For Diffs and Blame\" in git-gui options. If checked, git-gui calls diff and blame with --textconv option\n\nSigned-off-by: Clément Poulain \nSigned-off-by: Diane Gasselin \nSigned-off-by: Axel Bonnet \n---\n git-gui/git-gui.sh | 28 +++++++++++++++++++++++++++-\n git-gui/lib/blame.tcl | 18 ++++++++++++++++--\n git-gui/lib/diff.tcl | 5 ++++-\n git-gui/lib/option.tcl | 1 +\n 4 files changed, 48 insertions(+), 4 deletions(-)\n\ndiff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh\nindex 7d54511..59edf39 100755\n--- a/git-gui/git-gui.sh\n+++ b/git-gui/git-gui.sh\n@@ -269,6 +269,17 @@ proc is_config_true {name} {\n \t}\n }\n \n+proc is_config_false {name} {\n+\tglobal repo_config\n+\tif {[catch {set v $repo_config($name)}]} {\n+\t\treturn 0\n+\t} elseif {$v eq {false} || $v eq {0} || $v eq {no}} {\n+\t\treturn 1\n+\t} else {\n+\t\treturn 0\n+\t}\n+}\n+\n proc get_config {name} {\n \tglobal repo_config\n \tif {[catch {set v $repo_config($name)}]} {\n@@ -782,6 +793,7 @@ set default_config(user.email) {}\n \n set default_config(gui.encoding) [encoding system]\n set default_config(gui.matchtrackingbranch) false\n+set default_config(gui.textconv) true\n set default_config(gui.pruneduringfetch) false\n set default_config(gui.trustmtime) false\n set default_config(gui.fastcopyblame) false\n@@ -3405,6 +3417,19 @@ lappend diff_actions [list $ctxmsm entryconf [$ctxmsm index last] -state]\n $ctxmsm add separator\n create_common_diff_popup $ctxmsm\n \n+proc has_textconv {path} {\n+\tif {[is_config_false gui.textconv]} {\n+\t\treturn 0\n+\t}\n+\tset filter [gitattr $path diff set]\n+\tset textconv [get_config [join [list diff $filter textconv] .]]\n+\tif {$filter ne {set} && $textconv ne {}} {\n+\t\treturn 1\n+\t} else {\n+\t\treturn 0\n+\t}\n+}\n+\n proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {\n \tglobal current_diff_path file_states\n \tset ::cursorX $x\n@@ -3440,7 +3465,8 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {\n \t\t\t|| {__} eq $state\n \t\t\t|| {_O} eq $state\n \t\t\t|| {_T} eq $state\n-\t\t\t|| {T_} eq $state} {\n+\t\t\t|| {T_} eq $state\n+\t\t\t|| [has_textconv $current_diff_path]} {\n \t\t\tset s disabled\n \t\t} else {\n \t\t\tset s normal\ndiff --git a/git-gui/lib/blame.tcl b/git-gui/lib/blame.tcl\nindex 786b50b..45adf64 100644\n--- a/git-gui/lib/blame.tcl\n+++ b/git-gui/lib/blame.tcl\n@@ -450,10 +450,24 @@ method _load {jump} {\n \t$status show [mc \"Reading %s...\" \"$commit:[escape_path $path]\"]\n \t$w_path conf -text [escape_path $path]\n \tif {$commit eq {}} {\n-\t\tset fd [open $path r]\n+\t\tif {![is_config_false gui.textconv]} {\n+\t\t\tset filter [gitattr $path diff set]\n+\t\t\tset textconv [get_config [join [list diff $filter textconv] .]]\n+\t\t\tif {$filter ne {set} && $textconv ne {}} {\n+\t\t\t\tset fd [open \"|$textconv $path\" r]\n+\t\t\t} else {\n+\t\t\t\tset fd [open $path r]\n+\t\t\t}\n+\t\t} else {\n+\t\t\tset fd [open $path r]\n+\t\t}\n \t\tfconfigure $fd -eofchar {}\n \t} else {\n-\t\tset fd [git_read cat-file blob \"$commit:$path\"]\n+\t\tif {![is_config_false gui.textconv]} {\n+\t\t\tset fd [git_read cat-file --textconv \"$commit:$path\"]\n+\t\t} else {\n+\t\t\tset fd [git_read cat-file blob \"$commit:$path\"]\n+\t\t}\n \t}\n \tfconfigure $fd \\\n \t\t-blocking 0 \\\ndiff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl\nindex ec8c11e..c628750 100644\n--- a/git-gui/lib/diff.tcl\n+++ b/git-gui/lib/diff.tcl\n@@ -55,7 +55,7 @@ proc handle_empty_diff {} {\n \n \tset path $current_diff_path\n \tset s $file_states($path)\n-\tif {[lindex $s 0] ne {_M}} return\n+\tif {[lindex $s 0] ne {_M} || [has_textconv $path]} return\n \n \t# Prevent infinite rescan loops\n \tincr diff_empty_count\n@@ -280,6 +280,9 @@ proc start_show_diff {cont_info {add_opts {}}} {\n \t\t\tlappend cmd diff-files\n \t\t}\n \t}\n+\tif {![is_config_false gui.textconv] && [git-version >= 1.6.1]} {\n+\t\tlappend cmd --textconv\n+\t}\n \n \tif {[string match {160000 *} [lindex $s 2]]\n \t || [string match {160000 *} [lindex $s 3]]} {\ndiff --git a/git-gui/lib/option.tcl b/git-gui/lib/option.tcl\nindex d4c5e45..3807c8d 100644\n--- a/git-gui/lib/option.tcl\n+++ b/git-gui/lib/option.tcl\n@@ -148,6 +148,7 @@ proc do_options {} {\n \t\t{b gui.trustmtime {mc \"Trust File Modification Timestamps\"}}\n \t\t{b gui.pruneduringfetch {mc \"Prune Tracking Branches During Fetch\"}}\n \t\t{b gui.matchtrackingbranch {mc \"Match Tracking Branches\"}}\n+\t\t{b gui.textconv {mc \"Use Textconv For Diffs and Blames\"}}\n \t\t{b gui.fastcopyblame {mc \"Blame Copy Only On Changed Files\"}}\n \t\t{i-20..200 gui.copyblamethreshold {mc \"Minimum Letters To Blame Copy On\"}}\n \t\t{i-0..300 gui.blamehistoryctx {mc \"Blame History Context Radius (days)\"}}\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143081","messageId":"1275823848-7151-5-git-send-email-clement.poulain@ensimag.imag.fr","threadId":"24022","inReplyTo":"1275823848-7151-4-git-send-email-clement.poulain@ensimag.imag.fr","subject":"[RFC/PATCH 4/4] t/t8007: test textconv support for cat-file","fromName":"Clément Poulain","fromEmail":"clement.poulain@ensimag.imag.fr","sentAt":"2010-06-06T11:30:48Z","receivedAt":"2010-06-06T11:30:48Z","isPatch":true,"sender":{"key":"clement.poulain@ensimag.imag.fr","avatar":null},"body":"Test the correct functionning of textconv with cat-file and cat-file HEAD^ .\nTest the case when no driver is specified.\n\nSigned-off-by: Clément Poulain \nSigned-off-by: Diane Gasselin \nSigned-off-by: Axel Bonnet \n---\n t/t8007-cat-file-textconv.sh | 66 ++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 66 insertions(+), 0 deletions(-)\n create mode 100755 t/t8007-cat-file-textconv.sh\n\ndiff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh\nnew file mode 100755\nindex 0000000..0789912\n--- /dev/null\n+++ b/t/t8007-cat-file-textconv.sh\n@@ -0,0 +1,66 @@\n+#!/bin/sh\n+\n+test_description='git cat-file textconv support'\n+. ./test-lib.sh\n+\n+cat >helper <<'EOF'\n+#!/bin/sh\n+sed 's/^/converted: /' \"$@\"\n+EOF\n+chmod +x helper\n+\n+test_expect_success 'setup ' '\n+\techo test >one.bin &&\n+\tgit add . &&\n+\tGIT_AUTHOR_NAME=Number1 git commit -a -m First --date=\"2010-01-01 18:00:00\" &&\n+\techo test version 2 >one.bin &&\n+\tGIT_AUTHOR_NAME=Number2 git commit -a -m Second --date=\"2010-01-01 20:00:00\"\n+'\n+\n+cat >expected <result &&\n+\ttest_cmp expected result\n+'\n+\n+test_expect_success 'setup textconv filters' '\n+\techo \"*.bin diff=test\" >.gitattributes &&\n+\tgit config diff.test.textconv ./helper &&\n+\tgit config diff.test.cachetextconv false\n+'\n+\n+test_expect_success 'cat-file without --textconv' '\n+\tgit cat-file blob :one.bin >result &&\n+\ttest_cmp expected result\n+'\n+\n+cat >expected <result &&\n+\ttest_cmp expected result\n+'\n+\n+cat >expected <result &&\n+\ttest_cmp expected result\n+'\n+\n+cat >expected <result &&\n+\ttest_cmp expected result\n+'\n+test_done\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143086","messageId":"vpqpr045f46.fsf@bauges.imag.fr","threadId":"24022","inReplyTo":"1275823848-7151-2-git-send-email-clement.poulain@ensimag.imag.fr","subject":"Re: [RFC/PATCH 1/4] sha1_name: creating context cache","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-06-06T15:56:41Z","receivedAt":"2010-06-06T15:56:41Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Clément Poulain writes:\n\n> 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\n\nPlease, wrap your message to make it fit in 80 characters (M-q under\nEmacs can help). This applies to the cover email too, but is\nparticularly important for this one, since it'll become the commit\nmessage if the patch is accepted.\n\n> diff --git a/cache.h b/cache.h\n> index 0f4263c..26a1faf 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -730,6 +730,14 @@ static inline unsigned int hexval(unsigned char c)\n> #define MINIMUM_ABBREV 4\n> #define DEFAULT_ABBREV 7\n> \n> +struct object_resolve_context {\n> +\tunsigned char tree[20];\n> +\tchar path[PATH_MAX];\n> +\tunsigned mode;\n> +};\n> +extern struct object_resolve_context object_context;\n\nIs it really a good idea to make this a global variable? As I\nunderstand it, the semantics of this variable is that it contains\ninformation on the last sha1 name parsed (BTW, you probably want to\nadd a comment here explaining that). Wouldn't it be more robust to\nhave this value returned by the function doing the parsing?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"143117","messageId":"20100606221557.GF6993@coredump.intra.peff.net","threadId":"24022","inReplyTo":"vpqpr045f46.fsf@bauges.imag.fr","subject":"Re: [RFC/PATCH 1/4] sha1_name: creating context cache","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-06T22:15:57Z","receivedAt":"2010-06-06T22:15:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 06, 2010 at 05:56:41PM +0200, Matthieu Moy wrote:\n\n> > +struct object_resolve_context {\n> > +\tunsigned char tree[20];\n> > +\tchar path[PATH_MAX];\n> > +\tunsigned mode;\n> > +};\n> > +extern struct object_resolve_context object_context;\n> \n> Is it really a good idea to make this a global variable? As I\n> understand it, the semantics of this variable is that it contains\n> information on the last sha1 name parsed (BTW, you probably want to\n> add a comment here explaining that). Wouldn't it be more robust to\n> have this value returned by the function doing the parsing?\n\nThis is straight from my earlier patch, but I think I said in that\nthread that it could use some cleaning up. You added the 'mode' field,\nbut it probably makes sense to then get rid of the distinction between\nget_sha1 and get_sha1_with_mode, and just have:\n\n get_sha1_with_context(const char *name, unsigned char *sha1, struct\n object_resolve_context *orc) {\n /* basically get_sha1_with_mode, but set mode in orc */\n }\n get_sha1(const char *name, unsigned char *sha1) {\n struct object_resolve_context orc; /* to be thrown away */\n return get_sha1_with_context(name, sha1, &orc);\n }\n\n-Peff\n"},{"id":"143147","messageId":"AANLkTimLzfmF060d_8mNjrXwTV72A9KudtpEeJUkMWJt@mail.gmail.com","threadId":"24022","inReplyTo":"20100606221557.GF6993@coredump.intra.peff.net","subject":"Re: [RFC/PATCH 1/4] sha1_name: creating context cache","fromName":"Clément Poulain","fromEmail":"clement.poulain@ensimag.imag.fr","sentAt":"2010-06-07T08:34:01Z","receivedAt":"2010-06-07T08:34:01Z","isPatch":true,"sender":{"key":"clement.poulain@ensimag.imag.fr","avatar":null},"body":"2010/6/7 Jeff King :\n> On Sun, Jun 06, 2010 at 05:56:41PM +0200, Matthieu Moy wrote:\n>\n>> > +struct object_resolve_context {\n>> > +   unsigned char tree[20];\n>> > +   char path[PATH_MAX];\n>> > +   unsigned mode;\n>> > +};\n>> > +extern struct object_resolve_context object_context;\n>>\n>> Is it really a good idea to make this a global variable? As I\n>> understand it, the semantics of this variable is that it contains\n>> information on the last sha1 name parsed (BTW, you probably want to\n>> add a comment here explaining that). Wouldn't it be more robust to\n>> have this value returned by the function doing the parsing?\n>\n> This is straight from my earlier patch, but I think I said in that\n> thread that it could use some cleaning up. You added the 'mode' field,\n> but it probably makes sense to then get rid of the distinction between\n> get_sha1 and get_sha1_with_mode, and just have:\n>\n>  get_sha1_with_context(const char *name, unsigned char *sha1, struct\n>                        object_resolve_context *orc) {\n>     /* basically get_sha1_with_mode, but set mode in orc */\n>  }\n>  get_sha1(const char *name, unsigned char *sha1) {\n>         struct object_resolve_context orc; /* to be thrown away */\n>         return get_sha1_with_context(name, sha1, &orc);\n>  }\n>\n> -Peff\n>\n\nYes, we've already thought about it (you've said it, Jeff ;-) ), and we have\nactually changed it with something very close to what you say.\nWe'll soon propose a v2 for this part of the patch.\n\nThank you\n"}]}