# [PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees 10 messages from 2014-02-05 to 2014-02-06. Participants: Kirill Smelkov, Jeff King, Junio C Hamano, Thomas Rast. Thread: https://gitlist.dev/t/35805 ## Kirill Smelkov, 2014-02-05 16:57 Subject: [PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees Message-ID: URL: https://gitlist.dev/e/cover.1391619218.git.kirr%40mns.spb.ru ``` 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 ``` ## Kirill Smelkov, 2014-02-05 16:57 Subject: [PATCH 1/4] tree-diff: allow diff_tree_sha1 to accept NULL sha1 Message-ID: <5a71a2ddf1610b1a52d054b3f986c47f15d0412a.1391619218.git.kirr@mns.spb.ru> URL: https://gitlist.dev/e/5a71a2ddf1610b1a52d054b3f986c47f15d0412a.1391619218.git.kirr%40mns.spb.ru In-Reply-To: ``` 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 --- 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 ``` ## Kirill Smelkov, 2014-02-05 16:57 Subject: [PATCH 2/4] tree-diff: convert diff_root_tree_sha1() to just call diff_tree_sha1 with old=NULL Message-ID: URL: https://gitlist.dev/e/bac523d0d3c0b8d91450cc08c859f214bc97e59a.1391619218.git.kirr%40mns.spb.ru In-Reply-To: ``` 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 --- 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 ``` ## Kirill Smelkov, 2014-02-05 16:57 Subject: [PATCH 3/4] line-log: convert to using diff_tree_sha1() Message-ID: <0df5c2e1e93e4873bf276f3f500109249fe1afee.1391619218.git.kirr@mns.spb.ru> URL: https://gitlist.dev/e/0df5c2e1e93e4873bf276f3f500109249fe1afee.1391619218.git.kirr%40mns.spb.ru In-Reply-To: ``` 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 Signed-off-by: Kirill Smelkov --- 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 ``` ## Kirill Smelkov, 2014-02-05 16:57 Subject: [PATCH 4/4] revision: convert to using diff_tree_sha1() Message-ID: <975fbde9bdd2c5aad7376e398ca8001b9a41d2d6.1391619218.git.kirr@mns.spb.ru> URL: https://gitlist.dev/e/975fbde9bdd2c5aad7376e398ca8001b9a41d2d6.1391619218.git.kirr%40mns.spb.ru In-Reply-To: ``` 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 ``` ## Jeff King, 2014-02-05 17:25 Subject: Re: [PATCH 4/4] revision: convert to using diff_tree_sha1() Message-ID: <20140205172511.GA7268@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140205172511.GA7268%40sigill.intra.peff.net In-Reply-To: <975fbde9bdd2c5aad7376e398ca8001b9a41d2d6.1391619218.git.kirr@mns.spb.ru> ``` 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 ``` ## Jeff King, 2014-02-05 17:25 Subject: Re: [PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees Message-ID: <20140205172540.GB7268@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140205172540.GB7268%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2014-02-05 18:52 Subject: Re: [PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees Message-ID: URL: https://gitlist.dev/e/xmqqzjm54cjy.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` All four looked sensible; will queue. Thanks. ``` ## Thomas Rast, 2014-02-06 21:01 Subject: Re: [PATCH 3/4] line-log: convert to using diff_tree_sha1() Message-ID: <87wqh8arb2.fsf@thomasrast.ch> URL: https://gitlist.dev/e/87wqh8arb2.fsf%40thomasrast.ch In-Reply-To: <0df5c2e1e93e4873bf276f3f500109249fe1afee.1391619218.git.kirr@mns.spb.ru> ``` Kirill Smelkov 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 > Signed-off-by: Kirill Smelkov > --- > 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 ``` ## Junio C Hamano, 2014-02-06 21:50 Subject: Re: [PATCH 3/4] line-log: convert to using diff_tree_sha1() Message-ID: URL: https://gitlist.dev/e/xmqq8utnykq2.fsf%40gitster.dls.corp.google.com In-Reply-To: <87wqh8arb2.fsf@thomasrast.ch> ``` Thomas Rast writes: > Kirill Smelkov 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 >> Signed-off-by: Kirill Smelkov >> --- >> line-log.c | 26 ++------------------------ >> 1 file changed, 2 insertions(+), 24 deletions(-) > > You have to love a diffstat like that :-) > > Thanks. Yes, indeed. Thanks. ```