[PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees

[PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees

[Kirill Smelkov]

Some preparatory patches for my reworked nparent tree-walker. Please apply.

Thanks beforehand,
Kirill

Kirill Smelkov (4):
  tree-diff: allow diff_tree_sha1 to accept NULL sha1
  tree-diff: convert diff_root_tree_sha1() to just call diff_tree_sha1
    with old=NULL
  line-log: convert to using diff_tree_sha1()
  revision: convert to using diff_tree_sha1()

 line-log.c  | 26 ++------------------------
 revision.c  | 12 +-----------
 tree-diff.c | 27 +++++----------------------
 3 files changed, 8 insertions(+), 57 deletions(-)

-- 
1.9.rc1.181.g641f458

[PATCH 1/4] tree-diff: allow diff_tree_sha1 to accept NULL sha1

[Kirill Smelkov]

which would mean that corresponding tree - old or new - is empty.

As followup patches will show, that functionality was already needed in
several places of Git codebase, but there, we were preparing empty
tree_desc objects by hand, with some code duplication.

For handling sha1 = NULL case, let's reuse fill_tree_descriptor() which
returns just empty tree_desc in that case.

Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 tree-diff.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index f7b3ade..f438478 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -287,14 +287,10 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha
 	unsigned long size1, size2;
 	int retval;
 
-	tree1 = read_object_with_reference(old, tree_type, &size1, NULL);
-	if (!tree1)
-		die("unable to read source tree (%s)", sha1_to_hex(old));
-	tree2 = read_object_with_reference(new, tree_type, &size2, NULL);
-	if (!tree2)
-		die("unable to read destination tree (%s)", sha1_to_hex(new));
-	init_tree_desc(&t1, tree1, size1);
-	init_tree_desc(&t2, tree2, size2);
+	tree1 = fill_tree_descriptor(&t1, old);
+	tree2 = fill_tree_descriptor(&t2, new);
+	size1 = t1.size;
+	size2 = t2.size;
 	retval = diff_tree(&t1, &t2, base, opt);
 	if (!*base && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename()) {
 		init_tree_desc(&t1, tree1, size1);
-- 
1.9.rc1.181.g641f458

[PATCH 2/4] tree-diff: convert diff_root_tree_sha1() to just call diff_tree_sha1 with old=NULL

[Kirill Smelkov]

Now since diff_tree_sha1 understands NULL for both old and new, we could
indicate an empty tree for root commit by providing just NULL for old
sha1.

Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 tree-diff.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index f438478..6d82a3f 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -304,18 +304,5 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha
 
 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;
-
-	tree = read_object_with_reference(new, tree_type, &size, NULL);
-	if (!tree)
-		die("unable to read root tree (%s)", sha1_to_hex(new));
-	init_tree_desc(&real, tree, size);
-
-	init_tree_desc(&empty, "", 0);
-	retval = diff_tree(&empty, &real, base, opt);
-	free(tree);
-	return retval;
+	return diff_tree_sha1(NULL, new, base, opt);
 }
-- 
1.9.rc1.181.g641f458

[PATCH 3/4] line-log: 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.

Cc: Thomas Rast <tr@thomasrast.ch>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 line-log.c | 26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/line-log.c b/line-log.c
index 717638b..1500101 100644
--- a/line-log.c
+++ b/line-log.c
@@ -766,16 +766,6 @@ void line_log_init(struct rev_info *rev, const char *prefix, struct string_list
 	}
 }
 
-static void load_tree_desc(struct tree_desc *desc, void **tree,
-			   const unsigned char *sha1)
-{
-	unsigned long size;
-	*tree = read_object_with_reference(sha1, tree_type, &size, NULL);
-	if (!*tree)
-		die("Unable to read tree (%s)", sha1_to_hex(sha1));
-	init_tree_desc(desc, *tree, size);
-}
-
 static int count_parents(struct commit *commit)
 {
 	struct commit_list *parents = commit->parents;
@@ -842,18 +832,11 @@ static void queue_diffs(struct line_log_data *range,
 			struct diff_queue_struct *queue,
 			struct commit *commit, struct commit *parent)
 {
-	void *tree1 = NULL, *tree2 = NULL;
-	struct tree_desc desc1, desc2;
-
 	assert(commit);
-	load_tree_desc(&desc2, &tree2, commit->tree->object.sha1);
-	if (parent)
-		load_tree_desc(&desc1, &tree1, parent->tree->object.sha1);
-	else
-		init_tree_desc(&desc1, "", 0);
 
 	DIFF_QUEUE_CLEAR(&diff_queued_diff);
-	diff_tree(&desc1, &desc2, "", opt);
+	diff_tree_sha1(parent ? parent->tree->object.sha1 : NULL,
+			commit->tree->object.sha1, "", opt);
 	if (opt->detect_rename) {
 		filter_diffs_for_paths(range, 1);
 		if (diff_might_be_rename())
@@ -861,11 +844,6 @@ static void queue_diffs(struct line_log_data *range,
 		filter_diffs_for_paths(range, 0);
 	}
 	move_diff_queue(queue, &diff_queued_diff);
-
-	if (tree1)
-		free(tree1);
-	if (tree2)
-		free(tree2);
 }
 
 static char *get_nth_line(long line, unsigned long *ends, void *data)
-- 
1.9.rc1.181.g641f458

[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 <kirr@mns.spb.ru>
---
 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

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

Re: [PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees

[Jeff King]

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

> Kirill Smelkov (4):
>   tree-diff: allow diff_tree_sha1 to accept NULL sha1
>   tree-diff: convert diff_root_tree_sha1() to just call diff_tree_sha1
>     with old=NULL
>   line-log: convert to using diff_tree_sha1()
>   revision: convert to using diff_tree_sha1()
> 
>  line-log.c  | 26 ++------------------------
>  revision.c  | 12 +-----------
>  tree-diff.c | 27 +++++----------------------
>  3 files changed, 8 insertions(+), 57 deletions(-)

Yay, I like the diffstat. All of the patches look good to me.

-Peff

Re: [PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees

[Junio C Hamano]

All four looked sensible; will queue.  Thanks.

Re: [PATCH 3/4] line-log: convert to using diff_tree_sha1()

[Thomas Rast]

Kirill Smelkov <kirr@mns.spb.ru> writes:

> 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.
>
> Cc: Thomas Rast <tr@thomasrast.ch>
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> ---
>  line-log.c | 26 ++------------------------
>  1 file changed, 2 insertions(+), 24 deletions(-)

You have to love a diffstat like that :-)

Thanks.

-- 
Thomas Rast
tr@thomasrast.ch

Re: [PATCH 3/4] line-log: convert to using diff_tree_sha1()

[Junio C Hamano]

Thomas Rast <tr@thomasrast.ch> writes:

> Kirill Smelkov <kirr@mns.spb.ru> writes:
>
>> 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.
>>
>> Cc: Thomas Rast <tr@thomasrast.ch>
>> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
>> ---
>>  line-log.c | 26 ++------------------------
>>  1 file changed, 2 insertions(+), 24 deletions(-)
>
> You have to love a diffstat like that :-)
>
> Thanks.

Yes, indeed.  Thanks.