{"thread":{"id":"35822","subject":"[PATCH 01/11] tree-diff: show_tree() is not needed","startedAt":"2014-02-07T17:48:41Z","lastAt":"2014-02-13T17:52:14Z","messageCount":21,"participants":["Kirill Smelkov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"234447","messageId":"cover.1391794688.git.kirr@mns.spb.ru","threadId":"35822","inReplyTo":null,"subject":"[PATCH 00/11] More preparatory work for multiparent tree-walker","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-07T17:48:41Z","receivedAt":"2014-02-07T17:48:41Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Here I'm preparing tree-diff.c to be ready for the new tree-walker, so that the\nfinal change is tractable and looks good and non noisy. Some small speedups\nare gained along the way. The final bits are almost ready, but I don't want to\nrelease them in a hurry.\n\nPlease apply and thanks,\nKirill\n\nKirill Smelkov (11):\n  tree-diff: show_tree() is not needed\n  tree-diff: consolidate code for emitting diffs and recursion in one place\n  tree-diff: don't assume compare_tree_entry() returns -1,0,1\n  tree-diff: move all action-taking code out of compare_tree_entry()\n  tree-diff: rename compare_tree_entry -> tree_entry_pathcmp\n  tree-diff: show_path prototype is not needed anymore\n  tree-diff: simplify tree_entry_pathcmp\n  tree-diff: remove special-case diff-emitting code for empty-tree cases\n  tree-diff: rework diff_tree interface to be sha1 based\n  tree-diff: no need to call \"full\" diff_tree_sha1 from show_path()\n  tree-diff: reuse base str(buf) memory on sub-tree recursion\n\n diff.h      |   4 +-\n tree-diff.c | 270 ++++++++++++++++++++++++++++++++----------------------------\n 2 files changed, 145 insertions(+), 129 deletions(-)\n\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234446","messageId":"952728b4e184317d6d3c1095ecd5ef4a694d6105.1391794688.git.kirr@mns.spb.ru","threadId":"35822","inReplyTo":"cover.1391794688.git.kirr@mns.spb.ru","subject":"[PATCH 01/11] tree-diff: show_tree() is not needed","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-07T17:48:42Z","receivedAt":"2014-02-07T17:48:42Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"We don't need special code for showing added/removed subtree, because we\ncan do the same via diff_tree_sha1, just passing NULL for absent tree.\n\nAnd compared to show_tree(), which was calling show_entry() for every\ntree entry, that would lead to the same show_entry() callings:\n\n    show_tree(t):\n        for e in t.entries:\n            show_entry(e)\n\n    diff_tree_sha1(NULL, new):  /* the same applies to (old, NULL) */\n        diff_tree(t1=NULL, t2)\n            ...\n            if (!t1->size)\n                show_entry(t2)\n            ...\n\nand possible overhead is negligible, since after the patch, timing for\n\n    `git log --raw --no-abbrev --no-renames`\n\nfor navy.git and `linux.git v3.10..v3.11` is practically the same.\n\nSo let's say goodbye to show_tree() - it removes some code, but also,\nand what is important, consolidates more code for showing/recursing into\ntrees into one place.\n\nSigned-off-by: Kirill Smelkov <kirr@mns.spb.ru>\n---\n tree-diff.c | 35 +++--------------------------------\n 1 file changed, 3 insertions(+), 32 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex a8c2aec..2ad7788 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -55,25 +55,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,\n \treturn 0;\n }\n \n-/* A whole sub-tree went away or appeared */\n-static void show_tree(struct diff_options *opt, const char *prefix,\n-\t\t      struct tree_desc *desc, struct strbuf *base)\n-{\n-\tenum interesting match = entry_not_interesting;\n-\tfor (; desc->size; update_tree_entry(desc)) {\n-\t\tif (match != all_entries_interesting) {\n-\t\t\tmatch = tree_entry_interesting(&desc->entry, base, 0,\n-\t\t\t\t\t\t       &opt->pathspec);\n-\t\t\tif (match == all_entries_not_interesting)\n-\t\t\t\tbreak;\n-\t\t\tif (match == entry_not_interesting)\n-\t\t\t\tcontinue;\n-\t\t}\n-\t\tshow_entry(opt, prefix, desc, base);\n-\t}\n-}\n-\n-/* A file entry went away or appeared */\n+/* An entry went away or appeared */\n static void show_entry(struct diff_options *opt, const char *prefix,\n \t\t       struct tree_desc *desc, struct strbuf *base)\n {\n@@ -85,23 +67,12 @@ static void show_entry(struct diff_options *opt, const char *prefix,\n \n \tstrbuf_add(base, path, pathlen);\n \tif (DIFF_OPT_TST(opt, RECURSIVE) && S_ISDIR(mode)) {\n-\t\tenum object_type type;\n-\t\tstruct tree_desc inner;\n-\t\tvoid *tree;\n-\t\tunsigned long size;\n-\n-\t\ttree = read_sha1_file(sha1, &type, &size);\n-\t\tif (!tree || type != OBJ_TREE)\n-\t\t\tdie(\"corrupt tree sha %s\", sha1_to_hex(sha1));\n-\n \t\tif (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE))\n \t\t\topt->add_remove(opt, *prefix, mode, sha1, 1, base->buf, 0);\n \n \t\tstrbuf_addch(base, '/');\n-\n-\t\tinit_tree_desc(&inner, tree, size);\n-\t\tshow_tree(opt, prefix, &inner, base);\n-\t\tfree(tree);\n+\t\tdiff_tree_sha1(*prefix == '-' ? sha1 : NULL,\n+\t\t\t       *prefix == '+' ? sha1 : NULL, base->buf, opt);\n \t} else\n \t\topt->add_remove(opt, prefix[0], mode, sha1, 1, base->buf, 0);\n \n-- \n1.9.rc1.181.g641f458\n"},{"id":"234448","messageId":"f0616720dfff7fedbd148e67909fe669834d8e6a.1391794688.git.kirr@mns.spb.ru","threadId":"35822","inReplyTo":"cover.1391794688.git.kirr@mns.spb.ru","subject":"[PATCH 02/11] tree-diff: consolidate code for emitting diffs and recursion in one place","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-07T17:48:43Z","receivedAt":"2014-02-07T17:48:43Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Currently both compare_tree_entry() and show_path() invoke opt diff\ncallbacks (opt->add_remove() and opt->change()), and also they both have\ncode which decides whether to recurse into sub-tree, and whether to emit\na tree as separate entry if DIFF_OPT_TREE_IN_RECURSIVE is set.\n\nI.e. we have code duplication and logic scattered on two places.\n\nLet's consolidate it - all diff emmiting code and recurion logic moves\nto show_entry, which is now named as show_path, because it shows diff\nfor a path, based on up to two tree entries, with actual diff emitting\ncode being kept in new helper emit_diff() for clarity.\n\nWhat we have as the result, is that compare_tree_entry is now free from\ncode with logic for diff generation, and also performance is not\naffected as timings for\n\n    `git log --raw --no-abbrev --no-renames`\n\nfor navy.git and `linux.git v3.10..v3.11`, just like in previous patch,\nstay the same.\n\nSigned-off-by: Kirill Smelkov <kirr@mns.spb.ru>\n---\n tree-diff.c | 117 ++++++++++++++++++++++++++++++++++++++++++++----------------\n 1 file changed, 86 insertions(+), 31 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 2ad7788..0c8e3fc 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -6,8 +6,8 @@\n #include \"diffcore.h\"\n #include \"tree.h\"\n \n-static void show_entry(struct diff_options *opt, const char *prefix,\n-\t\t       struct tree_desc *desc, struct strbuf *base);\n+static void show_path(struct strbuf *base, struct diff_options *opt,\n+\t\t      struct tree_desc *t1, struct tree_desc *t2);\n \n static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,\n \t\t\t      struct strbuf *base, struct diff_options *opt)\n@@ -16,7 +16,6 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,\n \tconst char *path1, *path2;\n \tconst unsigned char *sha1, *sha2;\n \tint cmp, pathlen1, pathlen2;\n-\tint old_baselen = base->len;\n \n \tsha1 = tree_entry_extract(t1, &path1, &mode1);\n \tsha2 = tree_entry_extract(t2, &path2, &mode2);\n@@ -30,51 +29,107 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,\n \t */\n \tcmp = base_name_compare(path1, pathlen1, mode1, path2, pathlen2, mode2);\n \tif (cmp < 0) {\n-\t\tshow_entry(opt, \"-\", t1, base);\n+\t\tshow_path(base, opt, t1, /*t2=*/NULL);\n \t\treturn -1;\n \t}\n \tif (cmp > 0) {\n-\t\tshow_entry(opt, \"+\", t2, base);\n+\t\tshow_path(base, opt, /*t1=*/NULL, t2);\n \t\treturn 1;\n \t}\n \tif (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER) && !hashcmp(sha1, sha2) && mode1 == mode2)\n \t\treturn 0;\n \n-\tstrbuf_add(base, path1, pathlen1);\n-\tif (DIFF_OPT_TST(opt, RECURSIVE) && S_ISDIR(mode1)) {\n-\t\tif (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE)) {\n-\t\t\topt->change(opt, mode1, mode2,\n-\t\t\t\t    sha1, sha2, 1, 1, base->buf, 0, 0);\n-\t\t}\n-\t\tstrbuf_addch(base, '/');\n-\t\tdiff_tree_sha1(sha1, sha2, base->buf, opt);\n-\t} else {\n-\t\topt->change(opt, mode1, mode2, sha1, sha2, 1, 1, base->buf, 0, 0);\n-\t}\n-\tstrbuf_setlen(base, old_baselen);\n+\tshow_path(base, opt, t1, t2);\n \treturn 0;\n }\n \n-/* An entry went away or appeared */\n-static void show_entry(struct diff_options *opt, const char *prefix,\n-\t\t       struct tree_desc *desc, struct strbuf *base)\n+\n+/* convert path, t1/t2 -> opt->diff_*() callbacks */\n+static void emit_diff(struct diff_options *opt, struct strbuf *path,\n+\t\t      struct tree_desc *t1, struct tree_desc *t2)\n+{\n+\tunsigned int mode1 = t1 ? t1->entry.mode : 0;\n+\tunsigned int mode2 = t2 ? t2->entry.mode : 0;\n+\n+\tif (mode1 && mode2) {\n+\t\topt->change(opt, mode1, mode2, t1->entry.sha1, t2->entry.sha1,\n+\t\t\t1, 1, path->buf, 0, 0);\n+\t}\n+\telse {\n+\t\tconst unsigned char *sha1;\n+\t\tunsigned int mode;\n+\t\tint addremove;\n+\n+\t\tif (mode2) {\n+\t\t\taddremove = '+';\n+\t\t\tsha1 = t2->entry.sha1;\n+\t\t\tmode = mode2;\n+\t\t}\n+\t\telse {\n+\t\t\taddremove = '-';\n+\t\t\tsha1 = t1->entry.sha1;\n+\t\t\tmode = mode1;\n+\t\t}\n+\n+\t\topt->add_remove(opt, addremove, mode, sha1, 1, path->buf, 0);\n+\t}\n+}\n+\n+\n+/* new path should be added to diff\n+ *\n+ * 3 cases on how/when it should be called and behaves:\n+ *\n+ *\t!t1,  t2\t-> path added, parent lacks it\n+ *\t t1, !t2\t-> path removed from parent\n+ *\t t1,  t2\t-> path modified\n+ */\n+static void show_path(struct strbuf *base, struct diff_options *opt,\n+\t\t      struct tree_desc *t1, struct tree_desc *t2)\n {\n \tunsigned mode;\n \tconst char *path;\n-\tconst unsigned char *sha1 = tree_entry_extract(desc, &path, &mode);\n-\tint pathlen = tree_entry_len(&desc->entry);\n+\tconst unsigned char *sha1;\n+\tint pathlen;\n \tint old_baselen = base->len;\n+\tint isdir, recurse = 0, emitthis = 1;\n+\n+\t/* at least something has to be valid */\n+\tassert(t1 || t2);\n+\n+\tif (t2) {\n+\t\t/* path present in resulting tree */\n+\t\tsha1 = tree_entry_extract(t2, &path, &mode);\n+\t\tpathlen = tree_entry_len(&t2->entry);\n+\t\tisdir = S_ISDIR(mode);\n+\t}\n+\telse {\n+\t\t/* a path was removed - take path from parent. Also take\n+\t\t * mode from parent, to decide on recursion.\n+\t\t */\n+\t\ttree_entry_extract(t1, &path, &mode);\n+\t\tpathlen = tree_entry_len(&t1->entry);\n+\n+\t\tisdir = S_ISDIR(mode);\n+\t\tsha1 = NULL;\n+\t\tmode = 0;\n+\t}\n+\n+\tif (DIFF_OPT_TST(opt, RECURSIVE) && isdir) {\n+\t\trecurse = 1;\n+\t\temitthis = DIFF_OPT_TST(opt, TREE_IN_RECURSIVE);\n+\t}\n \n \tstrbuf_add(base, path, pathlen);\n-\tif (DIFF_OPT_TST(opt, RECURSIVE) && S_ISDIR(mode)) {\n-\t\tif (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE))\n-\t\t\topt->add_remove(opt, *prefix, mode, sha1, 1, base->buf, 0);\n \n+\tif (emitthis)\n+\t\temit_diff(opt, base, t1, t2);\n+\n+\tif (recurse) {\n \t\tstrbuf_addch(base, '/');\n-\t\tdiff_tree_sha1(*prefix == '-' ? sha1 : NULL,\n-\t\t\t       *prefix == '+' ? sha1 : NULL, base->buf, opt);\n-\t} else\n-\t\topt->add_remove(opt, prefix[0], mode, sha1, 1, base->buf, 0);\n+\t\tdiff_tree_sha1(t1 ? t1->entry.sha1 : NULL,\n+\t\t\t       t2 ? t2->entry.sha1 : NULL, base->buf, opt);\n+\t}\n \n \tstrbuf_setlen(base, old_baselen);\n }\n@@ -117,12 +172,12 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n \t\tif (!t1->size) {\n \t\t\tif (!t2->size)\n \t\t\t\tbreak;\n-\t\t\tshow_entry(opt, \"+\", t2, &base);\n+\t\t\tshow_path(&base, opt, /*t1=*/NULL, t2);\n \t\t\tupdate_tree_entry(t2);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!t2->size) {\n-\t\t\tshow_entry(opt, \"-\", t1, &base);\n+\t\t\tshow_path(&base, opt, t1, /*t2=*/NULL);\n \t\t\tupdate_tree_entry(t1);\n \t\t\tcontinue;\n \t\t}\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234450","messageId":"e61a30650c98ba5a1f1a9cbc96c4ba4801cee72d.1391794688.git.kirr@mns.spb.ru","threadId":"35822","inReplyTo":"cover.1391794688.git.kirr@mns.spb.ru","subject":"[PATCH 03/11] tree-diff: don't assume compare_tree_entry() returns -1,0,1","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-07T17:48:44Z","receivedAt":"2014-02-07T17:48:44Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"It does, but we'll be reworking it in the next patch after it won't, and\nbesides it is better to stick to standard\nstrcmp/memcmp/base_name_compare/etc... convention, where comparison\nfunction returns <0, =0, >0\n\nRegarding performance, comparing for <0, =0, >0 should be a little bit\nfaster, than switch, because it is just 1 test-without-immediate\ninstruction and then up to 3 conditional branches, and in switch you\nhave up to 3 tests with immediate and up to 3 conditional branches.\n\nNo worry, that update_tree_entry(t2) is duplicated for =0 and >0 - it\nwill be good after we'll be adding support for multiparent walker and\nwill stay that way.\n\n=0 case goes first, because it happens more often in real diffs - i.e.\npaths are the same.\n\nSigned-off-by: Kirill Smelkov <kirr@mns.spb.ru>\n---\n tree-diff.c | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 0c8e3fc..c3fbfba 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -181,18 +181,24 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n \t\t\tupdate_tree_entry(t1);\n \t\t\tcontinue;\n \t\t}\n-\t\tswitch (compare_tree_entry(t1, t2, &base, opt)) {\n-\t\tcase -1:\n+\n+\t\tcmp = compare_tree_entry(t1, t2, &base, opt);\n+\n+\t\t/* t1 = t2 */\n+\t\tif (cmp == 0) {\n \t\t\tupdate_tree_entry(t1);\n-\t\t\tcontinue;\n-\t\tcase 0:\n+\t\t\tupdate_tree_entry(t2);\n+\t\t}\n+\n+\t\t/* t1 < t2 */\n+\t\telse if (cmp < 0) {\n \t\t\tupdate_tree_entry(t1);\n-\t\t\t/* Fallthrough */\n-\t\tcase 1:\n+\t\t}\n+\n+\t\t/* t1 > t2 */\n+\t\telse {\n \t\t\tupdate_tree_entry(t2);\n-\t\t\tcontinue;\n \t\t}\n-\t\tdie(\"git diff-tree: internal error\");\n \t}\n \n \tstrbuf_release(&base);\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234451","messageId":"533204f5c956f2cb7442a48ac2a776eee4a43049.1391794688.git.kirr@mns.spb.ru","threadId":"35822","inReplyTo":"cover.1391794688.git.kirr@mns.spb.ru","subject":"[PATCH 04/11] tree-diff: move all action-taking code out of compare_tree_entry()","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-07T17:48:45Z","receivedAt":"2014-02-07T17:48:45Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"- let it do only comparison.\n\nThis way the code is cleaner and more structured - cmp function only\ncompares, and the driver takes action based on comparison result.\n\nThere should be no change in performance, as effectively, we just move\nif series from on place into another, and merge it to was-already-there\nsame switch/if, so the result is maybe a little bit faster.\n\nSigned-off-by: Kirill Smelkov <kirr@mns.spb.ru>\n---\n tree-diff.c | 28 ++++++++++++----------------\n 1 file changed, 12 insertions(+), 16 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex c3fbfba..54a3d23 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -9,8 +9,7 @@\n static void show_path(struct strbuf *base, struct diff_options *opt,\n \t\t      struct tree_desc *t1, struct tree_desc *t2);\n \n-static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,\n-\t\t\t      struct strbuf *base, struct diff_options *opt)\n+static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2)\n {\n \tunsigned mode1, mode2;\n \tconst char *path1, *path2;\n@@ -28,19 +27,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,\n \t * even when having the same name.\n \t */\n \tcmp = base_name_compare(path1, pathlen1, mode1, path2, pathlen2, mode2);\n-\tif (cmp < 0) {\n-\t\tshow_path(base, opt, t1, /*t2=*/NULL);\n-\t\treturn -1;\n-\t}\n-\tif (cmp > 0) {\n-\t\tshow_path(base, opt, /*t1=*/NULL, t2);\n-\t\treturn 1;\n-\t}\n-\tif (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER) && !hashcmp(sha1, sha2) && mode1 == mode2)\n-\t\treturn 0;\n-\n-\tshow_path(base, opt, t1, t2);\n-\treturn 0;\n+\treturn cmp;\n }\n \n \n@@ -163,6 +150,8 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n \tstrbuf_add(&base, base_str, baselen);\n \n \tfor (;;) {\n+\t\tint cmp;\n+\n \t\tif (diff_can_quit_early(opt))\n \t\t\tbreak;\n \t\tif (opt->pathspec.nr) {\n@@ -182,21 +171,28 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tcmp = compare_tree_entry(t1, t2, &base, opt);\n+\t\tcmp = compare_tree_entry(t1, t2);\n \n \t\t/* t1 = t2 */\n \t\tif (cmp == 0) {\n+\t\t\tif (DIFF_OPT_TST(opt, FIND_COPIES_HARDER) ||\n+\t\t\t    hashcmp(t1->entry.sha1, t2->entry.sha1) ||\n+\t\t\t    (t1->entry.mode != t2->entry.mode))\n+\t\t\t\tshow_path(&base, opt, t1, t2);\n+\n \t\t\tupdate_tree_entry(t1);\n \t\t\tupdate_tree_entry(t2);\n \t\t}\n \n \t\t/* t1 < t2 */\n \t\telse if (cmp < 0) {\n+\t\t\tshow_path(&base, opt, t1, /*t2=*/NULL);\n \t\t\tupdate_tree_entry(t1);\n \t\t}\n \n \t\t/* t1 > t2 */\n \t\telse {\n+\t\t\tshow_path(&base, opt, /*t1=*/NULL, t2);\n \t\t\tupdate_tree_entry(t2);\n \t\t}\n \t}\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234453","messageId":"dd82f6456fcd7efdbbd3ad82c364bc6974223f6c.1391794688.git.kirr@mns.spb.ru","threadId":"35822","inReplyTo":"cover.1391794688.git.kirr@mns.spb.ru","subject":"[PATCH 05/11] tree-diff: rename compare_tree_entry -> tree_entry_pathcmp","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-07T17:48:46Z","receivedAt":"2014-02-07T17:48:46Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Since previous commit, this function does not compare entry hashes, and\nmode are compared fully outside of it. So what it does is compare entry\nnames and DIR bit in modes. Reflect this in its name.\n\nAdd documentation stating the semantics, and move the note about\nfiles/dirs comparison to it.\n\nSigned-off-by: Kirill Smelkov <kirr@mns.spb.ru>\n---\n tree-diff.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 54a3d23..df90bbe 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -9,7 +9,14 @@\n static void show_path(struct strbuf *base, struct diff_options *opt,\n \t\t      struct tree_desc *t1, struct tree_desc *t2);\n \n-static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2)\n+/*\n+ * Compare two tree entries, taking into account only path/S_ISDIR(mode),\n+ * but not their sha1's.\n+ *\n+ * NOTE files and directories *always* compare differently, even when having\n+ *      the same name - thanks to base_name_compare().\n+ */\n+static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)\n {\n \tunsigned mode1, mode2;\n \tconst char *path1, *path2;\n@@ -22,10 +29,6 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2)\n \tpathlen1 = tree_entry_len(&t1->entry);\n \tpathlen2 = tree_entry_len(&t2->entry);\n \n-\t/*\n-\t * NOTE files and directories *always* compare differently,\n-\t * even when having the same name.\n-\t */\n \tcmp = base_name_compare(path1, pathlen1, mode1, path2, pathlen2, mode2);\n \treturn cmp;\n }\n@@ -171,7 +174,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tcmp = compare_tree_entry(t1, t2);\n+\t\tcmp = tree_entry_pathcmp(t1, t2);\n \n \t\t/* t1 = t2 */\n \t\tif (cmp == 0) {\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234449","messageId":"19670dbace36ef3996d697bf0fecd8eb9e8a98f2.1391794688.git.kirr@mns.spb.ru","threadId":"35822","inReplyTo":"cover.1391794688.git.kirr@mns.spb.ru","subject":"[PATCH 06/11] tree-diff: show_path prototype is not needed anymore","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-07T17:48:47Z","receivedAt":"2014-02-07T17:48:47Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"We moved all action-taking code below show_path() in recent HEAD~~\n(tree-diff: move all action-taking code out of compare_tree_entry).\n\nSigned-off-by: Kirill Smelkov <kirr@mns.spb.ru>\n---\n tree-diff.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex df90bbe..604dc57 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -6,9 +6,6 @@\n #include \"diffcore.h\"\n #include \"tree.h\"\n \n-static void show_path(struct strbuf *base, struct diff_options *opt,\n-\t\t      struct tree_desc *t1, struct tree_desc *t2);\n-\n /*\n  * Compare two tree entries, taking into account only path/S_ISDIR(mode),\n  * but not their sha1's.\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234452","messageId":"9186fb9d2c223241923111d4fd9b5360e474957e.1391794688.git.kirr@mns.spb.ru","threadId":"35822","inReplyTo":"cover.1391794688.git.kirr@mns.spb.ru","subject":"[PATCH 07/11] tree-diff: simplify tree_entry_pathcmp","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-07T17:48:48Z","receivedAt":"2014-02-07T17:48:48Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Since 74aa4a18 (Finally switch over tree descriptors to contain a\npre-parsed entry) we can safely access all tree_desc->entry fields directly\ninstead of first \"extracting\" them through tree_entry_extract.\n\nUse it. The code generated stays the same - only it now visually looks\ncleaner.\n\nSigned-off-by: Kirill Smelkov <kirr@mns.spb.ru>\n---\n tree-diff.c | 17 ++++++-----------\n 1 file changed, 6 insertions(+), 11 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 604dc57..330ca07 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -15,18 +15,13 @@\n  */\n static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)\n {\n-\tunsigned mode1, mode2;\n-\tconst char *path1, *path2;\n-\tconst unsigned char *sha1, *sha2;\n-\tint cmp, pathlen1, pathlen2;\n+\tstruct name_entry *e1, *e2;\n+\tint cmp;\n \n-\tsha1 = tree_entry_extract(t1, &path1, &mode1);\n-\tsha2 = tree_entry_extract(t2, &path2, &mode2);\n-\n-\tpathlen1 = tree_entry_len(&t1->entry);\n-\tpathlen2 = tree_entry_len(&t2->entry);\n-\n-\tcmp = base_name_compare(path1, pathlen1, mode1, path2, pathlen2, mode2);\n+\te1 = &t1->entry;\n+\te2 = &t2->entry;\n+\tcmp = base_name_compare(e1->path, tree_entry_len(e1), e1->mode,\n+\t\t\t\te2->path, tree_entry_len(e2), e2->mode);\n \treturn cmp;\n }\n \n-- \n1.9.rc1.181.g641f458\n"},{"id":"234455","messageId":"09efaa4d58cc00cc6e191104ba67a6230a7bef71.1391794688.git.kirr@mns.spb.ru","threadId":"35822","inReplyTo":"cover.1391794688.git.kirr@mns.spb.ru","subject":"[PATCH 08/11] tree-diff: remove special-case diff-emitting code for empty-tree cases","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-07T17:48:49Z","receivedAt":"2014-02-07T17:48:49Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"via teaching tree_entry_pathcmp() how to compare empty tree descriptors:\n\nWhile walking trees, we iterate their entries from lowest to highest in\nsort order, so empty tree means all entries were already went over.\n\nIf we artificially assign +infinity value to such tree \"entry\", it will\ngo after all usual entries, and through the usual driver loop we will be\ntaking the same actions, which were hand-coded for special cases, i.e.\n\n    t1 empty, t2 non-empty\n        pathcmp(+∞, t2) -> +1\n        show_path(/*t1=*/NULL, t2);     /* = t1 > t2 case in main loop */\n\n    t1 non-empty, t2-empty\n        pathcmp(t1, +∞) -> -1\n        show_path(t1, /*t2=*/NULL);     /* = t1 < t2 case in main loop */\n\nRight now we never go to when compared tree descriptors are infinity, as\nthis condition is checked in the loop beginning as finishing criteria,\nbut will do in the future, when there will be several parents iterated\nsimultaneously, and some pair of them would run to the end.\n\nSigned-off-by: Kirill Smelkov <kirr@mns.spb.ru>\n---\n tree-diff.c | 21 +++++++++------------\n 1 file changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 330ca07..7688402 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -12,12 +12,19 @@\n  *\n  * NOTE files and directories *always* compare differently, even when having\n  *      the same name - thanks to base_name_compare().\n+ *\n+ * NOTE empty (=invalid) descriptor(s) take part in comparison as +infty.\n  */\n static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)\n {\n \tstruct name_entry *e1, *e2;\n \tint cmp;\n \n+\tif (!t1->size)\n+\t\treturn t2->size ? +1 /* +∞ > c */  : 0 /* +∞ = +∞ */;\n+\telse if (!t2->size)\n+\t\treturn -1;\t/* c < +∞ */\n+\n \te1 = &t1->entry;\n \te2 = &t2->entry;\n \tcmp = base_name_compare(e1->path, tree_entry_len(e1), e1->mode,\n@@ -153,18 +160,8 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n \t\t\tskip_uninteresting(t1, &base, opt);\n \t\t\tskip_uninteresting(t2, &base, opt);\n \t\t}\n-\t\tif (!t1->size) {\n-\t\t\tif (!t2->size)\n-\t\t\t\tbreak;\n-\t\t\tshow_path(&base, opt, /*t1=*/NULL, t2);\n-\t\t\tupdate_tree_entry(t2);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!t2->size) {\n-\t\t\tshow_path(&base, opt, t1, /*t2=*/NULL);\n-\t\t\tupdate_tree_entry(t1);\n-\t\t\tcontinue;\n-\t\t}\n+\t\tif (!t1->size && !t2->size)\n+\t\t\tbreak;\n \n \t\tcmp = tree_entry_pathcmp(t1, t2);\n \n-- \n1.9.rc1.181.g641f458\n"},{"id":"234454","messageId":"0f68d2fe87cd34ca9d1a1c3d267054d07197949a.1391794688.git.kirr@mns.spb.ru","threadId":"35822","inReplyTo":"cover.1391794688.git.kirr@mns.spb.ru","subject":"[PATCH 09/11] tree-diff: rework diff_tree interface to be sha1 based","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-07T17:48:50Z","receivedAt":"2014-02-07T17:48:50Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"In the next commit this will allow to reduce intermediate calls, when\nrecursing into subtrees - at that stage we know only subtree sha1, and\nit is natural for tree walker to start from that phase. For now we do\n\n    diff_tree\n        show_path\n            diff_tree_sha1\n                diff_tree\n                    ...\n\nand the change will allow to reduce it to\n\n    diff_tree\n        show_path\n            diff_tree\n\nAlso, it will allow to omit allocating strbuf for each subtree, and just\nreuse the common strbuf via playing with its len.\n\nThe above-mentioned improvements go in the next 2 patches.\n\nThe downside is that try_to_follow_renames(), if active, we cause\nre-reading of 2 initial trees, which was negligible based on my timings,\nand which is outweighed cogently by the upsides.\n\nNOTE To keep with the current interface and semantics, I needed to\nrename the function. It will probably be renamed once more later, when\nits semantic will change to just generate paths for a diff, instead of\nproducing it. So the current name is appropriate, but probably temporary.\n\nSigned-off-by: Kirill Smelkov <kirr@mns.spb.ru>\n---\n diff.h      |  4 ++--\n tree-diff.c | 60 ++++++++++++++++++++++++++++--------------------------------\n 2 files changed, 30 insertions(+), 34 deletions(-)\n\ndiff --git a/diff.h b/diff.h\nindex a24a767..4994d15 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -189,8 +189,8 @@ const char *diff_line_prefix(struct diff_options *);\n \n extern const char mime_boundary_leader[];\n \n-extern int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n-\t\t     const char *base, struct diff_options *opt);\n+extern int __diff_tree_sha1(const unsigned char *old, const unsigned char *new,\n+\t\t\t    const char *base, struct diff_options *opt);\n extern int diff_tree_sha1(const unsigned char *old, const unsigned char *new,\n \t\t\t  const char *base, struct diff_options *opt);\n extern int diff_root_tree_sha1(const unsigned char *new, const char *base,\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 7688402..dd6c760 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -139,12 +139,17 @@ static void skip_uninteresting(struct tree_desc *t, struct strbuf *base,\n \t}\n }\n \n-int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n-\t      const char *base_str, struct diff_options *opt)\n+int __diff_tree_sha1(const unsigned char *old, const unsigned char *new,\n+\t\t     const char *base_str, struct diff_options *opt)\n {\n+\tstruct tree_desc t1, t2;\n+\tvoid *t1tree, *t2tree;\n \tstruct strbuf base;\n \tint baselen = strlen(base_str);\n \n+\tt1tree = fill_tree_descriptor(&t1, old);\n+\tt2tree = fill_tree_descriptor(&t2, new);\n+\n \t/* Enable recursion indefinitely */\n \topt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);\n \n@@ -157,39 +162,41 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n \t\tif (diff_can_quit_early(opt))\n \t\t\tbreak;\n \t\tif (opt->pathspec.nr) {\n-\t\t\tskip_uninteresting(t1, &base, opt);\n-\t\t\tskip_uninteresting(t2, &base, opt);\n+\t\t\tskip_uninteresting(&t1, &base, opt);\n+\t\t\tskip_uninteresting(&t2, &base, opt);\n \t\t}\n-\t\tif (!t1->size && !t2->size)\n+\t\tif (!t1.size && !t2.size)\n \t\t\tbreak;\n \n-\t\tcmp = tree_entry_pathcmp(t1, t2);\n+\t\tcmp = tree_entry_pathcmp(&t1, &t2);\n \n \t\t/* t1 = t2 */\n \t\tif (cmp == 0) {\n \t\t\tif (DIFF_OPT_TST(opt, FIND_COPIES_HARDER) ||\n-\t\t\t    hashcmp(t1->entry.sha1, t2->entry.sha1) ||\n-\t\t\t    (t1->entry.mode != t2->entry.mode))\n-\t\t\t\tshow_path(&base, opt, t1, t2);\n+\t\t\t    hashcmp(t1.entry.sha1, t2.entry.sha1) ||\n+\t\t\t    (t1.entry.mode != t2.entry.mode))\n+\t\t\t\tshow_path(&base, opt, &t1, &t2);\n \n-\t\t\tupdate_tree_entry(t1);\n-\t\t\tupdate_tree_entry(t2);\n+\t\t\tupdate_tree_entry(&t1);\n+\t\t\tupdate_tree_entry(&t2);\n \t\t}\n \n \t\t/* t1 < t2 */\n \t\telse if (cmp < 0) {\n-\t\t\tshow_path(&base, opt, t1, /*t2=*/NULL);\n-\t\t\tupdate_tree_entry(t1);\n+\t\t\tshow_path(&base, opt, &t1, /*t2=*/NULL);\n+\t\t\tupdate_tree_entry(&t1);\n \t\t}\n \n \t\t/* t1 > t2 */\n \t\telse {\n-\t\t\tshow_path(&base, opt, /*t1=*/NULL, t2);\n-\t\t\tupdate_tree_entry(t2);\n+\t\t\tshow_path(&base, opt, /*t1=*/NULL, &t2);\n+\t\t\tupdate_tree_entry(&t2);\n \t\t}\n \t}\n \n \tstrbuf_release(&base);\n+\tfree(t2tree);\n+\tfree(t1tree);\n \treturn 0;\n }\n \n@@ -204,7 +211,7 @@ static inline int diff_might_be_rename(void)\n \t\t!DIFF_FILE_VALID(diff_queued_diff.queue[0]->one);\n }\n \n-static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, const char *base, struct diff_options *opt)\n+static void try_to_follow_renames(const unsigned char *old, const unsigned char *new, const char *base, struct diff_options *opt)\n {\n \tstruct diff_options diff_opts;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n@@ -242,7 +249,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co\n \tdiff_opts.break_opt = opt->break_opt;\n \tdiff_opts.rename_score = opt->rename_score;\n \tdiff_setup_done(&diff_opts);\n-\tdiff_tree(t1, t2, base, &diff_opts);\n+\t__diff_tree_sha1(old, new, base, &diff_opts);\n \tdiffcore_std(&diff_opts);\n \tfree_pathspec(&diff_opts.pathspec);\n \n@@ -303,23 +310,12 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co\n \n int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const char *base, struct diff_options *opt)\n {\n-\tvoid *tree1, *tree2;\n-\tstruct tree_desc t1, t2;\n-\tunsigned long size1, size2;\n \tint retval;\n \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-\t\tinit_tree_desc(&t2, tree2, size2);\n-\t\ttry_to_follow_renames(&t1, &t2, base, opt);\n-\t}\n-\tfree(tree1);\n-\tfree(tree2);\n+\tretval = __diff_tree_sha1(old, new, base, opt);\n+\tif (!*base && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename())\n+\t\ttry_to_follow_renames(old, new, base, opt);\n+\n \treturn retval;\n }\n \n-- \n1.9.rc1.181.g641f458\n"},{"id":"234457","messageId":"cd28df7033a83d4930cb70ab8794b86cb2601de6.1391794688.git.kirr@mns.spb.ru","threadId":"35822","inReplyTo":"cover.1391794688.git.kirr@mns.spb.ru","subject":"[PATCH 10/11] tree-diff: no need to call \"full\" diff_tree_sha1 from show_path()","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-07T17:48:51Z","receivedAt":"2014-02-07T17:48:51Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"As described in previous commit, when recursing into sub-trees, we can\nuse lower-level tree walker, since its interface is now sha1 based.\n\nThe change is ok, because diff_tree_sha1() only invokes\n__diff_tree_sha1(), and also, if base is empty, try_to_follow_renames().\nBut base is not empty here, as we have added a path and '/' before\nrecursing.\n\nSigned-off-by: Kirill Smelkov <kirr@mns.spb.ru>\n---\n tree-diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex dd6c760..e385ed4 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -116,8 +116,8 @@ static void show_path(struct strbuf *base, struct diff_options *opt,\n \n \tif (recurse) {\n \t\tstrbuf_addch(base, '/');\n-\t\tdiff_tree_sha1(t1 ? t1->entry.sha1 : NULL,\n-\t\t\t       t2 ? t2->entry.sha1 : NULL, base->buf, opt);\n+\t\t__diff_tree_sha1(t1 ? t1->entry.sha1 : NULL,\n+\t\t\t\t t2 ? t2->entry.sha1 : NULL, base->buf, opt);\n \t}\n \n \tstrbuf_setlen(base, old_baselen);\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234456","messageId":"645beb40c13912e87f6fe67d46af2b5e81dcaaca.1391794688.git.kirr@mns.spb.ru","threadId":"35822","inReplyTo":"cover.1391794688.git.kirr@mns.spb.ru","subject":"[PATCH 11/11] tree-diff: reuse base str(buf) memory on sub-tree recursion","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-07T17:48:52Z","receivedAt":"2014-02-07T17:48:52Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"instead of allocating it all the time for every subtree in\n__diff_tree_sha1, let's allocate it once in diff_tree_sha1, and then all\ncallee just use it in stacking style, without memory allocations.\n\nThis should be faster, and for me this change gives the following\nslight speedups for `git log --raw --no-abbrev --no-renames`\n\n                navy.git    linux.git v3.10..v3.11\n\n    before      0.547s      1.791s\n    after       0.541s      1.777s\n    speedup     1.1%        0.8%\n\nSigned-off-by: Kirill Smelkov <kirr@mns.spb.ru>\n---\n diff.h      |  2 +-\n tree-diff.c | 36 ++++++++++++++++++------------------\n 2 files changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/diff.h b/diff.h\nindex 4994d15..14016ce 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -190,7 +190,7 @@ const char *diff_line_prefix(struct diff_options *);\n extern const char mime_boundary_leader[];\n \n extern int __diff_tree_sha1(const unsigned char *old, const unsigned char *new,\n-\t\t\t    const char *base, struct diff_options *opt);\n+\t\t\t    struct strbuf *base, struct diff_options *opt);\n extern int diff_tree_sha1(const unsigned char *old, const unsigned char *new,\n \t\t\t  const char *base, struct diff_options *opt);\n extern int diff_root_tree_sha1(const unsigned char *new, const char *base,\ndiff --git a/tree-diff.c b/tree-diff.c\nindex e385ed4..2adda04 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -117,7 +117,7 @@ static void show_path(struct strbuf *base, struct diff_options *opt,\n \tif (recurse) {\n \t\tstrbuf_addch(base, '/');\n \t\t__diff_tree_sha1(t1 ? t1->entry.sha1 : NULL,\n-\t\t\t\t t2 ? t2->entry.sha1 : NULL, base->buf, opt);\n+\t\t\t\t t2 ? t2->entry.sha1 : NULL, base, opt);\n \t}\n \n \tstrbuf_setlen(base, old_baselen);\n@@ -140,12 +140,10 @@ static void skip_uninteresting(struct tree_desc *t, struct strbuf *base,\n }\n \n int __diff_tree_sha1(const unsigned char *old, const unsigned char *new,\n-\t\t     const char *base_str, struct diff_options *opt)\n+\t\t     struct strbuf *base, struct diff_options *opt)\n {\n \tstruct tree_desc t1, t2;\n \tvoid *t1tree, *t2tree;\n-\tstruct strbuf base;\n-\tint baselen = strlen(base_str);\n \n \tt1tree = fill_tree_descriptor(&t1, old);\n \tt2tree = fill_tree_descriptor(&t2, new);\n@@ -153,17 +151,14 @@ int __diff_tree_sha1(const unsigned char *old, const unsigned char *new,\n \t/* Enable recursion indefinitely */\n \topt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);\n \n-\tstrbuf_init(&base, PATH_MAX);\n-\tstrbuf_add(&base, base_str, baselen);\n-\n \tfor (;;) {\n \t\tint cmp;\n \n \t\tif (diff_can_quit_early(opt))\n \t\t\tbreak;\n \t\tif (opt->pathspec.nr) {\n-\t\t\tskip_uninteresting(&t1, &base, opt);\n-\t\t\tskip_uninteresting(&t2, &base, opt);\n+\t\t\tskip_uninteresting(&t1, base, opt);\n+\t\t\tskip_uninteresting(&t2, base, opt);\n \t\t}\n \t\tif (!t1.size && !t2.size)\n \t\t\tbreak;\n@@ -175,7 +170,7 @@ int __diff_tree_sha1(const unsigned char *old, const unsigned char *new,\n \t\t\tif (DIFF_OPT_TST(opt, FIND_COPIES_HARDER) ||\n \t\t\t    hashcmp(t1.entry.sha1, t2.entry.sha1) ||\n \t\t\t    (t1.entry.mode != t2.entry.mode))\n-\t\t\t\tshow_path(&base, opt, &t1, &t2);\n+\t\t\t\tshow_path(base, opt, &t1, &t2);\n \n \t\t\tupdate_tree_entry(&t1);\n \t\t\tupdate_tree_entry(&t2);\n@@ -183,18 +178,17 @@ int __diff_tree_sha1(const unsigned char *old, const unsigned char *new,\n \n \t\t/* t1 < t2 */\n \t\telse if (cmp < 0) {\n-\t\t\tshow_path(&base, opt, &t1, /*t2=*/NULL);\n+\t\t\tshow_path(base, opt, &t1, /*t2=*/NULL);\n \t\t\tupdate_tree_entry(&t1);\n \t\t}\n \n \t\t/* t1 > t2 */\n \t\telse {\n-\t\t\tshow_path(&base, opt, /*t1=*/NULL, &t2);\n+\t\t\tshow_path(base, opt, /*t1=*/NULL, &t2);\n \t\t\tupdate_tree_entry(&t2);\n \t\t}\n \t}\n \n-\tstrbuf_release(&base);\n \tfree(t2tree);\n \tfree(t1tree);\n \treturn 0;\n@@ -211,7 +205,7 @@ static inline int diff_might_be_rename(void)\n \t\t!DIFF_FILE_VALID(diff_queued_diff.queue[0]->one);\n }\n \n-static void try_to_follow_renames(const unsigned char *old, const unsigned char *new, const char *base, struct diff_options *opt)\n+static void try_to_follow_renames(const unsigned char *old, const unsigned char *new, struct strbuf *base, struct diff_options *opt)\n {\n \tstruct diff_options diff_opts;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n@@ -308,13 +302,19 @@ static void try_to_follow_renames(const unsigned char *old, const unsigned char\n \tq->nr = 1;\n }\n \n-int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const char *base, struct diff_options *opt)\n+int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const char *base_str, struct diff_options *opt)\n {\n+\tstruct strbuf base;\n \tint retval;\n \n-\tretval = __diff_tree_sha1(old, new, base, opt);\n-\tif (!*base && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename())\n-\t\ttry_to_follow_renames(old, new, base, opt);\n+\tstrbuf_init(&base, PATH_MAX);\n+\tstrbuf_addstr(&base, base_str);\n+\n+\tretval = __diff_tree_sha1(old, new, &base, opt);\n+\tif (!*base_str && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename())\n+\t\ttry_to_follow_renames(old, new, &base, opt);\n+\n+\tstrbuf_release(&base);\n \n \treturn retval;\n }\n-- \n1.9.rc1.181.g641f458\n"},{"id":"234605","messageId":"xmqqeh3aqyq6.fsf@gitster.dls.corp.google.com","threadId":"35822","inReplyTo":"cover.1391794688.git.kirr@mns.spb.ru","subject":"Re: [PATCH 00/11] More preparatory work for multiparent tree-walker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-11T00:28:33Z","receivedAt":"2014-02-11T00:28:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov <kirr@mns.spb.ru> writes:\n\n> Here I'm preparing tree-diff.c to be ready for the new tree-walker, so that the\n> final change is tractable and looks good and non noisy. Some small speedups\n> are gained along the way. The final bits are almost ready, but I don't want to\n> release them in a hurry.\n\nNo worries.  We are not in a hurry to apply non-regression-fix\nchanges during a pre-release feature freeze period anyway.\n\nThis seems to somehow conflict with other topics and does not\ncleanly apply on top of your other tree-diff topic, by the way.\n"},{"id":"234607","messageId":"20140211081716.GA4455@tugrik.mns.mnsspb.ru","threadId":"35822","inReplyTo":"xmqqeh3aqyq6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 00/11] More preparatory work for multiparent tree-walker","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-11T08:17:16Z","receivedAt":"2014-02-11T08:17:16Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Mon, Feb 10, 2014 at 04:28:33PM -0800, Junio C Hamano wrote:\n> Kirill Smelkov <kirr@mns.spb.ru> writes:\n> \n> > Here I'm preparing tree-diff.c to be ready for the new tree-walker, so that the\n> > final change is tractable and looks good and non noisy. Some small speedups\n> > are gained along the way. The final bits are almost ready, but I don't want to\n> > release them in a hurry.\n> \n> No worries.  We are not in a hurry to apply non-regression-fix\n> changes during a pre-release feature freeze period anyway.\n\nI see.\n\n\n> This seems to somehow conflict with other topics and does not\n> cleanly apply on top of your other tree-diff topic, by the way.\n\nSorry for the confusion. Could you please do the following:\n\nPatches should be applied over to ks/tree-diff-walk\n(74aa4a18). Before applying the patches, please cherry-pick\n\n    c90483d9    (tree-diff: no need to manually verify that there is no\n                 mode change for a path)\n\n    ef4f0928    (tree-diff: no need to pass match to\n                 skip_uninteresting())\n\ninto that branch, and drop them from ks/combine-diff - we'll have one\nbranch reworking the diff tree-walker, and the other taking advantage of\nit for combine-diff.\n\nThen apply sent-here patches, which, as I've verified this time, should\napply cleanly.\n\nThanks and sorry again,\nKirill\n"},{"id":"234627","messageId":"xmqq61olpgjd.fsf@gitster.dls.corp.google.com","threadId":"35822","inReplyTo":"20140211081716.GA4455@tugrik.mns.mnsspb.ru","subject":"Re: [PATCH 00/11] More preparatory work for multiparent tree-walker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-11T19:59:02Z","receivedAt":"2014-02-11T19:59:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov <kirr@mns.spb.ru> writes:\n\n> Sorry for the confusion. Could you please do the following:\n>\n> Patches should be applied over to ks/tree-diff-walk\n> (74aa4a18). Before applying the patches, please cherry-pick\n>\n>     c90483d9    (tree-diff: no need to manually verify that there is no\n>                  mode change for a path)\n>\n>     ef4f0928    (tree-diff: no need to pass match to\n>                  skip_uninteresting())\n>\n> into that branch, and drop them from ks/combine-diff - we'll have one\n> branch reworking the diff tree-walker, and the other taking advantage of\n> it for combine-diff.\n\nAs long as that does not lose changes to tests and clean-ups, I'm\nfine with that direction.  For example, I do not know if you want to\nlose e3f62d12 (diffcore-order: export generic ordering interface,\n2014-01-20), which is part of the combine-diff topic.\n"},{"id":"234645","messageId":"20140212093057.GA3408@tugrik.mns.mnsspb.ru","threadId":"35822","inReplyTo":"xmqq61olpgjd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 00/11] More preparatory work for multiparent tree-walker","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-12T09:30:57Z","receivedAt":"2014-02-12T09:30:57Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Tue, Feb 11, 2014 at 11:59:02AM -0800, Junio C Hamano wrote:\n> Kirill Smelkov <kirr@mns.spb.ru> writes:\n> \n> > Sorry for the confusion. Could you please do the following:\n> >\n> > Patches should be applied over to ks/tree-diff-walk\n> > (74aa4a18). Before applying the patches, please cherry-pick\n> >\n> >     c90483d9    (tree-diff: no need to manually verify that there is no\n> >                  mode change for a path)\n> >\n> >     ef4f0928    (tree-diff: no need to pass match to\n> >                  skip_uninteresting())\n> >\n> > into that branch, and drop them from ks/combine-diff - we'll have one\n> > branch reworking the diff tree-walker, and the other taking advantage of\n> > it for combine-diff.\n> \n> As long as that does not lose changes to tests and clean-ups, I'm\n> fine with that direction.  For example, I do not know if you want to\n> lose e3f62d12 (diffcore-order: export generic ordering interface,\n> 2014-01-20), which is part of the combine-diff topic.\n\nSorry for the confusion again, and please don't worry: we are not going\nto lose anything - my only plea here was to transfer two of the patches\nto more appropriate topic.\n\nThat couple touches tree-diff.c - they were some initial cleanups I've\nnoticed while working on separate combine-diff tree-walker, which we\ndecided to drop instead of generalizing diff tree-walker to handle all\ncases. Only the cleanups are still relevant and needed as a base for\nwhat I've sent you here.\n\nAnd as to e3f62d12 (diffcore-order: export generic ordering interface,\n2014-01-20) and other patches on ks/diff-c-with-diff-order topic - they\nstay as they are - we do not need to rework them as ks/combine-diff\nbuilds on top of the topic and generalizing diff tree-walker is\northogonal work.\n\n\nSo in short: could you please transform the two \"tree-diff\" patches from\nks/combine-diff to ks/tree-diff-walk, then apply sent-here patches to\nks/tree-diff-walk; thats all.\n\n\nThanks,\nKirill\n"},{"id":"234665","messageId":"xmqqr478nsyo.fsf@gitster.dls.corp.google.com","threadId":"35822","inReplyTo":"xmqq61olpgjd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 00/11] More preparatory work for multiparent tree-walker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-12T17:25:51Z","receivedAt":"2014-02-12T17:25:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Kirill Smelkov <kirr@mns.spb.ru> writes:\n>\n>> Sorry for the confusion. Could you please do the following:\n>>\n>> Patches should be applied over to ks/tree-diff-walk\n>> (74aa4a18). Before applying the patches, please cherry-pick\n>>\n>>     c90483d9    (tree-diff: no need to manually verify that there is no\n>>                  mode change for a path)\n>>\n>>     ef4f0928    (tree-diff: no need to pass match to\n>>                  skip_uninteresting())\n>>\n>> into that branch, and drop them from ks/combine-diff - we'll have one\n>> branch reworking the diff tree-walker, and the other taking advantage of\n>> it for combine-diff.\n>\n> As long as that does not lose changes to tests and clean-ups, I'm\n> fine with that direction.  For example, I do not know if you want to\n> lose e3f62d12 (diffcore-order: export generic ordering interface,\n> 2014-01-20), which is part of the combine-diff topic.\n\nAhh, sorry, I misread the \"drop\" as \"salvage these two and drop the\nrest\".  The new series does apply cleanly on a commit in master..pu\nthat has both ks/tree-diff-walk and ks/combine-diff, and the latter\nis not yet in 'next' so we are free to reorganize.\n\nLet me flip the latter topic around, also queue these updates and\npush the result out on 'pu'.\n\nThanks.\n"},{"id":"234727","messageId":"20140213132524.GA15450@tugrik.mns.mnsspb.ru","threadId":"35822","inReplyTo":"645beb40c13912e87f6fe67d46af2b5e81dcaaca.1391794688.git.kirr@mns.spb.ru","subject":"Re: [PATCH 11/11] tree-diff: reuse base str(buf) memory on sub-tree recursion","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-13T13:25:24Z","receivedAt":"2014-02-13T13:25:24Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Fri, Feb 07, 2014 at 09:48:52PM +0400, Kirill Smelkov wrote:\n> instead of allocating it all the time for every subtree in\n> __diff_tree_sha1, let's allocate it once in diff_tree_sha1, and then all\n> callee just use it in stacking style, without memory allocations.\n> \n> This should be faster, and for me this change gives the following\n> slight speedups for `git log --raw --no-abbrev --no-renames`\n> \n>                 navy.git    linux.git v3.10..v3.11\n> \n>     before      0.547s      1.791s\n>     after       0.541s      1.777s\n>     speedup     1.1%        0.8%\n\nThe timings above was done with\n\n    `git log --raw --no-abbrev --no-renames --format='%H'`\n                                            ^^^^^^^^^^^^^\n\nPlease change them to correct timings:\n\n\n                navy.git    linux.git v3.10..v3.11\n\n    before      0.618s      1.903s\n    after       0.611s      1.889s\n    speedup     1.1%        0.7%\n\n\n\nThanks,\nKirill\n"},{"id":"234731","messageId":"20140213140557.GA4839@tugrik.mns.mnsspb.ru","threadId":"35822","inReplyTo":"xmqqr478nsyo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 00/11] More preparatory work for multiparent tree-walker","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-13T14:05:57Z","receivedAt":"2014-02-13T14:05:57Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Wed, Feb 12, 2014 at 09:25:51AM -0800, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Kirill Smelkov <kirr@mns.spb.ru> writes:\n> >\n> >> Sorry for the confusion. Could you please do the following:\n> >>\n> >> Patches should be applied over to ks/tree-diff-walk\n> >> (74aa4a18). Before applying the patches, please cherry-pick\n> >>\n> >>     c90483d9    (tree-diff: no need to manually verify that there is no\n> >>                  mode change for a path)\n> >>\n> >>     ef4f0928    (tree-diff: no need to pass match to\n> >>                  skip_uninteresting())\n> >>\n> >> into that branch, and drop them from ks/combine-diff - we'll have one\n> >> branch reworking the diff tree-walker, and the other taking advantage of\n> >> it for combine-diff.\n> >\n> > As long as that does not lose changes to tests and clean-ups, I'm\n> > fine with that direction.  For example, I do not know if you want to\n> > lose e3f62d12 (diffcore-order: export generic ordering interface,\n> > 2014-01-20), which is part of the combine-diff topic.\n> \n> Ahh, sorry, I misread the \"drop\" as \"salvage these two and drop the\n> rest\".  The new series does apply cleanly on a commit in master..pu\n> that has both ks/tree-diff-walk and ks/combine-diff, and the latter\n> is not yet in 'next' so we are free to reorganize.\n> \n> Let me flip the latter topic around, also queue these updates and\n> push the result out on 'pu'.\n> \n> Thanks.\n\nThank you. As we've managed to apply this to pu, I've send the final\nspeedup patches. Please review them as time permits.\n\nThanks beforehand,\nKirill\n"},{"id":"234732","messageId":"xmqqob2alxhc.fsf@gitster.dls.corp.google.com","threadId":"35822","inReplyTo":"f0616720dfff7fedbd148e67909fe669834d8e6a.1391794688.git.kirr@mns.spb.ru","subject":"Re: [PATCH 02/11] tree-diff: consolidate code for emitting diffs and recursion in one place","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-13T17:43:27Z","receivedAt":"2014-02-13T17:43:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov <kirr@mns.spb.ru> writes:\n\n> +static void show_path(struct strbuf *base, struct diff_options *opt,\n> +\t\t      struct tree_desc *t1, struct tree_desc *t2)\n>  {\n>  \tunsigned mode;\n>  \tconst char *path;\n> -\tconst unsigned char *sha1 = tree_entry_extract(desc, &path, &mode);\n> -\tint pathlen = tree_entry_len(&desc->entry);\n> +\tconst unsigned char *sha1;\n> +\tint pathlen;\n>  \tint old_baselen = base->len;\n> +\tint isdir, recurse = 0, emitthis = 1;\n> +\n> +\t/* at least something has to be valid */\n> +\tassert(t1 || t2);\n> +\n> +\tif (t2) {\n> +\t\t/* path present in resulting tree */\n> +\t\tsha1 = tree_entry_extract(t2, &path, &mode);\n> +\t\tpathlen = tree_entry_len(&t2->entry);\n> +\t\tisdir = S_ISDIR(mode);\n> +\t}\n> +\telse {\n> +\t\t/* a path was removed - take path from parent. Also take\n> +\t\t * mode from parent, to decide on recursion.\n> +\t\t */\n> +\t\ttree_entry_extract(t1, &path, &mode);\n> +\t\tpathlen = tree_entry_len(&t1->entry);\n> +\n> +\t\tisdir = S_ISDIR(mode);\n> +\t\tsha1 = NULL;\n> +\t\tmode = 0;\n> +\t}\n> +\n> +\tif (DIFF_OPT_TST(opt, RECURSIVE) && isdir) {\n> +\t\trecurse = 1;\n> +\t\temitthis = DIFF_OPT_TST(opt, TREE_IN_RECURSIVE);\n> +\t}\n>  \n>  \tstrbuf_add(base, path, pathlen);\n> -\tif (DIFF_OPT_TST(opt, RECURSIVE) && S_ISDIR(mode)) {\n> -\t\tif (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE))\n> -\t\t\topt->add_remove(opt, *prefix, mode, sha1, 1, base->buf, 0);\n>  \n> +\tif (emitthis)\n> +\t\temit_diff(opt, base, t1, t2);\n> +\n> +\tif (recurse) {\n>  \t\tstrbuf_addch(base, '/');\n> -\t\tdiff_tree_sha1(*prefix == '-' ? sha1 : NULL,\n> -\t\t\t       *prefix == '+' ? sha1 : NULL, base->buf, opt);\n> -\t} else\n> -\t\topt->add_remove(opt, prefix[0], mode, sha1, 1, base->buf, 0);\n> +\t\tdiff_tree_sha1(t1 ? t1->entry.sha1 : NULL,\n> +\t\t\t       t2 ? t2->entry.sha1 : NULL, base->buf, opt);\n> +\t}\n\n\nAfter this step, \"sha1\" is assigned but never gets used.  Please\ndouble-check the fix-up I queued in the series before merging it to\n'pu'.\n\nThanks.\n"},{"id":"234733","messageId":"20140213175214.GB6409@tugrik.mns.mnsspb.ru","threadId":"35822","inReplyTo":"xmqqob2alxhc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 02/11] tree-diff: consolidate code for emitting diffs and recursion in one place","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2014-02-13T17:52:14Z","receivedAt":"2014-02-13T17:52:14Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Thu, Feb 13, 2014 at 09:43:27AM -0800, Junio C Hamano wrote:\n> Kirill Smelkov <kirr@mns.spb.ru> writes:\n> \n> > +static void show_path(struct strbuf *base, struct diff_options *opt,\n> > +\t\t      struct tree_desc *t1, struct tree_desc *t2)\n> >  {\n> >  \tunsigned mode;\n> >  \tconst char *path;\n> > -\tconst unsigned char *sha1 = tree_entry_extract(desc, &path, &mode);\n> > -\tint pathlen = tree_entry_len(&desc->entry);\n> > +\tconst unsigned char *sha1;\n> > +\tint pathlen;\n> >  \tint old_baselen = base->len;\n> > +\tint isdir, recurse = 0, emitthis = 1;\n> > +\n> > +\t/* at least something has to be valid */\n> > +\tassert(t1 || t2);\n> > +\n> > +\tif (t2) {\n> > +\t\t/* path present in resulting tree */\n> > +\t\tsha1 = tree_entry_extract(t2, &path, &mode);\n> > +\t\tpathlen = tree_entry_len(&t2->entry);\n> > +\t\tisdir = S_ISDIR(mode);\n> > +\t}\n> > +\telse {\n> > +\t\t/* a path was removed - take path from parent. Also take\n> > +\t\t * mode from parent, to decide on recursion.\n> > +\t\t */\n> > +\t\ttree_entry_extract(t1, &path, &mode);\n> > +\t\tpathlen = tree_entry_len(&t1->entry);\n> > +\n> > +\t\tisdir = S_ISDIR(mode);\n> > +\t\tsha1 = NULL;\n> > +\t\tmode = 0;\n> > +\t}\n> > +\n> > +\tif (DIFF_OPT_TST(opt, RECURSIVE) && isdir) {\n> > +\t\trecurse = 1;\n> > +\t\temitthis = DIFF_OPT_TST(opt, TREE_IN_RECURSIVE);\n> > +\t}\n> >  \n> >  \tstrbuf_add(base, path, pathlen);\n> > -\tif (DIFF_OPT_TST(opt, RECURSIVE) && S_ISDIR(mode)) {\n> > -\t\tif (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE))\n> > -\t\t\topt->add_remove(opt, *prefix, mode, sha1, 1, base->buf, 0);\n> >  \n> > +\tif (emitthis)\n> > +\t\temit_diff(opt, base, t1, t2);\n> > +\n> > +\tif (recurse) {\n> >  \t\tstrbuf_addch(base, '/');\n> > -\t\tdiff_tree_sha1(*prefix == '-' ? sha1 : NULL,\n> > -\t\t\t       *prefix == '+' ? sha1 : NULL, base->buf, opt);\n> > -\t} else\n> > -\t\topt->add_remove(opt, prefix[0], mode, sha1, 1, base->buf, 0);\n> > +\t\tdiff_tree_sha1(t1 ? t1->entry.sha1 : NULL,\n> > +\t\t\t       t2 ? t2->entry.sha1 : NULL, base->buf, opt);\n> > +\t}\n> \n> \n> After this step, \"sha1\" is assigned but never gets used.  Please\n> double-check the fix-up I queued in the series before merging it to\n> 'pu'.\n\nYour fixup is correct - it was my overlook when preparing the patch -\nsha1 is needed for later patch (multiparent diff tree-walker), but I've\nmistakenly left it here.\n\nThe two interesting patches I've sent you today, are already adjusted to\nthis correction - it is safe to squash the fixup in.\n\nThanks for noticing,\nKirill\n"}]}