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

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

[Clément Poulain]

This patch adds support of textconv to git-gui blame.

It is based on our previous work which adds textconv support to blame:
http://mid.gmane.org/1275921713-3277-1-git-send-email-axel.bonnet@ensimag.imag.fr
It also uses a git-gui patch done by Clemens Buchacher which adds textconv
support to git-gui diff: http://mid.gmane.org/20100415193944.GA5848@localhost.

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.
The first part of this patch adds get_sha1_with_context() in order to know 
the pathname of the concerned blob, as textconv needs it to work.

Clément Poulain (4):
  sha1_name : creation of get_sha1_with_context
  cat_file : add textconv support
  git gui blame : add textconv support
  test textconv support for cat-file

 builtin/blame.c              |    8 ++--
 builtin/cat-file.c           |   32 +++++++++++++++++--
 cache.h                      |   11 ++++++
 git-gui/git-gui.sh           |   28 ++++++++++++++++-
 git-gui/lib/blame.tcl        |   21 +++++++++++-
 git-gui/lib/diff.tcl         |    5 ++-
 git-gui/lib/option.tcl       |    1 +
 sha1_name.c                  |   30 +++++++++++++++---
 t/t8007-cat-file-textconv.sh |   70 ++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 190 insertions(+), 16 deletions(-)
 create mode 100755 t/t8007-cat-file-textconv.sh

[PATCH v2 1/4] sha1_name: add get_sha1_with_context()

[Clément Poulain]

Textconv is defined by the diff driver, which is associated with a pathname,
not a blob. This fonction permits to know the context for the sha1 you're
looking for, especially his pathname

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> 
---
 cache.h     |   11 +++++++++++
 sha1_name.c |   30 +++++++++++++++++++++++++-----
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index 0f4263c..43a8c10 100644
--- a/cache.h
+++ b/cache.h
@@ -730,12 +730,23 @@ static inline unsigned int hexval(unsigned char c)
 #define MINIMUM_ABBREV 4
 #define DEFAULT_ABBREV 7
 
+struct object_context {
+	unsigned char tree[20];
+	char path[PATH_MAX];
+	unsigned mode;
+};
+#define OBJECT_CONTEXT_INIT  { 0, 0, 0 }
+
 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)
 {
 	return get_sha1_with_mode_1(str, sha1, mode, 1, NULL);
 }
+static inline int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc)
+{
+	return get_sha1_with_context_1(str, sha1, orc, 1, NULL);
+}
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref(const char *filename, unsigned char *sha1);
diff --git a/sha1_name.c b/sha1_name.c
index bf92417..02358f9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -933,8 +933,8 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
  */
 int get_sha1(const char *name, unsigned char *sha1)
 {
-	unsigned unused;
-	return get_sha1_with_mode(name, sha1, &unused);
+	struct object_context unused;
+	return get_sha1_with_context(name, sha1, &unused);
 }
 
 /* Must be called only when object_name:filename doesn't exist. */
@@ -1032,11 +1032,22 @@ static void diagnose_invalid_index_path(int stage,
 
 int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int gently, const char *prefix)
 {
+	struct object_context orc;
+	int ret;
+	ret = get_sha1_with_context_1(name, sha1, &orc, gently, prefix, NULL);
+	*mode = orc.mode;
+	return ret;
+}
+
+int get_sha1_with_context_1(const char *name, unsigned char *sha1,
+			    struct object_context *orc,
+			    int gently, const char *prefix)
+{
 	int ret, bracket_depth;
 	int namelen = strlen(name);
 	const char *cp;
 
-	*mode = S_IFINVALID;
+	orc->mode = S_IFINVALID;
 	ret = get_sha1_1(name, namelen, sha1);
 	if (!ret)
 		return ret;
@@ -1059,6 +1070,11 @@ int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,
 			cp = name + 3;
 		}
 		namelen = namelen - (cp - name);
+
+		strncpy(orc->path, cp,
+			sizeof(orc->path));
+		orc->path[sizeof(orc->path)] = '\0';
+
 		if (!active_cache)
 			read_cache();
 		pos = cache_name_pos(cp, namelen);
@@ -1071,7 +1087,6 @@ int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,
 				break;
 			if (ce_stage(ce) == stage) {
 				hashcpy(sha1, ce->sha1);
-				*mode = ce->ce_mode;
 				return 0;
 			}
 			pos++;
@@ -1098,12 +1113,17 @@ int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,
 		}
 		if (!get_sha1_1(name, cp-name, tree_sha1)) {
 			const char *filename = cp+1;
-			ret = get_tree_entry(tree_sha1, filename, sha1, mode);
+			ret = get_tree_entry(tree_sha1, filename, sha1, &orc->mode);
 			if (!gently) {
 				diagnose_invalid_sha1_path(prefix, filename,
 							   tree_sha1, object_name);
 				free(object_name);
 			}
+			hashcpy(orc->tree, tree_sha1);
+			strncpy(orc->path, filename,
+				sizeof(orc->path));
+			orc->path[sizeof(orc->path)] = '\0';
+
 			return ret;
 		} else {
 			if (!gently)
-- 
1.7.1.202.g79415.dirty

[PATCH v2 2/4] textconv: support for cat_file

[Clément Poulain]

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.
As files drivers need to be initialized, a new config is required in addition
to git_default_config. Therefore git_cat_file_config() is introduced

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 |   32 +++++++++++++++++++++++++++++---
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index f831e3a..64605f5 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,
-			   char **buf,
-			   size_t *buf_size)
+int textconv_object(const char *path,
+		    const unsigned char *sha1,
+		    char **buf,
+		    size_t *buf_size)
 {
 	struct diff_filespec *df;
 	struct userdiff_driver *textconv;
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index a933eaa..1457340 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
@@ -86,8 +87,9 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 	enum object_type type;
 	void *buf;
 	unsigned long size;
+	struct object_context obj_context = OBJECT_CONTEXT_INIT;
 
-	if (get_sha1(obj_name, sha1))
+	if (get_sha1_with_context(obj_name, sha1, &obj_context))
 		die("Not a valid object name %s", obj_name);
 
 	buf = NULL;
@@ -132,6 +134,17 @@ 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 (!obj_context.path)
+			die("git cat-file --textconv %s: <object> must be <sha1:path>",
+			    obj_name);
+
+		if(!textconv_object(obj_context.path, sha1, &buf, (size_t *) &size))
+			die("git cat-file --textconv: unable to run textconv on %s",
+			    obj_name);
+		break;
+
 	case 0:
 		buf = read_object_with_reference(sha1, exp_type, &size, NULL);
 		break;
@@ -201,11 +214,22 @@ 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
 };
 
+static int git_cat_file_config(const char *var, const char *value, void *cb)
+{
+	switch (userdiff_config(var, value)) {
+		case 0: break;
+		case -1: return -1;
+		default: return 0;
+	}
+
+	return git_default_config(var, value, cb);
+}
+
 int cmd_cat_file(int argc, const char **argv, const char *prefix)
 {
 	int opt = 0, batch = 0;
@@ -218,6 +242,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),
@@ -227,7 +253,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	git_config(git_default_config, NULL);
+	git_config(git_cat_file_config, NULL);
 
 	if (argc != 3 && argc != 2)
 		usage_with_options(cat_file_usage, options);
-- 
1.7.1.202.g79415.dirty

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

[Clément Poulain]

Create a checkbox "Use Textconv For Diffs and Blame" in git-gui options.
If checked and if the driver for the concerned file exists, 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  |   21 +++++++++++++++++++--
 git-gui/lib/diff.tcl   |    5 ++++-
 git-gui/lib/option.tcl |    1 +
 4 files changed, 51 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..b0f2f23 100644
--- a/git-gui/lib/blame.tcl
+++ b/git-gui/lib/blame.tcl
@@ -449,11 +449,28 @@ method _load {jump} {
 
 	$status show [mc "Reading %s..." "$commit:[escape_path $path]"]
 	$w_path conf -text [escape_path $path]
+
+	set do_textconv 0
+	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 do_textconv 1
+		}
+	}
 	if {$commit eq {}} {
-		set fd [open $path r]
+		if {$do_textconv ne 0} {
+			set fd [open "|$textconv $path" r]
+		} else {
+			set fd [open $path r]
+		}
 		fconfigure $fd -eofchar {}
 	} else {
-		set fd [git_read cat-file blob "$commit:$path"]
+		if {$do_textconv ne 0} {
+			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.7.1.202.g79415.dirty

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

[Clément Poulain]

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 |   70 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 70 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..38ac05e
--- /dev/null
+++ b/t/t8007-cat-file-textconv.sh
@@ -0,0 +1,70 @@
+#!/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
+fatal: git cat-file --textconv: unable to run textconv on :one.bin
+EOF
+
+test_expect_success 'no filter specified' '
+	git cat-file --textconv :one.bin 2>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
+'
+
+cat >expected <<EOF
+test version 2
+EOF
+
+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.7.1.202.g79415.dirty

Re: [PATCH v2 2/4] textconv: support for cat_file

[Matthieu Moy]

Clément Poulain <clement.poulain@ensimag.imag.fr> writes:

> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -9,6 +9,7 @@
> +	struct object_context obj_context = OBJECT_CONTEXT_INIT;
>  
> -	if (get_sha1(obj_name, sha1))
> +	if (get_sha1_with_context(obj_name, sha1, &obj_context))

Do you really need to initialize obj_context here? I'd say the
semantics of get_sha1_with_context should be "give me a pointer to an
object_context, and I'll fill it in with the object context, whatever
be its initial value", just like

int i;
scanf("%d", &i);

doesn't require i to be initialized.

> +	case 'c':
> +		if (!obj_context.path)
> +			die("git cat-file --textconv %s: <object> must be <sha1:path>",
> +			    obj_name);

obj_context.path is an array contained in the struct. It is always
non-null. Just tried:

$ ./git cat-file --textconv 99f036302a7e6d884369d1d3f4ce428e437cbccd | head
fatal: git cat-file --textconv: unable to run textconv on 99f036302a7e6d884369d1d3f4ce428e437cbccd

you want to check that obj_context.path contains an empty string.

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

Re: [PATCH v2 1/4] sha1_name: add get_sha1_with_context()

[Matthieu Moy]

This patch produces uncompilable code for me:

cc1: warnings being treated as errors
In file included from builtin.h:6,
                 from fast-import.c:147:
cache.h: In function ‘get_sha1_with_context’:
cache.h:748: error: implicit declaration of function ‘get_sha1_with_context_1’

Forgot to add get_sha1_with_context_1 to cache.h?

Clément Poulain <clement.poulain@ensimag.imag.fr> writes:

> +struct object_context {
> +	unsigned char tree[20];
> +	char path[PATH_MAX];
> +	unsigned mode;
> +};
> +#define OBJECT_CONTEXT_INIT  { 0, 0, 0 }
> +

I'm not an expert in struct initializers, but after doing experiments
with GCC, this raises a warning

builtin/cat-file.c:90: error: missing braces around initializer
builtin/cat-file.c:90: error: (near initialization for ‘obj_context.tree’)

and the behavior is to flatten the arrays contained inside the
structure. So, your OBJECT_CONTEXT_INIT initializes the 3 first bytes
of tree to 0, and leaves other fields uninitialized.

You probably want something like this instead if you want to
initialize the whole struct:

{{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
  0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, "", 0}

> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -933,8 +933,8 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
>   */
>  int get_sha1(const char *name, unsigned char *sha1)
>  {
> -	unsigned unused;
> -	return get_sha1_with_mode(name, sha1, &unused);
> +	struct object_context unused;
> +	return get_sha1_with_context(name, sha1, &unused);
>  }

This changes doesn't seem harmful, but it doesn't seem useful to me
either: get_sha1_with_mode still exists, right?

>  int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int gently, const char *prefix)
>  {
> +	struct object_context orc;

What does orc stand for? I understand "oc" for "object context", but
I'm curious about the r ;-).

> +		orc->path[sizeof(orc->path)] = '\0';
> +

Isn't this an off-by-one? The last element of an array of size N is
array[N-1] ...

> +			orc->path[sizeof(orc->path)] = '\0';

Same here.

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

Re: [PATCH v2 1/4] sha1_name: add get_sha1_with_context()

[Clément Poulain]

Le 8 juin 2010 19:57, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
> This patch produces uncompilable code for me:
>
> cc1: warnings being treated as errors
> In file included from builtin.h:6,
>                 from fast-import.c:147:
> cache.h: In function ‘get_sha1_with_context’:
> cache.h:748: error: implicit declaration of function ‘get_sha1_with_context_1’
>
> Forgot to add get_sha1_with_context_1 to cache.h?

Uh, we compiled it almost ten times on both our pc and ensibm (our
school server), whithout any problems. Seems that we need to check our
compilation configurations.

> I'm not an expert in struct initializers, but after doing experiments
> with GCC, this raises a warning
>
> builtin/cat-file.c:90: error: missing braces around initializer
> builtin/cat-file.c:90: error: (near initialization for ‘obj_context.tree’)
>
> and the behavior is to flatten the arrays contained inside the
> structure. So, your OBJECT_CONTEXT_INIT initializes the 3 first bytes
> of tree to 0, and leaves other fields uninitialized.
>
> You probably want something like this instead if you want to
> initialize the whole struct:
>
> {{0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
>  0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, "", 0}

As you pointed out in your second answer, initialization is maybe no
required, we have to check it tomorrow.
Otherwise, an easy way to do it can be something like :
void object_context_init(struct object_context *oc)
{
	memset(oc, 0, sizeof(*oc));
}

>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -933,8 +933,8 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
>>   */
>>  int get_sha1(const char *name, unsigned char *sha1)
>>  {
>> -     unsigned unused;
>> -     return get_sha1_with_mode(name, sha1, &unused);
>> +     struct object_context unused;
>> +     return get_sha1_with_context(name, sha1, &unused);
>>  }
>
> This changes doesn't seem harmful, but it doesn't seem useful to me
> either: get_sha1_with_mode still exists, right?

Right. But the aim was to skip one function call (see the call-stack below)
_with_mode => _with_mode_1 => _with_context_1
whereas:
 _with_context => _with_context_1

> What does orc stand for? I understand "oc" for "object context", but
> I'm curious about the r ;-).

"orc" was for "object resolve context". This is an artifact of our
previous version. We'll change it, it won't bother you no more ;-)

>> +             orc->path[sizeof(orc->path)] = '\0';
>> +
>
> Isn't this an off-by-one? The last element of an array of size N is
> array[N-1] ...
>
>> +                     orc->path[sizeof(orc->path)] = '\0';
>
> Same here.

That's true. Stupid error, we copied this line without checking it.

Re: [PATCH v2 1/4] sha1_name: add get_sha1_with_context()

[Jeff King]

On Wed, Jun 09, 2010 at 12:30:31AM +0200, Clément Poulain wrote:

> Le 8 juin 2010 19:57, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
> > This patch produces uncompilable code for me:
> >
> > cc1: warnings being treated as errors
> > In file included from builtin.h:6,
> >                 from fast-import.c:147:
> > cache.h: In function ‘get_sha1_with_context’:
> > cache.h:748: error: implicit declaration of function ‘get_sha1_with_context_1’
> >
> > Forgot to add get_sha1_with_context_1 to cache.h?
> 
> Uh, we compiled it almost ten times on both our pc and ensibm (our
> school server), whithout any problems. Seems that we need to check our
> compilation configurations.

Note the "warnings being treated as errors". Matthieu is compiling with
-Werror (and presumably -Wall). We strive to be warning-free in git, and
I think many of the developers compile with "-Wall -Werror".

> Right. But the aim was to skip one function call (see the call-stack below)
> _with_mode => _with_mode_1 => _with_context_1
> whereas:
>  _with_context => _with_context_1

Perhaps that was your goal, but my goal when I suggested it was to give
us a cleaner codebase. We don't want a proliferation of get_sha1_with_*
functions. Introducing _with_context instead of _with_tree or _with_path
was meant not to make things worse. But collapsing _with_mode into
_with_context actively makes things better.

> >> +                     orc->path[sizeof(orc->path)] = '\0';
> >
> > Same here.
> 
> That's true. Stupid error, we copied this line without checking it.

Oops, that's my fault for introducing the bug in the first place (I had
originally had an snprintf and changed it to strncpy at the last
minute). :)

-Peff

Re: [PATCH v2 1/4] sha1_name: add get_sha1_with_context()

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> Note the "warnings being treated as errors". Matthieu is compiling with
> -Werror (and presumably -Wall). We strive to be warning-free in git, and
> I think many of the developers compile with "-Wall -Werror".

Right. Try this:

echo 'CFLAGS = -g -Wall -Werror' > config.mak
make

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