{"thread":{"id":"35805","subject":"[PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees","startedAt":"2014-02-05T16:57:08Z","lastAt":"2014-02-06T21:50:13Z","messageCount":10,"participants":["Kirill Smelkov","Jeff King","Junio C Hamano","Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"234281","messageId":"cover.1391619218.git.kirr@mns.spb.ru","threadId":"35805","inReplyTo":null,"subject":"[PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-05T16:57:08Z","receivedAt":"2014-02-05T16:57:08Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Some preparatory patches for my reworked nparent tree-walker. Please apply.\n\nThanks beforehand,\nKirill\n\nKirill Smelkov (4):\n tree-diff: allow diff_tree_sha1 to accept NULL sha1\n tree-diff: convert diff_root_tree_sha1() to just call diff_tree_sha1\n with old=NULL\n line-log: convert to using diff_tree_sha1()\n revision: convert to using diff_tree_sha1()\n\n line-log.c | 26 ++------------------------\n revision.c | 12 +-----------\n tree-diff.c | 27 +++++----------------------\n 3 files changed, 8 insertions(+), 57 deletions(-)\n\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234282","messageId":"5a71a2ddf1610b1a52d054b3f986c47f15d0412a.1391619218.git.kirr@mns.spb.ru","threadId":"35805","inReplyTo":"cover.1391619218.git.kirr@mns.spb.ru","subject":"[PATCH 1/4] tree-diff: allow diff_tree_sha1 to accept NULL sha1","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-05T16:57:09Z","receivedAt":"2014-02-05T16:57:09Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"which would mean that corresponding tree - old or new - is empty.\n\nAs followup patches will show, that functionality was already needed in\nseveral places of Git codebase, but there, we were preparing empty\ntree_desc objects by hand, with some code duplication.\n\nFor handling sha1 = NULL case, let's reuse fill_tree_descriptor() which\nreturns just empty tree_desc in that case.\n\nSigned-off-by: Kirill Smelkov \n---\n tree-diff.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex f7b3ade..f438478 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -287,14 +287,10 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha\n \tunsigned long size1, size2;\n \tint retval;\n \n-\ttree1 = read_object_with_reference(old, tree_type, &size1, NULL);\n-\tif (!tree1)\n-\t\tdie(\"unable to read source tree (%s)\", sha1_to_hex(old));\n-\ttree2 = read_object_with_reference(new, tree_type, &size2, NULL);\n-\tif (!tree2)\n-\t\tdie(\"unable to read destination tree (%s)\", sha1_to_hex(new));\n-\tinit_tree_desc(&t1, tree1, size1);\n-\tinit_tree_desc(&t2, tree2, size2);\n+\ttree1 = fill_tree_descriptor(&t1, old);\n+\ttree2 = fill_tree_descriptor(&t2, new);\n+\tsize1 = t1.size;\n+\tsize2 = t2.size;\n \tretval = diff_tree(&t1, &t2, base, opt);\n \tif (!*base && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename()) {\n \t\tinit_tree_desc(&t1, tree1, size1);\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234283","messageId":"bac523d0d3c0b8d91450cc08c859f214bc97e59a.1391619218.git.kirr@mns.spb.ru","threadId":"35805","inReplyTo":"cover.1391619218.git.kirr@mns.spb.ru","subject":"[PATCH 2/4] tree-diff: convert diff_root_tree_sha1() to just call diff_tree_sha1 with old=NULL","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-05T16:57:10Z","receivedAt":"2014-02-05T16:57:10Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Now since diff_tree_sha1 understands NULL for both old and new, we could\nindicate an empty tree for root commit by providing just NULL for old\nsha1.\n\nSigned-off-by: Kirill Smelkov \n---\n tree-diff.c | 15 +--------------\n 1 file changed, 1 insertion(+), 14 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex f438478..6d82a3f 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -304,18 +304,5 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha\n \n int diff_root_tree_sha1(const unsigned char *new, const char *base, struct diff_options *opt)\n {\n-\tint retval;\n-\tvoid *tree;\n-\tunsigned long size;\n-\tstruct tree_desc empty, real;\n-\n-\ttree = read_object_with_reference(new, tree_type, &size, NULL);\n-\tif (!tree)\n-\t\tdie(\"unable to read root tree (%s)\", sha1_to_hex(new));\n-\tinit_tree_desc(&real, tree, size);\n-\n-\tinit_tree_desc(&empty, \"\", 0);\n-\tretval = diff_tree(&empty, &real, base, opt);\n-\tfree(tree);\n-\treturn retval;\n+\treturn diff_tree_sha1(NULL, new, base, opt);\n }\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234284","messageId":"0df5c2e1e93e4873bf276f3f500109249fe1afee.1391619218.git.kirr@mns.spb.ru","threadId":"35805","inReplyTo":"cover.1391619218.git.kirr@mns.spb.ru","subject":"[PATCH 3/4] line-log: convert to using diff_tree_sha1()","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-05T16:57:11Z","receivedAt":"2014-02-05T16:57:11Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Since diff_tree_sha1() can now accept empty trees via NULL sha1, we\ncould just call it without manually reading trees into tree_desc and\nduplicating code.\n\nCc: Thomas Rast \nSigned-off-by: Kirill Smelkov \n---\n line-log.c | 26 ++------------------------\n 1 file changed, 2 insertions(+), 24 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 717638b..1500101 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -766,16 +766,6 @@ void line_log_init(struct rev_info *rev, const char *prefix, struct string_list\n \t}\n }\n \n-static void load_tree_desc(struct tree_desc *desc, void **tree,\n-\t\t\t const unsigned char *sha1)\n-{\n-\tunsigned long size;\n-\t*tree = read_object_with_reference(sha1, tree_type, &size, NULL);\n-\tif (!*tree)\n-\t\tdie(\"Unable to read tree (%s)\", sha1_to_hex(sha1));\n-\tinit_tree_desc(desc, *tree, size);\n-}\n-\n static int count_parents(struct commit *commit)\n {\n \tstruct commit_list *parents = commit->parents;\n@@ -842,18 +832,11 @@ static void queue_diffs(struct line_log_data *range,\n \t\t\tstruct diff_queue_struct *queue,\n \t\t\tstruct commit *commit, struct commit *parent)\n {\n-\tvoid *tree1 = NULL, *tree2 = NULL;\n-\tstruct tree_desc desc1, desc2;\n-\n \tassert(commit);\n-\tload_tree_desc(&desc2, &tree2, commit->tree->object.sha1);\n-\tif (parent)\n-\t\tload_tree_desc(&desc1, &tree1, parent->tree->object.sha1);\n-\telse\n-\t\tinit_tree_desc(&desc1, \"\", 0);\n \n \tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n-\tdiff_tree(&desc1, &desc2, \"\", opt);\n+\tdiff_tree_sha1(parent ? parent->tree->object.sha1 : NULL,\n+\t\t\tcommit->tree->object.sha1, \"\", opt);\n \tif (opt->detect_rename) {\n \t\tfilter_diffs_for_paths(range, 1);\n \t\tif (diff_might_be_rename())\n@@ -861,11 +844,6 @@ static void queue_diffs(struct line_log_data *range,\n \t\tfilter_diffs_for_paths(range, 0);\n \t}\n \tmove_diff_queue(queue, &diff_queued_diff);\n-\n-\tif (tree1)\n-\t\tfree(tree1);\n-\tif (tree2)\n-\t\tfree(tree2);\n }\n \n static char *get_nth_line(long line, unsigned long *ends, void *data)\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234285","messageId":"975fbde9bdd2c5aad7376e398ca8001b9a41d2d6.1391619218.git.kirr@mns.spb.ru","threadId":"35805","inReplyTo":"cover.1391619218.git.kirr@mns.spb.ru","subject":"[PATCH 4/4] revision: convert to using diff_tree_sha1()","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-05T16:57:12Z","receivedAt":"2014-02-05T16:57:12Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Since diff_tree_sha1() can now accept empty trees via NULL sha1, we\ncould just call it without manually reading trees into tree_desc and\nduplicating code.\n\nBesides, that\n\n\tif (!tree)\n\t\treturn 0;\n\nlooked suspect - we were saying an invalid tree != empty tree, but maybe it is\nbetter to just say the tree is invalid here, which is what diff_tree_sha1()\ndoes for such case.\n\nSigned-off-by: Kirill Smelkov \n---\n revision.c | 12 +-----------\n 1 file changed, 1 insertion(+), 11 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 082dae6..bd027bc 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -497,24 +497,14 @@ static int rev_compare_tree(struct rev_info *revs,\n static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)\n {\n \tint retval;\n-\tvoid *tree;\n-\tunsigned long size;\n-\tstruct tree_desc empty, real;\n \tstruct tree *t1 = commit->tree;\n \n \tif (!t1)\n \t\treturn 0;\n \n-\ttree = read_object_with_reference(t1->object.sha1, tree_type, &size, NULL);\n-\tif (!tree)\n-\t\treturn 0;\n-\tinit_tree_desc(&real, tree, size);\n-\tinit_tree_desc(&empty, \"\", 0);\n-\n \ttree_difference = REV_TREE_SAME;\n \tDIFF_OPT_CLR(&revs->pruning, HAS_CHANGES);\n-\tretval = diff_tree(&empty, &real, \"\", &revs->pruning);\n-\tfree(tree);\n+\tretval = diff_tree_sha1(NULL, t1->object.sha1, \"\", &revs->pruning);\n \n \treturn retval >= 0 && (tree_difference == REV_TREE_SAME);\n }\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234288","messageId":"20140205172511.GA7268@sigill.intra.peff.net","threadId":"35805","inReplyTo":"975fbde9bdd2c5aad7376e398ca8001b9a41d2d6.1391619218.git.kirr@mns.spb.ru","subject":"Re: [PATCH 4/4] revision: convert to using diff_tree_sha1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-05T17:25:11Z","receivedAt":"2014-02-05T17:25:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 05, 2014 at 08:57:12PM +0400, Kirill Smelkov wrote:\n\n> Since diff_tree_sha1() can now accept empty trees via NULL sha1, we\n> could just call it without manually reading trees into tree_desc and\n> duplicating code.\n> \n> Besides, that\n> \n> \tif (!tree)\n> \t\treturn 0;\n> \n> looked suspect - we were saying an invalid tree != empty tree, but maybe it is\n> better to just say the tree is invalid here, which is what diff_tree_sha1()\n> does for such case.\n\nI think that is sensible. The assertion that \"invalid != empty\" is\nprobably sane, because we handle the empty tree as internal magic. But I\ndo not see any reason we should be hitting this code path regularly with\nan invalid tree, short of repository corruption, so in practice I don't\nthink it matters.\n\nThis does introduce a die() where there was not one previously, and that\ncan make things harder to diagnose/debug in a corrupted repository. But\nit looks like this is limited to the history-simplification code, and I\nsuspect that it is not commonly used in the case of corruption.\n\nSo I think the patch looks fine.\n\n-Peff\n"},{"id":"234289","messageId":"20140205172540.GB7268@sigill.intra.peff.net","threadId":"35805","inReplyTo":"cover.1391619218.git.kirr@mns.spb.ru","subject":"Re: [PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-05T17:25:40Z","receivedAt":"2014-02-05T17:25:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 05, 2014 at 08:57:08PM +0400, Kirill Smelkov wrote:\n\n> Kirill Smelkov (4):\n> tree-diff: allow diff_tree_sha1 to accept NULL sha1\n> tree-diff: convert diff_root_tree_sha1() to just call diff_tree_sha1\n> with old=NULL\n> line-log: convert to using diff_tree_sha1()\n> revision: convert to using diff_tree_sha1()\n> \n> line-log.c | 26 ++------------------------\n> revision.c | 12 +-----------\n> tree-diff.c | 27 +++++----------------------\n> 3 files changed, 8 insertions(+), 57 deletions(-)\n\nYay, I like the diffstat. All of the patches look good to me.\n\n-Peff\n"},{"id":"234310","messageId":"xmqqzjm54cjy.fsf@gitster.dls.corp.google.com","threadId":"35805","inReplyTo":"cover.1391619218.git.kirr@mns.spb.ru","subject":"Re: [PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-05T18:52:33Z","receivedAt":"2014-02-05T18:52:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"All four looked sensible; will queue. Thanks.\n"},{"id":"234416","messageId":"87wqh8arb2.fsf@thomasrast.ch","threadId":"35805","inReplyTo":"0df5c2e1e93e4873bf276f3f500109249fe1afee.1391619218.git.kirr@mns.spb.ru","subject":"Re: [PATCH 3/4] line-log: convert to using diff_tree_sha1()","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2014-02-06T21:01:53Z","receivedAt":"2014-02-06T21:01:53Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Kirill Smelkov writes:\n\n> Since diff_tree_sha1() can now accept empty trees via NULL sha1, we\n> could just call it without manually reading trees into tree_desc and\n> duplicating code.\n>\n> Cc: Thomas Rast \n> Signed-off-by: Kirill Smelkov \n> ---\n> line-log.c | 26 ++------------------------\n> 1 file changed, 2 insertions(+), 24 deletions(-)\n\nYou have to love a diffstat like that :-)\n\nThanks.\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"234418","messageId":"xmqq8utnykq2.fsf@gitster.dls.corp.google.com","threadId":"35805","inReplyTo":"87wqh8arb2.fsf@thomasrast.ch","subject":"Re: [PATCH 3/4] line-log: convert to using diff_tree_sha1()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-06T21:50:13Z","receivedAt":"2014-02-06T21:50:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Kirill Smelkov writes:\n>\n>> Since diff_tree_sha1() can now accept empty trees via NULL sha1, we\n>> could just call it without manually reading trees into tree_desc and\n>> duplicating code.\n>>\n>> Cc: Thomas Rast \n>> Signed-off-by: Kirill Smelkov \n>> ---\n>> line-log.c | 26 ++------------------------\n>> 1 file changed, 2 insertions(+), 24 deletions(-)\n>\n> You have to love a diffstat like that :-)\n>\n> Thanks.\n\nYes, indeed. Thanks.\n"}]}