Re: [PATCH 4/4] revision: convert to using diff_tree_sha1()

[Jeff King]

On Wed, Feb 05, 2014 at 08:57:12PM +0400, Kirill Smelkov wrote:

> Since diff_tree_sha1() can now accept empty trees via NULL sha1, we
> could just call it without manually reading trees into tree_desc and
> duplicating code.
> 
> Besides, that
> 
>   if (!tree)
>   return 0;
> 
> looked suspect - we were saying an invalid tree != empty tree, but maybe it is
> better to just say the tree is invalid here, which is what diff_tree_sha1()
> does for such case.

I think that is sensible. The assertion that "invalid != empty" is
probably sane, because we handle the empty tree as internal magic. But I
do not see any reason we should be hitting this code path regularly with
an invalid tree, short of repository corruption, so in practice I don't
think it matters.

This does introduce a die() where there was not one previously, and that
can make things harder to diagnose/debug in a corrupted repository. But
it looks like this is limited to the history-simplification code, and I
suspect that it is not commonly used in the case of corruption.

So I think the patch looks fine.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] revision: convert to using diff_tree_sha1()

[Kirill Smelkov]

Since diff_tree_sha1() can now accept empty trees via NULL sha1, we
could just call it without manually reading trees into tree_desc and
duplicating code.

Besides, that

if (!tree)
return 0;

looked suspect - we were saying an invalid tree != empty tree, but maybe it is
better to just say the tree is invalid here, which is what diff_tree_sha1()
does for such case.

Signed-off-by: Kirill Smelkov 
---
 revision.c | 12 +---
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/revision.c b/revision.c
index 082dae6..bd027bc 100644
--- a/revision.c
+++ b/revision.c
@@ -497,24 +497,14 @@ static int rev_compare_tree(struct rev_info *revs,
 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;
struct tree *t1 = commit->tree;
 
if (!t1)
return 0;
 
-   tree = read_object_with_reference(t1->object.sha1, tree_type, &size, 
NULL);
-   if (!tree)
-   return 0;
-   init_tree_desc(&real, tree, size);
-   init_tree_desc(&empty, "", 0);
-
tree_difference = REV_TREE_SAME;
DIFF_OPT_CLR(&revs->pruning, HAS_CHANGES);
-   retval = diff_tree(&empty, &real, "", &revs->pruning);
-   free(tree);
+   retval = diff_tree_sha1(NULL, t1->object.sha1, "", &revs->pruning);
 
return retval >= 0 && (tree_difference == REV_TREE_SAME);
 }
-- 
1.9.rc1.181.g641f458

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html