threads / patch / 26826

patch, 2 partsFix issues found by gcc 4.6.0

Subject: [PATCH 0/2] Fix issues found by gcc 4.6.0

## tl;dr

11 messages between Mar 22, 2011 and Mar 23, 2011. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Johannes Schindelin· Mar 22, 2011, 12:49 UTC · lore

In general, our sources are quite clean. However, a few variables were unused. Previous gcc version did not find them because they were assigned values, but those values were never used nontheless.

The tricky question is whether 2/2 really fixes an issue or not. Unfortunately, my time is too limited to go into detail, but I figured it'd be better to raise the issue than to remain silent.

Johannes Schindelin (2):
  Remove unused variables
  Actually use retval
 builtin/fsck.c       |    3 +--
 builtin/remote-ext.c |    4 ----
 diff.c               |    3 +--
 merge-recursive.c    |    4 ----
 reachable.c          |    5 -----
 test-subprocess.c    |    3 +--
 transport-helper.c   |    3 +--
 tree-diff.c          |    2 +-
 8 files changed, 5 insertions(+), 22 deletions(-)
-- 
1.7.4.msysgit.0.167.ge91fd
Johannes Schindelin· Mar 22, 2011, 12:50 UTC · re: Johannes Schindelin · lore

[PATCH 1/2] Remove unused variables

Noticed by gcc 4.6.0.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/fsck.c       |    3 +--
 builtin/remote-ext.c |    4 ----
 diff.c               |    3 +--
 merge-recursive.c    |    4 ----
 reachable.c          |    5 -----
 test-subprocess.c    |    3 +--
 transport-helper.c   |    3 +--
 7 files changed, 4 insertions(+), 21 deletions(-)
Show changes to 7 files +4 −21

builtin/fsck.c, builtin/remote-ext.c, diff.c, merge-recursive.c, reachable.c, test-subprocess.c, transport-helper.c

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 795aba0..5ae0366 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -140,11 +140,10 @@ static int traverse_reachable(void)
 	int result = 0;
 	while (pending.nr) {
 		struct object_array_entry *entry;
-		struct object *obj, *parent;
+		struct object *obj;
 
 		entry = pending.objects + --pending.nr;
 		obj = entry->item;
-		parent = (struct object *) entry->name;
 		result |= traverse_one_object(obj);
 	}
 	return !!result;
diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index ea71977..2343030 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -30,16 +30,12 @@ static char *strip_escapes(const char *str, const char *service,
 	size_t rpos = 0;
 	int escape = 0;
 	char special = 0;
-	size_t pslen = 0;
-	size_t pSlen = 0;
 	size_t psoff = 0;
 	struct strbuf ret = STRBUF_INIT;
 
 	/* Calculate prefix length for \s and lengths for \s and \S */
 	if (!strncmp(service, "git-", 4))
 		psoff = 4;
-	pSlen = strlen(service);
-	pslen = pSlen - psoff;
 
 	/* Pass the service to command. */
 	setenv("GIT_EXT_SERVICE", service, 1);
diff --git a/diff.c b/diff.c
index 42a107c..18f8e99 100644
--- a/diff.c
+++ b/diff.c
@@ -1242,7 +1242,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	uintmax_t max_change = 0, max_len = 0;
 	int total_files = data->nr;
 	int width, name_width;
-	const char *reset, *set, *add_c, *del_c;
+	const char *reset, *add_c, *del_c;
 	const char *line_prefix = "";
 	struct strbuf *msg = NULL;
 
@@ -1269,7 +1269,6 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 
 	/* Find the longest filename and max number of changes */
 	reset = diff_get_color_opt(options, DIFF_RESET);
-	set   = diff_get_color_opt(options, DIFF_PLAIN);
 	add_c = diff_get_color_opt(options, DIFF_FILE_NEW);
 	del_c = diff_get_color_opt(options, DIFF_FILE_OLD);
 
diff --git a/merge-recursive.c b/merge-recursive.c
index c8343d7..7c12673 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -357,7 +357,6 @@ static void make_room_for_directories_of_df_conflicts(struct merge_options *o,
 	 */
 	const char *last_file = NULL;
 	int last_len = 0;
-	struct stage_data *last_e;
 	int i;
 
 	/*
@@ -394,7 +393,6 @@ static void make_room_for_directories_of_df_conflicts(struct merge_options *o,
 		if (S_ISREG(e->stages[2].mode) || S_ISLNK(e->stages[2].mode)) {
 			last_file = path;
 			last_len = len;
-			last_e = e;
 		} else {
 			last_file = NULL;
 		}
@@ -969,7 +967,6 @@ static int process_renames(struct merge_options *o,
 	}
 
 	for (i = 0, j = 0; i < a_renames->nr || j < b_renames->nr;) {
-		char *src;
 		struct string_list *renames1, *renames2Dst;
 		struct rename *ren1 = NULL, *ren2 = NULL;
 		const char *branch1, *branch2;
@@ -1004,7 +1001,6 @@ static int process_renames(struct merge_options *o,
 			ren2 = ren1;
 			ren1 = tmp;
 		}
-		src = ren1->pair->one->path;
 
 		ren1->dst_entry->processed = 1;
 		ren1->src_entry->processed = 1;
diff --git a/reachable.c b/reachable.c
index a03fabf..3fc6b1d 100644
--- a/reachable.c
+++ b/reachable.c
@@ -70,16 +70,11 @@ static void process_tree(struct tree *tree,
 static void process_tag(struct tag *tag, struct object_array *p, const char *name)
 {
 	struct object *obj = &tag->object;
-	struct name_path me;
 
 	if (obj->flags & SEEN)
 		return;
 	obj->flags |= SEEN;
 
-	me.up = NULL;
-	me.elem = "tag:/";
-	me.elem_len = 5;
-
 	if (parse_tag(tag) < 0)
 		die("bad tag object %s", sha1_to_hex(obj->sha1));
 	if (tag->tagged)
diff --git a/test-subprocess.c b/test-subprocess.c
index 667d3e5..8926bc5 100644
--- a/test-subprocess.c
+++ b/test-subprocess.c
@@ -3,11 +3,10 @@
 
 int main(int argc, char **argv)
 {
-	const char *prefix;
 	struct child_process cp;
 	int nogit = 0;
 
-	prefix = setup_git_directory_gently(&nogit);
+	setup_git_directory_gently(&nogit);
 	if (nogit)
 		die("No git repo found");
 	if (!strcmp(argv[1], "--setup-work-tree")) {
diff --git a/transport-helper.c b/transport-helper.c
index 0c5b1bd..a4f40fb 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -561,10 +561,9 @@ static int push_refs_with_push(struct transport *transport,
 	int mirror = flags & TRANSPORT_PUSH_MIRROR;
 	struct helper_data *data = transport->data;
 	struct strbuf buf = STRBUF_INIT;
-	struct child_process *helper;
 	struct ref *ref;
 
-	helper = get_helper(transport);
+	get_helper(transport);
 	if (!data->push)
 		return 1;
 

-- 
1.7.4.msysgit.0.167.ge91fd
Junio C Hamano· Mar 22, 2011, 18:15 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/2] Remove unused variables

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Noticed by gcc 4.6.0.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
Thanks.

I'll cherry pick only some bits, not because the changes in this patch are wrong, but because I know some areas are in flux and I don't want to deal with merge conflicts of the "one side fixed a small bug, the other side majorly rewrote it and made the bug irrelevant" kind.

Johannes Schindelin· Mar 22, 2011, 18:42 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] Remove unused variables

Hi,
On Tue, 22 Mar 2011, Junio C Hamano wrote:
Show 13 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Noticed by gcc 4.6.0.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> 
> Thanks.
> 
> I'll cherry pick only some bits, not because the changes in this patch 
> are wrong, but because I know some areas are in flux and I don't want to 
> deal with merge conflicts of the "one side fixed a small bug, the other 
> side majorly rewrote it and made the bug irrelevant" kind.

Understandable. I will have to deal with the merge conflicts, though, as I am stuck with 4.6.0 (and self-imposed -Werror, as you might remember) for the 64-bit Windows build. So I will keep the remains of the patch after rebasing in our 'devel' branch.

Ciao, Johannes

Jonathan Nieder· Mar 22, 2011, 19:43 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/2] Remove unused variables

Hi,
These all look good.  Some notes:
Johannes Schindelin wrote:
Show 8 quoted lines
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -140,11 +140,10 @@ static int traverse_reachable(void)
>  	int result = 0;
>  	while (pending.nr) {
>  		struct object_array_entry *entry;
> -		struct object *obj, *parent;
> +		struct object *obj;

Cleaning up after v1.7.4.1~1^2~1 (fsck: drop unused parameter from traverse_one_object(), 2011-01-26). Sensible.

Show 8 quoted lines
> --- a/builtin/remote-ext.c
> +++ b/builtin/remote-ext.c
> @@ -30,16 +30,12 @@ static char *strip_escapes(const char *str, const char *service,
>  	size_t rpos = 0;
>  	int escape = 0;
>  	char special = 0;
> -	size_t pslen = 0;
> -	size_t pSlen = 0;
These were never used; sorry to have missed it before.
Show 8 quoted lines
> --- a/diff.c
> +++ b/diff.c
> @@ -1242,7 +1242,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>  	uintmax_t max_change = 0, max_len = 0;
>  	int total_files = data->nr;
>  	int width, name_width;
> -	const char *reset, *set, *add_c, *del_c;
> +	const char *reset, *add_c, *del_c;

After v1.6.4-rc0~159^2 (diff: do not color --stat output like patch context, 2009-04-25), the "patch context" color is not used in diffstats.

Show 7 quoted lines
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -357,7 +357,6 @@ static void make_room_for_directories_of_df_conflicts(struct merge_options *o,
>  	 */
>  	const char *last_file = NULL;
>  	int last_len = 0;
> -	struct stage_data *last_e;

Yep, was never used (and while the code to set it is correct, it is better to remove it than to let it bitrot).

Show 5 quoted lines
> @@ -969,7 +967,6 @@ static int process_renames(struct merge_options *o,
>  	}
>  
>  	for (i = 0, j = 0; i < a_renames->nr || j < b_renames->nr;) {
> -		char *src;
Likewise.
Show 19 quoted lines
> --- a/reachable.c
> +++ b/reachable.c
> @@ -70,16 +70,11 @@ static void process_tree(struct tree *tree,
>  static void process_tag(struct tag *tag, struct object_array *p, const char *name)
>  {
>  	struct object *obj = &tag->object;
> -	struct name_path me;
>  
>  	if (obj->flags & SEEN)
>  		return;
>  	obj->flags |= SEEN;
>  
> -	me.up = NULL;
> -	me.elem = "tag:/";
> -	me.elem_len = 5;
> -
>  	if (parse_tag(tag) < 0)
>  		die("bad tag object %s", sha1_to_hex(obj->sha1));
>  	if (tag->tagged)

Does this code make sense at all? It peels a tag once in order to mark the object it refers to with "add_object", which adds the object to the array referred to by "p", that nobody actually uses. Maybe this is a remnant of an earlier implementation for "git log --objects"?

Peeling of tags is already taken care of by revision::handle_commit, so I think the function could be stripped down to

 static void process_tag(struct tag *tag, struct object_array *p, const char *name)
 {
	struct object *obj = &tag->object;
 
	if (obj->flags & SEEN)
		return;
	obj->flags |= SEEN;
 }
which seems to work okay still.
> --- a/test-subprocess.c
> +++ b/test-subprocess.c
> @@ -3,11 +3,10 @@
[...]
> -	prefix = setup_git_directory_gently(&nogit);
> +	setup_git_directory_gently(&nogit);

It might be more idiomatic to use setup_git_directory, but this is better because conservative.

Show 7 quoted lines
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -561,10 +561,9 @@ static int push_refs_with_push(struct transport *transport,
>  	int mirror = flags & TRANSPORT_PUSH_MIRROR;
>  	struct helper_data *data = transport->data;
>  	struct strbuf buf = STRBUF_INIT;
> -	struct child_process *helper;

Cleaning up after v1.7.0-rc0~62^2~12 (Add remote helper debug mode, 2009-12-09). The remote helper process is accessed through transport->data->helper.

Thanks and hope that helps. Jonathan

Johannes Schindelin· Mar 22, 2011, 12:50 UTC · re: Johannes Schindelin · lore

[PATCH 2/2] Actually use retval

This is most likely a bug. Nocited by gcc 4.6.0.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	Alternatively, retval's declaration and assignment can go.
 tree-diff.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to tree-diff.c +1 −1
diff --git a/tree-diff.c b/tree-diff.c
index 3954281..9e2dc70 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -58,7 +58,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,
 		opt->change(opt, mode1, mode2, sha1, sha2, base->buf, 0, 0);
 	}
 	strbuf_setlen(base, old_baselen);
-	return 0;
+	return retval;
 }
 
 /* A whole sub-tree went away or appeared */

-- 
1.7.4.msysgit.0.167.ge91fd
Junio C Hamano· Mar 22, 2011, 18:10 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/2] Actually use retval

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> This is most likely a bug. Nocited by gcc 4.6.0.

While I don't doubt gcc 4.6.0 found the retval assigned is not used, I think you misunderstood the value returned from this function.

The caller uses the return value to decide if an entry from t1 (and not from t2) was consumed, if an entry each from both t1 and t2 were consumed, or an entry from t2 (and not from t1) was consumed. It doesn't change the fact that the entry at the beginning of each tree we looked at in this function at that point shared the same name and we consumed them, whatever the call to diff_tree_sha1() to run a recursive comparison between the trees found.

The likely fix would be to remove assignment to retval instead.
Johannes Schindelin· Mar 22, 2011, 18:20 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Actually use retval

Hi,
On Tue, 22 Mar 2011, Junio C Hamano wrote:
Show 6 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > This is most likely a bug. Nocited by gcc 4.6.0.
> 
> While I don't doubt gcc 4.6.0 found the retval assigned is not used, I 
> think you misunderstood the value returned from this function.

Nope. I did not misunderstand. I did not understand in the first place. That is what I described pretty explicitly in the cover letter.

Show 9 quoted lines
> The caller uses the return value to decide if an entry from t1 (and not 
> from t2) was consumed, if an entry each from both t1 and t2 were 
> consumed, or an entry from t2 (and not from t1) was consumed.  It 
> doesn't change the fact that the entry at the beginning of each tree we 
> looked at in this function at that point shared the same name and we 
> consumed them, whatever the call to diff_tree_sha1() to run a recursive 
> comparison between the trees found.
> 
> The likely fix would be to remove assignment to retval instead.
Thanks, that is the alternative I suggested after the dashdashdash.

Ciao, Johannes

Jonathan Nieder· Mar 23, 2011, 01:26 UTC · re: Junio C Hamano · lore

[PATCH 0/2] Re: [PATCH 2/2] Actually use retval

Junio C Hamano wrote:
> The likely fix would be to remove assignment to retval instead.
Like this, maybe?
Jonathan Nieder (2):
  tree-diff: make diff_tree() return void
  tree-diff: make diff_tree_sha1() return void
 diff.h      |    4 ++--
 revision.c  |    9 +++------
 tree-diff.c |   17 ++++++-----------
 3 files changed, 11 insertions(+), 19 deletions(-)
Jonathan Nieder· Mar 23, 2011, 01:28 UTC · re: Jonathan Nieder · lore

[PATCH 1/2] tree-diff: make diff_tree return void

diff_tree (which runs a diff between two trees) always returns 0 and dies on errors. Some day it may change to change it to return -1 on error but it will be easy to adjust callers then and until then the return value is just confusing.

This way at least callers are consistent in ignoring the return value and new readers don't have to wonder if diff_tree returns its diff result like "diff --exit-code" would.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 diff.h      |    2 +-
 revision.c  |    5 ++---
 tree-diff.c |   13 +++++--------
 3 files changed, 8 insertions(+), 12 deletions(-)
Show changes to 3 files +8 −12

diff.h, revision.c, tree-diff.c

diff --git a/diff.h b/diff.h
index 007a055..4fde7f3 100644
--- a/diff.h
+++ b/diff.h
@@ -163,7 +163,7 @@ extern const char mime_boundary_leader[];
 
 extern void diff_tree_setup_paths(const char **paths, struct diff_options *);
 extern void diff_tree_release_paths(struct diff_options *);
-extern int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
+extern void diff_tree(struct tree_desc *t1, struct tree_desc *t2,
 		     const char *base, struct diff_options *opt);
 extern int diff_tree_sha1(const unsigned char *old, const unsigned char *new,
 			  const char *base, struct diff_options *opt);
diff --git a/revision.c b/revision.c
index 86d2470..a6e78c9 100644
--- a/revision.c
+++ b/revision.c
@@ -337,7 +337,6 @@ static int rev_compare_tree(struct rev_info *revs, struct commit *parent, struct
 
 static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)
 {
-	int retval;
 	void *tree;
 	unsigned long size;
 	struct tree_desc empty, real;
@@ -354,10 +353,10 @@ static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)
 
 	tree_difference = REV_TREE_SAME;
 	DIFF_OPT_CLR(&revs->pruning, HAS_CHANGES);
-	retval = diff_tree(&empty, &real, "", &revs->pruning);
+	diff_tree(&empty, &real, "", &revs->pruning);
 	free(tree);
 
-	return retval >= 0 && (tree_difference == REV_TREE_SAME);
+	return tree_difference == REV_TREE_SAME;
 }
 
 static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
diff --git a/tree-diff.c b/tree-diff.c
index 3954281..d1a7ae9 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -138,7 +138,7 @@ static void skip_uninteresting(struct tree_desc *t, struct strbuf *base,
 	}
 }
 
-int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
+void diff_tree(struct tree_desc *t1, struct tree_desc *t2,
 	      const char *base_str, struct diff_options *opt)
 {
 	struct strbuf base;
@@ -190,7 +190,6 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
 	}
 
 	strbuf_release(&base);
-	return 0;
 }
 
 /*
@@ -285,7 +284,6 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha
 	void *tree1, *tree2;
 	struct tree_desc t1, t2;
 	unsigned long size1, size2;
-	int retval;
 
 	tree1 = read_object_with_reference(old, tree_type, &size1, NULL);
 	if (!tree1)
@@ -295,7 +293,7 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha
 		die("unable to read destination tree (%s)", sha1_to_hex(new));
 	init_tree_desc(&t1, tree1, size1);
 	init_tree_desc(&t2, tree2, size2);
-	retval = diff_tree(&t1, &t2, base, opt);
+	diff_tree(&t1, &t2, base, opt);
 	if (!*base && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename()) {
 		init_tree_desc(&t1, tree1, size1);
 		init_tree_desc(&t2, tree2, size2);
@@ -303,12 +301,11 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha
 	}
 	free(tree1);
 	free(tree2);
-	return retval;
+	return 0;
 }
 
 int diff_root_tree_sha1(const unsigned char *new, const char *base, struct diff_options *opt)
 {
-	int retval;
 	void *tree;
 	unsigned long size;
 	struct tree_desc empty, real;
@@ -319,9 +316,9 @@ int diff_root_tree_sha1(const unsigned char *new, const char *base, struct diff_
 	init_tree_desc(&real, tree, size);
 
 	init_tree_desc(&empty, "", 0);
-	retval = diff_tree(&empty, &real, base, opt);
+	diff_tree(&empty, &real, base, opt);
 	free(tree);
-	return retval;
+	return 0;
 }
 
 void diff_tree_release_paths(struct diff_options *opt)

-- 
1.7.4.1
Jonathan Nieder· Mar 23, 2011, 01:30 UTC · re: Jonathan Nieder · lore

[PATCH 2/2] tree-diff: make diff_tree_sha1 return void

diff_tree_sha1 does not return -1 on errors; it die()s. Now it would obviously be lovely to change that so long-running programs could call diff_tree_sha1 without fear, but until then, let's face the reality:

 - nobody has tested what would happen if it started returning
   instead of dying on failure;
 - the callers overwhelmingly ignore the return value.

Returning void also saves readers the trouble of checking documentation each time they use diff_tree_sha1 to see if its return value follows the GNU diff exit code convention.

So it's just more honest to return void.  Do so.

Noticed with gcc-4.6 -Wunused-but-set-variable (because one caller couldn't decide --- it saves the return value, but never does anything with it).

Noticed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 diff.h      |    2 +-
 revision.c  |    4 +---
 tree-diff.c |    6 ++----
 3 files changed, 4 insertions(+), 8 deletions(-)
Show changes to 3 files +4 −8

diff.h, revision.c, tree-diff.c

diff --git a/diff.h b/diff.h
index 4fde7f3..0a8a06c 100644
--- a/diff.h
+++ b/diff.h
@@ -165,7 +165,7 @@ extern void diff_tree_setup_paths(const char **paths, struct diff_options *);
 extern void diff_tree_release_paths(struct diff_options *);
 extern void diff_tree(struct tree_desc *t1, struct tree_desc *t2,
 		     const char *base, struct diff_options *opt);
-extern int diff_tree_sha1(const unsigned char *old, const unsigned char *new,
+extern void diff_tree_sha1(const unsigned char *old, const unsigned char *new,
 			  const char *base, struct diff_options *opt);
 extern int diff_root_tree_sha1(const unsigned char *new, const char *base,
                                struct diff_options *opt);
diff --git a/revision.c b/revision.c
index a6e78c9..0c810b4 100644
--- a/revision.c
+++ b/revision.c
@@ -329,9 +329,7 @@ static int rev_compare_tree(struct rev_info *revs, struct commit *parent, struct
 
 	tree_difference = REV_TREE_SAME;
 	DIFF_OPT_CLR(&revs->pruning, HAS_CHANGES);
-	if (diff_tree_sha1(t1->object.sha1, t2->object.sha1, "",
-			   &revs->pruning) < 0)
-		return REV_TREE_DIFFERENT;
+	diff_tree_sha1(t1->object.sha1, t2->object.sha1, "", &revs->pruning);
 	return tree_difference;
 }
 
diff --git a/tree-diff.c b/tree-diff.c
index d1a7ae9..03b8ca0 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -17,7 +17,6 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,
 	const unsigned char *sha1, *sha2;
 	int cmp, pathlen1, pathlen2;
 	int old_baselen = base->len;
-	int retval = 0;
 
 	sha1 = tree_entry_extract(t1, &path1, &mode1);
 	sha2 = tree_entry_extract(t2, &path2, &mode2);
@@ -53,7 +52,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,
 				    sha1, sha2, base->buf, 0, 0);
 		}
 		strbuf_addch(base, '/');
-		retval = diff_tree_sha1(sha1, sha2, base->buf, opt);
+		diff_tree_sha1(sha1, sha2, base->buf, opt);
 	} else {
 		opt->change(opt, mode1, mode2, sha1, sha2, base->buf, 0, 0);
 	}
@@ -279,7 +278,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
 	q->nr = 1;
 }
 
-int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const char *base, struct diff_options *opt)
+void diff_tree_sha1(const unsigned char *old, const unsigned char *new, const char *base, struct diff_options *opt)
 {
 	void *tree1, *tree2;
 	struct tree_desc t1, t2;
@@ -301,7 +300,6 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha
 	}
 	free(tree1);
 	free(tree2);
-	return 0;
 }
 
 int diff_root_tree_sha1(const unsigned char *new, const char *base, struct diff_options *opt)

-- 
1.7.4.1

← back to recent threads